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

Broken C++ Coverage with hermetic clang toolchain - No override for COVERAGE_GCOV_PATH and LLVM_COV possible with --test_env #23247

Open
omar-droubi opened this issue Aug 9, 2024 · 13 comments
Assignees
Labels
coverage potential 7.x cherry-picks Potential cherry-picks for the next 7.x release. We'll consider a new 7.x release if enough issues team-Rules-CPP Issues for C++ rules type: bug untriaged

Comments

@omar-droubi
Copy link

omar-droubi commented Aug 9, 2024

Description of the bug:

Hello Everyone,

Starting with Bazel 7.2.1 it's no longer possible to generate c++ code coverage using a hermetic Clang toolchain. I am using pw_toolchain_bazel to generate the toolchains definitions. Since our system doesn't have GCOV installed, and would like to use LLVM for the coverage, we override COVERAGE_GCOV_PATH and LLVM_COV with --test_env:

bazelrc content:

test --test_env=COVERAGE_GCOV_PATH=external/_main~_repo_rules~llvm_toolchain/bin/llvm-profdata
test --test_env=LLVM_COV=external/_main~_repo_rules~llvm_toolchain/bin/llvm-cov

By default Bazel extracts these two env variables from the tool_path for GCOV and LLVM_COV, but not from the action_configs for coverage actions.

cc_helper.bzl@1090

def _get_coverage_environment(ctx, cc_config, cc_toolchain):
    if not ctx.configuration.coverage_enabled:
        return {}

    env = {
        "COVERAGE_GCOV_PATH": cc_toolchain.tool_path(tool = "GCOV"),
        "LLVM_COV": cc_toolchain.tool_path(tool = "LLVM_COV"),
        "LLVM_PROFDATA": cc_toolchain.tool_path(tool = "LLVM_PROFDATA"),
        "GENERATE_LLVM_LCOV": "1" if cc_config.generate_llvm_lcov() else "0",
    }

Given that the current recommend way is to use action_configs and not tool_paths to define Action Tools, we do not set the tool path for GCOV and LLVM_COV, and their value is set to "".

This was fine since we could override their values using --test_env. However after the change 87b0a1f we can no longer override them.

This is happening because of the change in the order of env variables population in src/main/java/com/google/devtools/build/lib/exec/TestPolicy.java.

// Overwrite with the environment common to all tests, see --test_env.
// Omar: Here COVERAGE_GCOV_PATH and LLVM_COV take the correct values from --test_env
testAction.getConfiguration().getTestActionEnvironment().resolve(env, clientEnv);

// Rule-specified test env.
// Omar: getExtraTestEnv() has already COVERAGE_GCOV_PATH and LLVM_COV values defined 
// by cc_helper and they are set to "". These values overwrite what was supplied using --test_env
testAction.getExtraTestEnv().resolve(env, clientEnv);

Proposed Solutions:

  1. _get_coverage_environment in cc_helper.bzl should use get_tool_for_action instead of tool_path.
  2. Remove setting to default if tools are not found in _get_coverage_environment.
  3. Explicit filtering for COVERAGE_GCOV_PATH, LLVM_COV, LLVM_PROFDATA,GENERATE_LLVM_LCOV in resolve function.

Which category does this issue belong to?

No response

What's the simplest, easiest way to reproduce this bug? Please provide a minimal example if possible.

No response

Which operating system are you running Bazel on?

WSL Ubuntu 20.04

What is the output of bazel info release?

release 7.2.1

If bazel info release returns development version or (@non-git), tell us how you built Bazel.

No response

What's the output of git remote get-url origin; git rev-parse HEAD ?

No response

If this is a regression, please try to identify the Bazel commit where the bug was introduced with bazelisk --bisect.

87b0a1f

Have you found anything relevant by searching the web?

No

Any other information, logs, or outputs that you want to share?

No response

@satyanandak satyanandak added team-Configurability platforms, toolchains, cquery, select(), config transitions team-Rules-CPP Issues for C++ rules labels Aug 9, 2024
@fmeum
Copy link
Collaborator

fmeum commented Aug 9, 2024

@bazel-io flag

@fmeum
Copy link
Collaborator

fmeum commented Aug 9, 2024

@c-mita

@bazel-io bazel-io added the potential release blocker Flagged by community members using "@bazel-io flag". Should be added to a release blocker milestone label Aug 9, 2024
@c-mita c-mita added the coverage label Aug 9, 2024
@iancha1992
Copy link
Member

@bazel-io fork 7.4.0

@bazel-io bazel-io removed the potential release blocker Flagged by community members using "@bazel-io flag". Should be added to a release blocker milestone label Aug 9, 2024
@gregestren gregestren removed the team-Configurability platforms, toolchains, cquery, select(), config transitions label Oct 1, 2024
@omar-droubi
Copy link
Author

Hi @c-mita / @fmeum : Is this topic still in the pipeline, or are you waiting for the full migration to rules_cc to work on it there ?

@c-mita
Copy link
Member

c-mita commented Dec 4, 2024

Given that the current recommend way is to use action_configs and not tool_paths to define Action Tools, we do not set the tool path for GCOV and LLVM_COV, and their value is set to "".

I'm don't know if action_config does anything with the coverage tools - why not have the toolchain specify the path for the tools?

