[Bazel] feature flags for the Emscripten toolchain about emsdk HOT 18 OPEN

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

from emsdk.

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.

from emsdk.

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

from emsdk.

Go for it!

from emsdk.

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

from emsdk.

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

from emsdk.

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.

from emsdk.

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

from emsdk.

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.

from emsdk.

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?

from emsdk.

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

from emsdk.

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

from emsdk.

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

from emsdk.

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

from emsdk.

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

from emsdk.

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.

from emsdk.

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.

from emsdk.

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.

from emsdk.