mesos-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Joseph Wu <jos...@mesosphere.io>
Subject Re: Review Request 69311: Enabled parallel test runner to cmake build.
Date Wed, 14 Nov 2018 20:10:17 GMT


> On Nov. 13, 2018, 2: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.
> 
> Benjamin Bannier wrote:
>     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.

In terms of modifying the invocation of a command, there are two different ways CMake can
account for them:

1) During configuration time, CMake can access environment variables like `$ENV{MESOS_GTEST_RUNNER_FLAGS}`.
 So supposing we add logic which modifies the `test` target based on this environment variable,
each time you run the test we will pass in the compile-time value of the flag.

2) To pick up runtime environment variable changes would be more finnicky.  If we want the
user to be able to supply `MESOS_GTEST_RUNNER_FLAGS` at runtime, the `test` target will not
be allowed to `set(TEST MesosTests PROPERTY ENVIRONMENT ...)` because that would basically
ignore the user specified value.  We would basically need to modify the wrapper to pick up/modify
environment variables on its own, or wrap the wrapper in a command which performs the environment
variable modification we want.


- Joseph


-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/69311/#review210500
-----------------------------------------------------------


On Nov. 14, 2018, 5:11 a.m., Benjamin Bannier wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/69311/
> -----------------------------------------------------------
> 
> (Updated Nov. 14, 2018, 5:11 a.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
> 
>


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