Skip to content

[bazel] Switch to platforms-based toolchain resolution #1036

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
merged 4 commits into from
Nov 8, 2022
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
1 change: 1 addition & 0 deletions bazel/.bazelrc
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
build --incompatible_enable_cc_toolchain_resolution
7 changes: 7 additions & 0 deletions bazel/BUILD
Original file line number Diff line number Diff line change
Expand Up @@ -66,3 +66,10 @@ alias(
"//conditions:default": ":empty",
}),
)

platform(
name = "platform_wasm",
constraint_values = [
"@platforms//cpu:wasm32",
],
)
26 changes: 10 additions & 16 deletions bazel/README.md
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,9 @@ emsdk_deps()

load("@emsdk//:emscripten_deps.bzl", emsdk_emscripten_deps = "emscripten_deps")
emsdk_emscripten_deps(emscripten_version = "2.0.31")

load("@emsdk//:toolchains.bzl", "register_emscripten_toolchains")
register_emscripten_toolchains()
```
The SHA1 hash in the above `strip_prefix` and `url` parameters correspond to the git revision of
[emsdk 2.0.31](https://github.com/emscripten-core/emsdk/releases/tag/2.0.31). To get access to
Expand All @@ -26,8 +29,13 @@ parameter of `emsdk_emscripten_deps()`. Supported versions are listed in `revisi

## Building

### Using wasm_cc_binary (preferred)
First, write a new rule wrapping your `cc_binary`.
Put the following line into your `.bazelrc`:

```
build --incompatible_enable_cc_toolchain_resolution
```

Then write a new rule wrapping your `cc_binary`.

```
load("@rules_cc//cc:defs.bzl", "cc_binary")
Expand All @@ -54,17 +62,3 @@ and all of its dependencies, and does not require amending `.bazelrc`. This
is the preferred way, since it also unpacks the resulting tarball.

See `test_external/` for an example using [embind](https://emscripten.org/docs/porting/connecting_cpp_and_javascript/embind.html).

### Using --config=wasm

Put the following lines into your `.bazelrc`:
```
build:wasm --crosstool_top=@emsdk//emscripten_toolchain:everything
build:wasm --cpu=wasm
build:wasm --host_crosstool_top=@bazel_tools//tools/cpp:toolchain
```

Simply pass `--config=wasm` when building a normal `cc_binary`. The result of
this build will be a tar archive containing any files produced by emscripten.
See the [Bazel documentation](https://docs.bazel.build/versions/main/tutorial/cc-toolchain-config.html)
for more details
4 changes: 4 additions & 0 deletions bazel/WORKSPACE
Original file line number Diff line number Diff line change
Expand Up @@ -7,3 +7,7 @@ deps()
load(":emscripten_deps.bzl", "emscripten_deps")

emscripten_deps()

load(":toolchains.bzl", "register_emscripten_toolchains")

register_emscripten_toolchains()
7 changes: 2 additions & 5 deletions bazel/bazelrc
Original file line number Diff line number Diff line change
@@ -1,5 +1,2 @@
build:wasm --crosstool_top=//emscripten_toolchain:everything

build:wasm --cpu=wasm

build:wasm --host_crosstool_top=@bazel_tools//tools/cpp:toolchain
build:wasm --incompatible_enable_cc_toolchain_resolution
build:wasm --platforms=@emsdk//:platform_wasm
9 changes: 8 additions & 1 deletion bazel/emscripten_toolchain/BUILD.bazel
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
load(":crosstool.bzl", "emscripten_cc_toolchain_config_rule")
load(":toolchain.bzl", "emscripten_cc_toolchain_config_rule")

package(default_visibility = ["//visibility:public"])

Expand Down Expand Up @@ -86,6 +86,13 @@ cc_toolchain_suite(
},
)

toolchain(
name = "cc-toolchain-wasm",
target_compatible_with = ["@platforms//cpu:wasm32"],
toolchain = ":cc-compiler-wasm",
toolchain_type = "@bazel_tools//tools/cpp:toolchain_type",
)

py_binary(
name = "wasm_binary",
srcs = ["wasm_binary.py"],
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -1105,7 +1105,7 @@ emscripten_cc_toolchain_config_rule = rule(
attrs = {
"cpu": attr.string(mandatory = True, values = ["asmjs", "wasm"]),
"em_config": attr.label(mandatory = True, allow_single_file = True),
"emscripten_binaries": attr.label(mandatory = True),
"emscripten_binaries": attr.label(mandatory = True, cfg = "exec"),
"script_extension": attr.string(mandatory = True, values = ["sh", "bat"]),
},
provides = [CcToolchainConfigInfo],
Expand Down
2 changes: 1 addition & 1 deletion bazel/emscripten_toolchain/wasm_cc_binary.bzl
Original file line number Diff line number Diff line change
Expand Up @@ -32,7 +32,7 @@ def _wasm_transition_impl(settings, attr):
"//command_line_option:features": features,
"//command_line_option:dynamic_mode": "off",
"//command_line_option:linkopt": linkopts,
"//command_line_option:platforms": [],
"//command_line_option:platforms": ["@emsdk//:platform_wasm"],
"//command_line_option:custom_malloc": "@emsdk//emscripten_toolchain:malloc",
}

Expand Down
1 change: 1 addition & 0 deletions bazel/test_external/.bazelrc
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
build --incompatible_enable_cc_toolchain_resolution
4 changes: 4 additions & 0 deletions bazel/test_external/WORKSPACE
Original file line number Diff line number Diff line change
Expand Up @@ -10,3 +10,7 @@ deps()
load("@emsdk//:emscripten_deps.bzl", "emscripten_deps")

emscripten_deps()

load("@emsdk//:toolchains.bzl", "register_emscripten_toolchains")

register_emscripten_toolchains()
Copy link
Collaborator

Choose a reason for hiding this comment

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

Can/should we fold these two into a single step?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Seems like other rulesets tend to keep them separate. E.g. rules_go has go_rules_dependencies and go_register_toolchains , rules_nodejs has build_bazel_rules_nodejs_dependencies and node_repositories, rules_rust has rules_rust_dependencies and rust_register_toolchains.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I saw that #1068 spells this emsdk_register_toolchains. Based on other rulesets, it does seem like the prevailing convention is to prefix with the language or toolset, rather than the verb register. However, I would propose that we retain consistency with emscripten_deps, and therefore spell this emscripten_register_toolchains. @walkingeyerobot @jun-sheaf WDYT?

Choose a reason for hiding this comment

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

Hey @jfirebaugh! Your observation is correct. But just some information regarding this, I have upstreamed this to internal already. Hopefully by August I can get the internal toolchain open-sourced.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

What does "upstreamed to internal" mean?

Choose a reason for hiding this comment

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

I meant the patch has already been published within Google (the internal repo).

2 changes: 2 additions & 0 deletions bazel/toolchains.bzl
Original file line number Diff line number Diff line change
@@ -0,0 +1,2 @@
def register_emscripten_toolchains():
native.register_toolchains(str(Label("//emscripten_toolchain:cc-toolchain-wasm")))