diff options
2 files changed, 129 insertions, 35 deletions
diff --git a/src/main/java/com/google/devtools/build/lib/analysis/constraints/ConstraintSemantics.java b/src/main/java/com/google/devtools/build/lib/analysis/constraints/ConstraintSemantics.java index bc24ae9862..664d3feda4 100644 --- a/src/main/java/com/google/devtools/build/lib/analysis/constraints/ConstraintSemantics.java +++ b/src/main/java/com/google/devtools/build/lib/analysis/constraints/ConstraintSemantics.java @@ -535,53 +535,103 @@ public class ConstraintSemantics { DepsToCheck depsToCheck = getConstraintCheckedDependencies(ruleContext); for (TransitiveInfoCollection dep : depsToCheck.allDeps()) { - SupportedEnvironmentsProvider depEnvironments = - dep.getProvider(SupportedEnvironmentsProvider.class); if (!depsToCheck.isSelectOnly(dep)) { // TODO(bazel-team): support static constraint checking for selects. A selectable constraint // is valid if the union of all deps in the select includes all of this rule's static // environments. Determining that requires following the select paths that don't get chosen, // which means we won't have ConfiguredTargets for those deps and need to find another // way to get their environments. - Collection<Label> unsupportedEnvironments = - getUnsupportedEnvironments(depEnvironments.getStaticEnvironments(), staticEnvironments); - - if (!unsupportedEnvironments.isEmpty()) { - ruleContext.ruleError("dependency " + dep.getLabel() - + " doesn't support expected environment" - + (unsupportedEnvironments.size() == 1 ? "" : "s") - + ": " + Joiner.on(", ").join(unsupportedEnvironments)); - } + checkStaticConstraints(ruleContext, staticEnvironments, dep); } + refineEnvironmentsForDep(ruleContext, staticEnvironments, dep, labelsToEnvironments, + refinedEnvironmentsSoFar, groupsWithEnvironmentsRemoved, removedEnvironmentCulprits); + } - // Refine this rule's environments by intersecting with the dep's refined environments: - for (Label refinedEnvironmentToPrune : getUnsupportedEnvironments( - depEnvironments.getRefinedEnvironments(), staticEnvironments)) { - EnvironmentWithGroup envToPrune = labelsToEnvironments.get(refinedEnvironmentToPrune); - if (envToPrune == null) { - // If we have no record of this environment, that means the current rule implicitly uses - // the defaults for this group. So explicitly opt that group's defaults into the refined - // set before trying to remove specific items. - for (EnvironmentWithGroup defaultEnv : - getDefaults(refinedEnvironmentToPrune, depEnvironments.getRefinedEnvironments())) { - refinedEnvironmentsSoFar.add(defaultEnv); - labelsToEnvironments.put(defaultEnv.environment(), defaultEnv); - } - envToPrune = Verify.verifyNotNull(labelsToEnvironments.get(refinedEnvironmentToPrune)); + checkRefinedConstraints(ruleContext, groupsWithEnvironmentsRemoved, + refinedEnvironmentsSoFar, refinedEnvironments, removedEnvironmentCulprits); + } + + /** + * Performs static constraint checking against the given dep. + * + * @param ruleContext the rule being analyzed + * @param staticEnvironments the static environments of the rule being analyzed + * @param dep the dep to check + */ + private static void checkStaticConstraints(RuleContext ruleContext, + EnvironmentCollection staticEnvironments, TransitiveInfoCollection dep) { + SupportedEnvironmentsProvider depEnvironments = + dep.getProvider(SupportedEnvironmentsProvider.class); + Collection<Label> unsupportedEnvironments = + getUnsupportedEnvironments(depEnvironments.getStaticEnvironments(), staticEnvironments); + if (!unsupportedEnvironments.isEmpty()) { + ruleContext.ruleError("dependency " + dep.getLabel() + + " doesn't support expected environment" + + (unsupportedEnvironments.size() == 1 ? "" : "s") + + ": " + Joiner.on(", ").join(unsupportedEnvironments)); + } + } + + /** + * Helper method for {@link #checkConstraints}: refines a rule's environments with the given dep. + * + * <p>A rule's <b>complete</b> refined set applies this process to every dep. + */ + private static void refineEnvironmentsForDep( + RuleContext ruleContext, + EnvironmentCollection staticEnvironments, + TransitiveInfoCollection dep, + Map<Label, EnvironmentWithGroup> labelsToEnvironments, + Set<EnvironmentWithGroup> refinedEnvironmentsSoFar, + Set<EnvironmentGroup> groupsWithEnvironmentsRemoved, + Map<Label, Target> removedEnvironmentCulprits) { + + SupportedEnvironmentsProvider depEnvironments = + dep.getProvider(SupportedEnvironmentsProvider.class); + + // Stores the environments that are pruned from the refined set because of this dep. Even + // though they're removed, some subset of the environments they fulfill may belong in the + // refined set. For example, if environment "both" fulfills "a" and "b" and "lib" statically + // sets restricted_to = ["both"] and "dep" sets restricted_to = ["a"], then lib's refined set + // excludes "both". But rather than be emptied out it can be reduced to "a". + Set<Label> prunedEnvironmentsFromThisDep = new LinkedHashSet<>(); + + // Refine this rule's environments by intersecting with the dep's refined environments: + for (Label refinedEnvironmentToPrune : getUnsupportedEnvironments( + depEnvironments.getRefinedEnvironments(), staticEnvironments)) { + EnvironmentWithGroup envToPrune = labelsToEnvironments.get(refinedEnvironmentToPrune); + if (envToPrune == null) { + // If we have no record of this environment, that means the current rule implicitly uses + // the defaults for this group. So explicitly opt that group's defaults into the refined + // set before trying to remove specific items. + for (EnvironmentWithGroup defaultEnv : + getDefaults(refinedEnvironmentToPrune, depEnvironments.getRefinedEnvironments())) { + refinedEnvironmentsSoFar.add(defaultEnv); + labelsToEnvironments.put(defaultEnv.environment(), defaultEnv); + } + envToPrune = Verify.verifyNotNull(labelsToEnvironments.get(refinedEnvironmentToPrune)); + } + refinedEnvironmentsSoFar.remove(envToPrune); + groupsWithEnvironmentsRemoved.add(envToPrune.group()); + removedEnvironmentCulprits.put(envToPrune.environment(), + findOriginalRefiner(ruleContext, depEnvironments, envToPrune)); + prunedEnvironmentsFromThisDep.add(envToPrune.environment()); + } + + // Add in any dep environment that one of the environments we removed fulfills. In other + // words, the removed environment is no good, but some subset of it may be. + for (EnvironmentWithGroup depEnv : + depEnvironments.getRefinedEnvironments().getGroupedEnvironments()) { + for (Label fulfiller : depEnv.group().getFulfillers(depEnv.environment())) { + if (prunedEnvironmentsFromThisDep.contains(fulfiller)) { + refinedEnvironmentsSoFar.add(depEnv); } - refinedEnvironmentsSoFar.remove(envToPrune); - groupsWithEnvironmentsRemoved.add(envToPrune.group()); - removedEnvironmentCulprits.put(envToPrune.environment(), - findOriginalRefiner(ruleContext, depEnvironments, envToPrune)); } } - - checkRefinedEnvironmentConstraints(ruleContext, groupsWithEnvironmentsRemoved, - refinedEnvironmentsSoFar, refinedEnvironments, removedEnvironmentCulprits); } /** - * Helper method for checkConstraints: performs refined environment constraint checking. + * Helper method for {@link #checkConstraints}: performs refined environment constraint checking. * * <p>Refined environment expectations: no environment group should be emptied out due to * refining. This reflects the idea that some of the static declared environments get pruned @@ -589,7 +639,7 @@ public class ConstraintSemantics { * * <p>Violations of this expectation trigger rule analysis errors. */ - private static void checkRefinedEnvironmentConstraints( + private static void checkRefinedConstraints( RuleContext ruleContext, Set<EnvironmentGroup> groupsWithEnvironmentsRemoved, Set<EnvironmentWithGroup> refinedEnvironmentsSoFar, diff --git a/src/test/java/com/google/devtools/build/lib/analysis/constraints/ConstraintsTest.java b/src/test/java/com/google/devtools/build/lib/analysis/constraints/ConstraintsTest.java index fac4f006b6..1bf3b0f27a 100644 --- a/src/test/java/com/google/devtools/build/lib/analysis/constraints/ConstraintsTest.java +++ b/src/test/java/com/google/devtools/build/lib/analysis/constraints/ConstraintsTest.java @@ -1266,5 +1266,49 @@ public class ConstraintsTest extends AbstractConstraintsTest { + "\nenvironment group: //buildenv/bar:bar:\n" + " environment: //buildenv/bar:c removed by: //hello:lib (/workspace/hello/BUILD:9:1)"); } -} + private void writeRulesForRefiningSubsetTests(String topLevelRestrictedTo) throws Exception { + new EnvironmentGroupMaker("buildenv/foo") + .setEnvironments("a", "b", "all") + .setFulfills("all", "a") + .setFulfills("all", "b") + .setDefaults() + .make(); + scratch.file("hello/BUILD", + "cc_library(", + " name = 'lib',", + " srcs = [],", + " deps = [':dep1'],", + " restricted_to = ['//buildenv/foo:" + topLevelRestrictedTo + "'])", + "cc_library(", + " name = 'dep1',", + " srcs = [],", + // This is technically illegal because "dep1" declares support for both "a" and "b" but + // no dependency under the select can provide "b". This is known as "static select + // constraint checking" and is currently an unimplemented Bazel TODO. + " deps = select({", + " '//config:a': [':dep2'],", + " '//conditions:default': [':dep2'],", + " }),", + " restricted_to = ['//buildenv/foo:all'])", + "cc_library(", + " name = 'dep2',", + " srcs = [],", + " compatible_with = ['//buildenv/foo:a'])"); + } + + @Test + public void refiningReplacesRemovedEnvironmentWithValidFulfillingSubset() throws Exception { + writeRulesForRefiningSubsetTests("a"); + assertThat(getConfiguredTarget("//hello:lib")).isNotNull(); + } + + @Test + public void refiningReplacesRemovedEnvironmentWithInvalidFulfillingSubset() throws Exception { + writeRulesForRefiningSubsetTests("b"); + reporter.removeHandler(failFastHandler); + assertThat(getConfiguredTarget("//hello:lib")).isNull(); + assertContainsEvent("//hello:lib: the current command-line flags disqualify all supported " + + "environments because of incompatible select() paths"); + } +} |