Skip to content

Commit

Permalink
Fix a bug in SkyframeExecutor.getConfigurations() for dynamic
Browse files Browse the repository at this point in the history
configurations:

That calls getDynamicConfigOptions() to get the right BuildOptions
for a target, but that fails to trim the option's FragmentOptions.
That means, for example, a BuildConfiguration could get constructed
with just the CppConfiguration fragment but with a BuildOptions
that still has PythonOptions.

This isn't just sloppy. It breaks Bazel tests that use
SkyframeExecutor.getConfiguredTargetForTesting (which follows this
code path) to compare against the production code path (which
already properly trims deps). So two configured targets that should be
equal won't be because their BuildConfigurations won't match.

This is also a prerequisite change for trimming top-level configurations,
coming up soon.

TESTED: BuildViewTest#testNewActionsAreDifferentAndDontConflict (and other
tests) pass with this change + the still-pending top-level-trimming change.

--
MOS_MIGRATED_REVID=123676990
  • Loading branch information
gregestren authored and dslomov committed Jun 1, 2016
1 parent 9ab8c9d commit 6c11b66
Showing 1 changed file with 21 additions and 9 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -270,6 +270,8 @@ public abstract class SkyframeExecutor implements WalkableGraphFactory {

private final PathFragment blacklistedPackagePrefixesFile;

private final RuleClassProvider ruleClassProvider;

private static final Logger LOG = Logger.getLogger(SkyframeExecutor.class.getName());

protected SkyframeExecutor(
Expand Down Expand Up @@ -308,11 +310,12 @@ protected SkyframeExecutor(
this.blacklistedPackagePrefixesFile = blacklistedPackagePrefixesFile;
this.binTools = binTools;

this.ruleClassProvider = pkgFactory.getRuleClassProvider();
this.skyframeBuildView = new SkyframeBuildView(
directories,
this,
binTools,
(ConfiguredRuleClassProvider) pkgFactory.getRuleClassProvider());
(ConfiguredRuleClassProvider) ruleClassProvider);
this.artifactFactory.set(skyframeBuildView.getArtifactFactory());
this.externalFilesHelper = new ExternalFilesHelper(
pkgLocator, this.errorOnExternalFiles, directories);
Expand Down Expand Up @@ -1259,16 +1262,20 @@ private Map<Dependency, BuildConfiguration> getConfigurations(EventHandler event
if (!depsToEvaluate.contains(key) || labelsWithErrors.contains(key.getLabel())) {
continue;
}
configSkyKeys.add(BuildConfigurationValue.key(fragmentsMap.get(key.getLabel()),
getDynamicConfigOptions(key, fromOptions)));
Set<Class<? extends BuildConfiguration.Fragment>> depFragments =
fragmentsMap.get(key.getLabel());
configSkyKeys.add(BuildConfigurationValue.key(depFragments,
getDynamicConfigOptions(key, fromOptions, depFragments)));
}
EvaluationResult<SkyValue> configsResult = evaluateSkyKeys(eventHandler, configSkyKeys);
for (Dependency key : keys) {
if (!depsToEvaluate.contains(key) || labelsWithErrors.contains(key.getLabel())) {
continue;
}
SkyKey configKey = BuildConfigurationValue.key(fragmentsMap.get(key.getLabel()),
getDynamicConfigOptions(key, fromOptions));
Set<Class<? extends BuildConfiguration.Fragment>> depFragments =
fragmentsMap.get(key.getLabel());
SkyKey configKey = BuildConfigurationValue.key(depFragments,
getDynamicConfigOptions(key, fromOptions, depFragments));
builder.put(key, ((BuildConfigurationValue) configsResult.get(configKey)).getConfiguration());
}

Expand All @@ -1279,14 +1286,19 @@ private Map<Dependency, BuildConfiguration> getConfigurations(EventHandler event
* Computes the build options needed for the given key, accounting for transitions possibly
* specified in the key.
*/
private BuildOptions getDynamicConfigOptions(Dependency key, BuildOptions fromOptions) {
private BuildOptions getDynamicConfigOptions(Dependency key, BuildOptions fromOptions,
Iterable<Class<? extends BuildConfiguration.Fragment>> requiredFragments) {
if (key.hasStaticConfiguration()) {
return key.getConfiguration().getOptions();
} else if (key.getTransition() == Attribute.ConfigurationTransition.NONE) {
return fromOptions;
}
BuildOptions toOptions;
if (key.getTransition() == Attribute.ConfigurationTransition.NONE) {
toOptions = fromOptions;
} else {
return ((PatchTransition) key.getTransition()).apply(fromOptions);
toOptions = ((PatchTransition) key.getTransition()).apply(fromOptions);
}
return toOptions.trim(
BuildConfiguration.getOptionsClasses(requiredFragments, ruleClassProvider));
}

/**
Expand Down

0 comments on commit 6c11b66

Please sign in to comment.