Skip to content

Use emsdk as external bazel dependency #766

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

Merged

Conversation

celentes
Copy link
Contributor

@celentes celentes commented Mar 19, 2021

  • Makes provided bazel rules look up @emsdk workspace instead of local workspace
  • Uses system-specific emscripten binaries instead of defaulting to linux
  • Provides macros for loading emsdk dependencies (nodejs and emscripten binaries)
  • Unhardcodes paths in bazel rules and .sh wrappers
  • update_bazel_workspace.sh now updates revisions.bzl
  • emscripten_deps() can be fed with specific emscripten version
  • Adds external usage test

Addresses #650 and #696

@sbc100 sbc100 requested a review from walkingeyerobot March 19, 2021 19:09
@sbc100
Copy link
Collaborator

sbc100 commented Mar 19, 2021

I'm not too famiilar with this stuff but it looks like there are a few different things going on here. If its possible to split into smaller changes that would great (maybe its isn't?).

@celentes
Copy link
Contributor Author

celentes commented Mar 19, 2021

Most of the changes are interlinked, I think it was possible to stop early on after externalising @emsdk (i.e. tests would pass but the change would be incomplete still), but after that it was a rollercoaster.
Specifically, unhardcoding paths in bazel setup would be a lot of repeated changes if it was to be split up.

@celentes
Copy link
Contributor Author

@bkotsopoulossc
I was gonna have that in this PR but stopped myself, it's enough changes for now.

To implement this, you need to do the toolchain as you said, then register it in workspace (ideally provided in deps) and also add platform in transition in wasm_cc_binary.bzl

@celentes
Copy link
Contributor Author

Another problematic bit is that @platforms//cpu:wasm64 is not exposed by default, needs latest @platforms pulled

@celentes
Copy link
Contributor Author

celentes commented Mar 24, 2021

And yes, the trick with strip_prefix is how i am using it in externals right now :)

I'll add a more comprehensive readme post merge, need commit hash

@bkotsopoulossc
Copy link
Contributor

Makes total sense about keeping the scope of this PR down. I wonder if there is an interest in trying to support the platforms use case later on though?

@celentes
Copy link
Contributor Author

Well, bazel docs blatantly state that this is the future and all new crossplatform jazz should be developed with that in mind. The reason why cc rules are behind is because some platforms (android, primarily) does not support it yet

@sbc100
Copy link
Collaborator

sbc100 commented Mar 24, 2021

Another problematic bit is that @platforms//cpu:wasm64 is not exposed by default, needs latest @platforms pulled

Wait are you trying to actually use wasm64? That is something that doesn't really exist yet, at least not at the emscripten level. Do you mean wasm32?

@celentes
Copy link
Contributor Author

I think it's just a platforms flag. It's not "real" architecture, it's more of a label. Should be done against wasm32 once we get to this

@bkotsopoulossc
Copy link
Contributor

I didn't actually try either, but didn't know if it made a difference since they seem more like labels like you mentioned? Anyways, good to know about using wasm32, thanks

@celentes
Copy link
Contributor Author

@bkotsopoulossc
Another problem with automatic toolchain resolution and bazel-emscripten is that if you just run stuff with resolved toolchain, it creates tarballs instead of correct files, which need to be unpacked prior to using them first.
This is caused by bazel having opinions on which files you can and cannot produce with cc rules: bazelbuild/bazel#2298

So until that gets resolved, you do need to use wasm_cc_binary rule to unpack them, and transition resolves the toolchain correctly without any extra input like platforms, cpu type etc.

@walkingeyerobot walkingeyerobot enabled auto-merge (squash) March 24, 2021 22:36
@celentes
Copy link
Contributor Author

so because test-bazel has been renamed, circleci expects it to pass

should I rename it back and have test-bazel and test-bazel-mac instead?

@sbc100
Copy link
Collaborator

sbc100 commented Mar 24, 2021

No thats OK, we just need to teach github about these new test names.

@sbc100
Copy link
Collaborator

sbc100 commented Mar 24, 2021

I temporarly removed "test-bazel" from the list of required steps to submit.

@walkingeyerobot if you are happy with this change I an land with with force

@sbc100
Copy link
Collaborator

sbc100 commented Mar 24, 2021

Oh looks like it worked on its own.

@sbc100 sbc100 closed this Mar 24, 2021
auto-merge was automatically disabled March 24, 2021 23:34

Pull request was closed

@celentes
Copy link
Contributor Author

Cheers!

@sbc100 sbc100 reopened this Mar 24, 2021
@sbc100
Copy link
Collaborator

sbc100 commented Mar 24, 2021

Damit.. I closed it by accident.

I'm not sure what you want the final commit message to look like. I'll leave that to you and/or @walkingeyerobot .

@sbc100
Copy link
Collaborator

sbc100 commented Mar 24, 2021

Are you good with the current PR title and description for the squashed change?

@celentes
Copy link
Contributor Author

Use emsdk as external bazel dependency #766 works for me

@celentes
Copy link
Contributor Author

Does what it says on the tin. I'm gonna add a readme update tomorrow

@sbc100 sbc100 merged commit c1589b5 into emscripten-core:master Mar 24, 2021
@bkotsopoulossc
Copy link
Contributor

Sorry to revive this, any further thoughts on platform-based toolchain resolution? RE: #766 (comment)

@walkingeyerobot
Copy link
Collaborator

I am strongly in favor of using bazel's platforms in this toolchain. The original creation of this toolchain (long before it was committed here) predated the concept of bazel platforms. Supporting the users of that version is my day job, making this a particularly hairy problem for me.

I will sadly not have time until at least Q3 (July) to start working on this, but I would be more than happy to help out with or review a design or PR for this in the meantime. If not, I will get around to doing this myself eventually (tm).

cc @trybka

radekdoulik referenced this pull request in dotnet/emsdk May 20, 2021
* Makes provided bazel rules look up @emsdk workspace instead of local workspace
* Uses system-specific emscripten binaries instead of defaulting to linux
* Provides macros for loading emsdk dependencies (nodejs and emscripten binaries)
* Unhardcodes paths in bazel rules and .sh wrappers
* `update_bazel_workspace.sh` now updates `revisions.bzl`
* `emscripten_deps()` can be fed with specific emscripten version
* Adds external usage test

Addresses #650 and #696
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.

4 participants