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