----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/47374/#review137245 -----------------------------------------------------------
Overall, I am not convinced this is reaching the goals we have just yet... Let me try to specify our goals; (a) we want to enable mesos-modules (tests) to reuse our test helpers/utils - things like `cluster.cpp` etc. (b) we want to make sure that everything used within the modules is actually publicly exposed to prevent creating dependencies on internal stuff that is not going through deprecations (c) we try to be least disruptive on mesos a. reached b. not reached as we do not expose the headers - a module test using that library will still have to tap into the non public mesos folders to get to the needed headers c. reached - this patch only changes a makefile So (b) is the culprit and solving it properly will likely require us to do some serious refactoring of those headers (e.g. `src/tests/mesos.hpp`) to have a clean cut between stuff we want to expose and things we can keep internal to mesos. Having public test helpers available for module developers is great and very much needed - but I also believe that it might need much more work. src/Makefile.am (line 1930) <https://reviews.apache.org/r/47374/#comment202406> Why would we want to install this static library together with our tests? The installation of tests was meant as provisioning tests; to make sure the host system can successfully run mesos which is validated by running mesos-tests without having to build them on that host. src/Makefile.am (line 1933) <https://reviews.apache.org/r/47374/#comment202407> I would propose to move this out of the if-clause and make it a general `noinst`. src/Makefile.am (line 1963) <https://reviews.apache.org/r/47374/#comment202411> Not yours but our indentation seems to be inconsistent here. We might want to clean that up in another patch. src/Makefile.am (lines 1971 - 1980) <https://reviews.apache.org/r/47374/#comment202408> Good point, I very much agree. - Till Toenshoff On June 11, 2016, 12:03 a.m., Joseph Wu wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/47374/ > ----------------------------------------------------------- > > (Updated June 11, 2016, 12:03 a.m.) > > > Review request for mesos, Adam B, Artem Harutyunyan, Jie Yu, Kapil Arya, Jan > Schlicht, and Till Toenshoff. > > > Repository: mesos > > > Description > ------- > > This gives external projects easier access to the test helpers used in > mesos tests. > > For example, a module writer may want to write a test like > `src/tests/oversubscription_tests.cpp`. To build and link against > this library, the module writer would mimic the build flags for tests: > ``` > # Main test file is taken directly from Mesos. > my_module_tests_SOURCES = \ > $(MESOS)/src/tests/main.cpp > > my_module_tests_CPPFLAGS = \ > -I$(GMOCK)/include \ > -I$(GTEST)/include \ > -I$(MESOS)/include/mesos \ > -I$(ZOOKEEPER)/include \ > -I$(ZOOKEEPER)/generated \ > $(AM_CPPFLAGS) > > my_module_tests_LDADD = \ > $(MESOS)/3rdparty/.libs/libgmock.la \ > $(MESOS)/src/.libs/libmesos.la \ > $(MESOS)/src/.libs/libmesos_tests.la > ``` > > > Diffs > ----- > > src/Makefile.am ce5245883f3d2661812272702c0d2060513b6d88 > > Diff: https://reviews.apache.org/r/47374/diff/ > > > Testing > ------- > > make check on OSX, CentOS 7, Ubuntu 14 > > > Thanks, > > Joseph Wu > >