@omar-droubi
Copy link
Author

In Pigweed it was not allowed to mix ActionConfig and plain tool_path, and also now in rules_cc, the toolchain config only accepts a cc_tool_map which is just a list of ActionConfigs. There is no way I can find to inject a plain old tool_path.

keith added a commit to keith/bazel that referenced this issue Dec 12, 2024
@keith
Copy link
Member

keith commented Dec 12, 2024

i submitted #24670

keith added a commit to keith/bazel that referenced this issue Dec 13, 2024
iancha1992 pushed a commit that referenced this issue Dec 19, 2024
Fixes #23247

Closes #24670.

PiperOrigin-RevId: 707176116
Change-Id: I71ef3c630f8130467cc6a0c730c1278ae6b0817f
keith added a commit to keith/bazel that referenced this issue Jan 6, 2025
Fixes bazelbuild#23247

Closes bazelbuild#24768

PiperOrigin-RevId: 707176116
Change-Id: I71ef3c630f8130467cc6a0c730c1278ae6b0817f
(cherry picked from commit 03eae37)
meteorcloudy pushed a commit that referenced this issue Jan 8, 2025
Fixes #23247

Closes #24768

PiperOrigin-RevId: 707176116
Change-Id: I71ef3c630f8130467cc6a0c730c1278ae6b0817f
(cherry picked from commit 03eae37)
bazel-io pushed a commit to bazel-io/bazel that referenced this issue Jan 18, 2025
Fixes bazelbuild#23247

Closes bazelbuild#24670.

PiperOrigin-RevId: 707176116
Change-Id: I71ef3c630f8130467cc6a0c730c1278ae6b0817f
github-merge-queue bot pushed a commit that referenced this issue Jan 20, 2025
Fixes #23247

Closes #24670.

PiperOrigin-RevId: 707176116
Change-Id: I71ef3c630f8130467cc6a0c730c1278ae6b0817f

Commit
03eae37

Co-authored-by: Keith Smiley <[email protected]>
@iancha1992
Copy link
Member

A fix for this issue has been included in Bazel 7.5.0 RC2. Please test out the release candidate and report any issues as soon as possible.
If you're using Bazelisk, you can point to the latest RC by setting USE_BAZEL_VERSION=7.5.0rc2. Thanks!

@axeluhlig
Copy link

We use --test_env=CC_CODE_COVERAGE_SCRIPT=external/cxx_tooling/cc_code_coverage_script.sh and still get the GCov does not exist at the given path: '' error when using Bazel 7.5.0rc2. It does not pick up the value we provide but instead uses external/bazel_tools/tools/test/collect_cc_coverage.sh.

I just tested it again, it Bazel 7.1 our value for CC_CODE_COVERAGE_SCRIPT gets propagated through.

Are we just using an unsupported feature here or is this bug only partially fixed?

@keith
Copy link
Member

keith commented Jan 23, 2025

I think that one is complaining about either BAZEL_CC_COVERAGE_TOOL or GCOV, maybe setting one of those will sidestep?

@jmdsouza
Copy link

I think that one is complaining about either BAZEL_CC_COVERAGE_TOOL or GCOV, maybe setting one of those will sidestep?

I'm Axel's colleague. We have resolved the previous error mentioned of GCov not existing by running coverage --build_runfile_links and this helps us execute coverage runs by using the default Bazel coverage scripts (collect_cc_coverage.sh).

However, previously we were able to provide a custom script to execute instead of collect_cc_coverage.sh. Just want to provide more details on our use case. We currently use --test_env=CC_CODE_COVERAGE_SCRIPT= to override the default Bazel coverage script in order to add -sparse to help maintain the file size and help with disk space issues in CI

This line in particular from collect_cc_coverage.sh is modified for this purpose.

  "${COVERAGE_GCOV_PATH}" merge -output "${output_file}.data" \
      "${COVERAGE_DIR}"/*.profraw 

Is this something that is supported? Thanks!

@keith
Copy link
Member

keith commented Jan 24, 2025

I'm not sure i would say it was ever "supported", but you aren't the only people using it so i think we should fix it

fmeum pushed a commit to fmeum/bazel that referenced this issue Jan 29, 2025
Fixes bazelbuild#23247

Closes bazelbuild#24670.

PiperOrigin-RevId: 707176116
Change-Id: I71ef3c630f8130467cc6a0c730c1278ae6b0817f
@keith keith reopened this Feb 3, 2025
@jmdsouza
Copy link

jmdsouza commented Feb 5, 2025

I'm not sure i would say it was ever "supported", but you aren't the only people using it so i think we should fix it

I just wanted to provide an update in case any others might need a workaround. Instead of setting CC_CODE_COVERAGE_SCRIPT through --test-env, we set the environment variables through cc_test. We did encounter a similar issue with LCOV_MERGER. RunEnvironmentInfo should also work to set these environment variables.

@iancha1992 iancha1992 added the potential 7.x cherry-picks Potential cherry-picks for the next 7.x release. We'll consider a new 7.x release if enough issues label Feb 26, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
coverage potential 7.x cherry-picks Potential cherry-picks for the next 7.x release. We'll consider a new 7.x release if enough issues team-Rules-CPP Issues for C++ rules type: bug untriaged
Projects
None yet
Development

Successfully merging a pull request may close this issue.