You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@mesos.apache.org by James Peach <jp...@apache.org> on 2016/11/29 00:37:13 UTC
Re: Review Request 53791: Use the stout ELF parser to collect Linux
rootfs files.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/53791/
-----------------------------------------------------------
(Updated Nov. 29, 2016, 12:37 a.m.)
Review request for mesos, Gilbert Song, Jie Yu, Kevin Klues, and Jiang Yan Xu.
Changes
-------
Rebase and update from review comments.
Bugs: MESOS-6588
https://issues.apache.org/jira/browse/MESOS-6588
Repository: mesos
Description
-------
The dependencies for the programs we need in the Linux root
filesystem vary over time and across distributions. Since Stout
has support for parsing the library dependencies of ELF binaries,
use this to collect the necessary dependencies when constructing
a root filesystem for the tests.
Diffs (updated)
-----
src/CMakeLists.txt 9ff47d7a95c3baa5aa10039039e5ad065180ba45
src/Makefile.am 85eda538caf39f81f052896e744b7b0c724f81bb
src/linux/ldd.hpp PRE-CREATION
src/linux/ldd.cpp PRE-CREATION
src/tests/containerizer/rootfs.hpp 6bc3835cbb62536ec933ef38c9e15138b8611e5f
src/tests/containerizer/rootfs.cpp PRE-CREATION
Diff: https://reviews.apache.org/r/53791/diff/
Testing
-------
sudo make check (Fedora 24)
Thanks,
James Peach
Re: Review Request 53791: Use the stout ELF parser to collect Linux
rootfs files.
Posted by Mesos ReviewBot <re...@mesos.apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/53791/#review157202
-----------------------------------------------------------
Patch looks great!
Reviews applied: [53790, 53791]
Passed command: export OS='ubuntu:14.04' BUILDTOOL='autotools' COMPILER='gcc' CONFIGURATION='--verbose' ENVIRONMENT='GLOG_v=1 MESOS_VERBOSE=1'; ./support/docker_build.sh
- Mesos ReviewBot
On Nov. 29, 2016, 1:21 a.m., James Peach wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/53791/
> -----------------------------------------------------------
>
> (Updated Nov. 29, 2016, 1:21 a.m.)
>
>
> Review request for mesos, Gilbert Song, Jie Yu, Kevin Klues, and Jiang Yan Xu.
>
>
> Bugs: MESOS-6588
> https://issues.apache.org/jira/browse/MESOS-6588
>
>
> Repository: mesos
>
>
> Description
> -------
>
> The dependencies for the programs we need in the Linux root
> filesystem vary over time and across distributions. Since Stout
> has support for parsing the library dependencies of ELF binaries,
> use this to collect the necessary dependencies when constructing
> a root filesystem for the tests.
>
>
> Diffs
> -----
>
> src/CMakeLists.txt 0884c6bb6f2ebddadc5cfc6b72d704086709748d
> src/Makefile.am 85eda538caf39f81f052896e744b7b0c724f81bb
> src/linux/ldd.hpp PRE-CREATION
> src/linux/ldd.cpp PRE-CREATION
> src/tests/containerizer/rootfs.hpp 6bc3835cbb62536ec933ef38c9e15138b8611e5f
> src/tests/containerizer/rootfs.cpp PRE-CREATION
>
> Diff: https://reviews.apache.org/r/53791/diff/
>
>
> Testing
> -------
>
> sudo make check (Fedora 24)
>
>
> Thanks,
>
> James Peach
>
>
Re: Review Request 53791: Use the stout ELF parser to collect Linux
rootfs files.
Posted by James Peach <jp...@apache.org>.
> On Dec. 13, 2016, 10:13 a.m., Benjamin Bannier wrote:
> > src/linux/ldd.cpp, line 32
> > <https://reviews.apache.org/r/53791/diff/6/?file=1571593#file1571593line32>
> >
> > Mesos does not like anon namespaces. Since this namespace e.g., declares no types, you could just replace its usage with making `collectDependencies` `static` without loss.
There's nothing in the style guide, and the code contains a mix of traditional `static` and unnamed namespaces. I changed it anyway.
> On Dec. 13, 2016, 10:13 a.m., Benjamin Bannier wrote:
> > src/linux/ldd.cpp, lines 82-83
> > <https://reviews.apache.org/r/53791/diff/6/?file=1571593#file1571593line82>
> >
> > Let's move this up right after the check `needed.contains(path)`.
> >
> > Right now in pathological cases like e.g., a`libA` depending on `libB`, and `libB` depending on `libA`, we would recurse indefinitely.
Adding the path to the `needed` set means that we need the path and have already calculated the dependencies for that path. If we add a path before having all its dependencies, we would return early in the `needed.contains(path)` check.
> On Dec. 13, 2016, 10:13 a.m., Benjamin Bannier wrote:
> > src/linux/ldd.cpp, line 37
> > <https://reviews.apache.org/r/53791/diff/6/?file=1571593#file1571593line37>
> >
> > Let's not use an out parameter, but instead return a `Try<hashset<T>`.
> >
> > Note that this requires making a copy of an in parameter `needed` and to explicitly calculate the union in below `foreach` loop. In the end the code might require more copies, but have clearer side effects.
I don't think we should do this.
> On Dec. 13, 2016, 10:13 a.m., Benjamin Bannier wrote:
> > src/tests/containerizer/rootfs.cpp, line 137
> > <https://reviews.apache.org/r/53791/diff/6/?file=1571595#file1571595line137>
> >
> > nit: Not yours, but should be 4 spaces.
Apparently 2 is correct.
> On Dec. 13, 2016, 10:13 a.m., Benjamin Bannier wrote:
> > src/tests/containerizer/rootfs.cpp, line 157
> > <https://reviews.apache.org/r/53791/diff/6/?file=1571595#file1571595line157>
> >
> > nit: Not yours, but should be 4 spaces.
Apparantly 2 is correct.
> On Dec. 13, 2016, 10:13 a.m., Benjamin Bannier wrote:
> > src/tests/containerizer/rootfs.cpp, line 191
> > <https://reviews.apache.org/r/53791/diff/6/?file=1571595#file1571595line191>
> >
> > nit: Not yours, but should be 4 spaces.
Apparantly 2 is correct.
- James
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/53791/#review158971
-----------------------------------------------------------
On Nov. 29, 2016, 1:21 a.m., James Peach wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/53791/
> -----------------------------------------------------------
>
> (Updated Nov. 29, 2016, 1:21 a.m.)
>
>
> Review request for mesos, Gilbert Song, Jie Yu, Kevin Klues, and Jiang Yan Xu.
>
>
> Bugs: MESOS-6588
> https://issues.apache.org/jira/browse/MESOS-6588
>
>
> Repository: mesos
>
>
> Description
> -------
>
> The dependencies for the programs we need in the Linux root
> filesystem vary over time and across distributions. Since Stout
> has support for parsing the library dependencies of ELF binaries,
> use this to collect the necessary dependencies when constructing
> a root filesystem for the tests.
>
>
> Diffs
> -----
>
> src/CMakeLists.txt 0884c6bb6f2ebddadc5cfc6b72d704086709748d
> src/Makefile.am 85eda538caf39f81f052896e744b7b0c724f81bb
> src/linux/ldd.hpp PRE-CREATION
> src/linux/ldd.cpp PRE-CREATION
> src/tests/containerizer/rootfs.hpp 6bc3835cbb62536ec933ef38c9e15138b8611e5f
> src/tests/containerizer/rootfs.cpp PRE-CREATION
>
> Diff: https://reviews.apache.org/r/53791/diff/
>
>
> Testing
> -------
>
> sudo make check (Fedora 24)
>
>
> Thanks,
>
> James Peach
>
>
Re: Review Request 53791: Use the stout ELF parser to collect Linux
rootfs files.
Posted by Benjamin Bannier <be...@mesosphere.io>.
> On Dec. 13, 2016, 11:13 a.m., Benjamin Bannier wrote:
> > src/linux/ldd.cpp, lines 82-83
> > <https://reviews.apache.org/r/53791/diff/6/?file=1571593#file1571593line82>
> >
> > Let's move this up right after the check `needed.contains(path)`.
> >
> > Right now in pathological cases like e.g., a`libA` depending on `libB`, and `libB` depending on `libA`, we would recurse indefinitely.
>
> James Peach wrote:
> Adding the path to the `needed` set means that we need the path and have already calculated the dependencies for that path. If we add a path before having all its dependencies, we would return early in the `needed.contains(path)` check.
An entry in `needed` is the only indication on whether we have already visited a node in the dependency graph (which is not necessarily a tree).
If you do not add `path` to `needed` before recursing into `collectDependencies`, `path` might be visited again as a dependency in pathological cases involving cyclic shared library dependencies (which the loader might be fine with). In such a case this code would recurse indefinitely.
It looks like a fix might be to add `path` to `needed` after you have found that it currently is not in `needed`, but before you recurse.
- Benjamin
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/53791/#review158971
-----------------------------------------------------------
On Dec. 13, 2016, 7:34 p.m., James Peach wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/53791/
> -----------------------------------------------------------
>
> (Updated Dec. 13, 2016, 7:34 p.m.)
>
>
> Review request for mesos, Gilbert Song, Jie Yu, Kevin Klues, and Jiang Yan Xu.
>
>
> Bugs: MESOS-6588
> https://issues.apache.org/jira/browse/MESOS-6588
>
>
> Repository: mesos
>
>
> Description
> -------
>
> The dependencies for the programs we need in the Linux root
> filesystem vary over time and across distributions. Since Stout
> has support for parsing the library dependencies of ELF binaries,
> use this to collect the necessary dependencies when constructing
> a root filesystem for the tests.
>
>
> Diffs
> -----
>
> src/Makefile.am a4c03c2b918816e6dd8872d37e5208f055619c47
> src/tests/containerizer/rootfs.hpp 6bc3835cbb62536ec933ef38c9e15138b8611e5f
> src/tests/containerizer/rootfs.cpp PRE-CREATION
>
> Diff: https://reviews.apache.org/r/53791/diff/
>
>
> Testing
> -------
>
> sudo make check (Fedora 24)
>
>
> Thanks,
>
> James Peach
>
>
Re: Review Request 53791: Use the stout ELF parser to collect Linux
rootfs files.
Posted by Benjamin Bannier <be...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/53791/#review158971
-----------------------------------------------------------
Thanks for looking into this James.
Looks mostly good for me; I was also able to execute tests using the created rootfs under e.g., fedora-25 which wasn't possible before.
I left some mostly minor comments on style and edge cases.
src/linux/ldd.hpp (lines 31 - 32)
<https://reviews.apache.org/r/53791/#comment229861>
Please add includes for `string` and `vector`.
src/linux/ldd.cpp (line 32)
<https://reviews.apache.org/r/53791/#comment229864>
Mesos does not like anon namespaces. Since this namespace e.g., declares no types, you could just replace its usage with making `collectDependencies` `static` without loss.
src/linux/ldd.cpp (line 37)
<https://reviews.apache.org/r/53791/#comment229874>
Let's not use an out parameter, but instead return a `Try<hashset<T>`.
Note that this requires making a copy of an in parameter `needed` and to explicitly calculate the union in below `foreach` loop. In the end the code might require more copies, but have clearer side effects.
src/linux/ldd.cpp (line 46)
<https://reviews.apache.org/r/53791/#comment229863>
`load.isError()`
src/linux/ldd.cpp (line 53)
<https://reviews.apache.org/r/53791/#comment229862>
nit: 4 spaces.
src/linux/ldd.cpp (line 54)
<https://reviews.apache.org/r/53791/#comment229871>
`dependencies.isError()`
src/linux/ldd.cpp (line 58)
<https://reviews.apache.org/r/53791/#comment229875>
nit: space after `foreach`.
src/linux/ldd.cpp (lines 82 - 83)
<https://reviews.apache.org/r/53791/#comment229869>
Let's move this up right after the check `needed.contains(path)`.
Right now in pathological cases like e.g., a`libA` depending on `libB`, and `libB` depending on `libA`, we would recurse indefinitely.
src/linux/ldd.cpp (line 90)
<https://reviews.apache.org/r/53791/#comment229877>
We often don't try hard enough, but since this is new code, could we use `Path` for paths everywhere here to make semantics clearer?
src/tests/containerizer/rootfs.cpp (line 56)
<https://reviews.apache.org/r/53791/#comment229879>
We also need to check for `realpath.isNone()`.
src/tests/containerizer/rootfs.cpp (lines 64 - 70)
<https://reviews.apache.org/r/53791/#comment229881>
This will fail to create a working link if `file` is not a direct link to `realpath`, e.g., if we have multiple levels of links.
This error already existed in the original implementation, could you add a `TODO`, or even better create a ticket?
src/tests/containerizer/rootfs.cpp (line 79)
<https://reviews.apache.org/r/53791/#comment229887>
+1!
src/tests/containerizer/rootfs.cpp (line 92)
<https://reviews.apache.org/r/53791/#comment229888>
nit: Let's break this line after `Error('.
src/tests/containerizer/rootfs.cpp (line 136)
<https://reviews.apache.org/r/53791/#comment229885>
Not originally yours, but we could just as well use a set (e.g., `set` or `hashset`) here to express that duplicates are not meaningful.
Here and below.
src/tests/containerizer/rootfs.cpp (line 137)
<https://reviews.apache.org/r/53791/#comment229889>
nit: Not yours, but should be 4 spaces.
src/tests/containerizer/rootfs.cpp (line 144)
<https://reviews.apache.org/r/53791/#comment229882>
Could we move this right above the loop using it?
src/tests/containerizer/rootfs.cpp (line 145)
<https://reviews.apache.org/r/53791/#comment229890>
nit: Not yours, but should be 4 spaces.
src/tests/containerizer/rootfs.cpp (line 153)
<https://reviews.apache.org/r/53791/#comment229891>
Let's break this line after `Error(`.
src/tests/containerizer/rootfs.cpp (line 175)
<https://reviews.apache.org/r/53791/#comment229893>
nit: Not yours, but should be 4 spaces.
- Benjamin Bannier
On Nov. 29, 2016, 2:21 a.m., James Peach wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/53791/
> -----------------------------------------------------------
>
> (Updated Nov. 29, 2016, 2:21 a.m.)
>
>
> Review request for mesos, Gilbert Song, Jie Yu, Kevin Klues, and Jiang Yan Xu.
>
>
> Bugs: MESOS-6588
> https://issues.apache.org/jira/browse/MESOS-6588
>
>
> Repository: mesos
>
>
> Description
> -------
>
> The dependencies for the programs we need in the Linux root
> filesystem vary over time and across distributions. Since Stout
> has support for parsing the library dependencies of ELF binaries,
> use this to collect the necessary dependencies when constructing
> a root filesystem for the tests.
>
>
> Diffs
> -----
>
> src/CMakeLists.txt 0884c6bb6f2ebddadc5cfc6b72d704086709748d
> src/Makefile.am 85eda538caf39f81f052896e744b7b0c724f81bb
> src/linux/ldd.hpp PRE-CREATION
> src/linux/ldd.cpp PRE-CREATION
> src/tests/containerizer/rootfs.hpp 6bc3835cbb62536ec933ef38c9e15138b8611e5f
> src/tests/containerizer/rootfs.cpp PRE-CREATION
>
> Diff: https://reviews.apache.org/r/53791/diff/
>
>
> Testing
> -------
>
> sudo make check (Fedora 24)
>
>
> Thanks,
>
> James Peach
>
>
Re: Review Request 53791: Use the stout ELF parser to collect Linux
rootfs files.
Posted by Jiang Yan Xu <xu...@apple.com>.
> On Feb. 1, 2017, 3:51 p.m., James Peach wrote:
> > src/tests/containerizer/rootfs.cpp, line 90
> > <https://reviews.apache.org/r/53791/diff/8/?file=1589112#file1589112line90>
> >
> > What are you asking for here? This can fail for a number of reasons and `errno` describes them. Squashing that to a fixed string would be misleading.
>
> Jiang Yan Xu wrote:
> I am saying we know "This can fail for a number of reasons and errno describes them" because we are breaking the abstraction here. Perhaps my arugment seems silly because `os::exists()` has a 9-line implementation and we can look at the source. But if it's 900 lines or we can't see the source the same principle still applies right?
>
> `os::exists()` current exposes a `bool` and expects it to be used to interpret what's happened. If we peek inside the implementation and write code based on the internals, it's likely to break when the implementation changes.
>
> Perhaps `bool os::exists()` is currently lacking in ability to reveal errors, should we then change it to `Try<bool> exists()`? If that's more suitable as a TODO or we are not sure, I think we should just leave it as it is right now. There are hundreds of other uses in the codebase which may be susceptible to conditions like "the path does exist but we are not permitted to access it" so maybe it's worth a JIRA to fix them anyways? (Of course that shouldn't block our change here)
Filed https://issues.apache.org/jira/browse/MESOS-7052 for this.
- Jiang Yan
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/53791/#review163834
-----------------------------------------------------------
On Feb. 1, 2017, 3:51 p.m., James Peach wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/53791/
> -----------------------------------------------------------
>
> (Updated Feb. 1, 2017, 3:51 p.m.)
>
>
> Review request for mesos, Gilbert Song, Jie Yu, Kevin Klues, and Jiang Yan Xu.
>
>
> Bugs: MESOS-6588
> https://issues.apache.org/jira/browse/MESOS-6588
>
>
> Repository: mesos
>
>
> Description
> -------
>
> The dependencies for the programs we need in the Linux root
> filesystem vary over time and across distributions. Since Stout
> has support for parsing the library dependencies of ELF binaries,
> use this to collect the necessary dependencies when constructing
> a root filesystem for the tests.
>
>
> Diffs
> -----
>
> src/tests/containerizer/rootfs.hpp 9648638c1d4b1f1c49708e058e9b930c5ae32138
> src/tests/containerizer/rootfs.cpp f11f126fa4f3130b6778bfe7e7d0f025a638f06e
>
> Diff: https://reviews.apache.org/r/53791/diff/
>
>
> Testing
> -------
>
> sudo make check (Fedora 24)
>
>
> Thanks,
>
> James Peach
>
>
Re: Review Request 53791: Use the stout ELF parser to collect Linux
rootfs files.
Posted by Jiang Yan Xu <xu...@apple.com>.
> On Feb. 1, 2017, 3:51 p.m., James Peach wrote:
> >
Could you directly reply to the comment in the future? It's pretty hard to follow with this format...
> On Feb. 1, 2017, 3:51 p.m., James Peach wrote:
> > src/tests/containerizer/rootfs.hpp, line 50
> > <https://reviews.apache.org/r/53791/diff/8/?file=1589111#file1589111line50>
> >
> > `copy()` would normally take a const reference to something and return a copy of that. Since this is not what we are doing, best not to use that name.
Alright.
> On Feb. 1, 2017, 3:51 p.m., James Peach wrote:
> > src/tests/containerizer/rootfs.cpp, line 90
> > <https://reviews.apache.org/r/53791/diff/8/?file=1589112#file1589112line90>
> >
> > What are you asking for here? This can fail for a number of reasons and `errno` describes them. Squashing that to a fixed string would be misleading.
I am saying we know "This can fail for a number of reasons and errno describes them" because we are breaking the abstraction here. Perhaps my arugment seems silly because `os::exists()` has a 9-line implementation and we can look at the source. But if it's 900 lines or we can't see the source the same principle still applies right?
`os::exists()` current exposes a `bool` and expects it to be used to interpret what's happened. If we peek inside the implementation and write code based on the internals, it's likely to break when the implementation changes.
Perhaps `bool os::exists()` is currently lacking in ability to reveal errors, should we then change it to `Try<bool> exists()`? If that's more suitable as a TODO or we are not sure, I think we should just leave it as it is right now. There are hundreds of other uses in the codebase which may be susceptible to conditions like "the path does exist but we are not permitted to access it" so maybe it's worth a JIRA to fix them anyways? (Of course that shouldn't block our change here)
- Jiang Yan
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/53791/#review163834
-----------------------------------------------------------
On Feb. 1, 2017, 3:51 p.m., James Peach wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/53791/
> -----------------------------------------------------------
>
> (Updated Feb. 1, 2017, 3:51 p.m.)
>
>
> Review request for mesos, Gilbert Song, Jie Yu, Kevin Klues, and Jiang Yan Xu.
>
>
> Bugs: MESOS-6588
> https://issues.apache.org/jira/browse/MESOS-6588
>
>
> Repository: mesos
>
>
> Description
> -------
>
> The dependencies for the programs we need in the Linux root
> filesystem vary over time and across distributions. Since Stout
> has support for parsing the library dependencies of ELF binaries,
> use this to collect the necessary dependencies when constructing
> a root filesystem for the tests.
>
>
> Diffs
> -----
>
> src/tests/containerizer/rootfs.hpp 9648638c1d4b1f1c49708e058e9b930c5ae32138
> src/tests/containerizer/rootfs.cpp f11f126fa4f3130b6778bfe7e7d0f025a638f06e
>
> Diff: https://reviews.apache.org/r/53791/diff/
>
>
> Testing
> -------
>
> sudo make check (Fedora 24)
>
>
> Thanks,
>
> James Peach
>
>
Re: Review Request 53791: Use the stout ELF parser to collect Linux
rootfs files.
Posted by James Peach <jp...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/53791/#review163834
-----------------------------------------------------------
src/tests/containerizer/rootfs.hpp (line 50)
<https://reviews.apache.org/r/53791/#comment235333>
`copy()` would normally take a const reference to something and return a copy of that. Since this is not what we are doing, best not to use that name.
src/tests/containerizer/rootfs.cpp (lines 61 - 63)
<https://reviews.apache.org/r/53791/#comment235322>
It's better to format consistently than to micro-manage line lengths.
src/tests/containerizer/rootfs.cpp (line 90)
<https://reviews.apache.org/r/53791/#comment235324>
What are you asking for here? This can fail for a number of reasons and `errno` describes them. Squashing that to a fixed string would be misleading.
- James Peach
On Feb. 1, 2017, 11:51 p.m., James Peach wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/53791/
> -----------------------------------------------------------
>
> (Updated Feb. 1, 2017, 11:51 p.m.)
>
>
> Review request for mesos, Gilbert Song, Jie Yu, Kevin Klues, and Jiang Yan Xu.
>
>
> Bugs: MESOS-6588
> https://issues.apache.org/jira/browse/MESOS-6588
>
>
> Repository: mesos
>
>
> Description
> -------
>
> The dependencies for the programs we need in the Linux root
> filesystem vary over time and across distributions. Since Stout
> has support for parsing the library dependencies of ELF binaries,
> use this to collect the necessary dependencies when constructing
> a root filesystem for the tests.
>
>
> Diffs
> -----
>
> src/tests/containerizer/rootfs.hpp 9648638c1d4b1f1c49708e058e9b930c5ae32138
> src/tests/containerizer/rootfs.cpp f11f126fa4f3130b6778bfe7e7d0f025a638f06e
>
> Diff: https://reviews.apache.org/r/53791/diff/
>
>
> Testing
> -------
>
> sudo make check (Fedora 24)
>
>
> Thanks,
>
> James Peach
>
>
Re: Review Request 53791: Use the stout ELF parser to collect Linux
rootfs files.
Posted by Mesos Reviewbot <re...@mesos.apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/53791/#review163947
-----------------------------------------------------------
Patch looks great!
Reviews applied: [53790, 54712, 54878, 53791]
Passed command: export OS='ubuntu:14.04' BUILDTOOL='autotools' COMPILER='gcc' CONFIGURATION='--verbose' ENVIRONMENT='GLOG_v=1 MESOS_VERBOSE=1'; ./support/docker-build.sh
- Mesos Reviewbot
On Feb. 1, 2017, 11:51 p.m., James Peach wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/53791/
> -----------------------------------------------------------
>
> (Updated Feb. 1, 2017, 11:51 p.m.)
>
>
> Review request for mesos, Gilbert Song, Jie Yu, Kevin Klues, and Jiang Yan Xu.
>
>
> Bugs: MESOS-6588
> https://issues.apache.org/jira/browse/MESOS-6588
>
>
> Repository: mesos
>
>
> Description
> -------
>
> The dependencies for the programs we need in the Linux root
> filesystem vary over time and across distributions. Since Stout
> has support for parsing the library dependencies of ELF binaries,
> use this to collect the necessary dependencies when constructing
> a root filesystem for the tests.
>
>
> Diffs
> -----
>
> src/tests/containerizer/rootfs.hpp 9648638c1d4b1f1c49708e058e9b930c5ae32138
> src/tests/containerizer/rootfs.cpp f11f126fa4f3130b6778bfe7e7d0f025a638f06e
>
> Diff: https://reviews.apache.org/r/53791/diff/
>
>
> Testing
> -------
>
> sudo make check (Fedora 24)
>
>
> Thanks,
>
> James Peach
>
>
Re: Review Request 53791: Use the stout ELF parser to collect Linux
rootfs files.
Posted by Jiang Yan Xu <xu...@apple.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/53791/#review164243
-----------------------------------------------------------
Ship it!
Ship It!
- Jiang Yan Xu
On Feb. 3, 2017, 4:44 p.m., James Peach wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/53791/
> -----------------------------------------------------------
>
> (Updated Feb. 3, 2017, 4:44 p.m.)
>
>
> Review request for mesos, Gilbert Song, Jie Yu, Kevin Klues, and Jiang Yan Xu.
>
>
> Bugs: MESOS-6588
> https://issues.apache.org/jira/browse/MESOS-6588
>
>
> Repository: mesos
>
>
> Description
> -------
>
> The dependencies for the programs we need in the Linux root
> filesystem vary over time and across distributions. Since Stout
> has support for parsing the library dependencies of ELF binaries,
> use this to collect the necessary dependencies when constructing
> a root filesystem for the tests.
>
>
> Diffs
> -----
>
> src/tests/containerizer/rootfs.hpp 9648638c1d4b1f1c49708e058e9b930c5ae32138
> src/tests/containerizer/rootfs.cpp f11f126fa4f3130b6778bfe7e7d0f025a638f06e
>
> Diff: https://reviews.apache.org/r/53791/diff/
>
>
> Testing
> -------
>
> sudo make check (Fedora 24)
>
>
> Thanks,
>
> James Peach
>
>
Re: Review Request 53791: Use the stout ELF parser to collect Linux
rootfs files.
Posted by James Peach <jp...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/53791/
-----------------------------------------------------------
(Updated Feb. 4, 2017, 12:44 a.m.)
Review request for mesos, Gilbert Song, Jie Yu, Kevin Klues, and Jiang Yan Xu.
Bugs: MESOS-6588
https://issues.apache.org/jira/browse/MESOS-6588
Repository: mesos
Description
-------
The dependencies for the programs we need in the Linux root
filesystem vary over time and across distributions. Since Stout
has support for parsing the library dependencies of ELF binaries,
use this to collect the necessary dependencies when constructing
a root filesystem for the tests.
Diffs (updated)
-----
src/tests/containerizer/rootfs.hpp 9648638c1d4b1f1c49708e058e9b930c5ae32138
src/tests/containerizer/rootfs.cpp f11f126fa4f3130b6778bfe7e7d0f025a638f06e
Diff: https://reviews.apache.org/r/53791/diff/
Testing
-------
sudo make check (Fedora 24)
Thanks,
James Peach
Re: Review Request 53791: Use the stout ELF parser to collect Linux
rootfs files.
Posted by Jiang Yan Xu <xu...@apple.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/53791/#review164172
-----------------------------------------------------------
Fix it, then Ship it!
Just a few nits.
src/tests/containerizer/rootfs.cpp (lines 62 - 63)
<https://reviews.apache.org/r/53791/#comment235767>
This is not a valid indentation style.
```
return Error(
"Failed to resolve '" + path + "': " + target.error());
```
or
```
return Error("Failed to resolve '" + path + "': " +
target.error());
```
here and below.
src/tests/containerizer/rootfs.cpp (line 79)
<https://reviews.apache.org/r/53791/#comment235811>
In Mesos full spelling of variable names is prefered.
s/src/source/
s/dst/destination/
src/tests/containerizer/rootfs.cpp (lines 89 - 90)
<https://reviews.apache.org/r/53791/#comment235828>
Given the comment above:
`destinationDirectory` is at least no worse than: https://github.com/apache/mesos/blob/3a537a1d18e500fb1efe2f4d4f0718fb25c51848/src/launcher/fetcher.cpp#L65
`destinationFullPath` slightly better? (i.e., what are the differences between destination vs. destinationPath?)
- Jiang Yan Xu
On Feb. 2, 2017, 9:32 a.m., James Peach wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/53791/
> -----------------------------------------------------------
>
> (Updated Feb. 2, 2017, 9:32 a.m.)
>
>
> Review request for mesos, Gilbert Song, Jie Yu, Kevin Klues, and Jiang Yan Xu.
>
>
> Bugs: MESOS-6588
> https://issues.apache.org/jira/browse/MESOS-6588
>
>
> Repository: mesos
>
>
> Description
> -------
>
> The dependencies for the programs we need in the Linux root
> filesystem vary over time and across distributions. Since Stout
> has support for parsing the library dependencies of ELF binaries,
> use this to collect the necessary dependencies when constructing
> a root filesystem for the tests.
>
>
> Diffs
> -----
>
> src/tests/containerizer/rootfs.hpp 9648638c1d4b1f1c49708e058e9b930c5ae32138
> src/tests/containerizer/rootfs.cpp f11f126fa4f3130b6778bfe7e7d0f025a638f06e
>
> Diff: https://reviews.apache.org/r/53791/diff/
>
>
> Testing
> -------
>
> sudo make check (Fedora 24)
>
>
> Thanks,
>
> James Peach
>
>
Re: Review Request 53791: Use the stout ELF parser to collect Linux
rootfs files.
Posted by Mesos Reviewbot <re...@mesos.apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/53791/#review164013
-----------------------------------------------------------
Patch looks great!
Reviews applied: [53790, 54712, 54878, 53791]
Passed command: export OS='ubuntu:14.04' BUILDTOOL='autotools' COMPILER='gcc' CONFIGURATION='--verbose' ENVIRONMENT='GLOG_v=1 MESOS_VERBOSE=1'; ./support/docker-build.sh
- Mesos Reviewbot
On Feb. 2, 2017, 5:32 p.m., James Peach wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/53791/
> -----------------------------------------------------------
>
> (Updated Feb. 2, 2017, 5:32 p.m.)
>
>
> Review request for mesos, Gilbert Song, Jie Yu, Kevin Klues, and Jiang Yan Xu.
>
>
> Bugs: MESOS-6588
> https://issues.apache.org/jira/browse/MESOS-6588
>
>
> Repository: mesos
>
>
> Description
> -------
>
> The dependencies for the programs we need in the Linux root
> filesystem vary over time and across distributions. Since Stout
> has support for parsing the library dependencies of ELF binaries,
> use this to collect the necessary dependencies when constructing
> a root filesystem for the tests.
>
>
> Diffs
> -----
>
> src/tests/containerizer/rootfs.hpp 9648638c1d4b1f1c49708e058e9b930c5ae32138
> src/tests/containerizer/rootfs.cpp f11f126fa4f3130b6778bfe7e7d0f025a638f06e
>
> Diff: https://reviews.apache.org/r/53791/diff/
>
>
> Testing
> -------
>
> sudo make check (Fedora 24)
>
>
> Thanks,
>
> James Peach
>
>
Re: Review Request 53791: Use the stout ELF parser to collect Linux
rootfs files.
Posted by James Peach <jp...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/53791/
-----------------------------------------------------------
(Updated Feb. 2, 2017, 5:32 p.m.)
Review request for mesos, Gilbert Song, Jie Yu, Kevin Klues, and Jiang Yan Xu.
Changes
-------
Addressed review feedback.
Bugs: MESOS-6588
https://issues.apache.org/jira/browse/MESOS-6588
Repository: mesos
Description
-------
The dependencies for the programs we need in the Linux root
filesystem vary over time and across distributions. Since Stout
has support for parsing the library dependencies of ELF binaries,
use this to collect the necessary dependencies when constructing
a root filesystem for the tests.
Diffs (updated)
-----
src/tests/containerizer/rootfs.hpp 9648638c1d4b1f1c49708e058e9b930c5ae32138
src/tests/containerizer/rootfs.cpp f11f126fa4f3130b6778bfe7e7d0f025a638f06e
Diff: https://reviews.apache.org/r/53791/diff/
Testing
-------
sudo make check (Fedora 24)
Thanks,
James Peach
Re: Review Request 53791: Use the stout ELF parser to collect Linux
rootfs files.
Posted by James Peach <jp...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/53791/
-----------------------------------------------------------
(Updated Feb. 1, 2017, 11:51 p.m.)
Review request for mesos, Gilbert Song, Jie Yu, Kevin Klues, and Jiang Yan Xu.
Changes
-------
Rebase and address review feedback.
Bugs: MESOS-6588
https://issues.apache.org/jira/browse/MESOS-6588
Repository: mesos
Description
-------
The dependencies for the programs we need in the Linux root
filesystem vary over time and across distributions. Since Stout
has support for parsing the library dependencies of ELF binaries,
use this to collect the necessary dependencies when constructing
a root filesystem for the tests.
Diffs (updated)
-----
src/tests/containerizer/rootfs.hpp 9648638c1d4b1f1c49708e058e9b930c5ae32138
src/tests/containerizer/rootfs.cpp f11f126fa4f3130b6778bfe7e7d0f025a638f06e
Diff: https://reviews.apache.org/r/53791/diff/
Testing
-------
sudo make check (Fedora 24)
Thanks,
James Peach
Re: Review Request 53791: Use the stout ELF parser to collect Linux
rootfs files.
Posted by Jiang Yan Xu <xu...@apple.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/53791/#review163720
-----------------------------------------------------------
src/tests/containerizer/rootfs.hpp (line 21)
<https://reviews.apache.org/r/53791/#comment235203>
We have mixed instances of sentences with and without periods following `#error` but in general we omit the final period "if it's code" so remove it to follow the general rule? We can clean up the other inconsistent instances.
src/tests/containerizer/rootfs.hpp (line 39)
<https://reviews.apache.org/r/53791/#comment235235>
The word `directory` is still in the comment but you removed the logic that adds directories from the implementation?
src/tests/containerizer/rootfs.hpp (line 42)
<https://reviews.apache.org/r/53791/#comment235208>
Why remove `const &`?
src/tests/containerizer/rootfs.hpp (line 50)
<https://reviews.apache.org/r/53791/#comment235206>
s/copyPath/copy/? `path` already appears in the argument so no need to duplicate?
src/tests/containerizer/rootfs.cpp (lines 21 - 25)
<https://reviews.apache.org/r/53791/#comment235205>
We generally order things this way:
```
#include <stout/error.hpp>
#include <stout/foreach.hpp>
#include <stout/os.hpp>
#include <stout/strings.hpp>
#include <stout/os/stat.hpp>
```
The stuff with three-level directory structure are put below the ones with two.
src/tests/containerizer/rootfs.cpp (line 46)
<https://reviews.apache.org/r/53791/#comment235207>
Why use a different argument name `file` as opposed to `path` in the declaration?
src/tests/containerizer/rootfs.cpp (line 52)
<https://reviews.apache.org/r/53791/#comment235209>
Can we maintain a variable different from the input arugment for this?
src/tests/containerizer/rootfs.cpp (lines 55 - 57)
<https://reviews.apache.org/r/53791/#comment235213>
I can see the awkward situtation we are in: we call a helper method and the errno is hidden by it. Now we have to break the encapsulation and reconstruct it...
However it's probably best to avoid it if possible. In this case, I feel
```
Error("Failed to resolve '" + file + "');
```
already provides enough information so stringifying `ENOENT` doesn't buy us much?
src/tests/containerizer/rootfs.cpp (lines 61 - 63)
<https://reviews.apache.org/r/53791/#comment235216>
This fits in one line?
```
return Error("Failed to resolve '" + file + "': " + realpath.error());
```
If it didn't:
```
return Error(
"Failed to resolve '" + file + "': " + realpath.error());
```
In general, we break down further when it's mutliple arguments one per each line, like
```
Error(
arg1,
arg2,
arg3);
```
This is string continuation.
src/tests/containerizer/rootfs.cpp (line 62)
<https://reviews.apache.org/r/53791/#comment235214>
s/' :/':/
src/tests/containerizer/rootfs.cpp (line 66)
<https://reviews.apache.org/r/53791/#comment235218>
s/result/Try<Nothing> copy/?
We don't really need a method-level `result`. We already short-circuit whenever any error occurs. We don't need to save it.
src/tests/containerizer/rootfs.cpp (line 73)
<https://reviews.apache.org/r/53791/#comment235220>
Seems that `realpath()` leads to the eventual "real" path even with multiple symlinks like "link2 -> link1 -> real"? We don't need an iterative algorithm?
A related question is, if so, with "link2 -> link1 -> real", how do we copy `link1` over?
src/tests/containerizer/rootfs.cpp (line 87)
<https://reviews.apache.org/r/53791/#comment235238>
s/std::string/string/ here and elsewhere in this file?
src/tests/containerizer/rootfs.cpp (line 90)
<https://reviews.apache.org/r/53791/#comment235221>
I would still prefer a simple sentense "Path doesn't exist" so we don't break the encapsulation of `os::exists()`. I know it's a simple helper but it's a good principle IMO to just interpret what the API gives you. If the API is inadequate, we can change it.
src/tests/containerizer/rootfs.cpp
<https://reviews.apache.org/r/53791/#comment235224>
Why are we getting rid of the ability to add directories?
src/tests/containerizer/rootfs.cpp (line 114)
<https://reviews.apache.org/r/53791/#comment235237>
Indentation: the original looks better to me. This feels as if we were continuing the `if` statement and not the method call inside.
src/tests/containerizer/rootfs.cpp (line 147)
<https://reviews.apache.org/r/53791/#comment235226>
s/needed/files/ and use `|=` below?
src/tests/containerizer/rootfs.cpp (line 149)
<https://reviews.apache.org/r/53791/#comment235225>
s/foreach/foreach /
src/tests/containerizer/rootfs.cpp (line 157)
<https://reviews.apache.org/r/53791/#comment235233>
Use `files` here?
```
files |= dependencies.get();
```
src/tests/containerizer/rootfs.cpp (line 167)
<https://reviews.apache.org/r/53791/#comment235231>
I can see that you want to distinguish programs from non-programs but programs are also files? How about s/files/configs/?
src/tests/containerizer/rootfs.cpp (lines 171 - 176)
<https://reviews.apache.org/r/53791/#comment235232>
Can we avoid having a duplicate code block by just process the union together? i.e.,
```
files |= configs;
foreach (const string& file, files) {} // Move down the identical block from above.
```
- Jiang Yan Xu
On Dec. 19, 2016, 3:24 p.m., James Peach wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/53791/
> -----------------------------------------------------------
>
> (Updated Dec. 19, 2016, 3:24 p.m.)
>
>
> Review request for mesos, Gilbert Song, Jie Yu, Kevin Klues, and Jiang Yan Xu.
>
>
> Bugs: MESOS-6588
> https://issues.apache.org/jira/browse/MESOS-6588
>
>
> Repository: mesos
>
>
> Description
> -------
>
> The dependencies for the programs we need in the Linux root
> filesystem vary over time and across distributions. Since Stout
> has support for parsing the library dependencies of ELF binaries,
> use this to collect the necessary dependencies when constructing
> a root filesystem for the tests.
>
>
> Diffs
> -----
>
> src/Makefile.am 0f62ec70816e8b48e19d35036285656a6e7cd02b
> src/tests/containerizer/rootfs.hpp 6bc3835cbb62536ec933ef38c9e15138b8611e5f
> src/tests/containerizer/rootfs.cpp PRE-CREATION
>
> Diff: https://reviews.apache.org/r/53791/diff/
>
>
> Testing
> -------
>
> sudo make check (Fedora 24)
>
>
> Thanks,
>
> James Peach
>
>
Re: Review Request 53791: Use the stout ELF parser to collect Linux
rootfs files.
Posted by Mesos ReviewBot <re...@mesos.apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/53791/#review159711
-----------------------------------------------------------
Patch looks great!
Reviews applied: [53790, 54712, 54878, 53791]
Passed command: export OS='ubuntu:14.04' BUILDTOOL='autotools' COMPILER='gcc' CONFIGURATION='--verbose' ENVIRONMENT='GLOG_v=1 MESOS_VERBOSE=1'; ./support/docker_build.sh
- Mesos ReviewBot
On Dec. 19, 2016, 11:24 p.m., James Peach wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/53791/
> -----------------------------------------------------------
>
> (Updated Dec. 19, 2016, 11:24 p.m.)
>
>
> Review request for mesos, Gilbert Song, Jie Yu, Kevin Klues, and Jiang Yan Xu.
>
>
> Bugs: MESOS-6588
> https://issues.apache.org/jira/browse/MESOS-6588
>
>
> Repository: mesos
>
>
> Description
> -------
>
> The dependencies for the programs we need in the Linux root
> filesystem vary over time and across distributions. Since Stout
> has support for parsing the library dependencies of ELF binaries,
> use this to collect the necessary dependencies when constructing
> a root filesystem for the tests.
>
>
> Diffs
> -----
>
> src/Makefile.am 0f62ec70816e8b48e19d35036285656a6e7cd02b
> src/tests/containerizer/rootfs.hpp 6bc3835cbb62536ec933ef38c9e15138b8611e5f
> src/tests/containerizer/rootfs.cpp PRE-CREATION
>
> Diff: https://reviews.apache.org/r/53791/diff/
>
>
> Testing
> -------
>
> sudo make check (Fedora 24)
>
>
> Thanks,
>
> James Peach
>
>
Re: Review Request 53791: Use the stout ELF parser to collect Linux
rootfs files.
Posted by James Peach <jp...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/53791/
-----------------------------------------------------------
(Updated Dec. 19, 2016, 11:24 p.m.)
Review request for mesos, Gilbert Song, Jie Yu, Kevin Klues, and Jiang Yan Xu.
Changes
-------
Rebased and addressed review feedback.
Bugs: MESOS-6588
https://issues.apache.org/jira/browse/MESOS-6588
Repository: mesos
Description
-------
The dependencies for the programs we need in the Linux root
filesystem vary over time and across distributions. Since Stout
has support for parsing the library dependencies of ELF binaries,
use this to collect the necessary dependencies when constructing
a root filesystem for the tests.
Diffs (updated)
-----
src/Makefile.am 0f62ec70816e8b48e19d35036285656a6e7cd02b
src/tests/containerizer/rootfs.hpp 6bc3835cbb62536ec933ef38c9e15138b8611e5f
src/tests/containerizer/rootfs.cpp PRE-CREATION
Diff: https://reviews.apache.org/r/53791/diff/
Testing
-------
sudo make check (Fedora 24)
Thanks,
James Peach
Re: Review Request 53791: Use the stout ELF parser to collect Linux
rootfs files.
Posted by James Peach <jp...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/53791/#review159025
-----------------------------------------------------------
src/linux/ldd.cpp (line 37)
<https://reviews.apache.org/r/53791/#comment229954>
IMHO this code is fairly obvious as is. We already do unnecessary parsing and copying in the rootfs caller, I don't think we should also make things worse here too.
src/tests/containerizer/rootfs.cpp (line 136)
<https://reviews.apache.org/r/53791/#comment229958>
I think that a vector is obvious and a set doesn't make any improvement.
src/tests/containerizer/rootfs.cpp (line 145)
<https://reviews.apache.org/r/53791/#comment229959>
AFAICT 2 is correct.
src/tests/containerizer/rootfs.cpp (line 175)
<https://reviews.apache.org/r/53791/#comment229960>
Not AFAICT :)
- James Peach
On Dec. 13, 2016, 6:34 p.m., James Peach wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/53791/
> -----------------------------------------------------------
>
> (Updated Dec. 13, 2016, 6:34 p.m.)
>
>
> Review request for mesos, Gilbert Song, Jie Yu, Kevin Klues, and Jiang Yan Xu.
>
>
> Bugs: MESOS-6588
> https://issues.apache.org/jira/browse/MESOS-6588
>
>
> Repository: mesos
>
>
> Description
> -------
>
> The dependencies for the programs we need in the Linux root
> filesystem vary over time and across distributions. Since Stout
> has support for parsing the library dependencies of ELF binaries,
> use this to collect the necessary dependencies when constructing
> a root filesystem for the tests.
>
>
> Diffs
> -----
>
> src/Makefile.am a4c03c2b918816e6dd8872d37e5208f055619c47
> src/tests/containerizer/rootfs.hpp 6bc3835cbb62536ec933ef38c9e15138b8611e5f
> src/tests/containerizer/rootfs.cpp PRE-CREATION
>
> Diff: https://reviews.apache.org/r/53791/diff/
>
>
> Testing
> -------
>
> sudo make check (Fedora 24)
>
>
> Thanks,
>
> James Peach
>
>
Re: Review Request 53791: Use the stout ELF parser to collect Linux
rootfs files.
Posted by Mesos ReviewBot <re...@mesos.apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/53791/#review159117
-----------------------------------------------------------
Patch looks great!
Reviews applied: [53790, 54712, 53791]
Passed command: export OS='ubuntu:14.04' BUILDTOOL='autotools' COMPILER='gcc' CONFIGURATION='--verbose' ENVIRONMENT='GLOG_v=1 MESOS_VERBOSE=1'; ./support/docker_build.sh
- Mesos ReviewBot
On Dec. 13, 2016, 6:34 p.m., James Peach wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/53791/
> -----------------------------------------------------------
>
> (Updated Dec. 13, 2016, 6:34 p.m.)
>
>
> Review request for mesos, Gilbert Song, Jie Yu, Kevin Klues, and Jiang Yan Xu.
>
>
> Bugs: MESOS-6588
> https://issues.apache.org/jira/browse/MESOS-6588
>
>
> Repository: mesos
>
>
> Description
> -------
>
> The dependencies for the programs we need in the Linux root
> filesystem vary over time and across distributions. Since Stout
> has support for parsing the library dependencies of ELF binaries,
> use this to collect the necessary dependencies when constructing
> a root filesystem for the tests.
>
>
> Diffs
> -----
>
> src/Makefile.am a4c03c2b918816e6dd8872d37e5208f055619c47
> src/tests/containerizer/rootfs.hpp 6bc3835cbb62536ec933ef38c9e15138b8611e5f
> src/tests/containerizer/rootfs.cpp PRE-CREATION
>
> Diff: https://reviews.apache.org/r/53791/diff/
>
>
> Testing
> -------
>
> sudo make check (Fedora 24)
>
>
> Thanks,
>
> James Peach
>
>
Re: Review Request 53791: Use the stout ELF parser to collect Linux
rootfs files.
Posted by James Peach <jp...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/53791/
-----------------------------------------------------------
(Updated Dec. 13, 2016, 6:34 p.m.)
Review request for mesos, Gilbert Song, Jie Yu, Kevin Klues, and Jiang Yan Xu.
Bugs: MESOS-6588
https://issues.apache.org/jira/browse/MESOS-6588
Repository: mesos
Description
-------
The dependencies for the programs we need in the Linux root
filesystem vary over time and across distributions. Since Stout
has support for parsing the library dependencies of ELF binaries,
use this to collect the necessary dependencies when constructing
a root filesystem for the tests.
Diffs (updated)
-----
src/Makefile.am a4c03c2b918816e6dd8872d37e5208f055619c47
src/tests/containerizer/rootfs.hpp 6bc3835cbb62536ec933ef38c9e15138b8611e5f
src/tests/containerizer/rootfs.cpp PRE-CREATION
Diff: https://reviews.apache.org/r/53791/diff/
Testing
-------
sudo make check (Fedora 24)
Thanks,
James Peach
Re: Review Request 53791: Use the stout ELF parser to collect Linux
rootfs files.
Posted by James Peach <jp...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/53791/
-----------------------------------------------------------
(Updated Nov. 29, 2016, 1:21 a.m.)
Review request for mesos, Gilbert Song, Jie Yu, Kevin Klues, and Jiang Yan Xu.
Bugs: MESOS-6588
https://issues.apache.org/jira/browse/MESOS-6588
Repository: mesos
Description
-------
The dependencies for the programs we need in the Linux root
filesystem vary over time and across distributions. Since Stout
has support for parsing the library dependencies of ELF binaries,
use this to collect the necessary dependencies when constructing
a root filesystem for the tests.
Diffs (updated)
-----
src/CMakeLists.txt 0884c6bb6f2ebddadc5cfc6b72d704086709748d
src/Makefile.am 85eda538caf39f81f052896e744b7b0c724f81bb
src/linux/ldd.hpp PRE-CREATION
src/linux/ldd.cpp PRE-CREATION
src/tests/containerizer/rootfs.hpp 6bc3835cbb62536ec933ef38c9e15138b8611e5f
src/tests/containerizer/rootfs.cpp PRE-CREATION
Diff: https://reviews.apache.org/r/53791/diff/
Testing
-------
sudo make check (Fedora 24)
Thanks,
James Peach