Skip to content

Commit

Permalink
Fix constraint checking for OutputFileConfiguredTargets
Browse files Browse the repository at this point in the history
When a rule creates additional labels using output-typed attributes, and then those output-file labels are used inside a select(), Bazel checks the restricted_to constraints for that dependency "statically", as if they weren't in a select at all. This leads to the confounding behavior that the following construct passes analysis fine:

genrule(
    name = "foo",
    outs = ["foo.txt"],
    cmd = "echo foo > \"$@\"",
    restricted_to = [":a"],
)

filegroup(
    name = "uses_foo",
    srcs = select({
        ":some_condition": [":foo"],
        "//conditions:default": [],
    }),
    restricted_to = [":a", ":b"]
)

But if the label in the srcs select is changed to ":foo.txt" (which is the exact same thing) bazel will error with

ERROR: in filegroup rule //:uses_foo:
  dependency //:foo doesn't support expected environment: //:b

Fix by moving the calculation of depLabelInSelect up above the point where the OutputFileConfiguredTarget is converted into the underlying ConfiguredTarget.

Closes #21539.

PiperOrigin-RevId: 612507761
Change-Id: Icc77bcf56729792268f1edac76a95dbffa72a4f5
  • Loading branch information
pcjanzen authored and copybara-github committed Mar 4, 2024
1 parent 099897d commit 5ebdbfc
Show file tree
Hide file tree
Showing 2 changed files with 30 additions and 8 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -748,6 +748,14 @@ private static DepsToCheck getConstraintCheckedDependencies(RuleContext ruleCont
Set<Label> selectOnlyDepsForThisAttribute =
getDepsOnlyInSelects(ruleContext, attr, attributes.getAttributeType(attr));
for (TransitiveInfoCollection dep : ruleContext.getPrerequisites(attr)) {
// For normal configured targets the target's label is the same label appearing in the
// select(). But for AliasConfiguredTargets the label in the select() refers to the alias,
// while dep.getLabel() refers to the target the alias points to. So add this quick check
// to make sure we're comparing the same labels.
Label depLabelInSelect =
(dep instanceof ConfiguredTarget)
? ((ConfiguredTarget) dep).getOriginalLabel()
: dep.getLabel();
// Output files inherit the environment spec of their generating rule.
if (dep instanceof OutputFileConfiguredTarget) {
// Note this reassignment means constraint violation errors reference the generating
Expand All @@ -758,14 +766,6 @@ private static DepsToCheck getConstraintCheckedDependencies(RuleContext ruleCont
// checking, but for now just pass them by.
if (dep.getProvider(SupportedEnvironmentsProvider.class) != null) {
depsToCheck.add(dep);
// For normal configured targets the target's label is the same label appearing in the
// select(). But for AliasConfiguredTargets the label in the select() refers to the alias,
// while dep.getLabel() refers to the target the alias points to. So add this quick check
// to make sure we're comparing the same labels.
Label depLabelInSelect =
(dep instanceof ConfiguredTarget)
? ((ConfiguredTarget) dep).getOriginalLabel()
: dep.getLabel();
if (!selectOnlyDepsForThisAttribute.contains(depLabelInSelect)) {
depsOutsideSelects.add(dep);
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -1041,6 +1041,28 @@ public void selectableAliasDepsTreatedLikeOtherDeps() throws Exception {
assertThat(getConfiguredTarget("//hello:lib")).isNotNull();
}

@Test
public void selectableOutputFileDepsTreatedLikeOtherDeps() throws Exception {
new EnvironmentGroupMaker("buildenv/foo").setEnvironments("a", "b").setDefaults().make();
writeDepsForSelectTests();
scratch.file(
"hello/BUILD",
"genrule(",
" name = 'src_a',",
" outs = ['src_a.c'],",
" cmd = 'touch $@',",
" restricted_to = ['//buildenv/foo:a'])",
"cc_library(",
" name = 'lib',",
" srcs = select({",
" '//config:a': [':src_a.c'],",
" '//config:b': [],",
" }),",
" compatible_with = ['//buildenv/foo:a', '//buildenv/foo:b'])");
useConfiguration("--define", "mode=a");
assertThat(getConfiguredTarget("//hello:lib")).isNotNull();
}

@Test
public void staticCheckingOnSelectsTemporarilyDisabled() throws Exception {
// TODO(bazel-team): update this test once static checking on selects is implemented. When
Expand Down

0 comments on commit 5ebdbfc

Please sign in to comment.