> On Nov. 18, 2016, 1:24 a.m., Jiang Yan Xu wrote: > > src/tests/containerizer/rootfs.cpp, line 234 > > <https://reviews.apache.org/r/53791/diff/2/?file=1564519#file1564519line234> > > > > 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; > > } > > ```
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 > > <https://reviews.apache.org/r/53791/diff/2/?file=1564519#file1564519line107> > > > > 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 > > <https://reviews.apache.org/r/53791/diff/2/?file=1564519#file1564519line69> > > > > 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? 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 > >
