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

Accumulate multiple uses of --instrumentation_filter cli flag #22959

Open
tsiddharth opened this issue Jul 5, 2024 · 5 comments
Open

Accumulate multiple uses of --instrumentation_filter cli flag #22959

tsiddharth opened this issue Jul 5, 2024 · 5 comments
Labels
coverage team-Rules-API API for writing rules/aspects: providers, runfiles, actions, artifacts type: feature request untriaged

Comments

@tsiddharth
Copy link

Description of the feature request:

Multiple uses of —instrumentation_filter cli flag aren’t accumulated. This makes it difficult to have short hand config for a combination of coverage flags in a bazelrc file. e.g.,

build:covA --flagA --instrumentation_filter=afoo/abar,-afoo/abaz
build:covB --flagB --instrumentation_filter=bfoo/bbar,-bfoo/bbaz
build:covAB --flagA --flagB --instrumentation_filter=afoo/abar,-afoo/abaz,bfoo/bbar,-bfoo/bbaz # Filters list need to be created again 

If --instrumentation_filter has the ability to accumulate multiple uses, having coverage for a combination of flags would have been as simple as,

build:covAB --config covA --config covB

Hence the feature being requested is to accumulate multiple uses of --instrumentation_filter cli flag.

Which category does this issue belong to?

Configurability

What underlying problem are you trying to solve with this feature?

Please see Description.

Which operating system are you running Bazel on?

Linux

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

Have you found anything relevant by searching the web?

No response

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

https://bazelbuild.slack.com/archives/CA31HN1T3/p1720168700052469

@github-actions github-actions bot added the team-Configurability platforms, toolchains, cquery, select(), config transitions label Jul 5, 2024
@fmeum
Copy link
Collaborator

fmeum commented Jul 5, 2024

@c-mita

@c-mita
Copy link
Member

c-mita commented Jul 8, 2024

I left a couple of comments on Slack which I will reproduce here.

The first is a question of how "incompatible" filters combine.

e.g:
--instrumentation_filter=foo.*,-foo/bar
and
--instrumentation_filter=foo/bar/baz.*

A couple of approaches come to mind:

  • All "positive" filters are combined and all "negative" filters are combined. If a target matches the first and not the former, it's marked as "to be covered".
  • Each use of the flag adds a filter to a set. If a target matches any filter, it is marked as "to be covered".

I think the second is more palatable than the first.

But to be honest, I'm not entirely comfortable with the idea.

If I add --instrumentation_filter=//foo/bar.* to a test command line, I expect the only things to be covered are things under the package "//foo/bar/...". But if a (perhaps necessary) blazerc I also have is adding things, that would no longer be true.

@tsiddharth
Copy link
Author

If I add --instrumentation_filter=//foo/bar.* to a test command line, I expect the only things to be covered are things under the package "//foo/bar/...". But if a (perhaps necessary) blazerc I also have is adding things, that would no longer be true.

I think/guess that if semantics of --instrumentation_filter cli option are changed to accumulate its multiple uses, then above should perhaps not be a problem because the user would be aware that bazelrc supplied --instrumentation_filter cli would be accumulated with that set explicitly on the bazel command line. Or is there a gap in my understanding?

@rbeasley-avgo
Copy link

I left a couple of comments on Slack which I will reproduce here.

The first is a question of how "incompatible" filters combine.

e.g: --instrumentation_filter=foo.*,-foo/bar and --instrumentation_filter=foo/bar/baz.*

A couple of approaches come to mind:

  • All "positive" filters are combined and all "negative" filters are combined. If a target matches the first and not the former, it's marked as "to be covered".
  • Each use of the flag adds a filter to a set. If a target matches any filter, it is marked as "to be covered".

I think the second is more palatable than the first.

But to be honest, I'm not entirely comfortable with the idea.

If I add --instrumentation_filter=//foo/bar.* to a test command line, I expect the only things to be covered are things under the package "//foo/bar/...". But if a (perhaps necessary) blazerc I also have is adding things, that would no longer be true.

What about considering + as a special operator that effectively translates to "append this to the filter list"? This would be somewhat similar to the handling of --output_groups.

  • --instrumentation_filter=//baz/.* --instrumentation_filter=//foo/bar/.* evaluates to --instrumentation_filter=//foo/bar/.*.
  • --instrumentation_filter=//baz/.* --instrumentation_filter=+//foo/bar/.* evaluates to --instrumetnation_filter=//baz/.*,//foo/bar/.*.

