----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/52543/#review153473 -----------------------------------------------------------
configure.ac (line 203) <https://reviews.apache.org/r/52543/#comment222790> Please do not add options here, instead just use AC_ARG_ENABLE([new-cli], AS_HELP_STRING([--enable-new-cli], [build new CLI instead of old CLI default: no])) The pattern you copied here leads to fun issues like https://issues.apache.org/jira/browse/MESOS-2537. Also see https://autotools.io/autoconf/arguments.html. configure.ac (line 2178) <https://reviews.apache.org/r/52543/#comment222803> Please `s/python/Python/g` here and below, especially in user-facing strings. configure.ac (lines 2179 - 2180) <https://reviews.apache.org/r/52543/#comment222816> Use `AS_IF` here instead of shell `if` (there's already a `TODO` above to clean up existing ones, let's not add new ones). configure.ac (lines 2181 - 2205) <https://reviews.apache.org/r/52543/#comment222804> This could be simplified if you used the action-less version with aborts if no Python with at least the given version can be found, i.e., AM_PATH_PYTHON([2.6]) configure.ac (line 2194) <https://reviews.apache.org/r/52543/#comment222792> You could just use the first argument of `AM_PATH_PYTHON` here. configure.ac (line 2219) <https://reviews.apache.org/r/52543/#comment222814> `AS_IF` src/Makefile.am (line 1410) <https://reviews.apache.org/r/52543/#comment222794> Any reason you didn't want to explicitly list the files here? I believe this would make it clearer what is hidden :/ src/Makefile.am (line 1416) <https://reviews.apache.org/r/52543/#comment222806> Could you explicitly list the dependencies? This not only would make `make` run faster in NOP builds, but also would make the build more reliable (less magic effects if e.g., junk sits in that directory). Also out of curiosity, why is `EXEEXT` needed here? src/Makefile.am (line 1417) <https://reviews.apache.org/r/52543/#comment222820> I would remove this `echo` and instead just let below invocation echo, at least in verbose builds. src/Makefile.am (line 1418) <https://reviews.apache.org/r/52543/#comment222825> Make all commands here a single shell invocation. If you then prefix this command with `$(AM_V_GEN)` it would produce silenced output in silent mode (`--enable-silent-rules` ...). src/Makefile.am (line 1424) <https://reviews.apache.org/r/52543/#comment222809> Not sure if this already happens earlier in the chain, but a comment explaining why we need to bundle the CLI would be helpful. Feel free to drop if this obvious. src/Makefile.am (line 1432) <https://reviews.apache.org/r/52543/#comment222807> Could you also add a `dist_` target? src/Makefile.am (line 2382) <https://reviews.apache.org/r/52543/#comment222828> No pun intended, but this explicit `echo` does not seem to add a lot (but a newline). I believe this isn't needed and should be removed. src/Makefile.am (line 2384) <https://reviews.apache.org/r/52543/#comment222810> Does mesos-cli-tests introduce any new dependencies? Right now this target just depends on `tests`. - Benjamin Bannier On Oct. 20, 2016, 8:13 p.m., Kevin Klues wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/52543/ > ----------------------------------------------------------- > > (Updated Oct. 20, 2016, 8:13 p.m.) > > > Review request for mesos and Joseph Wu. > > > Bugs: MESOS-6008 and MESOS-6032 > https://issues.apache.org/jira/browse/MESOS-6008 > https://issues.apache.org/jira/browse/MESOS-6032 > > > Repository: mesos > > > Description > ------- > > Added configure/make options to build the new CLI and run unit tests. > > > Diffs > ----- > > configure.ac c8d48be41d526c45ee00b404d0ad2b67ea11587f > docs/getting-started.md de3fba4a821ca7fa9b9e2fd692b33b40edd35b68 > src/Makefile.am 05fe8fa3e3409b35ba66d7e7238e988fb0fab07d > > Diff: https://reviews.apache.org/r/52543/diff/ > > > Testing > ------- > > ../configure --enable-new-cli > make -C src mesos > src/mesos > > GTEST_FILTER="" make -j check > sudo GTEST_FILTER="" make -j check > > > Thanks, > > Kevin Klues > >
