Skip to content
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

wasm_cc_binary: Use WASI OS for standalone builds #1262

Merged
merged 2 commits into from
Jul 28, 2023

Conversation

martijneken
Copy link
Contributor

@martijneken martijneken commented Jul 28, 2023

Problem: proxy-wasm/proxy-wasm-cpp-sdk#157 (comment)

After discussion, we decided to target @platforms//os:wasi based on attr.standalone.

Problem: proxy-wasm/proxy-wasm-cpp-sdk#157 (comment)

This is solving the problem in two different ways. Please let me know
your thoughts about both approaches, as either will work.

Signed-off-by: Martijn Stevenson <[email protected]>
Copy link
Collaborator

@walkingeyerobot walkingeyerobot left a comment

Choose a reason for hiding this comment

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

There's definitely larger amounts of work that could be done here to support wasi, but I think this change can be unobtrusive enough that we can accept this in some form.

@@ -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

@@ -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

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.

Copy link
Collaborator

@walkingeyerobot walkingeyerobot left a comment

Choose a reason for hiding this comment

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

Thanks very much!

@walkingeyerobot walkingeyerobot enabled auto-merge (squash) July 28, 2023 20:56
@walkingeyerobot walkingeyerobot merged commit ef2a8e9 into emscripten-core:main Jul 28, 2023
@martijneken martijneken deleted the wasm-platform branch July 28, 2023 21:10
@martijneken martijneken changed the title wasm_cc_binary: Specify a default OS. Allow users to override platform. wasm_cc_binary: Use WASI OS for standalone builds Jul 28, 2023
shlomif pushed a commit to shlomif/emsdk that referenced this pull request Sep 29, 2023
…m. (emscripten-core#1262)

* wasm_cc_binary: Specify a default OS. Allow users to override platform.

Problem: proxy-wasm/proxy-wasm-cpp-sdk#157 (comment)

This is solving the problem in two different ways. Please let me know
your thoughts about both approaches, as either will work.

Signed-off-by: Martijn Stevenson <[email protected]>

* Rework platform selection to trigger os:wasi off standalone attr

Signed-off-by: Martijn Stevenson <[email protected]>

---------

Signed-off-by: Martijn Stevenson <[email protected]>
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.

2 participants