@tsiddharth What's your take on +? Is that something you'd want to try prototyping internally to see how it feels?

@tsiddharth
Copy link
Author

@mzeren-vmw

tsiddharth added a commit to tsiddharth/bazel that referenced this issue Sep 8, 2024
This option is similar to `--instrumentation_filter` option. The difference
is this option can be used multiple times.

If this option is provided --instrumentation_filter has no effect.

Multiple uses of `--instrumentation_filter` are not accumulated. This makes
it difficult to have short hand config for a combination of coverage flags
in a bazelrc file. e.g.,
```
build:covA --flagA --instrumentation_filter=afoo/abar,-afoo/abaz
build:covB --flagB --instrumentation_filter=bfoo/bbar,-bfoo/bbaz
\# Filters list need to be listed again
build:covAB --flagA --flagB --instrumentation_filter=afoo/abar,-afoo/abaz,bfoo/bbar,-bfoo/bbaz
```

`--broadcom_instrumentation_filter_fragment` option can accumulate
multiple uses. Thus with this option having coverage for a combination
of flags will be simple as,
```
build:covAB --config covA --config covB
```

Add tests for --broadcom_instrumentation_filter_fragment cli to verify that its
multiple uses are accumulated and --instrumentation_filter has no effect
when --broadcom_instrumentation_filter_fragment is used.

bazelbuild#22959 is the upstream issue.

Testing Done:
* Specify multiple `--broadcom_instrumentation_filter_fragment`. gcno files
are generated for matching labels (excluding those starting with "-").
```
$ export GOBUILD_CAYMAN_BAZEL_ROOT=/dbc/sc-dbc2163/stuli/cayman_bazel/build/component
$ vbazel run --config={esx,obj} \
--collect_code_coverage \
--//bora/esx/bazel:coverage_vmkernel \
--broadcom_instrumentation_filter_fragment=//bora/vmkernel:backdoor,//bora/vmkernel:cityhash  \
--broadcom_instrumentation_filter_fragment=//bora/vmkernel:bootConfigLineParse,-//bora/vmkernel:backdoor \
--broadcom_instrumentation_filter_fragment=//bora/vmkernel:cpuidInfo \
//bora/vmkernel
$ find -L bazel-out/ | grep gcno$
bazel-out/k8-dbg-obj-ST-666afe520531/bin/bora/vmkernel/_objs/cityhash/cityHash.pic.gcno
bazel-out/k8-dbg-obj-ST-666afe520531/bin/bora/vmkernel/_objs/cpuidInfo/cpuidInfo.pic.gcno
bazel-out/k8-dbg-obj-ST-666afe520531/bin/bora/vmkernel/_objs/bootConfigLineParse/bootConfigLineParse.pic.gcno
```
* --instrumentation_filter has not effect when --broadcom_instrumentation_filter_fragment is used.
Add `--instrumentation_filter=//bora/vmkernel:lfqueue` cli to above `vbazel run` command.
No additional gcno files are created.

* --instrumentation_filter works as-is.
```
$ find -L bazel-out/ | grep gcno$ | xargs rm
$ vbazel run --config={esx,obj} \
--collect_code_coverage \
--//bora/esx/bazel:coverage_vmkernel \
--instrumentation_filter=//bora/vmkernel:lfqueue \
//bora/vmkernel
$ find -L bazel-out/ | grep gcno$
bazel-out/k8-dbg-obj-ST-666afe520531/bin/bora/vmkernel/_objs/lfqueue/lfqueue.pic.gcno
bazel-out/k8-dbg-obj-ST-666afe520531/bin/bora/vmkernel/_objs/lfqueue/slfqueue.pic.gcno
```
* Apply this patch on upstream bazel.git and execute `bazel test`.
```
$ bazelisk  test src/test/java/com/google/devtools/build/lib/starlark:all
INFO: Analyzed 4 targets (0 packages loaded, 0 targets configured).
INFO: Found 1 target and 3 test targets...
INFO: Elapsed time: 242.197s, Critical Path: 216.72s
INFO: 27 processes: 1 internal, 25 darwin-sandbox, 1 worker.
INFO: Build completed successfully, 27 total actions
//src/test/java/com/google/devtools/build/lib/starlark:BindTest (cached) PASSED in 7.5s
//src/test/java/com/google/devtools/build/lib/starlark:StarlarkRuleClassFunctionsTest (cached) PASSED in 78.0s
  Stats over 5 runs: max = 78.0s, min = 59.8s, avg = 73.9s, dev = 7.1s
//src/test/java/com/google/devtools/build/lib/starlark:StarlarkTests     PASSED in 121.1s
  Stats over 25 runs: max = 121.1s, min = 24.8s, avg = 88.6s, dev = 32.1s

Executed 1 out of 3 tests: 3 tests pass.
There were tests whose specified size is too big. Use the --test_verbose_timeout_warnings command line option to see which ones these are.
```
tsiddharth added a commit to tsiddharth/bazel that referenced this issue Sep 14, 2024
This option is similar to `--instrumentation_filter` option. The difference
is this option can be used multiple times to accumulate its values. If this
option is provided `--instrumentation_filter` has no effect.

