-
Notifications
You must be signed in to change notification settings - Fork 683
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
Bazel: @emsdk
is broken with --incompatible_enable_cc_toolchain_resolution
#984
Comments
This is definitely on the higher end of my list of things to do. If no one beats me to it, I'll probably have time Q2. |
cc @trybka |
Very excited for this! Would love to help test a branch once the time comes. @PiotrSikora what I have been doing as a workaround, in .bazelrc, is to set:
And then building with |
@bkotsopoulossc thanks for the suggestion, but that doesn't really work, since we use output of |
@PiotrSikora neat, could you share more details on what the cc_test is doing? Are you using gtest or similar frameworks? I wonder if you even need to depend on the wasm_cc_binary at all, or if you could just have cc_test depend on cc_library or similar? |
Do platform mappings work as a stop gap? |
I think platform mappings as @jesseschalken mentions would work here. As far as emsdk goes, I think in the interim we may want the transition to do both (set crosstool_top / cpu and platform), since incompatible_enable_cc_toolchain_resolution is not yet universal. AIs (as I see them): |
@bkotsopoulossc sorry, I've missed this earlier - we're using this in Envoy and Proxy-Wasm C++ SDK, where we're testing hostcalls exposed by the runtime to the Wasm modules, so using
I couldn't get it to work by itself in the past. The issue I'm facing can be reproduced using this branch (migration to $ git clone -b emsdk_resolution https://github.com/PiotrSikora/envoy.git envoy-emsdk
$ cd envoy-emsdk Normal build works fine (target compiled using $ bazel build //test/extensions/common/wasm/test_data:test_cpp.wasm
[...]
Target //test/extensions/common/wasm/test_data:test_cpp.wasm up-to-date:
bazel-bin/test/extensions/common/wasm/test_data/test_cpp.wasm Using $ bazel build --incompatible_enable_cc_toolchain_resolution //test/extensions/common/wasm/test_data:test_cpp.wasm
[...]
ERROR: .../envoy-emsdk/test/extensions/common/wasm/test_data/BUILD:68:21: Compiling test/extensions/common/wasm/test_data/test_cpp.cc failed: (Exit 1): gcc failed: error executing command Same with $ cat platforms.map
platforms:
//bazel:wasm32
--cpu=wasm32
flags:
--cpu=wasm32
//bazel:wasm32 $ bazel build --incompatible_enable_cc_toolchain_resolution --platform_mappings=./platforms.map //test/extensions/common/wasm/test_data:test_cpp.wasm
ERROR: .../envoy-emsdk/test/extensions/common/wasm/test_data/BUILD:68:21: Compiling test/extensions/common/wasm/test_data/test_cpp.cc failed: (Exit 1): gcc failed: error executing command Patching ERROR: .../envoy-emsdk/test/extensions/common/wasm/test_data/BUILD:68:21: While resolving toolchains for target //test/extensions/common/wasm/test_data:proxy_wasm_test_cpp: No matching toolchains found for types @bazel_tools//tools/cpp:toolchain_type. Maybe --incompatible_use_cc_configure_from_rules_cc has been flipped and there is no default C++ toolchain added in the WORKSPACE file? See https://github.com/bazelbuild/bazel/issues/10134 for details and migration instructions. So it looks that a few more things are needed (see @trybka's list above), and |
Fixes emscripten-core#984. However, this requires `--incompatible_enable_cc_toolchain_resolution` and no longer supports `--crosstool_top=...`. I'm not sure how to support both at the same time.
Fixes emscripten-core#984. However, this requires `--incompatible_enable_cc_toolchain_resolution` and no longer supports `--crosstool_top=...`. I'm not sure how to support both at the same time.
The big change here is having the C++ toolchain use Bazel platforms instead of the C++ specific flags/setup. In Bazel, platforms are a general purpose way to define things like os, cpu architecture, etc. We were not using platforms previously, because the best documentation at the time focused on the old ways. However, the old ways were clumsy/difficult when trying to manage cross-compilation, specifically when trying to have a Mac host trigger a build on our Linux RBE system targeting a Linux x64 system. Thus, rather than keep investing in the legacy system, this CL migrates us to using platforms where possible. Suggested background reading to better understand this CL: - https://bazel.build/concepts/platforms-intro - https://bazel.build/docs/platforms - https://bazel.build/docs/toolchains#registering-building-toolchains The hermetic toolchain itself is not changing in this CL (and likely does not need to), only how we tell Bazel about it (i.e. registering it) and how Bazel decides to use it (i.e. resolving toolchains). Here is my understanding of how platforms and toolchains interact (supported by some evidence from [1][2]) - Bazel needs to resolve platforms for the Host, Execution, and Target. - If not specified via flags, these are the machine from which Bazel is invoked, aka "@local_config_platform//:host". - With this CL, the Host could be a Mac laptop, the Execution platform is our Linux RBE pool, and the Target is "a Linux system with a x64 CPU" - To specify the Host, that is, describe to Bazel the capabilities of the system it is running on, one can set --host_platform [3] with a label pointing to a platform() containing the appropriate settings. Tip: have this platform inherit from @local_config_platform//:host so it can add to any of the constraint_settings and constraint_values that Bazel deduces automatically. - To specify the Target platform(s), that is, the system on which a final output resides and can execute, one can set the --platforms flag with a label referencing a platform(). - Bazel will then choose an execution platform to fulfill that request. Bazel will look through a list of available platforms, which can be augmented* with the --extra_execution_platforms. Platforms specified by this flag will be considered higher than the default platforms! - Having selected the appropriate platforms, Bazel now needs to select a toolchain to actually run the actions of the appropriate type. - Bazel looks through the list of available toolchains and finds one that "matches" the Execution and the Target platform. This means, the toolchain's exec_compatible_with is a strict subset of the Execution platform and the toolchain's target_compatible_with is a strict subset of the Target platform. To register toolchains* (i.e. add them to the resolution list), we use --extra_toolchains. Once Bazel finds a match, it stops looking. Using --toolchain_resolution_debug=".*" makes Bazel log how it is resolving these toolchains and what execution platform it picked. * We can also register execution platforms and toolchains in WORKSPACE.bazel [4], but the flags come with higher priority and that made resolution a bit tricky. Also, when we want to conditionally add them (e.g. --config=linux_rbe), we cannot remove them conditionally in the WORKSPACE.bazel file. The above resolution flow directly necessitated the changes in this CL. Example usage of the new configs and platforms: # Can be run on a x64 Linux host and uses the hermetic toolchain. bazel build //:skia_public # Can be run on Mac or Linux and uses the Linux RBE system along # with the hermetic toolchain to compile a binary for Linux x64. bazel build //:skia_public --config=linux_rbe --config=for_linux_x64 # Shorthand for above bazel build //:skia_public --config=for_linux_x64_with_rbe Notice we don't have to type out --config=clang_linux anymore! That was due to me reading the Bazel docs more carefully and realizing we can set options for *all* Bazel build commands. Current Limitations: - Targets which require a py_binary (e.g. Dawn's genrules) will not work on RBE when cross compiling because the python runtime we download is for the host machine, not the executor. This means //example:hello_world_dawn does not work on Mac when cross-compiling via linux_rbe. - Mac M1 linking not quite working with SkOpts settings. Probably need to set -target [5] Suggested Review order: - toolchain/BUILD.bazel Notice how we do away with cc_toolchain_suite for toolchain. These have the same role: giving Bazel the information about where a toolchain can run. The platforms one is more expressive (IMO), allowing us to say both where to run the toolchain and what it can make. In order to more easily force the use of our hermetic toolchain, but also allow the hermetic toolchain to be used on RBE, we specify "use_hermetic_toolchain" only on the target, because the RBE image does not have the hermetic toolchain on it by default (but can certainly run it). - bazel/platform/BUILD.bazel to see the custom constraint_setting and corresponding constraint_value. The names for both of these are completely arbitrary - they do not need to have any deeper meaning or relation to any file or Docker image or system or any other constraints. Think of the constraint_setting as an Enum and the constraint_value being the one and only member. We need to pass around a constant value, not a type, so we need to provide the constraint_value (e.g. in toolchain/BUILD.bazel) but not a constraint_setting. However we need a constraint_setting declared so we can make a constraint_value of that "type". Notice the platform declared here - it allows us to force Bazel to use the hermetic toolchain because of the extra constraint_value. - .bazelrc I set a few flags that will be on for all bazel build commands. Importantly, this causes the C++ build logic to use platforms and not the old, bespoke way. I also found a way to avoid using the local toolchain on the host, which will hopefully lead to clearer errors if platforms are mis-specified instead of odd compile errors because the host toolchain is too old or something. There are also a few RBE settings tweaked to be a bit more modern, as well the new shorthands for specifying target platforms (e.g. for_linux_x64). - bazel/buildrc where we have to turn off the platforms logic for emscripten emscripten-core/emsdk#984 - bazel/rbe/BUILD.bazel for a fix in the platform description that makes it work on Mac. - Notice that _m1 has been removed from the mac-related toolchain files because the same toolchain should work on both architectures. - All other changes in any order. [1] https://bazel.build/docs/toolchains#debugging-toolchains [2] https://bazel.build/docs/toolchains#toolchain-resolution [3] https://bazel.build/reference/command-line-reference [4] https://bazel.build/docs/toolchains#registering-building-toolchains [5] https://github.com/google/skia/blob/17dc3f16fc78477503ba1ef484c3b47bc3aab893/gn/skia/BUILD.gn#L258-L271 Change-Id: I515c114099d659639a808f74e47d489a68b7af62 Bug: skia:12541 Reviewed-on: https://skia-review.googlesource.com/c/skia/+/549737 Reviewed-by: Erik Rose <erikrose@google.com> Reviewed-by: Jorge Betancourt <jmbetancourt@google.com>
Fixes emscripten-core#984. However, this requires `--incompatible_enable_cc_toolchain_resolution` and no longer supports `--crosstool_top=...`. I'm not sure how to support both at the same time.
Fixes emscripten-core#984. However, this requires `--incompatible_enable_cc_toolchain_resolution` and no longer supports `--crosstool_top=...`. I'm not sure how to support both at the same time.
Fixes emscripten-core#984. However, this requires `--incompatible_enable_cc_toolchain_resolution` and no longer supports `--crosstool_top=...`. I'm not sure how to support both at the same time.
This was already discussed in #766 (comment), but that PR was merged without a fix, and this isn't tracked anywhere else.
Platforms-based toolchain resoution (
--incompatible_enable_cc_toolchain_resolution
) is the future (bazelbuild/bazel#7260), and majority of Bazel rules/toolchains already support it, but@emsdk
doesn't (build falls back to using the default CC toolchain), which breaks cross-compilation in any Bazel workspace that uses@emsdk
.cc @walkingeyerobot @celentes @bkotsopoulossc
The text was updated successfully, but these errors were encountered: