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

Provide --experimental_instrumentation_filter_fragment cli option #23623

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

tsiddharth
Copy link

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.

#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.

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.
```
@tsiddharth tsiddharth requested a review from a team as a code owner September 14, 2024 10:40
@tsiddharth tsiddharth requested review from katre and removed request for a team September 14, 2024 10:40
Copy link

google-cla bot commented Sep 14, 2024

Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

View this failed invocation of the CLA check for more information.

For the most up to date status, view the checks section at the bottom of the pull request.

@github-actions github-actions bot added team-Configurability platforms, toolchains, cquery, select(), config transitions awaiting-review PR is awaiting review from an assigned reviewer labels Sep 14, 2024
@katre katre requested a review from c-mita September 16, 2024 11:41
@c-mita
Copy link
Member

c-mita commented Oct 7, 2024

As I commented earlier, I still think --instrumentation-filter should override this rather than the other way around (that is, if the regular filter is set, this should be ignore), since that lets me easily set a custom filter on a command line without having to worry about what's configured in various bazelrcs.

But otherwise, I don't mind accepting this as an experimental feature.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
awaiting-review PR is awaiting review from an assigned reviewer team-Configurability platforms, toolchains, cquery, select(), config transitions
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants