> On Feb. 20, 2018, 4:25 p.m., Benjamin Bannier wrote: > > configure.ac > > Lines 2458-2469 (patched) > > <https://reviews.apache.org/r/64211/diff/12/?file=1962861#file1962861line2458> > > > > 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. > > Kevin Klues wrote: > There was a reason I originally had this separate, and I don't remember > why. Can you take a closer look if something will go wrong if this is simply > removed?
I cannot see how the Python version of the bindings and the CLI are related (they might be the same, but do not need to be forever). We also immediately follow this up with another call to `AM_PATH_PYTHON` which overrides the results here. I tested that that this works as expected. > On Feb. 20, 2018, 4:25 p.m., Benjamin Bannier wrote: > > configure.ac > > Lines 2458-2469 (patched) > > <https://reviews.apache.org/r/64211/diff/12/?file=1962861#file1962861line2458> > > > > This seems unrelated (`--enable-python`?) and redundant since we > > explicitly check another >=python-2.6 below. Kill this. > > Kevin Klues wrote: > This again has to do with what I commented on above. I remember having to > compensate for cases where enable_python was true/false with making sure > python was still available for the CL if `--enable-cli-new` was set. Like I wrote above, cli and binding dependencies are not necessarily the same. If we really want to detect a common Python version we should pull the detection out of any conditionals and then only check compatibility where we need it. I felt this was overkill for this patch. - Benjamin ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/64211/#review197774 ----------------------------------------------------------- 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 > >
