From d0c75ffb717adfebc96faa54612ed0c9344a673c Mon Sep 17 00:00:00 2001 From: Siddharth Tuli Date: Sun, 8 Sep 2024 10:18:06 +0530 Subject: [PATCH] Provide `--experimental_instrumentation_filter_fragment` cli option 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. https://github.com/bazelbuild/bazel/issues/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. ``` --- .../config/BuildConfigurationValue.java | 19 ++++- .../lib/analysis/config/CoreOptions.java | 17 +++++ .../lib/starlark/StarlarkRuleContextTest.java | 74 +++++++++++++++++++ 3 files changed, 109 insertions(+), 1 deletion(-) diff --git a/src/main/java/com/google/devtools/build/lib/analysis/config/BuildConfigurationValue.java b/src/main/java/com/google/devtools/build/lib/analysis/config/BuildConfigurationValue.java index 9d9cfde8559d33..c4362b3ae4347b 100644 --- a/src/main/java/com/google/devtools/build/lib/analysis/config/BuildConfigurationValue.java +++ b/src/main/java/com/google/devtools/build/lib/analysis/config/BuildConfigurationValue.java @@ -15,6 +15,7 @@ package com.google.devtools.build.lib.analysis.config; import com.google.common.annotations.VisibleForTesting; +import com.google.common.base.Joiner; import com.google.common.collect.ImmutableCollection; import com.google.common.collect.ImmutableMap; import com.google.common.collect.ImmutableSet; @@ -44,6 +45,7 @@ import com.google.devtools.build.lib.vfs.PathFragment; import com.google.devtools.build.skyframe.SkyValue; import com.google.devtools.common.options.TriState; +import com.google.devtools.common.options.OptionsParsingException; import java.io.PrintStream; import java.util.HashMap; import java.util.List; @@ -143,6 +145,8 @@ public void reportInvalidOptions(EventHandler reporter) { } } + private final RegexFilter instrumentationFilter; + /** * Compute the test environment, which, at configuration level, is a pair consisting of the * statically set environment variables with their values and the set of environment variables to @@ -298,6 +302,19 @@ private static ImmutableSortedMap, Fragment> getConfig this.reservedActionMnemonics = reservedActionMnemonics; this.commandLineLimits = new CommandLineLimits(options.minParamFileSize); this.defaultFeatures = FeatureSet.parse(options.defaultFeatures); + + if (!options.instrumentationFilterFragment.isEmpty()) { + // A regex-based instrumentation filter instance is formed by concatenating + // string values of --experimental_instrumentation_filter_fragment. + try { + this.instrumentationFilter = new RegexFilter.RegexFilterConverter().convert( + Joiner.on(",").join(options.instrumentationFilterFragment)); + } catch (OptionsParsingException e) { + throw new RuntimeException(e); + } + } else { + this.instrumentationFilter = options.instrumentationFilter; + } } @Override @@ -566,7 +583,7 @@ public Iterable getVariableShellEnvironment() { * identify targets to be instrumented in the coverage mode. */ public RegexFilter getInstrumentationFilter() { - return options.instrumentationFilter; + return instrumentationFilter; } /** diff --git a/src/main/java/com/google/devtools/build/lib/analysis/config/CoreOptions.java b/src/main/java/com/google/devtools/build/lib/analysis/config/CoreOptions.java index 8d576d909e7bb7..a995ea208c45ee 100644 --- a/src/main/java/com/google/devtools/build/lib/analysis/config/CoreOptions.java +++ b/src/main/java/com/google/devtools/build/lib/analysis/config/CoreOptions.java @@ -17,6 +17,7 @@ import static com.google.common.collect.ImmutableList.toImmutableList; import static com.google.common.collect.ImmutableMap.toImmutableMap; +import com.google.common.base.Joiner; import com.google.common.collect.ImmutableList; import com.google.common.collect.ImmutableMap; import com.google.devtools.build.lib.analysis.config.CoreOptionConverters.LabelListConverter; @@ -274,6 +275,22 @@ public class CoreOptions extends FragmentOptions implements Cloneable { + "instrumented unless --instrument_test_targets is enabled.") public RegexFilter instrumentationFilter; + @Option( + name = "experimental_instrumentation_filter_fragment", + allowMultiple = true, + defaultValue = "null", + documentationCategory = OptionDocumentationCategory.OUTPUT_PARAMETERS, + effectTags = {OptionEffectTag.AFFECTS_OUTPUTS}, + help = + "When coverage is enabled, only rules with names included by the " + + "specified regex-based filter will be instrumented. Rules prefixed " + + "with '-' are excluded instead. Note that only non-test rules are " + + "instrumented unless --instrument_test_targets is enabled. " + + "Excluded filters always override included ones. This option can be used " + + "multiple times. If this option is provided --instrumentation_filter " + + "has no effect.") + public List instrumentationFilterFragment; + @Option( name = "instrument_test_targets", defaultValue = "false", diff --git a/src/test/java/com/google/devtools/build/lib/starlark/StarlarkRuleContextTest.java b/src/test/java/com/google/devtools/build/lib/starlark/StarlarkRuleContextTest.java index 4a502f7cda6269..35ea39dd809ef7 100644 --- a/src/test/java/com/google/devtools/build/lib/starlark/StarlarkRuleContextTest.java +++ b/src/test/java/com/google/devtools/build/lib/starlark/StarlarkRuleContextTest.java @@ -3020,6 +3020,80 @@ private void setUpCoverageInstrumentedTest() throws Exception { """); } + @Test + public void testCoverageExperimentalInstrumentedFragmentCoverageDisabled() throws Exception { + setUpCoverageInstrumentedTest(); + useConfiguration( + "--nocollect_code_coverage", + "--experimental_instrumentation_filter_fragment=."); + StarlarkRuleContext ruleContext = createRuleContext("//test:foo"); + setRuleContext(ruleContext); + Object result = ev.eval("ruleContext.coverage_instrumented()"); + assertThat((Boolean) result).isFalse(); + } + + @Test + public void testCoverageExperimentalInstrumentedFragmentFalseForSourceFileLabel() throws Exception { + setUpCoverageInstrumentedTest(); + useConfiguration( + "--collect_code_coverage", + "--experimental_instrumentation_filter_fragment=:foo", + "--experimental_instrumentation_filter_fragment=:bar"); + setRuleContext(createRuleContext("//test:foo")); + Object result = ev.eval("ruleContext.coverage_instrumented(ruleContext.attr.srcs[0])"); + assertThat((Boolean) result).isFalse(); + } + + @Test + public void testCoverageExperimentalInstrumentedFragmentDoesNotMatchFilter() throws Exception { + setUpCoverageInstrumentedTest(); + useConfiguration( + "--collect_code_coverage", + "--experimental_instrumentation_filter_fragment=:foo", + "--experimental_instrumentation_filter_fragment=-:bar"); + setRuleContext(createRuleContext("//test:bar")); + Object result = ev.eval("ruleContext.coverage_instrumented()"); + assertThat((Boolean) result).isFalse(); + } + + @Test + public void testCoverageExperimentalInstrumentedFragmentMatchesFilter() throws Exception { + setUpCoverageInstrumentedTest(); + useConfiguration( + "--collect_code_coverage", + "--experimental_instrumentation_filter_fragment=:foo", + "--experimental_instrumentation_filter_fragment=:bar"); + setRuleContext(createRuleContext("//test:foo")); + Object result = ev.eval("ruleContext.coverage_instrumented()"); + assertThat((Boolean) result).isTrue(); + } + + @Test + public void testCoverageExperimentalInstrumentedFragmentDoesNotMatchFilterNonDefaultLabel() throws Exception { + setUpCoverageInstrumentedTest(); + useConfiguration( + "--collect_code_coverage", + "--experimental_instrumentation_filter_fragment=:foo", + // --instrumentation_filter has no effect because --experimental_instrumentation_filter_fragment is used. + "--instrumentation_filter=:bar"); + setRuleContext(createRuleContext("//test:foo")); + // //test:bar does not match :foo, though //test:foo would. + Object result = ev.eval("ruleContext.coverage_instrumented(ruleContext.attr.deps[0])"); + assertThat((Boolean) result).isFalse(); + } + + @Test + public void testCoverageExperimentalInstrumentedFragmentMatchesFilterNonDefaultLabel() throws Exception { + setUpCoverageInstrumentedTest(); + useConfiguration( + "--collect_code_coverage", + "--experimental_instrumentation_filter_fragment=:bar"); + setRuleContext(createRuleContext("//test:foo")); + // //test:bar does match :bar, though //test:foo would not. + Object result = ev.eval("ruleContext.coverage_instrumented(ruleContext.attr.deps[0])"); + assertThat((Boolean) result).isTrue(); + } + @Test public void testCoverageInstrumentedCoverageDisabled() throws Exception { setUpCoverageInstrumentedTest();