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