mesos-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Benjamin Bannier <benjamin.bann...@mesosphere.io>
Subject Re: Review Request 64211: Added options to build the Python CLI and run unit tests.
Date Tue, 20 Feb 2018 17:08:36 GMT


> 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
> 
>


Mime
  • Unnamed multipart/alternative (inline, None, 0 bytes)
View raw message