Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Bazel windows support #929

Merged
merged 1 commit into from
Dec 20, 2021
Merged

Bazel windows support #929

merged 1 commit into from
Dec 20, 2021

Conversation

zaucy
Copy link
Contributor

@zaucy zaucy commented Nov 13, 2021

closes #814

  • Updated scripts/update_bazel_workspace.sh to calculate sha from .zip archive for windows
  • Adds .bat file equivalent scripts for use on windows
  • Removes use of cp and file commands in favour of python modules
    • Introduces filetype dependency for wasm_binary.py instead of relying on file

@zaucy zaucy marked this pull request as ready for review November 13, 2021 07:34
Copy link
Collaborator

@sbc100 sbc100 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice one!

Mostly makes sense to me. The pip install thing seems a little heavy weight, but if its just one once on first use I guess its fine.

Will leave the final lgtm to @walkingeyerobot

@walkingeyerobot
Copy link
Collaborator

Haven't had a chance to dig into this fully yet, but looks good so far! I'll submit a full review later this week.

@walkingeyerobot
Copy link
Collaborator

Overall I'm happy with this. I'd like to eliminate the filetype dep, not because I think adding such a dep is bad, but because we shouldn't actually need to check the mimetype of the file. I'll try to get a PR that eliminates the need to check the mimetype entirely in the next day or two, which will eliminate the need for the dep.

For a bit more context, a long time ago this was purely an asmjs toolchain. As such, it would only output a single file, which was a js file. When wasm support was added, suddenly we needed to output the .wasm and the .js files, but cc_binary wouldn't support that, so we decided to roll them into a tar, leaving in place the logic that if only a single file had been outputted we would simply copy it. Looking back, it would have been simpler and better to just always tar the result.

@walkingeyerobot
Copy link
Collaborator

Ok, I made #936 to remove the need to add the filetype dep. Once that's merged you can integrate it with this PR.

I think link_wrapper.py also needs to be updated in this PR to be cross-platform. link_wrapper.py is always run as part of the toolchain and is what creates the tarball in the first place. wasm_binary.py is only run when using the wasm_cc_binary rule. After that's done and we resolve @trybka 's concerns this will be good to go.

@walkingeyerobot
Copy link
Collaborator

oh, and we need a test-bazel-windows on circleci

@walkingeyerobot
Copy link
Collaborator

Sorry for the delay, but you should be able to get away without the python dep now that #936 is merged

@zaucy
Copy link
Contributor Author

zaucy commented Dec 2, 2021

@walkingeyerobot I've rebased against the latest main branch and added a (untested) circle CI job for windows with bazel.

I didn't change link_wrapper.py to use the tarfile module only because I didn't have any trouble with it on my local windows machine, but I can make that change.

@walkingeyerobot walkingeyerobot enabled auto-merge (squash) December 6, 2021 18:58
@walkingeyerobot
Copy link
Collaborator

This is almost certainly stuck on circleci. @zaucy I think you have to log in to circleci (no paid account required or anything) and then rebase, and that should be enough of a kick to actually run the tests.

auto-merge was automatically disabled December 6, 2021 20:22

Head branch was pushed to by a user without write access

@zaucy zaucy marked this pull request as draft December 6, 2021 20:51
@walkingeyerobot
Copy link
Collaborator

Looks like the windows test can't find python. maybe it's pointing to the wrong path somewhere?

@zaucy
Copy link
Contributor Author

zaucy commented Dec 14, 2021

I couldn't seem to find if there was a python3.exe somewhere in the windows vm. I was planning on looking again this weekend sometime.

@zaucy zaucy marked this pull request as ready for review December 19, 2021 02:10
@zaucy zaucy marked this pull request as draft December 19, 2021 02:10
@zaucy zaucy marked this pull request as ready for review December 19, 2021 02:54
@zaucy
Copy link
Contributor Author

zaucy commented Dec 19, 2021

@walkingeyerobot The bazel CI is passing now. I used py -3 instead of python3 which should be available for most windows python users. I also removed a hardcoded python3 subprocess call and used sys.executable instead.

Hope those changes are acceptable :)

@walkingeyerobot walkingeyerobot merged commit 774b871 into emscripten-core:main Dec 20, 2021
@walkingeyerobot
Copy link
Collaborator

Thanks very much for this, and I appreciate your patience with the lengthy review!

@zaucy zaucy deleted the windows-support branch December 20, 2021 20:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Bazel toolchain does not work on Windows
4 participants