[Bazel] feature flags for the Emscripten toolchain

Question

[Bazel] feature flags for the Emscripten toolchain

allsey87 opened this issue 8 months ago · 18 comments

What is the current state of the feature flags for the Emscripten toolchain? It seems like this is in a bit of a half finished state with a lot of options missing. For example:

The exceptions feature that adds either -fno-exceptions or -fexceptions, but no way to set fwasm-exceptions.
Options like PROXY_TO_PTHREAD and WASM_BIGINT are completely missing
There is a feature called wasm_warnings_as_errors which just sets -Werror and seems on by default? I mean, this feature has nothing to do WebAssembly or Emscripten and doesn't seem like it should exist in the toolchain at all?

Is there interest in cleaning this up via some PRs?

Answer 1 · 2024-06-10T20:28:21.000Z

The bazel toolchain is just a best effort level of support, maintains by member of the community. Any PRs would be most welcome I imagine.

@walkingeyerobot is the de facto maintainer. (I think?)

Answer 2 · 2024-06-11T07:07:54.000Z

I would be happy to make some contributions! Is it just @walkingeyerobot that needs to sign off to get something merged?

Answer 3 · 2024-06-11T15:42:59.000Z

Somebody needs to sign off, and @walkingeyerobot is the most likely person if the PR is bazel related. If you are going to be making significant changes you might also want to discuss them here first to make sure they are in line with any plans.

Answer 4 · 2024-06-11T16:08:15.000Z

Just to test the mood about making these changes then, @walkingeyerobot would you sign off on a PR that:

Add support for -fwasm-exceptions, PROXY_TO_PTHREAD, and WASM_BIGINT
Removes the flag wasm_warnings_as_errors

Answer 5 · 2024-06-11T16:40:18.000Z

I'm generally happy to accept PRs. More specifically:

-fwasm-exceptions: Adding a feature for -fwasm-exceptions seems fine because it needs to be passed at both compile and link times.
PROXY_TO_PTHREAD / WASM_BIGINT: I'm not sure what's to be gained by adding bazel specific features for these. These are link-only flags, so I would expect users to simply add these to linkopts on their cc_binary.
Removing wasm_warnings_as_errors: I'm not sure the motivation here for removing it; this is a crosstool feature that is on by default but can be disabled by the user. I'm wary of changing defaults out from under people.

Answer 6 · 2024-06-11T17:03:39.000Z

I'm generally happy to accept PRs. More specifically:

-fwasm-exceptions: Adding a feature for -fwasm-exceptions seems fine because it needs to be passed at both compile and link times.

Agree

PROXY_TO_PTHREAD / WASM_BIGINT: I'm not sure what's to be gained by adding bazel specific features for these. These are link-only flags, so I would expect users to simply add these to linkopts on their cc_binary.

Agree

Removing wasm_warnings_as_errors: I'm not sure the motivation here for removing it; this is a crosstool feature that is on by default but can be disabled by the user. I'm wary of changing defaults out from under people.

What does this setting do?

Answer 7 · 2024-06-11T17:16:08.000Z

--features=wasm_warnings_as_errors sets -Werror for all compile actions: https://github.com/emscripten-core/emsdk/blob/main/bazel/emscripten_toolchain/toolchain.bzl#L1036-L1040

Answer 8 · 2024-06-11T17:18:52.000Z

--features=wasm_warnings_as_errors sets -Werror for all compile actions: https://github.com/emscripten-core/emsdk/blob/main/bazel/emscripten_toolchain/toolchain.bzl#L1036-L1040

I see.. does bazel not a have a builtin way to do that? I guess there is no harm in keeping it around if folks are using it. Is there a way to issue deprecation warnings for settings that we might want to remove in the future (assuming this one is redundant).

Answer 9 · 2024-06-11T17:27:26.000Z

--features=wasm_warnings_as_errors sets -Werror for all compile actions: https://github.com/emscripten-core/emsdk/blob/main/bazel/emscripten_toolchain/toolchain.bzl#L1036-L1040

I see.. does bazel not a have a builtin way to do that? I guess there is no harm in keeping it around if folks are using it.

I don't believe so; I'm pretty sure it has to be set on a per-toolchain basis.

Is there a way to issue deprecation warnings for settings that we might want to remove in the future (assuming this one is redundant).

Currently no such mechanism or policy exists.

Answer 10 · 2024-06-11T20:45:55.000Z

--features=wasm_warnings_as_errors sets -Werror for all compile actions: https://github.com/emscripten-core/emsdk/blob/main/bazel/emscripten_toolchain/toolchain.bzl#L1036-L1040

I see.. does bazel not a have a builtin way to do that? I guess there is no harm in keeping it around if folks are using it.

I don't believe so; I'm pretty sure it has to be set on a per-toolchain basis.

Nitpicking, but additionally, it is a bit strange having it called wasm_warnings_as_errors since the feature suggests that it has something to do with WebAssembly which is not the case.

Regarding PROXY_TO_PTHREAD / WASM_BIGINT, the motivation for adding them as features would be so that all Emscripten related settings are set in one place/apply to all targets in a workspace. I think it is counter-intuitive to have two different mechanisms for configuring the Emscripten settings, i.e., some defined through features while other through linkopts. What do you think?

Answer 11 · 2024-06-11T20:57:57.000Z

--features=wasm_warnings_as_errors sets -Werror for all compile actions: https://github.com/emscripten-core/emsdk/blob/main/bazel/emscripten_toolchain/toolchain.bzl#L1036-L1040

I see.. does bazel not a have a builtin way to do that? I guess there is no harm in keeping it around if folks are using it.

I don't believe so; I'm pretty sure it has to be set on a per-toolchain basis.

Nitpicking, but additionally, it is a bit strange having it called wasm_warnings_as_errors since the feature suggests that it has something to do with WebAssembly which is not the case.

Regarding PROXY_TO_PTHREAD / WASM_BIGINT, the motivation for adding them as features would be so that all Emscripten related settings are set in one place/apply to all targets in a workspace. I think it is counter-intuitive to have two different mechanisms for configuring the Emscripten settings, i.e., some defined through features while other through linkopts. What do you think?

I think its OK for most settings to simply be linker flags. The only time you really need special bazel setting is for when the setting applies to both compilation and linking. I think this is acceptable since compile-and-link settings are kind of already very different to link-only settings.

Reinventing all emscripten settings as different bazel swtiches would (I think) just add extra complexity for folks trying to move between toolchains, or coming from plain emscripten.

Answer 12 · 2024-06-11T20:58:50.000Z

(There are only a handfull of compile-and-link settings so this means we don't need to add too many special bazel options)

Answer 13 · 2024-06-12T20:42:14.000Z

About --features=wasm_warnings_as_errors I agree with @allsey87 that it shouldn't be in wasm toolchain since it is already possible to set that per target, via command line or inside .bazelrc with copts. Also what happens if a codebase has third party dependency that for some reason have warnings? A patch is not always possible in those cases.

Answer 14 · 2024-06-13T05:49:25.000Z

Also what happens if a codebase has third party dependency that for some reason have warnings

The fact that this is on by default is also frustrating. It recently caused a test in CPython's configure script to fail, reporting that libc was broken or unavailable.

Answer 15 · 2024-06-14T16:13:08.000Z

Ok, we can turn off wasm_warnings_as_errors by default if someone wants to send that PR. I did some digging and it looks like other bazel toolchains don't do this, so I'm happy to conform a bit better. I'd like to leave the feature there in case there are folks using it.

Answer 16 · 2024-06-14T17:06:31.000Z

Cool. @allsey87 seems like you have a lot on your hands at the moment, I can turnwasm_warnings_as_errors off if that's ok with you

Answer 17 · 2024-06-14T17:54:48.000Z

Go for it!

Answer 18 · 2024-06-17T12:59:54.000Z

I would strongly advocate disabling PRINTF_LONG_DOUBLE by default. Not only does this contradict the defaults in settings.js, but this causes a link error when combined with MAIN_MODULE=1 (see emscripten-core/emscripten#22102)

I would also add --oformat=js (output_format_js) to that list. I understand that disabling some of these flags may cause other peoples builds to break, but these flags should have never been set as defaults in the first place and violate the rule of least surprise.