> On Nov. 13, 2018, 11:48 a.m., Benno Evers wrote: > > cmake/MesosConfigure.cmake > > Lines 56 (patched) > > <https://reviews.apache.org/r/69311/diff/1/?file=2106802#file2106802line56> > > > > If I configure with `ENABLE_PARALLEL_TEST_EXECUTION=ON` and then > > reconfigure with `ENABLE_PARALLEL_TEST_EXECUTION=OFF`, would this line > > still take the old value for `TEST_DRIVER` from the cache? > > Joseph Wu wrote: > Let's see: > ``` > $ cmake .. -DENABLE_PARALLEL_TEST_EXECUTION=ON > $ cmake .. -LAH | grep TEST_DRIVER > TEST_DRIVER:STRING=/Users/josephwu/mesos/support/mesos-gtest-runner.py > > $ cmake .. -DENABLE_PARALLEL_TEST_EXECUTION=OFF > CMake Error at cmake/MesosConfigure.cmake:56 (set): > set given invalid arguments for CACHE mode. > Call Stack (most recent call first): > CMakeLists.txt:48 (include) > ``` > > Ruh roh! > > Changing the line to `unset(TEST_DRIVER CACHE)` instead: > ``` > $ cmake .. -DENABLE_PARALLEL_TEST_EXECUTION=OFF > $ cmake .. -LAH | grep TEST_DRIVER > <empty> > ```
Went for the more compatible approach suggested by Joseph. Re: cache, what he said ;) > On Nov. 13, 2018, 11:48 a.m., Benno Evers wrote: > > src/tests/CMakeLists.txt > > Lines 365 (patched) > > <https://reviews.apache.org/r/69311/diff/1/?file=2106803#file2106803line365> > > > > Does this preserve user-specified `MESOS_GTEST_RUNNER_FLAGS`? Or, the > > other way round, will a user accidentally overwrite this when he specifies > > his own flags? > > > > Also, it seems a bit unfortunate to hard-code the exemption for > > `*ROOT_*` into the build system. intuitively I'd propose something like an > > advanced setting `SEQUENTIAL_TESTS` and then setting > > > > MESOS_GTEST_RUNNER_FLAGS="--sequential=${SEQUENTIAL_TESTS} > > $MESOS_GTEST_RUNNER_FLAGS" > > > > (I'm not sure if the above is correct CMake syntax, but you get the > > idea) > > > > Of course, if CMake already does the correct thing anyways, disregard > > this issue. > > Joseph Wu wrote: > https://cmake.org/cmake/help/v3.8/prop_test/ENVIRONMENT.html > > Cmake should overwrite the environment when running the test and then set > the environment back afterwards. Like Joseph wrote, this causes any user setting to be override for execution under `ctest` (e.g., from `make check`). In the autotools setup we allow users to override flags, even over the hardcoded flags; the approach I took here works around issues with constructing test command lines and is less flexible. @Joseph, do you have any idea how we would directly append these flags to the invocation, so that users can pass additional flags via an env var? As for `SEQUENTIAL_TESTS` etc., this is a very interesting subject, but IMO not in scope for this change. We here just bring cmake up to date with capabilities already existing in the autotools build. - Benjamin ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/69311/#review210500 ----------------------------------------------------------- On Nov. 14, 2018, 2:11 p.m., Benjamin Bannier wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/69311/ > ----------------------------------------------------------- > > (Updated Nov. 14, 2018, 2:11 p.m.) > > > Review request for mesos, Benjamin Mahler and Joseph Wu. > > > Repository: mesos > > > Description > ------- > > Enabled parallel test runner to cmake build. > > > Diffs > ----- > > cmake/MesosConfigure.cmake de7dc08cc5ba4eaead017a97dcfeaf96bd0f4dbe > src/tests/CMakeLists.txt 553516ad66cab4480b7211950fc726b7d9a3869b > > > Diff: https://reviews.apache.org/r/69311/diff/2/ > > > Testing > ------- > > Tested as part of https://reviews.apache.org/r/69313/. > > > Thanks, > > Benjamin Bannier > >
