Don't filter starlark flag resolution based on --build_tests_only and friends
Previously when --build_tests_only was passed alongside any starlark flag a crash would occur because the starlark flag is not a test and therefore would be filtered from the targets available at the top level.
With this change no flags that affect top-level target availability should impact the loading of starlark flags.
RELNOTES: None
PiperOrigin-RevId: 281397052
diff --git a/src/main/java/com/google/devtools/build/lib/buildtool/AnalysisPhaseRunner.java b/src/main/java/com/google/devtools/build/lib/buildtool/AnalysisPhaseRunner.java
index 89af14f..30cf557 100644
--- a/src/main/java/com/google/devtools/build/lib/buildtool/AnalysisPhaseRunner.java
+++ b/src/main/java/com/google/devtools/build/lib/buildtool/AnalysisPhaseRunner.java
@@ -167,7 +167,7 @@
boolean keepGoing = request.getKeepGoing();
TargetPatternPhaseValue result =
env.getSkyframeExecutor()
- .loadTargetPatterns(
+ .loadTargetPatternsWithFilters(
env.getReporter(),
request.getTargets(),
env.getRelativeWorkingDirectory(),
diff --git a/src/main/java/com/google/devtools/build/lib/runtime/BlazeOptionHandler.java b/src/main/java/com/google/devtools/build/lib/runtime/BlazeOptionHandler.java
index 2584bfb..79d4eb7 100644
--- a/src/main/java/com/google/devtools/build/lib/runtime/BlazeOptionHandler.java
+++ b/src/main/java/com/google/devtools/build/lib/runtime/BlazeOptionHandler.java
@@ -246,8 +246,7 @@
return ExitCode.SUCCESS;
}
try {
- StarlarkOptionsParser.newStarlarkOptionsParser(env, optionsParser)
- .parse(commandAnnotation, eventHandler);
+ StarlarkOptionsParser.newStarlarkOptionsParser(env, optionsParser).parse(eventHandler);
} catch (OptionsParsingException e) {
env.getReporter().handle(Event.error(e.getMessage()));
return ExitCode.PARSING_FAILURE;
diff --git a/src/main/java/com/google/devtools/build/lib/runtime/StarlarkOptionsParser.java b/src/main/java/com/google/devtools/build/lib/runtime/StarlarkOptionsParser.java
index 624c5d5..cbf611a 100644
--- a/src/main/java/com/google/devtools/build/lib/runtime/StarlarkOptionsParser.java
+++ b/src/main/java/com/google/devtools/build/lib/runtime/StarlarkOptionsParser.java
@@ -30,7 +30,6 @@
import com.google.devtools.build.lib.packages.Rule;
import com.google.devtools.build.lib.packages.Target;
import com.google.devtools.build.lib.packages.Type;
-import com.google.devtools.build.lib.pkgcache.LoadingOptions;
import com.google.devtools.build.lib.skyframe.SkyframeExecutor;
import com.google.devtools.build.lib.skyframe.TargetPatternPhaseValue;
import com.google.devtools.build.lib.util.Pair;
@@ -80,8 +79,7 @@
// OptionsParserImpl.identifyOptionAndPossibleArgument. Consider combining. This would probably
// require multiple rounds of parsing to fit starlark-defined options into native option format.
@VisibleForTesting
- public void parse(Command command, ExtendedEventHandler eventHandler)
- throws OptionsParsingException {
+ public void parse(ExtendedEventHandler eventHandler) throws OptionsParsingException {
ImmutableList.Builder<String> residue = new ImmutableList.Builder<>();
// Map of <option name (label), <unparsed option value, loaded option>>.
Map<String, Pair<String, Target>> unparsedOptions =
@@ -98,7 +96,7 @@
continue;
}
- parseArg(arg, unparsedArgs, unparsedOptions, command, eventHandler);
+ parseArg(arg, unparsedArgs, unparsedOptions, eventHandler);
}
residue.addAll(nativeOptionsParser.getPostDoubleDashResidue());
nativeOptionsParser.setResidue(residue.build());
@@ -146,7 +144,6 @@
String arg,
Iterator<String> unparsedArgs,
Map<String, Pair<String, Target>> unparsedOptions,
- Command command,
ExtendedEventHandler eventHandler)
throws OptionsParsingException {
int equalsAt = arg.indexOf('=');
@@ -158,8 +155,7 @@
if (value != null) {
// --flag=value or -flag=value form
- Target buildSettingTarget =
- loadBuildSetting(name, nativeOptionsParser, command, eventHandler);
+ Target buildSettingTarget = loadBuildSetting(name, nativeOptionsParser, eventHandler);
unparsedOptions.put(name, new Pair<>(value, buildSettingTarget));
} else {
boolean booleanValue = true;
@@ -168,8 +164,7 @@
booleanValue = false;
name = name.substring(2);
}
- Target buildSettingTarget =
- loadBuildSetting(name, nativeOptionsParser, command, eventHandler);
+ Target buildSettingTarget = loadBuildSetting(name, nativeOptionsParser, eventHandler);
BuildSetting current =
buildSettingTarget.getAssociatedRule().getRuleClassObject().getBuildSetting();
if (current.getType().equals(BOOLEAN)) {
@@ -194,20 +189,17 @@
private Target loadBuildSetting(
String targetToBuild,
OptionsParser optionsParser,
- Command command,
ExtendedEventHandler eventHandler)
throws OptionsParsingException {
Target buildSetting;
try {
TargetPatternPhaseValue result =
- skyframeExecutor.loadTargetPatterns(
+ skyframeExecutor.loadTargetPatternsWithoutFilters(
reporter,
Collections.singletonList(targetToBuild),
relativeWorkingDirectory,
- optionsParser.getOptions(LoadingOptions.class),
SkyframeExecutor.DEFAULT_THREAD_COUNT,
- optionsParser.getOptions(KeepGoingOption.class).keepGoing,
- command.name().equals("test"));
+ optionsParser.getOptions(KeepGoingOption.class).keepGoing);
buildSetting =
Iterables.getOnlyElement(
result.getTargets(eventHandler, skyframeExecutor.getPackageManager()));
diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/SkyframeExecutor.java b/src/main/java/com/google/devtools/build/lib/skyframe/SkyframeExecutor.java
index 439c01c..dd7a1a2 100644
--- a/src/main/java/com/google/devtools/build/lib/skyframe/SkyframeExecutor.java
+++ b/src/main/java/com/google/devtools/build/lib/skyframe/SkyframeExecutor.java
@@ -203,7 +203,6 @@
import java.util.UUID;
import java.util.concurrent.Callable;
import java.util.concurrent.Semaphore;
-import java.util.concurrent.TimeUnit;
import java.util.concurrent.atomic.AtomicBoolean;
import java.util.concurrent.atomic.AtomicInteger;
import java.util.concurrent.atomic.AtomicReference;
@@ -2808,7 +2807,39 @@
return packageProgress;
}
- public TargetPatternPhaseValue loadTargetPatterns(
+ /**
+ * Loads the given target patterns without applying any filters (such as removing non-test targets
+ * if {@code --build_tests_only} is set).
+ *
+ * @param eventHandler handler which accepts update events
+ * @param targetPatterns patterns to be loaded
+ * @param threadCount number of threads to use for this skyframe evaluation
+ * @param keepGoing whether to attempt to ignore errors. See also {@link KeepGoingOption}
+ */
+ public TargetPatternPhaseValue loadTargetPatternsWithoutFilters(
+ ExtendedEventHandler eventHandler,
+ List<String> targetPatterns,
+ PathFragment relativeWorkingDirectory,
+ int threadCount,
+ boolean keepGoing)
+ throws TargetParsingException, InterruptedException {
+ SkyKey key =
+ TargetPatternPhaseValue.keyWithoutFilters(
+ ImmutableList.copyOf(targetPatterns), relativeWorkingDirectory.getPathString());
+ return getTargetPatternPhaseValue(eventHandler, targetPatterns, threadCount, keepGoing, key);
+ }
+
+ /**
+ * Loads the given target patterns after applying filters configured through parameters and
+ * options (such as removing non-test targets if {@code --build_tests_only} is set).
+ *
+ * @param eventHandler handler which accepts update events
+ * @param targetPatterns patterns to be loaded
+ * @param threadCount number of threads to use for this skyframe evaluation
+ * @param keepGoing whether to attempt to ignore errors. See also {@link KeepGoingOption}
+ * @param determineTests whether to ignore any targets that aren't tests or test suites
+ */
+ public TargetPatternPhaseValue loadTargetPatternsWithFilters(
ExtendedEventHandler eventHandler,
List<String> targetPatterns,
PathFragment relativeWorkingDirectory,
@@ -2817,7 +2848,6 @@
boolean keepGoing,
boolean determineTests)
throws TargetParsingException, InterruptedException {
- Stopwatch timer = Stopwatch.createStarted();
SkyKey key =
TargetPatternPhaseValue.key(
ImmutableList.copyOf(targetPatterns),
@@ -2829,9 +2859,20 @@
options.buildManualTests,
options.expandTestSuites,
TestFilter.forOptions(options, eventHandler, pkgFactory.getRuleClassNames()));
- EvaluationResult<TargetPatternPhaseValue> evalResult;
+ return getTargetPatternPhaseValue(eventHandler, targetPatterns, threadCount, keepGoing, key);
+ }
+
+ private TargetPatternPhaseValue getTargetPatternPhaseValue(
+ ExtendedEventHandler eventHandler,
+ List<String> targetPatterns,
+ int threadCount,
+ boolean keepGoing,
+ SkyKey key)
+ throws InterruptedException, TargetParsingException {
+ Stopwatch timer = Stopwatch.createStarted();
eventHandler.post(new LoadingPhaseStartedEvent(packageProgress));
- evalResult = evaluate(ImmutableList.of(key), keepGoing, threadCount, eventHandler);
+ EvaluationResult<TargetPatternPhaseValue> evalResult =
+ evaluate(ImmutableList.of(key), keepGoing, threadCount, eventHandler);
if (evalResult.hasError()) {
ErrorInfo errorInfo = evalResult.getError(key);
TargetParsingException exc;
@@ -2863,11 +2904,8 @@
}
throw exc;
}
- long timeMillis = timer.stop().elapsed(TimeUnit.MILLISECONDS);
- eventHandler.post(new TargetParsingPhaseTimeEvent(timeMillis));
-
- TargetPatternPhaseValue patternParsingValue = evalResult.get(key);
- return patternParsingValue;
+ eventHandler.post(new TargetParsingPhaseTimeEvent(timer.stop().elapsed().toMillis()));
+ return evalResult.get(key);
}
public PrepareAnalysisPhaseValue prepareAnalysisPhase(
diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/TargetPatternPhaseValue.java b/src/main/java/com/google/devtools/build/lib/skyframe/TargetPatternPhaseValue.java
index bf6337c..a5998ae 100644
--- a/src/main/java/com/google/devtools/build/lib/skyframe/TargetPatternPhaseValue.java
+++ b/src/main/java/com/google/devtools/build/lib/skyframe/TargetPatternPhaseValue.java
@@ -166,6 +166,19 @@
testFilter);
}
+ /**
+ * Creates a new target pattern sky key which represents the given target patterns without
+ * attempting to filter them in any way (for example, ignores options such as only loading tests).
+ *
+ * @param targetPatterns list of targets to evaluate
+ * @param offset relative path to the working directory
+ */
+ @ThreadSafe
+ public static SkyKey keyWithoutFilters(ImmutableList<String> targetPatterns, String offset) {
+ return new TargetPatternPhaseKey(
+ targetPatterns, offset, false, false, false, ImmutableList.of(), false, false, null);
+ }
+
/** The configuration needed to run the target pattern evaluation phase. */
@ThreadSafe
@VisibleForSerialization
diff --git a/src/test/java/com/google/devtools/build/lib/analysis/util/AnalysisTestCase.java b/src/test/java/com/google/devtools/build/lib/analysis/util/AnalysisTestCase.java
index 614cfb0..a7ca224 100644
--- a/src/test/java/com/google/devtools/build/lib/analysis/util/AnalysisTestCase.java
+++ b/src/test/java/com/google/devtools/build/lib/analysis/util/AnalysisTestCase.java
@@ -379,7 +379,7 @@
reporter, ModifiedFileSet.EVERYTHING_MODIFIED, Root.fromPath(rootDirectory));
TargetPatternPhaseValue loadingResult =
- skyframeExecutor.loadTargetPatterns(
+ skyframeExecutor.loadTargetPatternsWithFilters(
reporter,
ImmutableList.copyOf(labels),
PathFragment.EMPTY_FRAGMENT,
diff --git a/src/test/java/com/google/devtools/build/lib/analysis/util/BuildViewTestCase.java b/src/test/java/com/google/devtools/build/lib/analysis/util/BuildViewTestCase.java
index 714a6e6..6ded04e 100644
--- a/src/test/java/com/google/devtools/build/lib/analysis/util/BuildViewTestCase.java
+++ b/src/test/java/com/google/devtools/build/lib/analysis/util/BuildViewTestCase.java
@@ -1818,7 +1818,7 @@
AnalysisOptions viewOptions = Options.getDefaults(AnalysisOptions.class);
TargetPatternPhaseValue loadingResult =
- skyframeExecutor.loadTargetPatterns(
+ skyframeExecutor.loadTargetPatternsWithFilters(
reporter,
targets,
PathFragment.EMPTY_FRAGMENT,
diff --git a/src/test/java/com/google/devtools/build/lib/pkgcache/LoadingPhaseRunnerTest.java b/src/test/java/com/google/devtools/build/lib/pkgcache/LoadingPhaseRunnerTest.java
index 5c2d84a..1ab99d4 100644
--- a/src/test/java/com/google/devtools/build/lib/pkgcache/LoadingPhaseRunnerTest.java
+++ b/src/test/java/com/google/devtools/build/lib/pkgcache/LoadingPhaseRunnerTest.java
@@ -82,7 +82,7 @@
import org.junit.runner.RunWith;
import org.junit.runners.JUnit4;
-/** Tests for {@link SkyframeExecutor#loadTargetPatterns}. */
+/** Tests for {@link SkyframeExecutor#loadTargetPatternsWithFilters}. */
@RunWith(JUnit4.class)
public class LoadingPhaseRunnerTest {
@@ -1308,7 +1308,7 @@
sync();
storedErrors.clear();
TargetPatternPhaseValue result =
- skyframeExecutor.loadTargetPatterns(
+ skyframeExecutor.loadTargetPatternsWithFilters(
storedErrors,
ImmutableList.copyOf(patterns),
relativeWorkingDirectory,
diff --git a/src/test/java/com/google/devtools/build/lib/skylark/StarlarkOptionsParsingTest.java b/src/test/java/com/google/devtools/build/lib/skylark/StarlarkOptionsParsingTest.java
index 9d0b565..fbcd510 100644
--- a/src/test/java/com/google/devtools/build/lib/skylark/StarlarkOptionsParsingTest.java
+++ b/src/test/java/com/google/devtools/build/lib/skylark/StarlarkOptionsParsingTest.java
@@ -23,17 +23,12 @@
import com.google.devtools.build.lib.packages.StarlarkSemanticsOptions;
import com.google.devtools.build.lib.pkgcache.LoadingOptions;
import com.google.devtools.build.lib.pkgcache.PackageCacheOptions;
-import com.google.devtools.build.lib.runtime.BlazeCommand;
-import com.google.devtools.build.lib.runtime.BlazeCommandResult;
import com.google.devtools.build.lib.runtime.ClientOptions;
-import com.google.devtools.build.lib.runtime.Command;
-import com.google.devtools.build.lib.runtime.CommandEnvironment;
import com.google.devtools.build.lib.runtime.CommonCommandOptions;
import com.google.devtools.build.lib.runtime.KeepGoingOption;
import com.google.devtools.build.lib.runtime.StarlarkOptionsParser;
import com.google.devtools.build.lib.runtime.UiOptions;
import com.google.devtools.build.lib.skylark.util.SkylarkTestCase;
-import com.google.devtools.build.lib.util.ExitCode;
import com.google.devtools.build.lib.vfs.PathFragment;
import com.google.devtools.common.options.OptionsBase;
import com.google.devtools.common.options.OptionsParser;
@@ -75,36 +70,9 @@
skyframeExecutor, reporter, PathFragment.EMPTY_FRAGMENT, optionsParser);
}
- @Command(
- name = "residue",
- builds = true,
- options = {
- PackageCacheOptions.class,
- StarlarkSemanticsOptions.class,
- KeepGoingOption.class,
- LoadingOptions.class,
- ClientOptions.class,
- UiOptions.class,
- },
- allowResidue = true,
- shortDescription =
- "a dummy command for testing that allows residue and recognizes all"
- + " relevant options for starlark options parsing.",
- help = "")
- private static class ResidueCommand implements BlazeCommand {
- @Override
- public BlazeCommandResult exec(CommandEnvironment env, OptionsParsingResult options) {
- return BlazeCommandResult.exitCode(ExitCode.SUCCESS);
- }
-
- @Override
- public void editOptions(OptionsParser optionsParser) {}
- }
-
private OptionsParsingResult parseStarlarkOptions(String options) throws Exception {
starlarkOptionsParser.setResidueForTesting(Arrays.asList(options.split(" ")));
- starlarkOptionsParser.parse(
- ResidueCommand.class.getAnnotation(Command.class), new StoredEventHandler());
+ starlarkOptionsParser.parse(new StoredEventHandler());
return starlarkOptionsParser.getNativeOptionsParserFortesting();
}
@@ -421,4 +389,14 @@
assertThat(result.getStarlarkOptions()).isEmpty();
}
+
+ @Test
+ public void testOptionsAreParsedWithBuildTestsOnly() throws Exception {
+ writeBasicIntFlag();
+ optionsParser.parse("--build_tests_only");
+
+ OptionsParsingResult result = parseStarlarkOptions("--//test:my_int_setting=15");
+
+ assertThat(result.getStarlarkOptions().get("//test:my_int_setting")).isEqualTo(15);
+ }
}
