Skip to content

wasm_cc_binary: Use WASI OS for standalone builds #1262

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

Merged
merged 2 commits into from
Jul 28, 2023
Merged

wasm_cc_binary: Use WASI OS for standalone builds #1262

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
9c2f9d2
wasm_cc_binary: Specify a default OS. Allow users to override platform.
martijneken Jul 28, 2023
310ba94
Rework platform selection to trigger os:wasi off standalone attr
martijneken Jul 28, 2023
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/BUILD
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -82,5 +82,6 @@ platform(
name = "platform_wasm",
constraint_values = [
"@platforms//cpu:wasm32",
"@platforms//os:wasi",
Copy link
Collaborator

Choose a reason for hiding this comment

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

definitely against this. the os is more appropriately emscripten, although that doesn't exist in bazel's upstream os list.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ACK, added as platform_wasi instead

],
)
5 changes: 4 additions & 1 deletion bazel/emscripten_toolchain/wasm_cc_binary.bzl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -35,7 +35,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": ["@emsdk//:platform_wasm"],
"//command_line_option:platforms": [attr.platform],
"//command_line_option:custom_malloc": "@emsdk//emscripten_toolchain:malloc",
}

Expand Down Expand Up @@ -82,6 +82,9 @@ _WASM_BINARY_COMMON_ATTRS = {
"exit_runtime": attr.bool(
default = False,
),
"platform": attr.label(
default = "@emsdk//:platform_wasm",
Copy link
Collaborator

Choose a reason for hiding this comment

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

instead of giving the user the ability to input an arbitrary platform string, what if we always set the platform to "emsdk//:platform_wasi" if attr.standalone == True and defaulted to "@emsdk//:platform_wasm" otherwise? if you want wasi, you're always going to set standalone. I don't know that the reverse is true, but I think it is, and we can try it and see if anyone complains (I suspect they will not).

Copy link
Contributor Author

@martijneken martijneken Jul 28, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

Sounds good, toggled off of attr.standalone.

),
"threads": attr.string(
default = "_default",
values = ["_default", "emscripten", "off"],
Expand Down