Update wasm_bindgen to not use deprecated/unmaintained JS rules #2119
Conversation
|
@UebelAndre @illicitonion I saw you messing with this previously in #1311, could you review? |
dzbarsky
force-pushed
the
main
branch
3 times, most recently
from
479e07a to
568ecd7
Compare
|
I see there is one CI job failing but I'm not sure what's up with the RBE ubuntu16 config, any ideas? |
UebelAndre
left a comment
UebelAndre
left a comment
There was a problem hiding this comment.
Hi! Sorry for the delay in the review, thanks so much for putting this together! Do you know if it'd be possible to maintain both? I thought the wasm_bindgen rules only relied on the core rules_nodejs repo which is stated to be in use within rules_js.
|
Concretely, here is the current state of the world:
So if you want to use a newish nodejs version, you need to load your own (newer) It seems our options are:
Personally I think (2) is better, especially because projects that are not upgrading their rulesets probably won't upgrade this one either. I'm also happy to continue carrying this patch locally (I have other rules_rust patches anyway), but I figured this would help the majority of users who are using bazel for JS. Re: supporting both, since the issue here is a load of an undefined symbol, I'm not aware of a way to handle/recover that failure in Starlark, do you have any ideas? Failing that, I don't think it's feasible to support both.` |
| ) | ||
|
|
||
| load("@build_bazel_rules_nodejs//:index.bzl", "node_repositories") | ||
| load("@bazel_features//:deps.bzl", "bazel_features_deps") |
There was a problem hiding this comment.
What is this? The repo seems to come out of nowhere. Is it required to use aspect_rules_js?
There was a problem hiding this comment.
tl;dr This is a bit of a wart, yes.
The package we load the JsInfo provider from depends on that repo to define a config setting: https://github.com/aspect-build/rules_js/blob/63862adcfb565b0a53fb45c60dded0f8a7893c49/js/BUILD.bazel#L3C1-L15 This seems designed to support multiple Bazel versions so hopefully it can go away at some point. That repo also has a small usability trick - it automatically calls bazel_features_deps if you use npm_translate_lock, and since the vast majority of JS projects use npm packages, they don't have to do anything special here.
I do agree it's a bit weird to have to pull this in just to use the provider, @alexeagle perhaps you can consider rearranging the layout to make the providers more standalone?
There was a problem hiding this comment.
@UebelAndre What do you think? (BTW once we are using bzlmod I think this wouldn't be necessary)
There was a problem hiding this comment.
Hm, yeah I see why this is undesirable. We had to make those part of the public API in aspect-build/rules_js#1187 - but maybe rules_rust can load() the providers without having to load that package? providers.bzl is in its own file for this reason.
There was a problem hiding this comment.
The problem is the //docs:docs_deps rule in this repo. The bzl_library requires depending on "@aspect_rules_js//js:providers" to satisfy the stardoc rules/lint, which means anything loaded in //js:BUILD is a dependency. I think if you moved the providers.bzl to its own package (say //js/providers/defs.bzl) it would work.
There was a problem hiding this comment.
The bzl_library here can just depend on the bzl file directly in srcs to avoid that
There was a problem hiding this comment.
Not sure I'm following the suggestion, do you mean like so?
bzl_library(
name = "docs_deps",
srcs = [
"@aspect_rules_js//js:providers.bzl",
"@bazel_tools//tools:bzl_srcs",
"@com_google_protobuf//:bzl_srcs",
],
deps = [...]
)
That hits visibility issues: https://github.com/aspect-build/rules_js/blob/main/js/BUILD.bazel#L6-L9
Even ignoring visibility, it will have a problem with missing the js_info bzl
com.google.devtools.build.skydoc.SkydocMain$StarlarkEvaluationException: File /private/var/tmp/_bazel_zbarsky/e27223f43abb18db045469720c4d6593/sandbox/darwin-sandbox/28/execroot/rules_rust_docs/bazel-out/darwin-opt-exec-2B5CBBC6/bin/rust_repositories_md_stardoc.runfiles/aspect_rules_js/js/providers.bzl imported '//js/private:js_info.bzl', yet /private/var/tmp/_bazel_zbarsky/e27223f43abb18db045469720c4d6593/sandbox/darwin-sandbox/28/execroot/rules_rust_docs/bazel-out/darwin-opt-exec-2B5CBBC6/bin/rust_repositories_md_stardoc.runfiles/aspect_rules_js/js/private/js_info.bzl was not found.
There was a problem hiding this comment.
@UebelAndre ping? What can I do to get this PR moving?
There was a problem hiding this comment.
I’ll try to make time this weekend to do another pass. Sorry for the delay!
|
I have not forgotten about this change. I have uses of these rules that are still reliant on |
No problem, thanks for the heads up! The approach you're taking in the branch seems reasonable to me |
|
Closing in favor of #2284 |
3 participants
The providers the rule is using were deleted (I believe in bazel-contrib/rules_nodejs#3655).
It seems the community has standardized on https://github.com/aspect-build/rules_js as the future.
I removed the JS dependency from the bzl_lib as well, I didn't see an easy way to pull in the bzl_lib from providers, but please let me know if I missed something.
cd examples && bazel test //wasm/...works for me with this branch.