windows,tests: design Bash-less test execution #16

laszlocsomor · 2018-07-24T14:03:45Z

laszlocsomor · 2018-07-24T14:04:18Z

Lead reviewer: @ulfjack
Optional reviewers: @lberki , @dslomov

laszlocsomor · 2018-07-24T14:38:19Z

Thanks! To be clear, are you approving of the design itself, or just approving that the doc contains all important points and it's ready for review?

dslomov · 2018-07-24T14:43:42Z

Ready for review, as per https://bazel.build/designs/index.html#create-a-pull-request

jasharpe · 2018-07-24T15:17:20Z

designs/2018-07-18-windows-native-test-runner.md

+
+    If `$TEST_SHORT_EXEC_PATH` is defined, it sets an alternative `$TEST_PATH`.
+
+    **Why**: To avoid too long paths on Windows with remote execution.


FYI: Microsoft has fixed the bug that required this so we should be able to drop this once the next Windows Server release is out (a few months from now likely).

I'm not sure if this means we can drop this from the new wrapper entirely. Depends on when it is usable.

Thanks! What exactly did they fix: is it now possible to set the CWD to a path longer than MAX_PATH?
Btw I think using a junction ("Design adequacy" > "step 9") will beat the purpose of $TEST_SHORT_EXEC_PATH.

It wasn't a MAX_PATH issue, but an instability in Docker when passing commands above a certain length (around 107 characters or so).

Ah I see. But it seems, according to MSDN, SetCurrentDirectoryW now also supports long paths.

jasharpe · 2018-07-24T15:20:07Z

designs/2018-07-18-windows-native-test-runner.md

+
+# Non-requirements of the solution
+
+The solution does not have to cover remote test execution, because Bazel does


If I understand correctly, this isn't true. We run test-setup.sh on remote Windows machines now, and would likely run whatever wrapper binary replaces test-setup.sh on remote Windows machines as well.

That said, I don't see a reason why your proposal (which if I understand correctly is basically replacing test-setup.sh with a native program) wouldn't work with remote execution.

jasharpe · 2018-07-24T15:21:18Z

designs/2018-07-18-windows-native-test-runner.md

+# Implementation language
+
+We'll implement the test wrapper in C++, compile it as a x86\_64 Windows binary,
+and bundle it with Bazel as `@bazel_tools//tools/test:test-wrapper.exe`.


Out of curiosity, why bundle the binary rather than build it on the fly like the launchers are?

Because we want to require no C++ compiler. The launchers are actually pre-built: Bazel just appends data at the end of a pre-built binary.

ulfjack · 2018-07-25T07:26:46Z

designs/2018-07-18-windows-native-test-runner.md

+
+    **Why**: To avoid too long paths on Windows with remote execution.
+
+1.  Traps all signals to be handled by `write_xml_output_file()`.


Some of the steps will move out of the test wrapper script.

Which ones?

ulfjack · 2018-07-25T07:28:36Z

designs/2018-07-18-windows-native-test-runner.md

+# Non-requirements of the solution
+
+The solution does not have to cover remote test execution, because Bazel does
+not manage remote processes. To run a test remotely, Bazel sends a request to a


Bazel does upload the test-setup.sh script to the remote machine right now.

Right, @jasharpe already mentioned that. I'll rework this section and ping @jasharpe 's comment thread.

ulfjack · 2018-07-25T07:29:28Z

designs/2018-07-18-windows-native-test-runner.md

+*   a child process, which runs either the test binary or the coverage collector
+
+*   optionally a second child process after the first one finished, which runs
+    the `$LCOV_MERGER` when coverage collection is requested


We only run LCOV_MERGER if the test exits successfully.

ulfjack · 2018-07-25T07:29:55Z

designs/2018-07-18-windows-native-test-runner.md

+
+1.  wait some time for the process to complete its shutdown protocol
+
+1.  forcefully terminate the process only if it's still running after a timeout.


Add explanation why you want to change that.

ulfjack · 2018-07-25T07:30:32Z

designs/2018-07-18-windows-native-test-runner.md

+message. This communication protocol is easily exendable if necessary.
+
+Using `stdin` is also safe: no other process has a handle to the test wrapper's
+`stdin`, so no other process will inadvertently send the interruption request.


We need to sync with any remote execution implementations on this.

ulfjack · 2018-07-25T07:31:13Z

designs/2018-07-18-windows-native-test-runner.md

+
+The primary output of test execution is the XML test log: it carries the most
+useful information for the user. The XML file records the test's status (passed
+or failed) and the test's textual output. The test wrapper should ensure it


I'm in the process of making Bazel ensure that a test.xml exists, rather than relying on the test wrapper.

ulfjack · 2018-07-25T07:34:23Z

designs/2018-07-18-windows-native-test-runner.md

+We will roll out this feature over several Bazel minor versions:
+
+1.  version `0.N.*`: contains both the new and old test execution mechanisms and
+    supports the `--[no]windows_bashless_test` flag. By default the flag is


Double negation is not ideal.

lberki · 2018-07-25T07:41:16Z

designs/2018-07-18-windows-native-test-runner.md

+
+There will be two or three processes per test (same numbers as today):
+
+*   a parent process, which runs the test wrapper


Have you considered implementing this not as a separate executable, but as JNI code within the Bazel binary? This would bring a few advantages:

One less process to create (process creation is expensive on Windows)

No need to come up with a protocol between the Bazel process and the test wrapper

One less moving part (we already have JNI code for process management)

The downside is that a badly-written test wrapper could bring down the whole Bazel process, but I assume the code wouldn't be that complicated for that to be a great risk.

lberki · 2018-07-25T07:42:50Z

designs/2018-07-18-windows-native-test-runner.md

