----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/49688/#review141822 -----------------------------------------------------------
cmake/MesosConfigure.cmake (line 94) <https://reviews.apache.org/r/49688/#comment207214> Can you please explain a bit why this moved? I'd like to keep all configuration files for tests in the `CMakeLists.txt` that build those tests, if it is at all possible. Just a few notes on the design, in case the context is helpful for understanding why this is important. stout, libprocess, and libmesos each have a relevant `[ProjectName]Configure.cmake`, and `[ProjectName]TestsConfigure.cmake` scripts. The `[ProjectName]Configure.cmake` are intended to be complete, self-contained configuration scripts that will fully configure any project that wishes to consume those libraries -- all they need to do is to `include([ProjectName]Configure)` in the relevant `CMakeLists.txt`, and they're good to go. In contrast, the `[ProjectName]TestsConfigure.cmake` files are included only in the `CMakeLists.txt` that declares the data needed to build the test target, and nowhere else. The rationale for this really strong split between test and library configuration is to keep the test configuration from "bleeding" into the library configuration. This keeps the build extremely simple, because in general we can know that the test configuration is limited to the `CMakeLists.txt` that builds the test, which greatly limits the applicable domain of error in the build system. Since we can't write tests for the build system, this separation is really important: we need to keep things in the build system really, really simple if we want to be able to keep reasoning about the behavior of the build on multiple platforms. cmake/MesosConfigure.cmake (lines 96 - 97) <https://reviews.apache.org/r/49688/#comment207211> Hmm, this seems like it should break the build. We're defining these scripts in `MesosConfigure.cmake` before we define `MESOS_TARGET`, yet `CONTAINERIZER_TEST_LIBS` lists `MESOS_TARGET` as a dependency, no? So we should fail to emit flags that link to libmesos. Am I missing something obvious? src/tests/CMakeLists.txt (line 17) <https://reviews.apache.org/r/49688/#comment207210> General comment: when we have an `if`, can we please indent the code in the conditional block? src/tests/CMakeLists.txt (line 18) <https://reviews.apache.org/r/49688/#comment207223> The style guide is totally _ad hoc_, but can we please not add extraneous spaces here? src/tests/CMakeLists.txt (line 21) <https://reviews.apache.org/r/49688/#comment207226> It looks like at least some of the following tests are missing, and I don't see them grep'ing the codebase. Rough guess: we should be including these in the relevant test subdirectories? If this is the rationale, can we please add them all now? If this is not the rationale, can we please leave a comment explaining? ``` tests/common/command_utils_tests.cpp \ tests/common/http_tests.cpp \ tests/common/recordio_tests.cpp \ tests/containerizer/composing_containerizer_tests.cpp \ tests/containerizer/docker_containerizer_tests.cpp \ tests/containerizer/docker_spec_tests.cpp \ tests/containerizer/docker_tests.cpp \ tests/containerizer/external_containerizer_test.cpp \ tests/containerizer/isolator_tests.cpp \ tests/containerizer/launcher.cpp \ tests/containerizer/memory_test_helper.cpp \ tests/containerizer/mesos_containerizer_tests.cpp \ tests/containerizer/provisioner_appc_tests.cpp \ tests/containerizer/provisioner_backend_tests.cpp \ tests/containerizer/provisioner_docker_tests.cpp ``` Also, though not a test, the following file also seems to be not listed here: ``` slave/qos_controllers/load.cpp ``` src/tests/CMakeLists.txt (line 35) <https://reviews.apache.org/r/49688/#comment207224> I don't think we have a `HAS_JAVA` option, do we? Does it make sense to add an `option` in the root `CMakeLists.txt`, in the same way we have for the `VERBOSE`, `REBUNDLED`, and `ENABLE_LIBEVENT` options? (If you don't know, an `option` exposes a binary choice to the CMake command line, so you can do something like `cmake .. -DENABLE_LIBEVENT=1`, which would compile Mesos with libevent.) src/tests/CMakeLists.txt (line 36) <https://reviews.apache.org/r/49688/#comment207227> See "please indent the `if` block" comment above. src/tests/CMakeLists.txt (line 43) <https://reviews.apache.org/r/49688/#comment207229> Double space here intentional? src/tests/CMakeLists.txt (line 71) <https://reviews.apache.org/r/49688/#comment207231> Can we please use the Mesos style TODO format? Specifically: * add semicolons after the `TODO(xxx):` * capitalize the first letter of the message * end message with a period * indent to line up with the rest of the block (i.e., put 2 spaces between the start of the line and the `#` character) In this case, it should look something like: ``` TODO(srbrahma): Needs leveldb to compile. ``` Also would be good to have a bug # that corresponds to the issue. src/tests/CMakeLists.txt (line 110) <https://reviews.apache.org/r/49688/#comment207225> Looks like this line is duplicated? src/tests/CMakeLists.txt (line 121) <https://reviews.apache.org/r/49688/#comment207228> See "please indent the `if` block" comment above. src/tests/CMakeLists.txt (line 129) <https://reviews.apache.org/r/49688/#comment207232> In general, we tend to use strongly-emphasized comments to delineate logically separate pieces of `CMakeLists.txt` files. This (IMO) greatly adds to readability, because otherwise, the `CMakeLists.txt` files (which are already normally prone to becoming huge monoliths) are consumable by outsiders just starting with the project. So I have two suggestions: * Re-format this block to look something like what I demonstrate below * Move it to the top (but below the calls to `include(*Configure)` -- I think this subdirectory has no dependencies, right? Putting it at the top makes it much clearer that they are mostly independent. I think (but have not verified) that we need at least a couple of `add_subdirectory` calls when we add all the tests, so this section might look like: ``` # BUILD TESTS IN SUBDIRECTORIES. ################################ add_subdirectory(common) add_subdirectory(containerizer) ... ``` src/tests/CMakeLists.txt (line 156) <https://reviews.apache.org/r/49688/#comment207236> Above this, please put the usual delineating comment, namely: ``` # ADD TEST TARGET (runs when you do, e.g., `make check`). ######################################################### add_test(NAME MesosTests COMMAND ./${MESOS_TESTS_TARGET}) ``` Also we should strictly speaking be updating the `check` target in the root `CMakeLists.txt` but it's so out of disrepair I think it's fine to also not do this. src/tests/cmake/TestsConfigure.cmake (line 1) <https://reviews.apache.org/r/49688/#comment207216> Just looking at this file, it seems that this should probably be combined with `MesosTestsConfigure.cmake`. Thoughts? It might be structured as follow: ``` set(MESOS_TESTS_DEPENDENCIES ${MESOS_TESTS_DEPENDENCIES} ${MESOS_DEPENDENCIES} ${MESOS_TARGET} ${GMOCK_TARGET} ) set(CONTAINERIZER_TEST_DEPENDENCIES ${CONTAINERIZER_TEST_DEPENDENCIES} ${MESOS_TESTS_DEPENDENCIES} ) ``` i.e., we could take the relevant rules and put them next to each other, and make the relevant containerizer variables just depend on the mesos test variables. NOTE: you will probably have to uncomment the `GMOCK_LFLAG` definition in `MesosTestsConfigure.cmake` in order to combine the code in this way. (And if that `MesosTestsConfigure.cmake` seems like a mess, the truth is that file is a pile of technical debt that shoudl be cleaned up soon, by me, since I'm the one who created it in the big death march to Mesos v1. But, for now, these variables do fit in there, at least IMO.) src/tests/cmake/TestsConfigure.cmake (line 17) <https://reviews.apache.org/r/49688/#comment207237> Though this file will likely go away, please in the future be wary of double-`include`ing config files. - Alex Clemmer On July 11, 2016, 3:46 p.m., Srinivas Brahmaroutu wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/49688/ > ----------------------------------------------------------- > > (Updated July 11, 2016, 3:46 p.m.) > > > Review request for mesos, Alex Clemmer and Joseph Wu. > > > Bugs: MESOS-5792 > https://issues.apache.org/jira/browse/MESOS-5792 > > > Repository: mesos > > > Description > ------- > > Added cmake build for mesos tests. > > > Diffs > ----- > > cmake/MesosConfigure.cmake b2318ed8eb4e11de43abfc15f51d12b2c0ff8fa1 > src/tests/CMakeLists.txt 3c530631d22aa1cfdc2c600112059601bba7d6b7 > src/tests/cmake/TestsConfigure.cmake PRE-CREATION > > Diff: https://reviews.apache.org/r/49688/diff/ > > > Testing > ------- > > cmake .. > cmake check > > > Thanks, > > Srinivas Brahmaroutu > >
