mesos-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Benjamin Bannier <bbann...@apache.org>
Subject Re: Review Request 71510: Windows: Fixed parallel test execution.
Date Thu, 19 Sep 2019 15:57:51 GMT


> On Sept. 19, 2019, 12:25 p.m., Benjamin Bannier wrote:
> > cmake/MesosConfigure.cmake
> > Lines 55 (patched)
> > <https://reviews.apache.org/r/71510/diff/1/?file=2165728#file2165728line55>
> >
> >     A feature of the current parallel test setup is that users can overwrite `TEST_DRIVER`
at configure time (in autotools even at execution time). With this change we hardcode the
assumption that `TEST_DRIVER` is a Python executable on Windows.
> >     
> >     I am not sure how to address this (e.g., setting the default `TEST_DRIVER` to
`python ...` is not possible as CTest would look for a non-existing executable name `python
...`). Would linking/copying `mesos-gtest-runner.py` to a file `mesos-gtest-runner.py.exe`
in the build tree at configure time and adjusting the default for `WIN32` be possible?
> >     
> >     In any case not an important regression, feel free to drop.
> 
> Joseph Wu wrote:
>     It should still be possible to override the `TEST_DRIVER` here.  The reason for the
`TEST_RUNNER` temporary variable is to transform the `TEST_DRIVER` into a list on Windows,
but keep it as a string on Posix.
>     
>     So on Windows, the variable looks like: `python;../support/mesos-gtest-runner.py`.
>     
>     If you were to override the value, (i.e. `cmake .. -DTEST_DRIVER=...`) that value
should be kept in the cache, like before.

Does it prefix the command with `python` on `WIN32` when executing?


> On Sept. 19, 2019, 12:25 p.m., Benjamin Bannier wrote:
> > src/tests/CMakeLists.txt
> > Lines 390 (patched)
> > <https://reviews.apache.org/r/71510/diff/1/?file=2165729#file2165729line390>
> >
> >     In follow-up patches you used a different logic,
> >     
> >         $<$<BOOL:$<CONFIG>>:$<CONFIG>/>stout-tests$<$<PLATFORM_ID:Windows>:.exe>
> >         
> >     Why do they need to be different? Is the `CONFIG` part even required there?
> 
> Joseph Wu wrote:
>     The extra stuff around the test name mostly becomes required due to the `mesos-gtest-runner.py`
script.  The script checks if the test binary exists before running it.  And the script can't
find the test binary unless we prefix with the build config on Windows; and suffix with the
actual file name extension.
>     
>     Prior to your test runner change, we didn't need the `CONFIG` part.  And I believe
that was cmake extending the binary search path automatically.
>     
>     Note: On Windows, if you have an executable like `test.exe`, you can run it while
omitting the `.exe`.

Okay. Why is that not required in the follow-up stout and libprocess patches? In any case,
it would go great for maintainability to document that in the actual cmake setup.


- Benjamin


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


On Sept. 19, 2019, 1 a.m., Joseph Wu wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/71510/
> -----------------------------------------------------------
> 
> (Updated Sept. 19, 2019, 1 a.m.)
> 
> 
> Review request for mesos, Benjamin Bannier, Greg Mann, and Till Toenshoff.
> 
> 
> Repository: mesos
> 
> 
> Description
> -------
> 
> Since the default 'check' target now uses the parallel test runner,
> there are some changes necessary to use this on Windows.
> 
> In the script itself, references to RLimits need to be removed,
> since those structures do not exist on Windows.
> 
> The TEST_RUNNER variable must include "python" on Windows, since
> '.py' files are not automatically run with Python on Windows.
> 
> The script also needs the full test executable name (+ '.exe').
> We could omit this previously, because you can run executables
> while omitting the extension.  But the script checks if the file
> exists, and that operation requires the full name plus extension.
> 
> 
> Diffs
> -----
> 
>   cmake/MesosConfigure.cmake 83d41addcd2c14358fba8bab2ac654475626a3e8 
>   src/tests/CMakeLists.txt 1e53b396569bf3e2f47199956a630afb6197b992 
>   support/mesos-gtest-runner.py 42c0a143477b5ccd411db482a8877e596f520342 
> 
> 
> Diff: https://reviews.apache.org/r/71510/diff/1/
> 
> 
> Testing
> -------
> 
> cmake --build . --target check (Windows)
> 
> 
> Thanks,
> 
> Joseph Wu
> 
>


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