+
+### Interruption request
+
+The communication channel between Bazel and the test wrapper process will be the


Is this the standard way to communicate with a child process on Windows?

lberki · 2018-07-25T07:43:51Z

designs/2018-07-18-windows-native-test-runner.md

+order to set up the test's environment, the test setup process must create the
+test process.
+
+## Processes


Are these processes going to be wrapped in a Job object (like all the current subprocesses on Windows)?

dslomov · 2018-07-25T09:18:04Z

Meta-points:

per https://bazel.build/designs/index.html#create-a-pull-request, review discussion needs to happen on bazel-dev thread, not on this pull request (which is just to get the document in the repo)
review discussion here is quite difficult to follow :(

Markdown format is very bad at supporting the discussion, Google Docs are just so much better
I think the right way would be to file github issues for anything that needs clarification

laszlocsomor · 2018-07-25T09:21:45Z

Re: #16 (comment)

per https://bazel.build/designs/index.html#create-a-pull-request, review discussion needs to happen on bazel-dev thread, not on this pull request (which is just to get the document in the repo)

I hear you, but I think the process is broken. The reality is, for everything else we do on GitHub (PRs, issues) we are used to commenting here. Expecting that people will behave differently just for proposal PRs is futile.

review discussion here is quite difficult to follow :(

I agree with that. That's a problem with GitHub's code review tool, IMO.

Perhaps MarkDown isn't the right format for design ~~discussions~~ docs and we should stick to GDocs, and maybe only use MarkDown for archival purposes. WDYT?

laszlocsomor · 2018-07-25T11:46:03Z

Thanks everyone for reviews!

Let's continue the review comments on this email thread: https://groups.google.com/d/msg/bazel-dev/Vnugr-wDTqU/0tKcYkkyBwAJ

That email also explains the reasons. I'll copy all comment threads from this PR to that email thread.

laszlocsomor · 2018-07-25T12:04:12Z

Actually, instead of trying to adjust this PR, let me create a new one just for the changes to README.md.

windows,tests: design Bash-less test execution

3594152

See bazelbuild/bazel#5508

laszlocsomor requested review from lberki, dslomov and ulfjack July 24, 2018 14:04

Merge branch 'master' into win-test-runner

ccd2192

dslomov approved these changes Jul 24, 2018

View reviewed changes

jasharpe reviewed Jul 24, 2018

View reviewed changes

laszlocsomor mentioned this pull request Jul 25, 2018

undocumented usage of perl for cc_test bazelbuild/bazel#4691

Closed

ulfjack reviewed Jul 25, 2018

View reviewed changes

lberki reviewed Jul 25, 2018

View reviewed changes

laszlocsomor closed this Jul 25, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

windows,tests: design Bash-less test execution #16

windows,tests: design Bash-less test execution #16

laszlocsomor commented Jul 24, 2018 •

edited

Loading

laszlocsomor commented Jul 24, 2018

laszlocsomor commented Jul 24, 2018

dslomov commented Jul 24, 2018

jasharpe Jul 24, 2018 •

edited

Loading

laszlocsomor Jul 24, 2018

jasharpe Jul 24, 2018

laszlocsomor Jul 24, 2018

jasharpe Jul 24, 2018

jasharpe Jul 24, 2018

laszlocsomor Jul 24, 2018

ulfjack Jul 25, 2018

laszlocsomor Jul 25, 2018

ulfjack Jul 25, 2018

laszlocsomor Jul 25, 2018 •

edited

Loading

ulfjack Jul 25, 2018

ulfjack Jul 25, 2018

ulfjack Jul 25, 2018

ulfjack Jul 25, 2018

ulfjack Jul 25, 2018

lberki Jul 25, 2018 •

edited

Loading

lberki Jul 25, 2018

lberki Jul 25, 2018

dslomov commented Jul 25, 2018 •

edited

Loading

laszlocsomor commented Jul 25, 2018 •

edited

Loading

laszlocsomor commented Jul 25, 2018

laszlocsomor commented Jul 25, 2018


		If `$TEST_SHORT_EXEC_PATH` is defined, it sets an alternative `$TEST_PATH`.

		Why: To avoid too long paths on Windows with remote execution.


		# Non-requirements of the solution

		The solution does not have to cover remote test execution, because Bazel does


		Why: To avoid too long paths on Windows with remote execution.

		1. Traps all signals to be handled by `write_xml_output_file()`.


		1. wait some time for the process to complete its shutdown protocol

		1. forcefully terminate the process only if it's still running after a timeout.


		There will be two or three processes per test (same numbers as today):

		* a parent process, which runs the test wrapper


		### Interruption request

		The communication channel between Bazel and the test wrapper process will be the

windows,tests: design Bash-less test execution #16

windows,tests: design Bash-less test execution #16

Conversation

laszlocsomor commented Jul 24, 2018 • edited Loading

laszlocsomor commented Jul 24, 2018

laszlocsomor commented Jul 24, 2018

dslomov commented Jul 24, 2018

jasharpe Jul 24, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

laszlocsomor Jul 25, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lberki Jul 25, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dslomov commented Jul 25, 2018 • edited Loading

laszlocsomor commented Jul 25, 2018 • edited Loading

laszlocsomor commented Jul 25, 2018

laszlocsomor commented Jul 25, 2018

laszlocsomor commented Jul 24, 2018 •

edited

Loading

jasharpe Jul 24, 2018 •

edited

Loading

laszlocsomor Jul 25, 2018 •

edited

Loading

lberki Jul 25, 2018 •

edited

Loading

dslomov commented Jul 25, 2018 •

edited

Loading

laszlocsomor commented Jul 25, 2018 •

edited

Loading