----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/64211/#review197774 -----------------------------------------------------------
Fix it, then Ship it! Looking great. I left some comments on the autoconf setup, sorry for the earlier partial review. In its final form it could look like this, AS_IF([test "x$enable_new_cli" = "xyes"], [ AM_PATH_PYTHON([2.6]) AX_COMPARE_VERSION([$PYTHON_VERSION], [gt], [2.7], AC_MSG_ERROR([Python version too new ------------------------------------------------------------------- The new CLI requires Python version 2.6 or 2.7 in order to build. Your Python version is $PYTHON_VERSION. You may wish to set the PYTHON environment variable to an appropriate value to assure the right Python executable is found. ------------------------------------------------------------------- ])) AC_CHECK_PROG([VIRTUALENV_CHECK], [virtualenv], [ok]) AS_IF([test "x$VIRTUALENV_CHECK" = "xok"],, [ AC_MSG_ERROR([Cannot find virtualenv ------------------------------------------------------------------- The new CLI requires 'virtualenv' be installed as part of your Python $PYTHON_VERSION installation. You may wish to install it via 'pip install virtualenv'. ------------------------------------------------------------------- ])]) ]) AM_CONDITIONAL([ENABLE_NEW_CLI], [test "$enable_new_cli" = "xyes"]) configure.ac Lines 240 (patched) <https://reviews.apache.org/r/64211/#comment278052> `s/new-cli/new_cli/` (you can check the output of e.g., `../configure --help` to see the difference). configure.ac Line 2442 (original), 2447 (patched) <https://reviews.apache.org/r/64211/#comment278053> Let's keep this conditional together with the conditionals related to e.g., `setuptools` and `pip` below. Suggest to move cli-related checks above this one. configure.ac Lines 2449-2453 (patched) <https://reviews.apache.org/r/64211/#comment278050> This is not really useful (e.g., displays `yes` even if we fail in cli-related checks below). Suggest to get rid of this completely. configure.ac Lines 2454 (patched) <https://reviews.apache.org/r/64211/#comment278054> We should initialize after the checks so we can avoid the initialization with a dummy value here. configure.ac Lines 2458-2469 (patched) <https://reviews.apache.org/r/64211/#comment278051> Let's kill this section and reword below error messages. Having a good error message is good, but we also should try to minimize the amount of logic we put into autoconf. configure.ac Lines 2458-2469 (patched) <https://reviews.apache.org/r/64211/#comment278055> This seems unrelated (`--enable-python`?) and redundant since we explicitly check another >=python-2.6 below. Kill this. configure.ac Lines 2472-2480 (patched) <https://reviews.apache.org/r/64211/#comment278056> We can just let the default error message from autoconf propagate; we `s/2.6/4.6/` I get checking for a Python interpreter with version >= 4.6... none configure: error: no suitable Python interpreter found configure.ac Lines 2493 (patched) <https://reviews.apache.org/r/64211/#comment278057> Let's quote these variables and add spaces around `,`, AC_CHECK_PROG([VIRTUALENV_CHECK], [virtualenv], [ok]) configure.ac Lines 2495 (patched) <https://reviews.apache.org/r/64211/#comment278058> Start error message with a captial letter. - Benjamin Bannier On Feb. 20, 2018, 12:09 p.m., Armand Grillet wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/64211/ > ----------------------------------------------------------- > > (Updated Feb. 20, 2018, 12:09 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 > 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/12/ > > > 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 > >