Skip to content
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

Open
wants to merge 8 commits into
base: main
Choose a base branch
from

Conversation

martijneken
Copy link
Contributor

@martijneken martijneken commented Jul 26, 2023

  • Define STANDALONE_WASM at compile time. This is needed by Abseil for now.
  • Upgrade emsdk to just past v3.1.44. Picks up a platforms (OS) fix for RE2.
  • Use wasm_cc_binary attributes where possible (e.g. standalone)
  • Update the emsdk npm install patch to work with the new version

- 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>
bazel/defs.bzl Outdated Show resolved Hide resolved
martijneken added a commit to martijneken/emsdk that referenced this pull request Jul 28, 2023
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>
walkingeyerobot pushed a commit to emscripten-core/emsdk that referenced this pull request Jul 28, 2023
…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>
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>
@martijneken martijneken changed the title Update compile/link options to work with Abseil. Update compile/link options to work with Abseil + RE2 Jul 29, 2023
@martijneken martijneken changed the title Update compile/link options to work with Abseil + RE2 Update emsdk, work with Abseil + RE2 Jul 29, 2023
@martijneken
Copy link
Contributor Author

martijneken commented Jul 29, 2023

Something funky's going on with (updated) emsdk and platform selection:

Doesn't work here in proxy-wasm-cpp-sdk:

$ bazelisk build --toolchain_resolution_debug //example:http_wasm_example.wasm
INFO: Build option --action_env has changed, discarding analysis cache.
INFO: ToolchainResolution: Target platform @local_config_platform//:host: Selected execution platform @local_config_platform//:host, 
INFO: ToolchainResolution: Target platform @local_config_platform//:host: Selected execution platform @local_config_platform//:host, 
INFO: ToolchainResolution:     Type @bazel_tools//tools/cpp:toolchain_type: target platform @local_config_platform//:host: Rejected toolchain @local_config_cc//:cc-compiler-armeabi-v7a; mismatching values: arm, android
INFO: ToolchainResolution:     Type @bazel_tools//tools/cpp:toolchain_type: target platform @emsdk//:platform_wasi: Rejected toolchain @local_config_cc//:cc-compiler-armeabi-v7a; mismatching values: arm, android
INFO: ToolchainResolution:   Type @bazel_tools//tools/python:toolchain_type: target platform @local_config_platform//:host: execution @local_config_platform//:host: Selected toolchain @bazel_tools//tools/python:_autodetecting_py_runtime_pair
INFO: ToolchainResolution:     Type @bazel_tools//tools/cpp:toolchain_type: target platform @emsdk//:platform_wasi: Rejected toolchain @local_config_cc//:cc-compiler-k8; mismatching values: x86_64, linux
INFO: ToolchainResolution:   Type @bazel_tools//tools/cpp:toolchain_type: target platform @local_config_platform//:host: execution @local_config_platform//:host: Selected toolchain @local_config_cc//:cc-compiler-k8
INFO: ToolchainResolution:   Type @bazel_tools//tools/cpp:toolchain_type: target platform @emsdk//:platform_wasi: No toolchains found.
INFO: ToolchainResolution: Target platform @local_config_platform//:host: Selected execution platform @local_config_platform//:host, type @bazel_tools//tools/python:toolchain_type -> toolchain @bazel_tools//tools/python:_autodetecting_py_runtime_pair, type @bazel_tools//tools/cpp:toolchain_type -> toolchain @local_config_cc//:cc-compiler-k8
ERROR: While resolving toolchains for target //example:proxy_wasm_http_wasm_example: 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.

Works when rules_rust is in the workspace, seemingly via dummy_cc_toolchain:

INFO: ToolchainResolution: Target platform @local_config_platform//:host: Selected execution platform @local_config_platform//:host,
INFO: ToolchainResolution:   Type @bazel_tools//tools/cpp:toolchain_type: target platform @emsdk//:platform_wasi: execution @local_config_platform//:host: Selected toolchain @rules_rust//rust/private/dummy_cc_toolchain:dummy_cc_wasm32_toolchain_cc
INFO: ToolchainResolution:     Type @bazel_tools//tools/cpp:toolchain_type: target platform @emsdk//:platform_wasi: Rejected toolchain @local_config_cc//:cc-compiler-armeabi-v7a; mismatching values: arm, android
INFO: ToolchainResolution:     Type @bazel_tools//tools/cpp:toolchain_type: target platform @emsdk//:platform_wasi: Rejected toolchain @local_config_cc//:cc-compiler-k8; mismatching values: x86_64, linux
INFO: ToolchainResolution: Target platform @emsdk//:platform_wasi: Selected execution platform @local_config_platform//:host, type @bazel_tools//tools/cpp:toolchain_type -> toolchain @rules_rust//rust/private/dummy_cc_toolchain:dummy_cc_wasm32_toolchain_cc
INFO: ToolchainResolution: Target platform @emsdk//:platform_wasi: Selected execution platform @local_config_platform//:host,

@martijneken
Copy link
Contributor Author

Ah, just needed to add register_emscripten_toolchains(). This changed in emscripten-core/emsdk#1036.

Signed-off-by: Martijn Stevenson <mstevenson@google.com>
@martijneken
Copy link
Contributor Author

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.

Upgrade utf8_range to latest fix a build issue.

Signed-off-by: Martijn Stevenson <mstevenson@google.com>
BUILD Outdated
@@ -13,7 +13,6 @@ cc_library(
"proxy_wasm_api.h",
"proxy_wasm_externs.h",
],
copts = ["-std=c++17"],
Copy link
Contributor

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.

Copy link
Contributor Author

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?

Copy link
Contributor

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.

Copy link
Contributor Author

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

Copy link
Contributor Author

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.

Copy link
Contributor Author

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",
)

Copy link
Contributor Author

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

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

Copy link
Contributor Author

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.

Copy link
Contributor

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"],
Copy link
Contributor

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.

bazel/repositories.bzl Outdated Show resolved Hide resolved
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>
martijneken added a commit to martijneken/emsdk that referenced this pull request Aug 26, 2023
INCOMPLETE, see proxy-wasm/proxy-wasm-cpp-sdk#157

Signed-off-by: Martijn Stevenson <mstevenson@google.com>
Signed-off-by: Martijn Stevenson <mstevenson@google.com>
This was referenced Sep 9, 2023
Copy link
Contributor

@PiotrSikora PiotrSikora left a 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).

shlomif pushed a commit to shlomif/emsdk that referenced this pull request Sep 29, 2023
…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>
martijneken added a commit to martijneken/envoy that referenced this pull request Feb 16, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants