Adding basic windows support #329

jelmansouri · 2020-06-09T17:49:53Z

Adding binary extension support to the rust_binary rule, this allows us to build on windows.

jelmansouri · 2020-06-09T20:45:10Z

I had the change for more than a month, in the meantime this got added:
https://github.com/bazelbuild/rules_rust/pull/322/files
I'm not sure if it breaks my original change

damienmg

Hi sounds great to start seeing Windows support along. We have a lot of unixism in this repository though.

Would this be possible to add windows support to the CI too? You need to edit .bazelci/presubmit.yml, you can get the syntax of the file there.

damienmg · 2020-06-10T10:36:51Z

rust/private/rust.bzl

-        output = ctx.actions.declare_file(ctx.label.name + ".wasm")
-    else:
-        output = ctx.actions.declare_file(ctx.label.name)
+    output = ctx.actions.declare_file(ctx.label.name + toolchain.binary_ext)


I am surprised WASM test still pass as there is no declared extension corresponding: https://github.com/jelmansouri/rules_rust/blob/win_support/rust/platform/triple_mappings.bzl#L39

I don't really understand the WASM change though so I cannot tell if its missing tests or because the extension is not important.

So I have a couple changes coming up after to not use run_shell in the subset of rules we are using. (cc_library and cc_binary). Similarly to this change we did on go:
https://github.com/bazelbuild/rules_go/blob/master/go/private/rules/binary.bzl#L120
It's a great thing to add the CI and I'm really happy to do it!
Our goal is not to rely on genrules nor run_shell on Windows.

So what is using the Windows CI to fail is the use of run_shell, my other that I did in our codebase to not use run_shell, needs to be redone given that the command used to run the compiler is gotten more complicated

Submitted some WIP to have a green CI, do you think it's ok if I get a similar change as in the go rule linked above in the function I added (will need to shuffle stuff around)?

Yes this is fine. I also prefer genrule over run_shell as the later needs a posix shell.

damienmg · 2020-06-10T10:42:49Z

I had the change for more than a month, in the meantime this got added:
https://github.com/bazelbuild/rules_rust/pull/322/files
I'm not sure if it breaks my original change

I don't think it is broken, at least the CI is green.

jelmansouri · 2020-06-24T05:48:09Z

The change has two parts to it:
Removing run_shell based rules since rustc doesn't behave when when launched through an msys shell to build an msvc based abi target.
Adding windows extension where necessary

Note that C++ has been chosen to not introduce more dependencies (a cc toolchain is necessary anyways to build rust code), there is a similar wrapper written in python for protoc, I'm planning to migrate it to rust or C++.
out_dir_tar is supported through the tar util posix systems only, I will discuss with @damienmg a clear path to remove out_dir_tar support.

damienmg

The structure of the change looks good I think this is the correct direction, do you mind splitting this in 3 PR though?

The c++ wrapper tool
The cargo build script wrapper change
The rule change to support windows

?

Thanks!

jelmansouri · 2020-06-25T15:52:22Z

The c++ wrapper tool and the cargo build script wrapper change are hard to split, since the wrapper consumes the files generated by it, and I had to do small changes to the format of the output files.
I can split the format from removing the use of the shell command maybe you were referring to that?
The windows changes are overall the small part of the change and I can split them. Are you ok with that?

damienmg · 2020-06-25T16:05:55Z

The c++ wrapper tool and the cargo build script wrapper change are hard to split, since the wrapper consumes the files generated by it, and I had to do small changes to the format of the output files.
I can split the format from removing the use of the shell command maybe you were referring to that?
The windows changes are overall the small part of the change and I can split them. Are you ok with that?

I think we are on the same page. Let me try to rephrase a bit how I see the 3 independent changes:

Change in env format, which require a change in cargo_build_script and the rust wrapper
Replacement of the shell script by a C++ wrapper
Some windows support specifics

Depending on your preference you can include the drop of the tar file support in the C++ wrapper or as a separate change (it make sense to put it with the C++ wrapper as this comes with the reason for it). Dropping the support is just calling fail() if out_dir_tar is provided. With a message pointing out to cargo_build_script if people use it.

damienmg

Just some nits left, thanks a lot for following through for this change, very nice addition!

damienmg · 2020-07-01T08:18:18Z

.bazelci/presubmit.yml

    - "--enable_runfiles" # this is not enabled by default on windows and is necessary for the cargo build scripts
+    windows_targets: &windows_targets


Do we plan to have multiple windows run? Maybe there is no need for the windows_targets indirection?

It's to add test_targets, I'm still trying to fix rustdoct_test rules to be able to add them. But I prefer the change to go in first without this.

