----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/53791/#review156259 -----------------------------------------------------------
src/tests/containerizer/rootfs.cpp (line 58) <https://reviews.apache.org/r/53791/#comment226461> 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) <https://reviews.apache.org/r/53791/#comment226462> This feels like an intermediate method for recursion. Would be nice to put a front-end method above it: ``` Try<hashset<string>> ldd(const string& path); ``` src/tests/containerizer/rootfs.cpp (line 61) <https://reviews.apache.org/r/53791/#comment226463> 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) <https://reviews.apache.org/r/53791/#comment226459> It feels like it's not worth extracting out the logic (similar code from volume.cpp) ``` Try<elf::File*> load = elf::File::load(realpath.get()); if (load.isError()) { return Error("Failed to elf::File::load '" + realpath.get() + "':" " " + load.error()); } Owned<elf::File> file(load.get()); ``` into a 10-line helper method and spend 4 lines here? src/tests/containerizer/rootfs.cpp (line 73) <https://reviews.apache.org/r/53791/#comment226460> Turn it into a hashset for easier checking? src/tests/containerizer/rootfs.cpp (lines 81 - 84) <https://reviews.apache.org/r/53791/#comment226458> 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) <https://reviews.apache.org/r/53791/#comment226457> No space. src/tests/containerizer/rootfs.cpp (line 107) <https://reviews.apache.org/r/53791/#comment226466> 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) <https://reviews.apache.org/r/53791/#comment226438> The following is more widely used. ``` #ifdef __linux__ ``` src/tests/containerizer/rootfs.cpp (lines 196 - 201) <https://reviews.apache.org/r/53791/#comment226464> The whole file should just be linux only so we don't have to do it here. src/tests/containerizer/rootfs.cpp (line 223) <https://reviews.apache.org/r/53791/#comment226465> Would the following look better? ``` hashset<string> needed(programs.begin(), programs.end()); foreach (const string& program, programs) { Try<hashset<string>> 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 > >
