----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/71510/#review217848 -----------------------------------------------------------
cmake/MesosConfigure.cmake Lines 55 (patched) <https://reviews.apache.org/r/71510/#comment305242> 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) <https://reviews.apache.org/r/71510/#comment305243> 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? support/mesos-gtest-runner.py Lines 39 (patched) <https://reviews.apache.org/r/71510/#comment305241> 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 > >
