Re: Review Request 53791: Use the stout ELF parser to collect Linux rootfs files.

[Jiang Yan Xu]

---
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.

[Mesos Reviewbot]

---
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.

[James Peach]

---
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.

[Mesos Reviewbot]

---
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.

[Jiang Yan Xu]

> On Feb. 1, 2017, 3:51 p.m., James Peach wrote:
> > src/tests/containerizer/rootfs.cpp, line 90
> > 
> >
> > 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 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.

[Jiang Yan Xu]

> 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
> > 
> >
> > `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
> > 
> >
> > 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 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.

[James Peach]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/53791/#review163834
---




src/tests/containerizer/rootfs.hpp (line 50)


`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)


It's better to format consistently than to micro-manage line lengths.



src/tests/containerizer/rootfs.cpp (line 90)


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.

[James Peach]

---
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.

[Jiang Yan Xu]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/53791/#review163720
---




src/tests/containerizer/rootfs.hpp (line 21)


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)


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)


Why remove `const &`?



src/tests/containerizer/rootfs.hpp (line 50)


s/copyPath/copy/? `path` already appears in the argument so no need to 
duplicate?



src/tests/containerizer/rootfs.cpp (lines 21 - 25)


We generally order things this way:

```
#include 
#include 
#include 
#include 

#include 
```

The stuff with three-level directory structure are put below the ones with 
two.



src/tests/containerizer/rootfs.cpp (line 46)


Why use a different argument name `file` as opposed to `path` in the 
declaration?



src/tests/containerizer/rootfs.cpp (line 52)


Can we maintain a variable different from the input arugment for this?



src/tests/containerizer/rootfs.cpp (lines 55 - 57)


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)


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)


s/' :/':/



src/tests/containerizer/rootfs.cpp (line 66)


s/result/Try 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)


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)


s/std::string/string/ here and elsewhere in this file?



src/tests/containerizer/rootfs.cpp (line 90)


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 


Why are we getting rid of the ability to add directories?



src/tests/containerizer/rootfs.cpp (line 114)


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)


s/needed/files/ and use `|=` below?



src/tests/containerizer/rootfs.cpp (line 149)


s/foreach/foreach /



src/tests/containerizer/rootfs.cpp (line 157)


Use `files` here?

```
files |= dependencies.get();
```

Re: Review Request 53791: Use the stout ELF parser to collect Linux rootfs files.

[Mesos ReviewBot]

---
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.

[James Peach]

---
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.

[Mesos ReviewBot]

---
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.

[Benjamin Bannier]

> On Dec. 13, 2016, 11:13 a.m., Benjamin Bannier wrote:
> > src/linux/ldd.cpp, lines 82-83
> > 
> >
> > 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.

[James Peach]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/53791/#review159025
---




src/linux/ldd.cpp (line 37)


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)


I think that a vector is obvious and a set doesn't make any improvement.



src/tests/containerizer/rootfs.cpp (line 145)


AFAICT 2 is correct.



src/tests/containerizer/rootfs.cpp (line 175)


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.

[James Peach]

---
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.

[James Peach]

> On Dec. 13, 2016, 10:13 a.m., Benjamin Bannier wrote:
> > src/linux/ldd.cpp, line 32
> > 
> >
> > 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
> > 
> >
> > 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
> > 
> >
> > Let's not use an out parameter, but instead return a `Try > 
> > 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
> > 
> >
> > 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
> > 
> >
> > 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
> > 
> >
> > 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.

[Benjamin Bannier]

---
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)


Please add includes for `string` and `vector`.



src/linux/ldd.cpp (line 32)


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)


Let's not use an out parameter, but instead return a `Try

`load.isError()`



src/linux/ldd.cpp (line 53)


nit: 4 spaces.



src/linux/ldd.cpp (line 54)


`dependencies.isError()`



src/linux/ldd.cpp (line 58)


nit: space after `foreach`.



src/linux/ldd.cpp (lines 82 - 83)


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)


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)


We also need to check for `realpath.isNone()`.



src/tests/containerizer/rootfs.cpp (lines 64 - 70)


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)


+1!



src/tests/containerizer/rootfs.cpp (line 92)


nit: Let's break this line after `Error('.



src/tests/containerizer/rootfs.cpp (line 136)


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)


nit: Not yours, but should be 4 spaces.



src/tests/containerizer/rootfs.cpp (line 144)


Could we move this right above the loop using it?



src/tests/containerizer/rootfs.cpp (line 145)


nit: Not yours, but should be 4 spaces.



src/tests/containerizer/rootfs.cpp (line 153)


Let's break this line after `Error(`.



