Re: Design review: Bash-less test execution on Windows

38 views
Skip to first unread message

László Csomor

unread,
Jul 24, 2018, 10:21:18 AM7/24/18
to bazel-discuss, bazel-dev
+bazel-dev 

On Tue, Jul 24, 2018 at 4:05 PM László Csomor <laszlo...@google.com> wrote:
FYI: https://github.com/bazelbuild/proposals/pull/16

--
László Csomor | Software Engineer | laszlo...@google.com

Google Germany GmbH | Erika-Mann-Str. 33 | 80636 München | Germany
Registergericht und -nummer: Hamburg, HRB 86891
Sitz der Gesellschaft: Hamburg
Geschäftsführer: Paul Manicle, Halimah DeLaine Prado

László Csomor

unread,
Jul 25, 2018, 7:33:48 AM7/25/18
to bazel-discuss, bazel-dev, Laurent Le Brun, Dmitry Lomov
I talked with +Laurent Le Brun and +Dmitry Lomov about the current review process for markdown design docs. Laurent agreed to summarize the outcome, so I'll let him get into details and only say what's relevant for this particular review thread:

1. Let's continue the review and commenting on this email thread, not on the PR. I'll copy all comment threads onto this email thread.
Why:
- If the doc and the change adding the link to the doc are in the same PR, and the PR is open until the doc is approved, then the Proposals repo won't link to the doc until it's already approved.
- PR comments are hard to track:
  - Updating comments and the code are not an atomic step.
  - GitHub collapses comment threads that it deems outdated.

2. I'll update the PR by removing the design doc itself and changing the link to point to the doc on my development branch in my own fork of the Proposals repo, then merge this PR.
Why:
- This way we have a live link in the Proposals repo that points to the draft doc.
- Following the link takes you to my development branch, where you always see the latest iteration of the design doc.

3. I'll make changes to the doc only on my development branch. For each iteration of the review, I'll push new commits. 
Why:
- You'll see the complete version history of the doc under the link found in the upstream Proposals repo.
- Updating the doc doesn't involve any PRs, so there's no temptation to move discussion away from this email thread.

4. Once my design is approved, I'll send a PR that adds the doc to the upstream Proposals repo, updates the link to point to the newly-added file, and updates its status to be "approved".


Thanks,
Laszlo



--
László Csomor | Software Engineer | laszlo...@google.com

Google Germany GmbH | Erika-Mann-Str. 33 | 80636 München | Germany
Registergericht und -nummer: Hamburg, HRB 86891
Sitz der Gesellschaft: Hamburg
Geschäftsführer: Paul Manicle, Halimah DeLaine Prado


László Csomor

unread,
Jul 25, 2018, 7:56:46 AM7/25/18
to bazel-discuss, bazel-dev, Laurent Le Brun, Dmitry Lomov
Comment threads:


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

- @jasharpe:
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.


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

  
--
László Csomor | Software Engineer | laszlo...@google.com

Google Germany GmbH | Erika-Mann-Str. 33 | 80636 München | Germany
Registergericht und -nummer: Hamburg, HRB 86891
Sitz der Gesellschaft: Hamburg
Geschäftsführer: Paul Manicle, Halimah DeLaine Prado

László Csomor

unread,
Jul 25, 2018, 7:59:18 AM7/25/18
to bazel-discuss, bazel-dev, Laurent Le Brun, Dmitry Lomov, Ulf Adams
Comment threads with @Ulf Adams :

- @ulfjack: 
Some of the steps will move out of the test wrapper script.

Which ones?

- @ulfjack: 
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: 
Add explanation why you want to change that.




--
László Csomor | Software Engineer | laszlo...@google.com

Google Germany GmbH | Erika-Mann-Str. 33 | 80636 München | Germany
Registergericht und -nummer: Hamburg, HRB 86891
Sitz der Gesellschaft: Hamburg
Geschäftsführer: Paul Manicle, Halimah DeLaine Prado


László Csomor

unread,
Jul 25, 2018, 8:01:15 AM7/25/18
to bazel-discuss, bazel-dev, Laurent Le Brun, Dmitry Lomov, Ulf Adams, Lukács T. Berki
Comment threads with +Lukács T. Berki :

