> On Sept. 19, 2019, 3:25 a.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.
>
> Benjamin Bannier wrote:
> Does it prefix the command with `python` on `WIN32` when executing?
I'll add a comment explaining this.
Basically, there are two cases:
---
On Posix:
`TEST_RUNNER` is not set.
So this line:
```
set(TEST_DRIVER ${TEST_RUNNER}
"${PROJECT_SOURCE_DIR}/support/mesos-gtest-runner.py" CACHE STRING)
```
Evaluates to:
```
TEST_DRIVER=../support/mesos-gtest-runner.py
```
This is exactly the same as before.
---
On Windows:
```
TEST_RUNNER=python
```
So the same `TEST_DRIVER` expression evaluates to:
```
TEST_DRIVER=python;../support/mesos-gtest-runner.py
```
---
The important detail is when we use `${TEST_DRIVER}` as part of a command. i.e.
```
add_test(
NAME MesosTests
COMMAND
${TEST_DRIVER}
"${CMAKE_BINARY_DIR}/src/mesos-tests")
```
`${TEST_DRIVER}` is a list on Windows, resulting in a proper command like:
```
"python" "../support/mesos-gtest-runner.py" "build/src/mesos-tests"
```
Note that quoting `"${TEST_DRIVER}"` would result in incorrectness, since a
quoted variable becomes a single argument.
> On Sept. 19, 2019, 3:25 a.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`.
>
> Benjamin Bannier wrote:
> 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.
The reason mesos-tests don't need the CONFIG is because we do the following in
`src/CMakeLists.txt`:
```
# Generate all binaries in the same folder.
set(CMAKE_RUNTIME_OUTPUT_DIRECTORY ${CMAKE_BINARY_DIR}/src)
if (WIN32)
SET(CMAKE_RUNTIME_OUTPUT_DIRECTORY_DEBUG ${CMAKE_BINARY_DIR}/src)
SET(CMAKE_RUNTIME_OUTPUT_DIRECTORY_RELEASE ${CMAKE_BINARY_DIR}/src)
endif ()
```
This was for convenience (of finding the binaries) and to produce a similar
build output directory as the autotools build.
- Joseph
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/71510/#review217848
-----------------------------------------------------------
On Sept. 18, 2019, 4 p.m., Joseph Wu wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/71510/
> -----------------------------------------------------------
>
> (Updated Sept. 18, 2019, 4 p.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
>
>
