You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@mesos.apache.org by Kapil Arya <ka...@mesosphere.io> on 2014/10/18 02:33:22 UTC
Review Request 26903: Added functions to manipulate LD_LIBRARY_PATH to
os::libraries.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/26903/
-----------------------------------------------------------
Review request for mesos, Benjamin Hindman and Niklas Nielsen.
Repository: mesos-git
Description
-------
Also added a utility function to expand a library name to a filename
by prefixing "lib" and adding the default extension.
Diffs
-----
3rdparty/libprocess/3rdparty/stout/include/stout/os.hpp 63bda7a87e24e1696f3e7bb0206c551ce39dfe27
Diff: https://reviews.apache.org/r/26903/diff/
Testing
-------
Ran make check.
Thanks,
Kapil Arya
Re: Review Request 26903: Added functions to manipulate
LD_LIBRARY_PATH to os::libraries.
Posted by Kapil Arya <ka...@mesosphere.io>.
> On Oct. 20, 2014, 1:56 p.m., Niklas Nielsen wrote:
> > 3rdparty/libprocess/3rdparty/stout/include/stout/os.hpp, line 1283
> > <https://reviews.apache.org/r/26903/diff/2/?file=725229#file725229line1283>
> >
> > How about flatten it to os::libraries::expand() ?
>
> Kapil Arya wrote:
> It's not clear what expand() would mean but expandName() is clearer. expandLibraryName() would have been the best, though.
>
> Niklas Nielsen wrote:
> library is already a part of the namespace, so that would become redundant?
> I mostly suggested it because you won't have other conflicting expand(std::string name) in that namespace and it would be more concise.
>
> Anyway, it is not a biggy; feel free to drop the issue if you want to keep expandName
I think the idea was to avoid repetition of
ifdef linux
return "LD_LIBRARY_PATH";
else
return "DYLD_LIBRARY_PATH";
endif
in paths() and sePaths(). If we don't mind repetition, then I think we should remove ldLibraryPath(). Alternatively, we can rename it to environmentVariable().
- Kapil
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/26903/#review57372
-----------------------------------------------------------
On Oct. 18, 2014, 3:04 p.m., Kapil Arya wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/26903/
> -----------------------------------------------------------
>
> (Updated Oct. 18, 2014, 3:04 p.m.)
>
>
> Review request for mesos, Benjamin Hindman and Niklas Nielsen.
>
>
> Repository: mesos-git
>
>
> Description
> -------
>
> Also added a utility function to expand a library name to a filename
> by prefixing "lib" and adding the default extension.
>
>
> Diffs
> -----
>
> 3rdparty/libprocess/3rdparty/stout/include/stout/os.hpp 63bda7a87e24e1696f3e7bb0206c551ce39dfe27
>
> Diff: https://reviews.apache.org/r/26903/diff/
>
>
> Testing
> -------
>
> Ran make check.
>
>
> Thanks,
>
> Kapil Arya
>
>
Re: Review Request 26903: Added functions to manipulate
LD_LIBRARY_PATH to os::libraries.
Posted by Kapil Arya <ka...@mesosphere.io>.
> On Oct. 20, 2014, 1:56 p.m., Niklas Nielsen wrote:
> > 3rdparty/libprocess/3rdparty/stout/include/stout/os.hpp, line 1283
> > <https://reviews.apache.org/r/26903/diff/2/?file=725229#file725229line1283>
> >
> > How about flatten it to os::libraries::expand() ?
It's not clear what expand() would mean but expandName() is clearer. expandLibraryName() would have been the best, though.
> On Oct. 20, 2014, 1:56 p.m., Niklas Nielsen wrote:
> > 3rdparty/libprocess/3rdparty/stout/include/stout/os.hpp, line 1271
> > <https://reviews.apache.org/r/26903/diff/2/?file=725229#file725229line1271>
> >
> > I looked through your patch set - it doesn't look like you use this function anywhere?
> >
> > It is a bit unclear to use 'ld library path' and 'path' interchangeably. How about sticking to 'path' or 'paths' (it is a path list right?)
> >
> > For example:
> >
> > // returns ':' separated list of library paths.
> > os::libraries::paths()
> >
> > // Resets library variable to ':' separated library list.
> > os::libraries::setPaths()
> >
> > // Appends library path to library list.
> > os::libraries::addPath()
> >
> > Let's think about the path() overload for a little bit. I think for getting and setting, we should call out explicitly what we do (
> > http://google-styleguide.googlecode.com/svn/trunk/cppguide.html#Function_Overloading).
We use ldLibraryPath() to get the corresponding environment variable string ("LD_LIBRARY_PATH"/"DYLD_LIBRARY_PATH").
I like the paths(), setPaths() and addPaths() function names and will update the diff accordingly.
- Kapil
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/26903/#review57372
-----------------------------------------------------------
On Oct. 18, 2014, 3:04 p.m., Kapil Arya wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/26903/
> -----------------------------------------------------------
>
> (Updated Oct. 18, 2014, 3:04 p.m.)
>
>
> Review request for mesos, Benjamin Hindman and Niklas Nielsen.
>
>
> Repository: mesos-git
>
>
> Description
> -------
>
> Also added a utility function to expand a library name to a filename
> by prefixing "lib" and adding the default extension.
>
>
> Diffs
> -----
>
> 3rdparty/libprocess/3rdparty/stout/include/stout/os.hpp 63bda7a87e24e1696f3e7bb0206c551ce39dfe27
>
> Diff: https://reviews.apache.org/r/26903/diff/
>
>
> Testing
> -------
>
> Ran make check.
>
>
> Thanks,
>
> Kapil Arya
>
>
Re: Review Request 26903: Added functions to manipulate
LD_LIBRARY_PATH to os::libraries.
Posted by Niklas Nielsen <ni...@qni.dk>.
> On Oct. 20, 2014, 10:56 a.m., Niklas Nielsen wrote:
> > 3rdparty/libprocess/3rdparty/stout/include/stout/os.hpp, line 1283
> > <https://reviews.apache.org/r/26903/diff/2/?file=725229#file725229line1283>
> >
> > How about flatten it to os::libraries::expand() ?
>
> Kapil Arya wrote:
> It's not clear what expand() would mean but expandName() is clearer. expandLibraryName() would have been the best, though.
library is already a part of the namespace, so that would become redundant?
I mostly suggested it because you won't have other conflicting expand(std::string name) in that namespace and it would be more concise.
Anyway, it is not a biggy; feel free to drop the issue if you want to keep expandName
> On Oct. 20, 2014, 10:56 a.m., Niklas Nielsen wrote:
> > 3rdparty/libprocess/3rdparty/stout/include/stout/os.hpp, line 1271
> > <https://reviews.apache.org/r/26903/diff/2/?file=725229#file725229line1271>
> >
> > I looked through your patch set - it doesn't look like you use this function anywhere?
> >
> > It is a bit unclear to use 'ld library path' and 'path' interchangeably. How about sticking to 'path' or 'paths' (it is a path list right?)
> >
> > For example:
> >
> > // returns ':' separated list of library paths.
> > os::libraries::paths()
> >
> > // Resets library variable to ':' separated library list.
> > os::libraries::setPaths()
> >
> > // Appends library path to library list.
> > os::libraries::addPath()
> >
> > Let's think about the path() overload for a little bit. I think for getting and setting, we should call out explicitly what we do (
> > http://google-styleguide.googlecode.com/svn/trunk/cppguide.html#Function_Overloading).
>
> Kapil Arya wrote:
> We use ldLibraryPath() to get the corresponding environment variable string ("LD_LIBRARY_PATH"/"DYLD_LIBRARY_PATH").
>
> I like the paths(), setPaths() and addPaths() function names and will update the diff accordingly.
Sure - but are you using it anywhere else? We expose two kinds of 'paths' - a ldLibraryPath (which is not a path but a env variable name) and 'path' which is a ':' separated directory list.
You should be able to hide ldLibraryPath with your new abstraction, so that would make the api cleaner IMHO.
Ben, what is your take?
I am not strongly attached to this, so if you think they are distinct enough - feel free to drop.
- Niklas
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/26903/#review57372
-----------------------------------------------------------
On Oct. 18, 2014, 12:04 p.m., Kapil Arya wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/26903/
> -----------------------------------------------------------
>
> (Updated Oct. 18, 2014, 12:04 p.m.)
>
>
> Review request for mesos, Benjamin Hindman and Niklas Nielsen.
>
>
> Repository: mesos-git
>
>
> Description
> -------
>
> Also added a utility function to expand a library name to a filename
> by prefixing "lib" and adding the default extension.
>
>
> Diffs
> -----
>
> 3rdparty/libprocess/3rdparty/stout/include/stout/os.hpp 63bda7a87e24e1696f3e7bb0206c551ce39dfe27
>
> Diff: https://reviews.apache.org/r/26903/diff/
>
>
> Testing
> -------
>
> Ran make check.
>
>
> Thanks,
>
> Kapil Arya
>
>
Re: Review Request 26903: Added functions to manipulate
LD_LIBRARY_PATH to os::libraries.
Posted by Niklas Nielsen <ni...@qni.dk>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/26903/#review57372
-----------------------------------------------------------
3rdparty/libprocess/3rdparty/stout/include/stout/os.hpp
<https://reviews.apache.org/r/26903/#comment98034>
I looked through your patch set - it doesn't look like you use this function anywhere?
It is a bit unclear to use 'ld library path' and 'path' interchangeably. How about sticking to 'path' or 'paths' (it is a path list right?)
For example:
// returns ':' separated list of library paths.
os::libraries::paths()
// Resets library variable to ':' separated library list.
os::libraries::setPaths()
// Appends library path to library list.
os::libraries::addPath()
Let's think about the path() overload for a little bit. I think for getting and setting, we should call out explicitly what we do (
http://google-styleguide.googlecode.com/svn/trunk/cppguide.html#Function_Overloading).
3rdparty/libprocess/3rdparty/stout/include/stout/os.hpp
<https://reviews.apache.org/r/26903/#comment98044>
How about flatten it to os::libraries::expand() ?
- Niklas Nielsen
On Oct. 18, 2014, 12:04 p.m., Kapil Arya wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/26903/
> -----------------------------------------------------------
>
> (Updated Oct. 18, 2014, 12:04 p.m.)
>
>
> Review request for mesos, Benjamin Hindman and Niklas Nielsen.
>
>
> Repository: mesos-git
>
>
> Description
> -------
>
> Also added a utility function to expand a library name to a filename
> by prefixing "lib" and adding the default extension.
>
>
> Diffs
> -----
>
> 3rdparty/libprocess/3rdparty/stout/include/stout/os.hpp 63bda7a87e24e1696f3e7bb0206c551ce39dfe27
>
> Diff: https://reviews.apache.org/r/26903/diff/
>
>
> Testing
> -------
>
> Ran make check.
>
>
> Thanks,
>
> Kapil Arya
>
>
Re: Review Request 26903: Added functions to manipulate
LD_LIBRARY_PATH to os::libraries.
Posted by Kapil Arya <ka...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/26903/#review57385
-----------------------------------------------------------
3rdparty/libprocess/3rdparty/stout/include/stout/os.hpp
<https://reviews.apache.org/r/26903/#comment98073>
I think the idea was to avoid repetition of
#ifdef __linux__
return "LD_LIBRARY_PATH";
#else
return "DYLD_LIBRARY_PATH";
#endif
in paths() and sePaths(). If we don't mind repetition, then I think we should remove ldLibraryPath(). Alternatively, we can rename it to environmentVariable().
- Kapil Arya
On Oct. 18, 2014, 3:04 p.m., Kapil Arya wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/26903/
> -----------------------------------------------------------
>
> (Updated Oct. 18, 2014, 3:04 p.m.)
>
>
> Review request for mesos, Benjamin Hindman and Niklas Nielsen.
>
>
> Repository: mesos-git
>
>
> Description
> -------
>
> Also added a utility function to expand a library name to a filename
> by prefixing "lib" and adding the default extension.
>
>
> Diffs
> -----
>
> 3rdparty/libprocess/3rdparty/stout/include/stout/os.hpp 63bda7a87e24e1696f3e7bb0206c551ce39dfe27
>
> Diff: https://reviews.apache.org/r/26903/diff/
>
>
> Testing
> -------
>
> Ran make check.
>
>
> Thanks,
>
> Kapil Arya
>
>
Re: Review Request 26903: Added functions to manipulate
LD_LIBRARY_PATH to os::libraries.
Posted by Kapil Arya <ka...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/26903/
-----------------------------------------------------------
(Updated Oct. 20, 2014, 7:57 p.m.)
Review request for mesos, Benjamin Hindman and Niklas Nielsen.
Changes
-------
Added a comment for the tests.
Repository: mesos-git
Description
-------
Also added a utility function to expand a library name to a filename
by prefixing "lib" and adding the default extension.
Diffs (updated)
-----
3rdparty/libprocess/3rdparty/stout/include/stout/os.hpp 63bda7a87e24e1696f3e7bb0206c551ce39dfe27
3rdparty/libprocess/3rdparty/stout/tests/os_tests.cpp 02293f2f2e4148045abfd3c15ef4882cf8042f55
Diff: https://reviews.apache.org/r/26903/diff/
Testing
-------
Ran make check.
Thanks,
Kapil Arya
Re: Review Request 26903: Added functions to manipulate
LD_LIBRARY_PATH to os::libraries.
Posted by Niklas Nielsen <ni...@qni.dk>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/26903/#review57473
-----------------------------------------------------------
Ship it!
Modulo adding a test comment.
- Niklas Nielsen
On Oct. 20, 2014, 4:53 p.m., Kapil Arya wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/26903/
> -----------------------------------------------------------
>
> (Updated Oct. 20, 2014, 4:53 p.m.)
>
>
> Review request for mesos, Benjamin Hindman and Niklas Nielsen.
>
>
> Repository: mesos-git
>
>
> Description
> -------
>
> Also added a utility function to expand a library name to a filename
> by prefixing "lib" and adding the default extension.
>
>
> Diffs
> -----
>
> 3rdparty/libprocess/3rdparty/stout/include/stout/os.hpp 63bda7a87e24e1696f3e7bb0206c551ce39dfe27
> 3rdparty/libprocess/3rdparty/stout/tests/os_tests.cpp 02293f2f2e4148045abfd3c15ef4882cf8042f55
>
> Diff: https://reviews.apache.org/r/26903/diff/
>
>
> Testing
> -------
>
> Ran make check.
>
>
> Thanks,
>
> Kapil Arya
>
>
Re: Review Request 26903: Added functions to manipulate
LD_LIBRARY_PATH to os::libraries.
Posted by Kapil Arya <ka...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/26903/
-----------------------------------------------------------
(Updated Oct. 20, 2014, 7:53 p.m.)
Review request for mesos, Benjamin Hindman and Niklas Nielsen.
Changes
-------
Added tests.
Repository: mesos-git
Description
-------
Also added a utility function to expand a library name to a filename
by prefixing "lib" and adding the default extension.
Diffs (updated)
-----
3rdparty/libprocess/3rdparty/stout/include/stout/os.hpp 63bda7a87e24e1696f3e7bb0206c551ce39dfe27
3rdparty/libprocess/3rdparty/stout/tests/os_tests.cpp 02293f2f2e4148045abfd3c15ef4882cf8042f55
Diff: https://reviews.apache.org/r/26903/diff/
Testing
-------
Ran make check.
Thanks,
Kapil Arya
Re: Review Request 26903: Added functions to manipulate
LD_LIBRARY_PATH to os::libraries.
Posted by Niklas Nielsen <ni...@qni.dk>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/26903/#review57465
-----------------------------------------------------------
One high-level comment: let's add a test for this :-)
- Niklas Nielsen
On Oct. 20, 2014, 3:53 p.m., Kapil Arya wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/26903/
> -----------------------------------------------------------
>
> (Updated Oct. 20, 2014, 3:53 p.m.)
>
>
> Review request for mesos, Benjamin Hindman and Niklas Nielsen.
>
>
> Repository: mesos-git
>
>
> Description
> -------
>
> Also added a utility function to expand a library name to a filename
> by prefixing "lib" and adding the default extension.
>
>
> Diffs
> -----
>
> 3rdparty/libprocess/3rdparty/stout/include/stout/os.hpp 63bda7a87e24e1696f3e7bb0206c551ce39dfe27
>
> Diff: https://reviews.apache.org/r/26903/diff/
>
>
> Testing
> -------
>
> Ran make check.
>
>
> Thanks,
>
> Kapil Arya
>
>
Re: Review Request 26903: Added functions to manipulate
LD_LIBRARY_PATH to os::libraries.
Posted by Kapil Arya <ka...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/26903/
-----------------------------------------------------------
(Updated Oct. 20, 2014, 6:53 p.m.)
Review request for mesos, Benjamin Hindman and Niklas Nielsen.
Changes
-------
Addressed Nik's comments.
Repository: mesos-git
Description
-------
Also added a utility function to expand a library name to a filename
by prefixing "lib" and adding the default extension.
Diffs (updated)
-----
3rdparty/libprocess/3rdparty/stout/include/stout/os.hpp 63bda7a87e24e1696f3e7bb0206c551ce39dfe27
Diff: https://reviews.apache.org/r/26903/diff/
Testing
-------
Ran make check.
Thanks,
Kapil Arya
Re: Review Request 26903: Added functions to manipulate
LD_LIBRARY_PATH to os::libraries.
Posted by Kapil Arya <ka...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/26903/
-----------------------------------------------------------
(Updated Oct. 18, 2014, 3:04 p.m.)
Review request for mesos, Benjamin Hindman and Niklas Nielsen.
Changes
-------
Rebased diff.
Repository: mesos-git
Description
-------
Also added a utility function to expand a library name to a filename
by prefixing "lib" and adding the default extension.
Diffs (updated)
-----
3rdparty/libprocess/3rdparty/stout/include/stout/os.hpp 63bda7a87e24e1696f3e7bb0206c551ce39dfe27
Diff: https://reviews.apache.org/r/26903/diff/
Testing
-------
Ran make check.
Thanks,
Kapil Arya