Multiple uses of `--instrumentation_filter` are not accumulated. This makes
it difficult to have short hand configuration flags for a combination of
coverage flags in a bazelrc file. e.g.,
```
build:covA  --instrumentation_filter=afoo/abar,-afoo/abaz
build:covB  --instrumentation_filter=bfoo/bbar,-bfoo/bbaz
build:covC  --instrumentation_filter=cfoo/cbar,-cfoo/cbaz
\# To combine the flags concatenate the respective filter strings
build:covAB  --instrumentation_filter=afoo/abar,-afoo/abaz,bfoo/bbar,-bfoo/bbaz
build:covAC  --instrumentation_filter=afoo/abar,-afoo/abaz,cfoo/cbar,-cfoo/cbaz
build:covABC --instrumentation_filter=afoo/abar,-afoo/abaz,bfoo/bbar,-bfoo/bbaz,cfoo/cbar,-cfoo/cbaz
```
With a large set of flags and their respective large filter strings it is uneasy
to combine the short hand configuration flags because the respective filter strings
need to be concatenated into a single string. This is uneasy to maintain because
filter strings are repeated in the combinations.

`--experimental_instrumentation_filter_fragment` simplifies combining multiple shorthand
configuration flags because its multiple uses are accumulated. e.g.,
```
build:covA  --experimental_instrumentation_filter_fragment=afoo/abar,-afoo/abaz
build:covB  --experimental_instrumentation_filter_fragment=bfoo/bbar,-bfoo/bbaz
build:covC  --experimental_instrumentation_filter_fragment=cfoo/cbar,-cfoo/cbaz
build:covAB --config covA --config covB
build:covAC --config covA --config covC
build:covABC --config covA --config covB --config covC
```

Add tests for --experimental_instrumentation_filter_fragment cli to verify that its
multiple uses are accumulated and --instrumentation_filter has no effect
when --experimental_instrumentation_filter_fragment is used.

bazelbuild#22959 is the upstream issue.

Testing Done:
```
$ bazelisk  test src/test/java/com/google/devtools/build/lib/starlark:all
INFO: Analyzed 4 targets (0 packages loaded, 0 targets configured).
INFO: Found 1 target and 3 test targets...
INFO: Elapsed time: 242.197s, Critical Path: 216.72s
INFO: 27 processes: 1 internal, 25 darwin-sandbox, 1 worker.
INFO: Build completed successfully, 27 total actions
//src/test/java/com/google/devtools/build/lib/starlark:BindTest (cached) PASSED in 7.5s
//src/test/java/com/google/devtools/build/lib/starlark:StarlarkRuleClassFunctionsTest (cached) PASSED in 78.0s
  Stats over 5 runs: max = 78.0s, min = 59.8s, avg = 73.9s, dev = 7.1s
//src/test/java/com/google/devtools/build/lib/starlark:StarlarkTests     PASSED in 121.1s
  Stats over 25 runs: max = 121.1s, min = 24.8s, avg = 88.6s, dev = 32.1s

Executed 1 out of 3 tests: 3 tests pass.
There were tests whose specified size is too big. Use the --test_verbose_timeout_warnings command line option to see which ones these are.
```
@gregestren gregestren added team-Rules-API API for writing rules/aspects: providers, runfiles, actions, artifacts coverage and removed team-Configurability platforms, toolchains, cquery, select(), config transitions labels Oct 1, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
coverage team-Rules-API API for writing rules/aspects: providers, runfiles, actions, artifacts type: feature request untriaged
Projects
None yet
Development

No branches or pull requests

8 participants