Ok no worries, I am going to merge this PR as-is this is already a huge improvement over the status-quo.

damienmg · 2020-07-01T08:19:05Z

rust/platform/triple_mappings.bzl

    "windows": ".exe",
+    "darwin": "",
+    # TODO(acmcarther): To be verified


Remove this TODO, darwin extension is definitely empty.

Suggested change

# TODO(acmcarther): To be verified

damienmg · 2020-07-01T08:19:23Z

rust/platform/triple_mappings.bzl

    "windows": ".dll",
+    # TODO(acmcarther): To be verified


Same here

Suggested change

# TODO(acmcarther): To be verified

damienmg · 2020-07-01T08:19:42Z

rust/platform/triple_mappings.bzl

    "windows": ".lib",
+    # TODO(acmcarther): To be verified


Idem

Suggested change

# TODO(acmcarther): To be verified

It failed to parse empty depenv files due to how 'split' function works.

Adding basic windows support

e5b4643

jelmansouri requested review from acmcarther, damienmg and mfarrugi as code owners June 9, 2020 17:49

googlebot added the cla: yes label Jun 9, 2020

Binary rule now uses ctx.label.name instead of crate_name

4621846

damienmg reviewed Jun 10, 2020

View reviewed changes

Jalal El Mansouri added 13 commits June 17, 2020 23:51

Adding tests support on windows

c8df2c5

Minimal tests that are currently supported

2f822a3

Merge branch 'master' into win_support

6dcce68

Using run instread of run_shell to support windows

8b71577

Removing test targets

4cea9b3

Other trial at getting the CI working

37e4936

Merging latest

f342f6d

WIP win support

1980f82

Merge remote-tracking branch 'upstream/master' into win_support

4762dff

Removing run_shell to improve win compatibility

03162cd

Fix linux build

a08a74c

Fix linux build

d69a314

Fix RBE build

fa66112

Jalal El Mansouri added 6 commits June 24, 2020 09:44

Small readability improvements

22a1972

Remove run_shell from cargo build script rules

9727ab9

Fix tests on windows

197216f

Add tests

5cfeb8b

Fix build

33fdeb1

Remove test needing runfiles

9c721cc

jelmansouri requested a review from damienmg June 25, 2020 05:20

damienmg reviewed Jun 25, 2020

View reviewed changes

damienmg mentioned this pull request Jun 26, 2020

Use a c++ wrapper to call rustc instead of shell #354

Merged

Jalal El Mansouri added 6 commits June 30, 2020 17:29

Merge remote-tracking branch 'upstream/master' into win_support

09e4ddc

Post merge fixes

e64283d

Additional post merge fixes

5aa2cbe

Fixing test list

fac98ec

Not using a bazelrc file

451ecc1

Fixing windows build

28e609f

damienmg approved these changes Jul 1, 2020

View reviewed changes

Remove todos

4d56b8e

damienmg merged commit d2c4b14 into bazelbuild:master Jul 1, 2020

a1ph pushed a commit to a1ph/rules_rust that referenced this pull request Jul 3, 2020

Fix .depenv file parsing after bazelbuild#329

2789162

It failed to parse empty depenv files due to how 'split' function works.

damienmg pushed a commit that referenced this pull request Jul 3, 2020

Fix .depenv file parsing after #329 (#367)

83962c9

It failed to parse empty depenv files due to how 'split' function works.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Adding basic windows support #329

Adding basic windows support #329

jelmansouri commented Jun 9, 2020

jelmansouri commented Jun 9, 2020

damienmg left a comment

damienmg Jun 10, 2020

jelmansouri Jun 17, 2020

jelmansouri Jun 18, 2020

jelmansouri Jun 18, 2020

damienmg Jun 18, 2020

damienmg commented Jun 10, 2020

jelmansouri commented Jun 24, 2020

damienmg left a comment

jelmansouri commented Jun 25, 2020

damienmg commented Jun 25, 2020

damienmg left a comment

damienmg Jul 1, 2020

jelmansouri Jul 1, 2020

damienmg Jul 1, 2020

damienmg Jul 1, 2020

damienmg Jul 1, 2020

damienmg Jul 1, 2020

		- "--enable_runfiles" # this is not enabled by default on windows and is necessary for the cargo build scripts
		windows_targets: &windows_targets

Adding basic windows support #329

Adding basic windows support #329

Conversation

jelmansouri commented Jun 9, 2020

jelmansouri commented Jun 9, 2020

damienmg left a 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

damienmg commented Jun 10, 2020

jelmansouri commented Jun 24, 2020

damienmg left a comment

Choose a reason for hiding this comment

jelmansouri commented Jun 25, 2020

damienmg commented Jun 25, 2020

damienmg left a 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