src/tests/containerizer/rootfs.cpp (line 175)


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 

Re: Review Request 53791: Use the stout ELF parser to collect Linux rootfs files.

[James Peach]

---
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

Re: Review Request 53791: Use the stout ELF parser to collect Linux rootfs files.

[Kevin Klues]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/53791/#review156407
---




src/Makefile.am (line 1042)


I think the abstraction of creating an `ldd.cpp` file and its 
implementation should probably pulled out into its own isolated commit.


- Kevin Klues


On Nov. 18, 2016, 10:53 p.m., James Peach wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/53791/
> ---
> 
> (Updated Nov. 18, 2016, 10:53 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 5a47c93388234a68c3c486a021ccdbe3213c5bac 
>   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.

[Kevin Klues]

> On Nov. 18, 2016, 1:24 a.m., Jiang Yan Xu wrote:
> > src/tests/containerizer/rootfs.cpp, line 58
> > 
> >
> > This feels like an implementation of `ldd()` that coule be generally 
> > useful.
> > 
> > Would it make sense to rename ldcache.hpp|cpp as ld.hpp|cpp and add 
> > `ldd()` as a method in ld.hpp|cpp make sense?

I think the implementation in `ldcache.cpp` is complicated enough that it 
probably warrants keeping it in its own file (especially given the massive 
comment block I had to put at the top to explain what is going on). That said, 
I wouldn't be opposed to renaming `ldcache.hpp` to `ldd.hpp` and adding an 
`ldd` call in there.


> On Nov. 18, 2016, 1:24 a.m., Jiang Yan Xu wrote:
> > src/tests/containerizer/rootfs.cpp, lines 81-84
> > 
> >
> > If we have to do this, format it this way
> > 
> > ```
> > auto entry = std::find_if(
> > cache.begin(),
> > cache.end(),
> > [dependency](const ldcache::Entry& e) {
> >   return strings::startsWith(e.name, dependency);
> > });
> > ```
> > 
> > But is prefix matching necessary? Can't we do extract matching 
> > (i.e.,`hashset.contains()`)?
> > 
> > Note that our seed list are just programs without versions.
> 
> James Peach wrote:
> I checked that you don't need the `startsWith`. The ld cache is a vector 
> so I still need to do the linear search.

Does it make sense to make the ldcache a `set` instead of a vector?


- Kevin


---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/53791/#review156259
---


On Nov. 18, 2016, 10:53 p.m., James Peach wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/53791/
> ---
> 
> (Updated Nov. 18, 2016, 10:53 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 5a47c93388234a68c3c486a021ccdbe3213c5bac 
>   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.

[Mesos ReviewBot]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/53791/#review156374
---



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. 18, 2016, 10:53 p.m., James Peach wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/53791/
> ---
> 
> (Updated Nov. 18, 2016, 10:53 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 5a47c93388234a68c3c486a021ccdbe3213c5bac 
>   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.

[James Peach]

> On Nov. 18, 2016, 1:24 a.m., Jiang Yan Xu wrote:
> > src/tests/containerizer/rootfs.cpp, lines 81-84
> > 
> >
> > If we have to do this, format it this way
> > 
> > ```
> > auto entry = std::find_if(
> > cache.begin(),
> > cache.end(),
> > [dependency](const ldcache::Entry& e) {
> >   return strings::startsWith(e.name, dependency);
> > });
> > ```
> > 
> > But is prefix matching necessary? Can't we do extract matching 
> > (i.e.,`hashset.contains()`)?
> > 
> > Note that our seed list are just programs without versions.

I checked that you don't need the `startsWith`. The ld cache is a vector so I 
still need to do the linear search.


- James


---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/53791/#review156259
---


On Nov. 18, 2016, 7:23 p.m., James Peach wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/53791/
> ---
> 
> (Updated Nov. 18, 2016, 7:23 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.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.

[James Peach]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/53791/
---

(Updated Nov. 18, 2016, 7:23 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/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.

[James Peach]

> On Nov. 18, 2016, 1:24 a.m., Jiang Yan Xu wrote:
> > src/tests/containerizer/rootfs.cpp, line 234
> > 
> >
> > Would the following look better?
> > 
> > ```
> > hashset needed(programs.begin(), programs.end());
> > 
> > foreach (const string& program, programs) {
> >   Try dependencies = ldd(file);
> > 
> >   // error handling.
> > 
> >   needed = needed | dependencies;
> > }
> > ```

We add the dependencies depth first, so initializing the needed set with the 
programs will make us think that we have already collected all the dependencies.


> On Nov. 18, 2016, 1:24 a.m., Jiang Yan Xu wrote:
> > src/tests/containerizer/rootfs.cpp, line 107
> > 
> >
> > Not convinced that we need to extract it out since it's used only in a 
> > single place?

It's used in 2 places.


> On Nov. 18, 2016, 1:24 a.m., Jiang Yan Xu wrote:
> > src/tests/containerizer/rootfs.cpp, lines 69-71
> > 
> >
> > It feels like it's not worth extracting out the logic
> > 
> > (similar code from volume.cpp)
> > ```
> > Try load = elf::File::load(realpath.get());
> > if (load.isError()) {
> >   return Error("Failed to elf::File::load '" + realpath.get() + 
> > "':"
> >" " + load.error());
> > }
> > 
> > Owned file(load.get());
> > ```
> > 
> > into a 10-line helper method and spend 4 lines here?

Yes I saw the usage in `volume.cpp` and that seemed awkward to have additional 
temporaries just to get the same pointer as an `Owned` pointer. I prefered this 
more verbose approach, though I don't mind switching back if you prefer the 
extra temporaries.


- James


---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/53791/#review156259
---


On Nov. 15, 2016, 11:34 p.m., James Peach wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/53791/
> ---
> 
> (Updated Nov. 15, 2016, 11: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/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.

[Jiang Yan Xu]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/53791/#review156259
---




src/tests/containerizer/rootfs.cpp (line 58)


This feels like an implementation of `ldd()` that coule be generally useful.

Would it make sense to rename ldcache.hpp|cpp as ld.hpp|cpp and add `ldd()` 
as a method in ld.hpp|cpp make sense?



src/tests/containerizer/rootfs.cpp (lines 58 - 61)


This feels like an intermediate method for recursion. Would be nice to put 
a front-end method above it:

```
Try ldd(const string& path);
```



src/tests/containerizer/rootfs.cpp (line 61)


Using `hashset` would make it more explicit that no order is implied 
(otherwise this has to be a vector).

And we can use `contains()`!



src/tests/containerizer/rootfs.cpp (lines 69 - 71)


It feels like it's not worth extracting out the logic

(similar code from volume.cpp)
```
Try load = elf::File::load(realpath.get());
if (load.isError()) {
  return Error("Failed to elf::File::load '" + realpath.get() + "':"
   " " + load.error());
}

Owned file(load.get());
```

into a 10-line helper method and spend 4 lines here?



src/tests/containerizer/rootfs.cpp (line 73)


Turn it into a hashset for easier checking?



src/tests/containerizer/rootfs.cpp (lines 81 - 84)


If we have to do this, format it this way

```
auto entry = std::find_if(
cache.begin(),
cache.end(),
[dependency](const ldcache::Entry& e) {
  return strings::startsWith(e.name, dependency);
});
```

But is prefix matching necessary? Can't we do extract matching 
(i.e.,`hashset.contains()`)?

Note that our seed list are just programs without versions.



src/tests/containerizer/rootfs.cpp (line 82)


No space.



src/tests/containerizer/rootfs.cpp (line 107)


Not convinced that we need to extract it out since it's used only in a 
single place?



src/tests/containerizer/rootfs.cpp (line 196)


The following is more widely used.

```
#ifdef __linux__
```



src/tests/containerizer/rootfs.cpp (lines 196 - 201)


The whole file should just be linux only so we don't have to do it here.



src/tests/containerizer/rootfs.cpp (line 223)


Would the following look better?

```
hashset needed(programs.begin(), programs.end());

foreach (const string& program, programs) {
  Try dependencies = ldd(file);

  // error handling.

  needed = needed | dependencies;
}
```


- Jiang Yan Xu


On Nov. 15, 2016, 3:34 p.m., James Peach wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/53791/
> ---
> 
> (Updated Nov. 15, 2016, 3: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/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.

[Mesos ReviewBot]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/53791/#review156100
---



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. 15, 2016, 11:34 p.m., James Peach wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/53791/
> ---
> 
> (Updated Nov. 15, 2016, 11: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/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.

[James Peach]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/53791/
---

(Updated Nov. 15, 2016, 6:28 p.m.)


Review request for mesos 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.cpp PRE-CREATION 

Diff: https://reviews.apache.org/r/53791/diff/


Testing
---

sudo make check (Fedora 24)


Thanks,

James Peach

Review Request 53791: Use the stout ELF parser to collect Linux rootfs files.

[James Peach]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/53791/
---

Review request for mesos 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.cpp PRE-CREATION 

Diff: https://reviews.apache.org/r/53791/diff/


Testing
---

sudo make check (Fedora 24)


Thanks,

James Peach