----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/64211/#review197748 -----------------------------------------------------------
cmake/CompilationConfigure.cmake Lines 113-118 (patched) <https://reviews.apache.org/r/64211/#comment278030> Can we make this look like the other errors? Here and below. cmake/CompilationConfigure.cmake Lines 113-118 (patched) <https://reviews.apache.org/r/64211/#comment278031> Can we make this look like the other errors? Here and below. cmake/CompilationConfigure.cmake Lines 113-118 (patched) <https://reviews.apache.org/r/64211/#comment278032> Can we make this look like the other errors? Here and below. cmake/CompilationConfigure.cmake Lines 113-118 (patched) <https://reviews.apache.org/r/64211/#comment278033> Can we make this output look like the others below? It might make send to emit a `FATAL_ERROR` like below for consistency. Here and below. cmake/CompilationConfigure.cmake Lines 132-133 (patched) <https://reviews.apache.org/r/64211/#comment278029> This is a little weird as 2.7 is the last release in the python2 series and I think they are pretty committed that that. We could check against `3.0.0` here. cmake/CompilationConfigure.cmake Lines 155 (patched) <https://reviews.apache.org/r/64211/#comment278034> I think after these checks we do not need to introduce an extra variable, but could just use `ENABLE_NEW_CLI` elsewhere (see what we do for e.g., `ENABLE_JAVA`). cmake/MesosConfigure.cmake Lines 64-66 (patched) <https://reviews.apache.org/r/64211/#comment278023> Let's kill these. We are only using the first one of this, and that one can be replaced with `${CMAKE_CURRENT_SOURCE_DIR}` everywhere. src/Makefile.am Lines 1769-1784 (patched) <https://reviews.apache.org/r/64211/#comment278025> Let's chain commands with `&&` instead of just executing them sequentially. That way we exit early if any step fails. src/Makefile.am Lines 1771 (patched) <https://reviews.apache.org/r/64211/#comment278026> Remove the space after `.virtualenv`. src/Makefile.am Lines 1765-1768 (original), 1792-1795 (patched) <https://reviews.apache.org/r/64211/#comment278027> No need to touch this? src/Makefile.am Line 2795 (original), 2823 (patched) <https://reviews.apache.org/r/64211/#comment278028> Let's remove this empty line so we more clearly suggest that his is part of `check-local`. src/python/cli_new/CMakeLists.txt Lines 39 (patched) <https://reviews.apache.org/r/64211/#comment278020> Move to previous line. src/python/cli_new/CMakeLists.txt Lines 45-46 (patched) <https://reviews.apache.org/r/64211/#comment278006> Let's indent these lines by 2 spaces relative to `COMMAND`. src/python/cli_new/CMakeLists.txt Lines 49 (patched) <https://reviews.apache.org/r/64211/#comment278021> Move to previous line. src/python/cli_new/CMakeLists.txt Lines 50 (patched) <https://reviews.apache.org/r/64211/#comment278019> Let's add a custom target wrapping above file creation. This e.g., would prevent any dependent custom command from just including it (works here, but breaks in parallel builds for virtualenv when building `cli`). add_custom_target(cli_hidden_imports DEPENDS cli_hidden_imports.txt) src/python/cli_new/CMakeLists.txt Lines 53 (patched) <https://reviews.apache.org/r/64211/#comment278009> Ninja has issues removing this directory when executing `ninja clean` since the directory is not empty. I'd suggest to just declare `.virtualenv/bin/activate` as only `OUTPUT`. src/python/cli_new/CMakeLists.txt Lines 55 (patched) <https://reviews.apache.org/r/64211/#comment278010> Indent this by spaces. src/python/cli_new/CMakeLists.txt Lines 56 (patched) <https://reviews.apache.org/r/64211/#comment278011> Let's add a custom target wrapping this one. This prevents the build of `cli` from including this which breaks in parallel builds. add_custom_target(cli_virtualenv DEPENDS .virtualenv/bin/activate) src/python/cli_new/CMakeLists.txt Lines 64 (patched) <https://reviews.apache.org/r/64211/#comment278012> Let's start this script with `set -e` so we abort in case of errors. src/python/cli_new/CMakeLists.txt Lines 66 (patched) <https://reviews.apache.org/r/64211/#comment278013> Indent by two spaces. src/python/cli_new/CMakeLists.txt Lines 69-71 (patched) <https://reviews.apache.org/r/64211/#comment278014> Indent by two spaces. src/python/cli_new/CMakeLists.txt Lines 76 (patched) <https://reviews.apache.org/r/64211/#comment278015> Depend on `cli_hidden_imports` and `cli_virtualenv` here. src/python/cli_new/CMakeLists.txt Lines 77 (patched) <https://reviews.apache.org/r/64211/#comment278017> Move to previous line. src/python/cli_new/tests/CMakeLists.txt Lines 23 (patched) <https://reviews.apache.org/r/64211/#comment278016> Let's start this script with `set -e` so we abort in case of errors. src/python/cli_new/tests/CMakeLists.txt Lines 29 (patched) <https://reviews.apache.org/r/64211/#comment278022> Depend on `cli_virtualenv` here. - Benjamin Bannier On Feb. 18, 2018, 4:15 p.m., Armand Grillet wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/64211/ > ----------------------------------------------------------- > > (Updated Feb. 18, 2018, 4:15 p.m.) > > > Review request for mesos, Benjamin Bannier and Kevin Klues. > > > Bugs: MESOS-8240 > https://issues.apache.org/jira/browse/MESOS-8240 > > > Repository: mesos > > > Description > ------- > > An update of the discarded review /r/52543. > > Works with Autotools and CMake. > > > Diffs > ----- > > CMakeLists.txt 6702f02245e3867c06bbd9efbbf4e3b961a7d9aa > cmake/CompilationConfigure.cmake 50cddf9476c8c5196c4824a7b060c2680a96b277 > cmake/MesosConfigure.cmake 0954a9cd31fa290ff9099be4b06d69d96b701f1e > configure.ac 30fbadc32d1d96f719d45fa8067f975283c25507 > docs/configuration/cmake.md 1e34657ea55fa324f65f865f7d0a67084c6719d9 > src/CMakeLists.txt 0c135034982a6a4844509cd0e9b2cc41804333ef > src/Makefile.am a2ba1e6de0868c7a4fe58304ec479dce039bc288 > src/python/cli_new/CMakeLists.txt PRE-CREATION > src/python/cli_new/tests/CMakeLists.txt PRE-CREATION > > > Diff: https://reviews.apache.org/r/64211/diff/11/ > > > Testing > ------- > > Testing done on Fedora 25. > > For Autotools: > ``` > $ ./bootstrap > $ mkdir build > $ cd build > $ ../configure --enable-new-cli --disable-java --disable-python > $ make check > ``` > > For CMake: > ``` > $ ./bootstrap > $ mkdir build > $ cd build > $ cmake .. -DENABLE_NEW_CLI=1 > $ cmake --build . -- -j16 > $ ./src/mesos > Mesos CLI > > Usage: > mesos (-h | --help) > mesos --version > mesos <command> [<args>...] > > Options: > -h --help Show this screen. > --version Show version info. > > Commands: > agent Interacts with the Mesos agents > config Interacts with the Mesos CLI configuration file > task Interacts with the tasks running in a Mesos cluster > > See 'mesos help <command>' for more information on a specific command. > $ cmake --build . --target tests -- -j16 > $ ctest -R CLI > Test project /home/agrillet/apache-mesos/build > Start 4: CLITests > 1/1 Test #4: CLITests ......................... Passed 3.63 sec > > 100% tests passed, 0 tests failed out of 1 > ``` > > Checked that the the CLI tests were run, that the content of the directory > `build/src/cli` was as expected, and that `build/src/mesos` was correctly > running. > > > Thanks, > > Armand Grillet > >
