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

Wasm coverage disabled due to rules_rust update #24164

Closed
phlax opened this issue Nov 23, 2022 · 23 comments · Fixed by #36337
Closed

Wasm coverage disabled due to rules_rust update #24164

phlax opened this issue Nov 23, 2022 · 23 comments · Fixed by #36337
Assignees
Labels
area/ci bug no stalebot Disables stalebot from closing an issue

Comments

@phlax
Copy link
Member

phlax commented Nov 23, 2022

Not sure why CI didnt fail in presubmit, but the recent update to rules_rust has broken ci (#22253 )

Fail is:

# Execution platform: @local_config_platform//:host

Use --sandbox_debug to see verbose messages from the sandbox and retain the sandbox build root for debugging
error[E0463]: can't find crate for `profiler_builtins`
  |
  = note: the compiler may have been built without the profiler runtime

error: aborting due to previous error

For more information about this error, try `rustc --explain E0463`.
INFO: Elapsed time: 181.252s, Critical Path: 45.34s
INFO: 11009 processes: 5405 remote cache hit, 5297 internal, 307 processwrapper-sandbox.
//test/extensions/bootstrap/wasm:wasm_test                      FAILED TO BUILD

https://dev.azure.com/cncf/envoy/_build/results?buildId=121396&view=logs&j=bbe4b42d-86e6-5e9c-8a0b-fea01d818a24&s=4f7d954b-a765-565f-91a2-c04870dab43f&t=e00c5a13-c6dc-5e9a-6104-69976170e881&l=147

@phlax phlax added bug triage Issue requires triage area/ci and removed triage Issue requires triage labels Nov 23, 2022
@phlax phlax mentioned this issue Nov 23, 2022
@phlax phlax self-assigned this Nov 23, 2022
@phlax
Copy link
Member Author

phlax commented Nov 23, 2022

cc @PiotrSikora

@phlax
Copy link
Member Author

phlax commented Nov 23, 2022

possibly related rust-lang/rust#81684

@phlax
Copy link
Member Author

phlax commented Nov 23, 2022

seems we may need to add --linkopt=-noprofilelib somewhere

bazelbuild/rules_rust#1324 (comment)

@phlax phlax changed the title CI broken due to rust update CI broken due to rules_rust update Nov 23, 2022
@phlax
Copy link
Member Author

phlax commented Nov 23, 2022

test pr here - #24168

@wbpcode
Copy link
Member

wbpcode commented Nov 23, 2022

I am not sure how to disable some specified test cases.

But it wouldn't be hard to add additional linkopts to some test cases.

@phlax
Copy link
Member Author

phlax commented Nov 23, 2022

But it wouldn't be hard to add additional linkopts to some test cases.

im grepping around but not sure where to add - i have tried adding to the wasm binary rule in my test PR but not sure that is correct

@wbpcode
Copy link
Member

wbpcode commented Nov 23, 2022

im grepping around but not sure where to add - i have tried adding to the wasm binary rule in my test PR but not sure that is correct

Hmmm, I just readed the error log. Seems that it's error from the rust runtime. But you added opts to the normal c++ wasm.

@phlax
Copy link
Member Author

phlax commented Nov 23, 2022

Hmmm, I just readed the error log. Seems that it's error from the rust runtime. But you added opts to the normal c++ wasm.

clutching at straws 8/

can you point me to where it should go ?

@phlax
Copy link
Member Author

phlax commented Nov 23, 2022

i considered the rust_binary but couldnt see a linkopts there http://bazelbuild.github.io/rules_rust/flatten.html#rust_binary

@phlax
Copy link
Member Author

phlax commented Nov 23, 2022

perhaps in test/extensions/common/wasm/test_data/BUILD

@phlax
Copy link
Member Author

phlax commented Nov 23, 2022

hmm - not sure about ^^ - they just seem to point to the rules i already hacked (which didnt work incidentally)

@wbpcode
Copy link
Member

wbpcode commented Nov 23, 2022

In fact, I am also searching rust rule for now and find nothing about the linkopts 🤣

@phlax
Copy link
Member Author

phlax commented Nov 23, 2022

rustc_flags perhaps

@phlax
Copy link
Member Author

phlax commented Nov 23, 2022

its this that is failing

 13 wasm_rust_binary(
 14     name = "test_rust.wasm",
 15     srcs = ["test_rust.rs"],
 16     rustc_flags = ["-Clink-arg=-zstack-size=32768"],
 17     precompile = envoy_select_wasm_v8_bool(),
 18 )

in test/extensions/common/wasm/test_data/BUILD

altho i guess it would fail elsewhere so if this fixes - i think we want to put it in the wasm.bzl file

@phlax
Copy link
Member Author

phlax commented Nov 23, 2022

testing this on test PR

diff --git a/bazel/wasm/wasm.bzl b/bazel/wasm/wasm.bzl
index 9350189d8b..1cb9b68f11 100644
--- a/bazel/wasm/wasm.bzl
+++ b/bazel/wasm/wasm.bzl
@@ -78,7 +78,7 @@ def envoy_wasm_cc_binary(name, additional_linker_inputs = [], linkopts = [], tag
         **kwargs
     )
 
-def wasm_rust_binary(name, tags = [], wasi = False, precompile = False, **kwargs):
+def wasm_rust_binary(name, tags = [], wasi = False, precompile = False, rustc_flags = [], **kwargs):
     wasm_name = "_wasm_" + name.replace(".", "_")
     kwargs.setdefault("visibility", ["//visibility:public"])
 
@@ -87,6 +87,7 @@ def wasm_rust_binary(name, tags = [], wasi = False, precompile = False, **kwargs
         edition = "2018",
         crate_type = "cdylib",
         out_binary = True,
+        rustc_flags = rustc_flags + ["--linkopt=-noprofilelib"],
         tags = ["manual"],
         **kwargs
     )

@wbpcode
Copy link
Member

wbpcode commented Nov 23, 2022

I don't know how to disable the whole test case. But seems that the rust wasm is an optional data input of test case, we can just disable this data input for coverage test to fix the ci first. Maybe we can give it a try?

def envoy_select_wasm_rust_tests(xs):
    return select({
        "@envoy//bazel:wasm_disabled": [],
        "@envoy//bazel:coverage_build": [],
        "//conditions:default": xs,
    })

@phlax
Copy link
Member Author

phlax commented Nov 23, 2022

cool - if my hack above doent work then lets do that

@phlax
Copy link
Member Author

phlax commented Nov 23, 2022

@phlax phlax changed the title CI broken due to rules_rust update Wasm coverage disabled due to rules_rust update Nov 23, 2022
@wangfakang
Copy link
Member

wangfakang commented Nov 24, 2022

@github-actions
Copy link

This issue has been automatically marked as stale because it has not had activity in the last 30 days. It will be closed in the next 7 days unless it is tagged "help wanted" or "no stalebot" or other activity occurs. Thank you for your contributions.

@github-actions github-actions bot added the stale stalebot believes this issue/PR has not been touched recently label Dec 24, 2022
@github-actions
Copy link

This issue has been automatically closed because it has not had activity in the last 37 days. If this issue is still valid, please ping a maintainer and ask them to label it as "help wanted" or "no stalebot". Thank you for your contributions.

@github-actions github-actions bot closed this as not planned Won't fix, can't repro, duplicate, stale Dec 31, 2022
@phlax phlax added no stalebot Disables stalebot from closing an issue and removed stale stalebot believes this issue/PR has not been touched recently labels May 23, 2024
@phlax phlax reopened this May 23, 2024
@botengyao
Copy link
Member

botengyao commented Sep 25, 2024

Hi @phlax, @mpwarres, @wbpcode, I tried to re-enable it with some explorations but failed the exam ;-(. Do you have more insights and want to take a look?

It looks like the rust profile build is not supported in wasm32-unkown-unkown, and we need to disable the profile build. Here is a summary:

  1. In addition to what @phlax tried and I also tried add rustc_flags = ["--cfg=disable_profiler"] + rustc_flags, but it doesn't build
  2. Tried the global RUSTFLAGS with# build --action_env=RUSTFLAGS="-C profile-generate=no", but it failed with some external build.
  3. We can try to use
build:coverage --instrumentation_filter="^//source(?!/common/quic/platform)[/:],^//envoy[/:],^//contrib(?!/.*/test)[/:], //(?!test/extensions/filters/http/wasm/test_data)[/:]", //(?!test/extensions/filters/http/wasm/test_data)[/:]"

or -@rules_rust[/:],-@proxy_wasm_rust_sdk[/:]
to filter out the instrument build, maybe my regex is wrong, but got

ERROR: /usr/local/google/home/boteng/.cache/bazel/_bazel_boteng/cc869c547587616e74431ebbdbd7f03c/external/crates_vendor__log-0.4.22/BUILD.bazel:13:13: Compiling Rust rlib log v0.4.22 (10 files) failed: (Exit 1): process_wrapper failed: error executing command (from target @crates_vendor__log-0.4.22//:log) 
  (cd /usr/local/google/home/boteng/.cache/bazel/_bazel_boteng/cc869c547587616e74431ebbdbd7f03c/execroot/envoy && \
  exec env - \
    BAZEL_COMPILER=clang \
    BAZEL_DO_NOT_DETECT_CPP_TOOLCHAIN=1 \
    BAZEL_LINKLIBS=-l%:libstdc++.a \
    BAZEL_LINKOPTS=-lm \
    BAZEL_USE_LLVM_NATIVE_COVERAGE=1 \
    CARGO_CFG_TARGET_ARCH=wasm32 \
    CARGO_CFG_TARGET_OS=wasi \
    CARGO_CRATE_NAME=log \
    CARGO_MANIFEST_DIR='${pwd}/external/crates_vendor__log-0.4.22' \
    CARGO_PKG_AUTHORS='' \
    CARGO_PKG_DESCRIPTION='' \
    CARGO_PKG_HOMEPAGE='' \
    CARGO_PKG_NAME=log \
    CARGO_PKG_VERSION=0.4.22 \
    CARGO_PKG_VERSION_MAJOR=0 \
    CARGO_PKG_VERSION_MINOR=4 \
    CARGO_PKG_VERSION_PATCH=22 \
    CARGO_PKG_VERSION_PRE='' \
    CC=clang \
    CXX=clang++ \
    CXXFLAGS='-stdlib=libc++' \
    GCOV=llvm-profdata \
    LDFLAGS='-stdlib=libc++' \
    PATH=/bin:/usr/bin:/usr/local/bin \
    REPOSITORY_NAME=crates_vendor__log-0.4.22 \
  bazel-out/k8-opt-exec-8E910DA8-ST-f70ac4ce2ceb/bin/external/rules_rust/util/process_wrapper/process_wrapper --subst 'pwd=${pwd}' -- bazel-out/k8-fastbuild-ST-15cae4aa1a0c/bin/external/rust_linux_x86_64__wasm32-wasi__stable_tools/rust_toolchain/bin/rustc external/crates_vendor__log-0.4.22/src/lib.rs '--crate-name=log' '--crate-type=rlib' '--error-format=human' '--codegen=metadata=-1021366919' '--codegen=extra-filename=-1021366919' '--out-dir=bazel-out/k8-fastbuild-ST-15cae4aa1a0c/bin/external/crates_vendor__log-0.4.22' '--codegen=opt-level=0' '--codegen=debuginfo=0' '--codegen=strip=none' '--remap-path-prefix=${pwd}=' '--emit=dep-info,link' '--color=always' '--target=wasm32-wasi' -L bazel-out/k8-fastbuild-ST-15cae4aa1a0c/bin/external/rust_linux_x86_64__wasm32-wasi__stable_tools/rust_toolchain/lib/rustlib/wasm32-wasi/lib -L bazel-out/k8-fastbuild-ST-15cae4aa1a0c/bin/external/rust_linux_x86_64__wasm32-wasi__stable_tools/rust_toolchain/lib/rustlib/wasm32-wasi/lib/self-contained '--cap-lints=allow' '--edition=2021' '--codegen=instrument-coverage' '--sysroot=bazel-out/k8-fastbuild-ST-15cae4aa1a0c/bin/external/rust_linux_x86_64__wasm32-wasi__stable_tools/rust_toolchain')
# Configuration: 1e27b5d18df1a392cb26e21847ab4e6935a33c925573d52f21f0753a26462cc7
# Execution platform: //bazel/rbe/toolchains:rbe_linux_clang_libcxx_platform
warning: the `wasm32-wasi` target is being renamed to `wasm32-wasip1` and the `wasm32-wasi` target will be removed from nightly in October 2024 and removed from stable Rust in January 2025

error[E0463]: can't find crate for `profiler_builtins`
  |
  = note: the compiler may have been built without the profiler runtime

error: aborting due to 1 previous error

My initial investigation is the global clang profile build will force include '--codegen=instrument-coverage', so that all external builds will also try to build rust while the rustc_flags in rust_library doesn't take effect. Is it possible a problem related to proxy-wasm-rust-sdk? - don't know right now

@botengyao
Copy link
Member

cc @mpwarres @phlax @wbpcode

I actually got a workaround to disable the instrument for rust through a patch, waiting on CI to verify.

--- rust/private/rustc.bzl
+++ rust/private/rustc.bzl
@@ -1043,7 +1043,7 @@ def construct_arguments(

     if toolchain.llvm_cov and ctx.configuration.coverage_enabled:
         # https://doc.rust-lang.org/rustc/instrument-coverage.html
-        rustc_flags.add("--codegen=instrument-coverage")
+        pass

     if toolchain._experimental_link_std_dylib:
         rustc_flags.add("--codegen=prefer-dynamic")

RyanTheOptimist pushed a commit that referenced this issue Sep 27, 2024
…build (#36337)

I’ve been explored other solutions to
#24164 (comment)
over the past few days, and here is the final fix, and I think we don't
need rust coverage as whole at current stage.

Commit Message: re-enable envoy wasm coverage build by disabling rust
coverage build
Additional Description:
Risk Level: no
Fix:  #24164
Testing:
Docs Changes:

---------

Signed-off-by: Boteng Yao <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/ci bug no stalebot Disables stalebot from closing an issue
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants