Skip to content

Update wasm_bindgen to not use deprecated/unmaintained JS rules #2119

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.

Sign up for GitHub

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

Closed
wants to merge 1 commit into from
+35 −30
Closed

Update wasm_bindgen to not use deprecated/unmaintained JS rules #2119

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion docs/BUILD.bazel
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,9 +7,9 @@ package(default_visibility = ["//visibility:private"])
bzl_library(
name = "docs_deps",
srcs = [
"@aspect_rules_js//js:providers",
"@bazel_tools//tools:bzl_srcs",
"@com_google_protobuf//:bzl_srcs",
"@rules_nodejs//nodejs:bzl",
],
deps = [
"@bazel_skylib//lib:paths",
Expand Down
8 changes: 8 additions & 0 deletions docs/WORKSPACE.bazel
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -37,6 +37,14 @@ rust_wasm_bindgen_dependencies()

rust_wasm_bindgen_register_toolchains()

load("@aspect_rules_js//js:repositories.bzl", "rules_js_dependencies")

rules_js_dependencies()

load("@bazel_features//:deps.bzl", "bazel_features_deps")

bazel_features_deps()

load("@bazel_tools//tools/build_defs/repo:http.bzl", "http_archive")

http_archive(
Expand Down
17 changes: 11 additions & 6 deletions examples/WORKSPACE.bazel
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -79,15 +79,20 @@ rust_repository_set(
# Examples dependencies
###############################################################################

http_archive(
name = "build_bazel_rules_nodejs",
sha256 = "c78216f5be5d451a42275b0b7dc809fb9347e2b04a68f68bad620a2b01f5c774",
urls = ["https://github.com/bazelbuild/rules_nodejs/releases/download/5.5.2/rules_nodejs-5.5.2.tar.gz"],
load("@aspect_rules_js//js:repositories.bzl", "rules_js_dependencies")

rules_js_dependencies()

load("@rules_nodejs//nodejs:repositories.bzl", "DEFAULT_NODE_VERSION", "nodejs_register_toolchains")

nodejs_register_toolchains(
name = "nodejs",
node_version = DEFAULT_NODE_VERSION,
)

load("@build_bazel_rules_nodejs//:index.bzl", "node_repositories")
load("@bazel_features//:deps.bzl", "bazel_features_deps")
Copy link
Collaborator

@UebelAndre UebelAndre Sep 4, 2023

Choose a reason for hiding this comment

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

What is this? The repo seems to come out of nowhere. Is it required to use aspect_rules_js?

Copy link
Contributor Author

@dzbarsky dzbarsky Sep 4, 2023

Choose a reason for hiding this comment

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

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?

Copy link
Contributor Author

@dzbarsky dzbarsky Sep 16, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

@UebelAndre What do you think? (BTW once we are using bzlmod I think this wouldn't be necessary)

Copy link
Contributor

@alexeagle alexeagle Sep 16, 2023

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

@dzbarsky dzbarsky Sep 16, 2023

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

@alexeagle alexeagle Sep 16, 2023

Choose a reason for hiding this comment

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

The bzl_library here can just depend on the bzl file directly in srcs to avoid that

Copy link
Contributor Author

@dzbarsky dzbarsky Sep 16, 2023

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

@dzbarsky dzbarsky Oct 3, 2023

Choose a reason for hiding this comment

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

@UebelAndre ping? What can I do to get this PR moving?

Copy link
Collaborator

@UebelAndre UebelAndre Oct 4, 2023

Choose a reason for hiding this comment

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

I’ll try to make time this weekend to do another pass. Sorry for the delay!


node_repositories()
bazel_features_deps()

http_archive(
name = "rules_foreign_cc",
Expand Down
4 changes: 2 additions & 2 deletions examples/wasm/BUILD.bazel
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,7 @@
# See the License for the specific language governing permissions and
# limitations under the License.

load("@build_bazel_rules_nodejs//:index.bzl", "nodejs_test")
load("@aspect_rules_js//js:defs.bzl", "js_test")
load("@rules_rust//rust:defs.bzl", "rust_binary", "rust_shared_library")
load("@rules_rust//wasm_bindgen:wasm_bindgen.bzl", "rust_wasm_bindgen")

Expand Down Expand Up @@ -65,7 +65,7 @@ rust_wasm_bindgen(
wasm_file = ":hello_world_lib_wasm",
)

nodejs_test(
js_test(
name = "hello_world_wasm_test",
data = [
":hello_world_bundler_wasm_bindgen",
Expand Down
8 changes: 3 additions & 5 deletions wasm_bindgen/providers.bzl
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,10 +1,8 @@
"""A module for re-exporting the providers used by the rust_wasm_bindgen rule"""

load(
"@rules_nodejs//nodejs:providers.bzl",
_DeclarationInfo = "DeclarationInfo",
_JSModuleInfo = "JSModuleInfo",
"@aspect_rules_js//js:providers.bzl",
_JsInfo = "JsInfo",
)

DeclarationInfo = _DeclarationInfo
JSModuleInfo = _JSModuleInfo
JsInfo = _JsInfo
9 changes: 5 additions & 4 deletions wasm_bindgen/repositories.bzl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -41,9 +41,10 @@ def rust_wasm_bindgen_dependencies():

maybe(
http_archive,
name = "rules_nodejs",
sha256 = "017e2348bb8431156d5cf89b6f502c2e7fcffc568729f74f89e4a12bd8279e90",
urls = ["https://github.com/bazelbuild/rules_nodejs/releases/download/5.5.2/rules_nodejs-core-5.5.2.tar.gz"],
name = "aspect_rules_js",
sha256 = "7b2a4d1d264e105eae49a27e2e78065b23e2e45724df2251eacdd317e95bfdfd",
strip_prefix = "rules_js-1.31.0",
url = "https://github.com/aspect-build/rules_js/releases/download/v1.31.0/rules_js-v1.31.0.tar.gz",
)

crate_repositories()
Expand All @@ -65,7 +66,7 @@ def rust_wasm_bindgen_register_toolchains(register_toolchains = True):
def rust_wasm_bindgen_repositories(register_default_toolchain = True):
"""Declare dependencies needed for [rust_wasm_bindgen](#rust_wasm_bindgen).

**Deprecated**: Use [rust_wasm_bindgen_dependencies](#rust_wasm_bindgen_depednencies) and [rust_wasm_bindgen_register_toolchains](#rust_wasm_bindgen_register_toolchains).
**Deprecated**: Use [rust_wasm_bindgen_dependencies](#rust_wasm_bindgen_dependencies) and [rust_wasm_bindgen_register_toolchains](#rust_wasm_bindgen_register_toolchains).

Args:
register_default_toolchain (bool, optional): If True, the default [rust_wasm_bindgen_toolchain](#rust_wasm_bindgen_toolchain)
Expand Down
17 changes: 5 additions & 12 deletions wasm_bindgen/wasm_bindgen.bzl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,11 +15,7 @@
"""Bazel rules for [wasm-bindgen](https://crates.io/crates/wasm-bindgen)"""

load("//rust:defs.bzl", "rust_common")
load(
"//wasm_bindgen:providers.bzl",
"DeclarationInfo",
"JSModuleInfo",
)
load("//wasm_bindgen:providers.bzl", "JsInfo")
load("//wasm_bindgen/private:transitions.bzl", "wasm_bindgen_transition")

_WASM_BINDGEN_DOC = """\
Expand Down Expand Up @@ -125,22 +121,19 @@ def _rust_wasm_bindgen_impl(ctx):
arguments = [args],
)

# Return a structure that is compatible with the deps[] of a ts_library.
# Return a structure that is compatible with the deps[] of the aspect JS/TS rules.
declarations = depset(ts_out)
es5_sources = depset(js_out)

return [
DefaultInfo(
files = depset(outputs),
),
DeclarationInfo(
JsInfo(
declarations = declarations,
transitive_declarations = declarations,
type_blocklisted_declarations = depset([]),
),
JSModuleInfo(
direct_sources = es5_sources,
sources = es5_sources,
transitive_declarations = declarations,
transitive_sources = es5_sources,
),
]

Expand Down