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: re-enable envoy wasm coverage build by disabling rust coverage build #36337

Merged
merged 5 commits into from
Sep 27, 2024

Conversation

botengyao
Copy link
Member

@botengyao botengyao commented Sep 25, 2024

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]>
Copy link

As a reminder, PRs marked as draft will not be automatically assigned reviewers,
or be handled by maintainer-oncall triage.

Please mark your PR as ready when you want it to be reviewed!

🐱

Caused by: #36337 was opened by botengyao.

see: more, trace.

@repokitteh-read-only repokitteh-read-only bot added the deps Approval required for changes to Envoy's external dependencies label Sep 26, 2024
Copy link

CC @envoyproxy/dependency-shepherds: Your approval is needed for changes made to (bazel/.*repos.*\.bzl)|(bazel/dependency_imports\.bzl)|(api/bazel/.*\.bzl)|(.*/requirements\.txt)|(.*\.patch).
envoyproxy/dependency-shepherds assignee is @RyanTheOptimist

🐱

Caused by: #36337 was synchronize by botengyao.

see: more, trace.

@botengyao botengyao changed the title [TEST]: only disable rust wasm [TEST]: wasm coverage Sep 26, 2024
Signed-off-by: Boteng Yao <[email protected]>
@botengyao botengyao changed the title [TEST]: wasm coverage wasm: re-enable wasm coverage build by disabling rust coverage build Sep 26, 2024
@botengyao botengyao changed the title wasm: re-enable wasm coverage build by disabling rust coverage build wasm: re-enable envoy wasm coverage build by disabling rust coverage build Sep 26, 2024
@botengyao
Copy link
Member Author

/retest

@botengyao botengyao marked this pull request as ready for review September 26, 2024 19:47
@botengyao
Copy link
Member Author

/assgin @phlax

@botengyao
Copy link
Member Author

/assign @mpwarres

@botengyao
Copy link
Member Author

/assgin @phlax

Signed-off-by: Boteng Yao <[email protected]>
Copy link

CC @envoyproxy/coverage-shephards: FYI only for changes made to (test/per_file_coverage.sh).
envoyproxy/coverage-shephards assignee is @RyanTheOptimist

🐱

Caused by: #36337 was synchronize by botengyao.

see: more, trace.

@botengyao
Copy link
Member Author

The CI /coverage doesn't work right now based on #36356 per @phlax.

Here is a local run:

image

Signed-off-by: Boteng Yao <[email protected]>
@phlax
Copy link
Member

phlax commented Sep 26, 2024

the coverage test itself should work but the way to view it from here is not there atm


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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Out of curiosity, why does this fix the wasm issue?

Copy link
Member Author

@botengyao botengyao Sep 26, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We are compiling rust under wasm-32-unkown-unkown and wasm-wasi targets in our //test repo, but actually instrument-coverage/pgo is not supported for these target envs for rust. Since Envoy at current stage will not profile Rust code, we should be fine to disable it as a whole by default.

@repokitteh-read-only repokitteh-read-only bot removed the deps Approval required for changes to Envoy's external dependencies label Sep 26, 2024
Copy link
Contributor

@mpwarres mpwarres left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, thanks!

@RyanTheOptimist RyanTheOptimist merged commit e3ed5a7 into envoyproxy:main Sep 27, 2024
21 checks passed
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.

Wasm coverage disabled due to rules_rust update
4 participants