[Bazel] feature flags for the Emscripten toolchain

[allsey87]

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:

1. The exceptions feature that adds either -fno-exceptions or -fexceptions, but no way to set fwasm-exceptions.
2. Options like PROXY_TO_PTHREAD and WASM_BIGINT are completely missing
3. 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?

[sbc100]

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?)

[allsey87]

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

[sbc100]

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.

[allsey87]

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

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

[walkingeyerobot]

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.

[sbc100]

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?

[walkingeyerobot]

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

[sbc100]

--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).

[walkingeyerobot]

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

[allsey87]

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

[sbc100]

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

[sbc100]

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

[WilliamIzzo83]

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.

[allsey87]

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.

[walkingeyerobot]

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.

[WilliamIzzo83]

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

[allsey87]

Go for it!

[allsey87]

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 https://github.com/emscripten-core/emscripten/issues/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.