- @lberki: 
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: 
Are these processes going to be wrapped in a Job object (like all the current subprocesses on Windows)?


- @lberki: 
Is this the standard way to communicate with a child process on Windows?



--
László Csomor | Software Engineer | laszlo...@google.com

Google Germany GmbH | Erika-Mann-Str. 33 | 80636 München | Germany
Registergericht und -nummer: Hamburg, HRB 86891
Sitz der Gesellschaft: Hamburg
Geschäftsführer: Paul Manicle, Halimah DeLaine Prado


László Csomor

unread,
Jul 25, 2018, 8:44:40 AM7/25/18
to bazel-discuss, bazel-dev, Laurent Le Brun, Dmitry Lomov, Jeremy Sharpe
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`.

- @jasharpe:
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.

(Resolved.)
 
 
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.

@ulfjack pointed out that the stdin-based communication protocol between Bazel and the test wrapper raises questions about remote test execution.
Do you know how Bazel interrupts remote actions when the user hits Ctrl+C?

 
- @jasharpe: 
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.

(Resolved.)

Lukács T. Berki

unread,
Jul 25, 2018, 8:50:11 AM7/25/18
to László Csomor, bazel-discuss, bazel-dev, Laurent Le Brun, Dmitry Lomov, Jeremy Sharpe
On Wed, Jul 25, 2018 at 2:44 PM, 'László Csomor' via bazel-dev <baze...@googlegroups.com> wrote:
+Jeremy Sharpe 

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

- @jasharpe:
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.

(Resolved.)
 
 
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.

@ulfjack pointed out that the stdin-based communication protocol between Bazel and the test wrapper raises questions about remote test execution.
Do you know how Bazel interrupts remote actions when the user hits Ctrl+C?
It's not only interruption -- the question is which part of the ceremony around remote test execution is done locally and which part is done remotely. 


 
- @jasharpe: 
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.

(Resolved.)


--
László Csomor | Software Engineer | laszlo...@google.com

Registergericht und -nummer: Hamburg, HRB 86891
Sitz der Gesellschaft: Hamburg
Geschäftsführer: Paul Manicle, Halimah DeLaine Prado 

--
You received this message because you are subscribed to the Google Groups "bazel-dev" group.
To unsubscribe from this group and stop receiving emails from it, send an email to bazel-dev+unsubscribe@googlegroups.com.
To post to this group, send email to baze...@googlegroups.com.
 To view this discussion on the web visit https://groups.google.com/d/msgid/bazel-dev/CAJ5fxH%2B0avbm0u-eixtOZvhTVnWQbbccMaAqnQ3gNC0o-TFR_A%40mail.gmail.com.

For more options, visit https://groups.google.com/d/optout.



--
Lukács T. Berki | Software Engineer | lbe...@google.com | 

Google Germany GmbH | Erika-Mann-Str. 33  | 80636 München | Germany | Geschäftsführer: Paul Manicle, Halimah DeLaine Prado | Registergericht und -nummer: Hamburg, HRB 86891

László Csomor

unread,
Jul 25, 2018, 8:52:55 AM7/25/18
to bazel-discuss, bazel-dev, Laurent Le Brun, Dmitry Lomov, Ulf Adams, Nicolas Lopez
(Still open.)
Right, @jasharpe already mentioned that. I'll rework this section and ping @jasharpe 's comment thread. 

 
https://github.com/laszlocsomor/proposals/commit/3594152692cfdcdb7e44d0863280ed4925572ae8#diff-81f744deb3c90920fd0c7787a74e5342R271
- @ulfjack: 
I'm in the process of making Bazel ensure that a test.xml exists, rather than relying on the test wrapper.
Ack. Let's keep this thread open until your changes are merged.

László Csomor

unread,
Jul 26, 2018, 10:47:52 AM7/26/18
to bazel-discuss, bazel-dev, Laurent Le Brun, Dmitry Lomov, Ulf Adams, Nicolas Lopez
Reviewers, please take another look:

Please try to comment on a higher level and try avoiding comments on individual lines. As I realized, it's very difficult to keep track of such subthreads of the discussion when we only use email.

I'm sorry for the awkwardness of this review process, we are still figuring it out. Google Docs probably would've been more convenient, but then we wouldn't have found the problems of the markdown-based workflow.

László Csomor

unread,
Jul 30, 2018, 3:03:10 AM7/30/18
to bazel-discuss, bazel-dev, Laurent Le Brun, Dmitry Lomov, Ulf Adams, Nicolas Lopez



--
László Csomor | Software Engineer | laszlo...@google.com

Google Germany GmbH | Erika-Mann-Str. 33 | 80636 München | Germany
Registergericht und -nummer: Hamburg, HRB 86891
Sitz der Gesellschaft: Hamburg
Geschäftsführer: Paul Manicle, Halimah DeLaine Prado


Ulf Adams

unread,
Aug 3, 2018, 4:55:15 AM8/3/18
to László Csomor, bazel-discuss, bazel-dev, Laurent Le Brun, Dmitry Lomov, Nicolas Lopez
The doc still doesn't take the split generation from cl/206292179 into account - I hope that will be the default in the future.

Laurent Le Brun

unread,
Aug 3, 2018, 5:12:12 AM8/3/18
to Ulf Adams, László Csomor, bazel-discuss, bazel-dev, Dmitry Lomov, Nicolas Lopez
(cl/206292179 means
https://github.com/bazelbuild/bazel/commit/0858ae1f6eb890c1e203a2aa21130ba34ca36a27)

László Csomor

unread,
Aug 3, 2018, 6:40:08 AM8/3/18
to Laurent Le Brun, Ulf Adams, bazel-discuss, bazel-dev, Dmitry Lomov, Nicolas Lopez
Ulf, thanks. Is that your only open concern? Also, you said [1] you'll be on vacation in August. Mind if I appoint a different lead reviewer?




--
László Csomor | Software Engineer | laszlo...@google.com

Google Germany GmbH | Erika-Mann-Str. 33 | 80636 München | Germany
Registergericht und -nummer: Hamburg, HRB 86891
Sitz der Gesellschaft: Hamburg
Geschäftsführer: Paul Manicle, Halimah DeLaine Prado


Ulf Adams

unread,
Aug 3, 2018, 6:43:52 AM8/3/18
to László Csomor, Laurent Le Brun, bazel-discuss, bazel-dev, Dmitry Lomov, Nicolas Lopez
I'm happy with the decision to implement in C++. I have no other concerns - feel free to go ahead.

László Csomor

unread,
Aug 3, 2018, 6:47:50 AM8/3/18
to Ulf Adams, Laurent Le Brun, bazel-discuss, bazel-dev, Dmitry Lomov, Nicolas Lopez
Thanks!



--
László Csomor | Software Engineer | laszlo...@google.com

Google Germany GmbH | Erika-Mann-Str. 33 | 80636 München | Germany
Registergericht und -nummer: Hamburg, HRB 86891
Sitz der Gesellschaft: Hamburg
Geschäftsführer: Paul Manicle, Halimah DeLaine Prado


Dmitry Lomov

unread,
Aug 3, 2018, 6:57:48 AM8/3/18
to László Csomor, Ulf Adams, Laurent Le Brun, bazel-discuss, bazel-dev, Nicolas Lopez
lgtm
--
Google Germany GmbH
Erika-Mann-Straße 33, 80636 München, Germany

László Csomor

unread,
Aug 3, 2018, 7:20:50 AM8/3/18
to Dmitry Lomov, Ulf Adams, Laurent Le Brun, bazel-discuss, bazel-dev, Nicolas Lopez



--
László Csomor | Software Engineer | laszlo...@google.com

Google Germany GmbH | Erika-Mann-Str. 33 | 80636 München | Germany
Registergericht und -nummer: Hamburg, HRB 86891
Sitz der Gesellschaft: Hamburg
Geschäftsführer: Paul Manicle, Halimah DeLaine Prado


Nicolas Lopez

unread,
Aug 3, 2018, 9:33:52 AM8/3/18
to László Csomor, foundry...@google.com, Dmitry Lomov, Ulf Adams, Laurent Le Brun, bazel-discuss, bazel-dev
 +Foundry Windows
A couple of clarifying questions:
- wrt section (https://github.com/laszlocsomor/proposals/blob/win-test-runner/designs/2018-07-18-windows-native-test-runner.md#implementation-language): The (windows only) test wrapper will also be available in linux Bazel versions so that, potentially, it would be possible to do Linux host-> Windows exec?
- does use of the test wrapper mean that there is no bash toolchain any longer in windows (I'm assuming that's the case in the followup questions - but if not ignore followups)? If so, if I was to build a custom skylark test for windows, which facilities (e.g., copying files) will the test wrapper provide to me to use in my test code (and how would I interact with them), or would my test code not interact at all with the test wrapper? If so, how does my test code declare it needs a set of tools to run (i.e., the equivalent of a bash toolchain)?

Overall doc lgtm, but I'd also like someone from foundry-windows team also take a look.


Vinayak Bansal

unread,
Aug 3, 2018, 11:57:53 AM8/3/18
to Nicolas Lopez, László Csomor, foundry...@google.com, Dmitry Lomov, Ulf Adams, Laurent Le Brun, bazel-...@googlegroups.com, baze...@googlegroups.com
Thanks for drawing this up, László!

possible to do Linux host-> Windows exec? >> Seems like this gets us one step closer to that vision!

test wrapper >> At the risk of looking like a fool, I'll say that that can be answered by Docker? You give a custom Docker image to run that test in, which provides the set of tools that that test needs?

 I'd also like someone from foundry-windows team also take a look. >> Jeremy reviewed this doc when it came out about 10 days ago. He's on vacation atm, but I can try to field any specific questions in his absence. 


--
You received this message because you are subscribed to the Google Groups "Foundry Windows" group.
To unsubscribe from this group and stop receiving emails from it, send an email to foundry-windo...@google.com.
To post to this group, send email to foundry...@google.com.
To view this discussion on the web visit https://groups.google.com/a/google.com/d/msgid/foundry-windows/CAENczYakhyvLX0krQUzE4gBdQYgXpkhZ5-EnqUhz%2B9%3DHQQH-dQ%40mail.gmail.com.

László Csomor

unread,
Aug 6, 2018, 3:18:16 AM8/6/18
to Vinayak Bansal, Nicolas Lopez, foundry...@google.com, Dmitry Lomov, Ulf Adams, Laurent Le Brun, bazel-discuss, bazel-dev
(Answering Nick and Vinayak.)

I didn't consider including the Windows test-wrapper in the Linux version of Bazel, but I see no reason against if it helps supporting mixed-platform remote execution.

The test-wrapper will provide no tools for tests. If a test needs functionality available for Bash scripts, then the test ought to be a sh_test. A generic Skylark test rule can use only what the "ctx" object exposes, for examples actions in ctx.actions [1]. Should the test use ctx.actions.run_shell, Bazel will check that it knows about a shell interpreter, and report an error if not. Unfortunately there is no shell toolchain yet, so the test can't yet express that it needs e.g. /usr/bin/grep, this will be a separate effort [2].

Vinayak: I don't follow what you suggest about Docker, could you elaborate please?


Thanks for your reviews!

Cheers,
Laszlo

--
László Csomor | Software Engineer | laszlo...@google.com

Google Germany GmbH | Erika-Mann-Str. 33 | 80636 München | Germany
Registergericht und -nummer: Hamburg, HRB 86891
Sitz der Gesellschaft: Hamburg
Geschäftsführer: Paul Manicle, Halimah DeLaine Prado

László Csomor

unread,
Aug 6, 2018, 7:42:53 AM8/6/18
to Vinayak Bansal, Nicolas Lopez, foundry...@google.com, Dmitry Lomov, Ulf Adams, Laurent Le Brun, bazel-discuss, bazel-dev
Come to think of it, I don't know how we could build a Linux version of Bazel while cross-compiling the test wrapper for Windows. Thus I don't know how to include the Windows test-wrapper in Linux Bazel binaries. (We could of course build the test wrapper separately, but then building a Bazel release requires an extra-build step.)

--
László Csomor | Software Engineer | laszlo...@google.com

Google Germany GmbH | Erika-Mann-Str. 33 | 80636 München | Germany
Registergericht und -nummer: Hamburg, HRB 86891
Sitz der Gesellschaft: Hamburg
Geschäftsführer: Paul Manicle, Halimah DeLaine Prado


Jeremy Sharpe

unread,
Aug 7, 2018, 11:02:17 AM8/7/18
to László Csomor, Vinayak Bansal, Nicolas Lopez, Foundry Windows, Dmitry Lomov, Ulf Adams, Laurent Le Brun, bazel-...@googlegroups.com, baze...@googlegroups.com
Leaving aside Skylark, I'm guessing genrules will still require bash so we'll need to keep it in the Windows RBE toolchain. Still, this is a good step. 

To view this discussion on the web visit https://groups.google.com/a/google.com/d/msgid/foundry-windows/CAJ5fxHKG2j4jX7mJrGS4uwjWSOvyrktRCOb-xNuemKG2nw4_Ww%40mail.gmail.com.

László Csomor

unread,
Aug 7, 2018, 11:20:53 AM8/7/18
to Jeremy Sharpe, Vinayak Bansal, Nicolas Lopez, foundry...@google.com, Dmitry Lomov, Ulf Adams, Laurent Le Brun, bazel-discuss, bazel-dev
Yes, I agree.  Solving genrules is a separate issue, we'll cross that bridge when we get there.



--
László Csomor | Software Engineer | laszlo...@google.com

Google Germany GmbH | Erika-Mann-Str. 33 | 80636 München | Germany
Registergericht und -nummer: Hamburg, HRB 86891
Sitz der Gesellschaft: Hamburg
Geschäftsführer: Paul Manicle, Halimah DeLaine Prado


Nicolas Lopez

unread,
Aug 7, 2018, 11:35:52 AM8/7/18
to László Csomor, Jeremy Sharpe, Vinayak Bansal, foundry...@google.com, Dmitry Lomov, Ulf Adams, Laurent Le Brun, bazel-discuss, bazel-dev
On Tue, Aug 7, 2018 at 11:20 AM László Csomor <laszlo...@google.com> wrote:
Yes, I agree.  Solving genrules is a separate issue, we'll cross that bridge when we get there.



--
László Csomor | Software Engineer | laszlo...@google.com

Google Germany GmbH | Erika-Mann-Str. 33 | 80636 München | Germany
Registergericht und -nummer: Hamburg, HRB 86891
Sitz der Gesellschaft: Hamburg
Geschäftsführer: Paul Manicle, Halimah DeLaine Prado


On Tue, Aug 7, 2018 at 5:02 PM Jeremy Sharpe <jsh...@google.com> wrote:
Leaving aside Skylark, I'm guessing genrules will still require bash so we'll need to keep it in the Windows RBE toolchain. Still, this is a good step. 

On Mon, Aug 6, 2018 at 7:42 AM László Csomor <laszlo...@google.com> wrote:
Come to think of it, I don't know how we could build a Linux version of Bazel while cross-compiling the test wrapper for Windows. Thus I don't know how to include the Windows test-wrapper in Linux Bazel binaries. (We could of course build the test wrapper separately, but then building a Bazel release requires an extra-build step.)
 
This is exactly what we do for ijar / singlejar binaries, so sounds reasonable to do it for the test wrapper.

Vinayak Bansal

unread,
Aug 8, 2018, 3:02:10 PM8/8/18
to Nicolas Lopez, László Csomor, Jeremy Sharpe, foundry...@google.com, Dmitry Lomov, Ulf Adams, Laurent Le Brun, bazel-...@googlegroups.com, baze...@googlegroups.com
Vinayak: I don't follow what you suggest about Docker, could you elaborate please? >> I misspoke. I meant shell toolchain can solve the problem. But, as you pointed out, we don't have one at the moment
Reply all
Reply to author
Forward
0 new messages