Re: Review Request 71510: Windows: Fixed parallel test execution.

[Joseph Wu]

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

(Updated Sept. 19, 2019, 11:30 a.m.)


Review request for mesos, Benjamin Bannier, Greg Mann, and Till Toenshoff.


Changes
---

* Added comment blurbs above the main changes.
* Accepted Benjamin's suggested change to mesos-gtest-runner.py.


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 (updated)
-

  cmake/MesosConfigure.cmake 83d41addcd2c14358fba8bab2ac654475626a3e8 
  src/tests/CMakeLists.txt 1e53b396569bf3e2f47199956a630afb6197b992 
  support/mesos-gtest-runner.py 42c0a143477b5ccd411db482a8877e596f520342 


Diff: https://reviews.apache.org/r/71510/diff/2/

Changes: https://reviews.apache.org/r/71510/diff/1-2/


Testing
---

cmake --build . --target check (Windows)


Thanks,

Joseph Wu

Re: Review Request 71510: Windows: Fixed parallel test execution.

[Joseph Wu]

> On Sept. 19, 2019, 3:25 a.m., Benjamin Bannier wrote:
> > cmake/MesosConfigure.cmake
> > Lines 55 (patched)
> > 
> >
> > 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)
> > 
> >
> > In follow-up patches you used a different logic,
> > 
> > 
> > $<$>:$/>stout-tests$<$:.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,
> 

Re: Review Request 71510: Windows: Fixed parallel test execution.

[Benjamin Bannier]

> On Sept. 19, 2019, 12:25 p.m., Benjamin Bannier wrote:
> > cmake/MesosConfigure.cmake
> > Lines 55 (patched)
> > 
> >
> > 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)
> > 
> >
> > In follow-up patches you used a different logic,
> > 
> > 
> > $<$>:$/>stout-tests$<$:.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
> 
>

Re: Review Request 71510: Windows: Fixed parallel test execution.

[Joseph Wu]

> On Sept. 19, 2019, 3:25 a.m., Benjamin Bannier wrote:
> > cmake/MesosConfigure.cmake
> > Lines 55 (patched)
> > 
> >
> > 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.

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.


> On Sept. 19, 2019, 3:25 a.m., Benjamin Bannier wrote:
> > src/tests/CMakeLists.txt
> > Lines 390 (patched)
> > 
> >
> > In follow-up patches you used a different logic,
> > 
> > 
> > $<$>:$/>stout-tests$<$:.exe>
> > 
> > Why do they need to be different? Is the `CONFIG` part even required 
> > there?

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`.


- 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
> 
>

Re: Review Request 71510: Windows: Fixed parallel test execution.

[Benjamin Bannier]

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




cmake/MesosConfigure.cmake
Lines 55 (patched)


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.



src/tests/CMakeLists.txt
Lines 390 (patched)


In follow-up patches you used a different logic,

$<$>:$/>stout-tests$<$:.exe>

Why do they need to be different? Is the `CONFIG` part even required there?



support/mesos-gtest-runner.py
Lines 39 (patched)


Not a big fan of the magic string here and below.

Since this is only used in some best-effort validation I'd suggest we 
instead do down in `validate_setup`:

```{.python}
try:
import resource

/// reset of validation code
except Exception:  /// Also handles `ImportError`.
print(Bcolors.colorize(
   "Could not check compatibility of ulimit settings", /// Drop 
printing of error.
   Bcolors.WARNING),
 file=sys.stderr)
/// Make this a soft error and remove `sys.exit()`.
```


- Benjamin Bannier


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
> 
>

Review Request 71510: Windows: Fixed parallel test execution.

[Joseph Wu]

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

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