-
Notifications
You must be signed in to change notification settings - Fork 72
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
Update emsdk, work with Abseil + RE2 #157
Conversation
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 <mstevenson@google.com>
…m. (#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 <mstevenson@google.com> * Rework platform selection to trigger os:wasi off standalone attr Signed-off-by: Martijn Stevenson <mstevenson@google.com> --------- Signed-off-by: Martijn Stevenson <mstevenson@google.com>
Something funky's going on with (updated) emsdk and platform selection: Doesn't work here in proxy-wasm-cpp-sdk:
Works when rules_rust is in the workspace, seemingly via dummy_cc_toolchain:
|
Ah, just needed to add |
Looks like we need to update protobuf in order to update zlib in order to address madler/zlib#633. I'm able to build with the latest, zlib-1.2.13. |
BUILD
Outdated
@@ -13,7 +13,6 @@ cc_library( | |||
"proxy_wasm_api.h", | |||
"proxy_wasm_externs.h", | |||
], | |||
copts = ["-std=c++17"], |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
.bazelrc
flags are not carried over to the dependent projects, so you're effectively removing -std=c++17
flag when building this as part of other workspaces.
Side-note: With PRs such as this, it's usually a good idea to test it locally with proxy-wasm-cpp-host
and envoy
(e.g. using --override_repository=
flag), and then to open draft PRs pointing to an unmerged commit to verify that it doesn't break anything across those projects.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since the Emscripten update, I am unable to get Abseil (used by protobuf) building without workspace-level copts. AIUI the problem with copts on targets is that those don't propagate to the dependencies. Maybe at issue are transitions or toolchain changes? Extra strange because Emscripten specifies gnu++17 in its toolchain here.
Sounds like you want me to keep digging at this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't mind adding workspace-level cxxopts
to .bazelrc
, that's not what this comment is about.
I only object to removing them from cc_library
targets, since you're effectively removing them from builds in the dependent projects (proxy-wasm-cpp-host
, envoy
, istio-proxy
, etc.), which don't inherit the workspace-level cxxopts
from this workspace.
I suspect most of them build with C++17 anyway, but it's a long chain of dependencies, so it seems like an unnecessary risk.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Gotcha thanks, I plan to do two things next:
- figure out if there are any build workarounds using cc_library targets (preferred)
- verify that the repos you mentioned continue to build/test as is
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
figure out if there are any build workarounds using cc_library targets (preferred)
I struggled with the protobuf v23.4 update for a while. I see at least some of the dependencies (such as the protoc compiler) not being built with emscripten, and not respecting per-target copts either. However, ~everything in protobuf depends on Abseil, and Abseil does require specifying c++14 or higher. In short, I didn't find a good a way to avoid .bazelrc flags with recent versions of protobuf. There is also other fallout from the protobuf update, such as the utf8_range build error and a required update to bazelversion.
I've reverted these changes in the most recent commit. What I propose instead:
- scope this PR more tightly: only update emsdk and disable a zlib warning in the emsdk transition
- [next] work on breaking CppHost --> CppSdk dependency
- [then] try again to update protobuf in CppSdk
verify that the repos you mentioned continue to build/test as is
Working on this now.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Bleh, I can't get my emcmake toolchain (martijneken/emsdk@a871cd0) to be resolved properly. Bazel uses @cmake-3.23.2-linux-x86_64
instead, because this toolchain uses exec_compatible_with
qualifiers, rather than target_compatible_with
:
toolchain(
name = "cmake-3.23.2-linux-x86_64_toolchain",
exec_compatible_with = ["@platforms//cpu:x86_64", "@platforms//os:linux"],
toolchain = "@cmake-3.23.2-linux-x86_64//:cmake_tool",
toolchain_type = "@rules_foreign_cc//toolchains:cmake_toolchain",
)
$ bazelisk build --toolchain_resolution_debug=zlib //test/extensions/filters/http/wasm/test_data:test_cpp.wasm
...
INFO: ToolchainResolution: Type @rules_foreign_cc//toolchains:cmake_toolchain: execution platform @local_config_platform//:host: Rejected toolchain @cmake-3.23.2-linux-aarch64//:cmake_tool; mismatching values: arm64
INFO: ToolchainResolution: Type @rules_foreign_cc//toolchains:cmake_toolchain: target platform @emsdk//:platform_wasm: execution @local_config_platform//:host: Selected toolchain @cmake-3.23.2-linux-x86_64//:cmake_tool
INFO: ToolchainResolution: Type @rules_foreign_cc//toolchains:cmake_toolchain: target platform @emsdk//:platform_wasm: execution platform @local_config_platform//:host: Skipping toolchain @cmake-3.23.2-macos-universal//:cmake_tool; execution platform already has selected toolchain
INFO: ToolchainResolution: Type @rules_foreign_cc//toolchains:cmake_toolchain: target platform @emsdk//:platform_wasm: execution platform @local_config_platform//:host: Skipping toolchain @cmake-3.23.2-windows-i386//:cmake_tool; execution platform already has selected toolchain
INFO: ToolchainResolution: Type @rules_foreign_cc//toolchains:cmake_toolchain: target platform @emsdk//:platform_wasm: execution platform @local_config_platform//:host: Skipping toolchain @cmake-3.23.2-windows-x86_64//:cmake_tool; execution platform already has selected toolchain
INFO: ToolchainResolution: Type @rules_foreign_cc//toolchains:cmake_toolchain: target platform @emsdk//:platform_wasm: execution platform @local_config_platform//:host: Skipping toolchain @rules_foreign_cc//toolchains:built_cmake; execution platform already has selected toolchain
INFO: ToolchainResolution: Type @rules_foreign_cc//toolchains:cmake_toolchain: target platform @emsdk//:platform_wasm: execution platform @local_config_platform//:host: Skipping toolchain @rules_foreign_cc//toolchains:preinstalled_cmake; execution platform already has selected toolchain
INFO: ToolchainResolution: Type @rules_foreign_cc//toolchains:cmake_toolchain: target platform @emsdk//:platform_wasm: execution platform @local_config_platform//:host: Skipping toolchain @emsdk//emscripten_toolchain:cmake-compiler-wasm; execution platform already has selected toolchain
INFO: ToolchainResolution: Target platform @emsdk//:platform_wasm: Selected execution platform @local_config_platform//:host,
type @rules_foreign_cc//toolchains:make_toolchain -> toolchain @rules_foreign_cc//toolchains:built_make,
type @bazel_tools//tools/cpp:toolchain_type -> toolchain @rules_rust//rust/private/dummy_cc_toolchain:dummy_cc_wasm32_toolchain_cc,
type @rules_foreign_cc//toolchains:m4_toolchain -> toolchain @rules_foreign_cc//toolchains:preinstalled_m4,
type @rules_foreign_cc//toolchains:cmake_toolchain -> toolchain @cmake-3.23.2-linux-x86_64//:cmake_tool,
type @rules_foreign_cc//foreign_cc/private/framework:shell_toolchain -> toolchain @rules_foreign_cc_framework_toolchain_linux//:commands,
type @rules_foreign_cc//toolchains:ninja_toolchain -> toolchain @ninja_1.11.0_linux//:ninja_tool,
type @rules_foreign_cc//toolchains:pkgconfig_toolchain -> toolchain @rules_foreign_cc//toolchains:preinstalled_pkgconfig
Side note: there's something wrong with cpp:toolchain_type too, since it resolves to a dummy toolchain from Rust:
# When compiling Rust code for wasm32, we avoid linking to cpp code so we introduce a dummy cc
# toolchain since we know we'll never look it up.
# TODO(jedmonds@spotify.com): Need to support linking C code to rust code when compiling for wasm32.
toolchain(
name = "dummy_cc_wasm32_toolchain",
target_compatible_with = ["//rust/platform/cpu:wasm32"],
toolchain = ":dummy_cc_wasm32_toolchain_cc",
toolchain_type = "@bazel_tools//tools/cpp:toolchain_type",
)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm pretty sure the above are Emscripten issues, so I've filed emscripten-core/emsdk#1273
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Resolved the emscripten issues with emscripten-core/emsdk#1274 (at least for now 😄)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks much! Seemingly the last remaining issue in envoyproxy/envoy#29118 is with the emsdk patch file that removes npm targets: https://github.com/proxy-wasm/proxy-wasm-cpp-sdk/blob/master/bazel/emsdk.patch
I'll have to chase down why that was added and why it's breaking now.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The NPM targets were removed, because those downloads were failing a lot (like, completely breaking Envoy's CI) and they were done outside of Bazel's caching system, so they were not cached, and coincidentally those targets were already included in emsdk's tarballs, so removing those fixed the CI at the time.
BUILD
Outdated
@@ -13,7 +13,6 @@ cc_library( | |||
"proxy_wasm_api.h", | |||
"proxy_wasm_externs.h", | |||
], | |||
copts = ["-std=c++17"], |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't mind adding workspace-level cxxopts
to .bazelrc
, that's not what this comment is about.
I only object to removing them from cc_library
targets, since you're effectively removing them from builds in the dependent projects (proxy-wasm-cpp-host
, envoy
, istio-proxy
, etc.), which don't inherit the workspace-level cxxopts
from this workspace.
I suspect most of them build with C++17 anyway, but it's a long chain of dependencies, so it seems like an unnecessary risk.
proxy-wasm/proxy-wasm-cpp-sdk#157 Signed-off-by: Martijn Stevenson <mstevenson@google.com>
Test proxy-wasm/proxy-wasm-cpp-sdk#157 Signed-off-by: Martijn Stevenson <mstevenson@google.com>
371bf65
to
b3e33e9
Compare
INCOMPLETE, see proxy-wasm/proxy-wasm-cpp-sdk#157 Signed-off-by: Martijn Stevenson <mstevenson@google.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, but merging should be gated on passing Envoy's CI (envoyproxy/envoy#29118).
…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 <mstevenson@google.com> * Rework platform selection to trigger os:wasi off standalone attr Signed-off-by: Martijn Stevenson <mstevenson@google.com> --------- Signed-off-by: Martijn Stevenson <mstevenson@google.com>
proxy-wasm/proxy-wasm-cpp-sdk#157 Signed-off-by: Martijn Stevenson <mstevenson@google.com>
Update: still stuck on Envoy emsdk update, which has a Cmake related build issue (envoyproxy/envoy#32432) |
- Define STANDALONE_WASM at compile time - Disable USE_PTHREADS at link time (it implies SHARED_MEMORY, which is incompatible with STANDALONE_WASM) Signed-off-by: Martijn Stevenson <mstevenson@google.com>
In effect this: - standardizes handling of standalone option - picks up a platforms/OS fix in emsdk - allows RE2 and Abseil to compile and link Signed-off-by: Martijn Stevenson <mstevenson@google.com>
Signed-off-by: Martijn Stevenson <mstevenson@google.com>
Upgrade utf8_range to latest fix a build issue. Signed-off-by: Martijn Stevenson <mstevenson@google.com>
Instead, silence a zlib warning in the emsdk transition. Signed-off-by: Martijn Stevenson <mstevenson@google.com>
Signed-off-by: Martijn Stevenson <mstevenson@google.com>
This reverts commit 38a0332. Emscripten depends on nodejs, so this was wrong. Root cause is likely some sort of CI-env fetch issue. Signed-off-by: Martijn Stevenson <mstevenson@google.com>
Signed-off-by: Martijn Stevenson <mstevenson@google.com>
Signed-off-by: Martijn Stevenson <mstevenson@google.com>
c3c8db4
to
816e403
Compare
npm install
patch to work with the new version