Skip to content

Commit 43fdcd3

Browse files
fmeumcopybara-github
fmeum
authored and
copybara-github
committed
Follow unconfigured aliases during options parsing
Fixes #20582 RELNOTES: Starlark command-line flags can now be referred to through `alias` targets. Closes #22192. PiperOrigin-RevId: 629865954 Change-Id: I6215c8484ddc08e75507191bfa1eb5bc709c5fc6
1 parent b90e53e commit 43fdcd3

2 files changed

Lines changed: 210 additions & 16 deletions

File tree

‎src/main/java/com/google/devtools/build/lib/runtime/StarlarkOptionsParser.java‎

Lines changed: 68 additions & 16 deletions
Original file line numberDiff line numberDiff line change
@@ -17,6 +17,7 @@
1717
import static com.google.devtools.build.lib.analysis.config.CoreOptionConverters.BUILD_SETTING_CONVERTERS;
1818
import static com.google.devtools.build.lib.packages.RuleClass.Builder.STARLARK_BUILD_SETTING_DEFAULT_ATTR_NAME;
1919
import static com.google.devtools.build.lib.packages.Type.BOOLEAN;
20+
import static java.util.stream.Collectors.joining;
2021

2122
import com.google.common.annotations.VisibleForTesting;
2223
import com.google.common.base.Preconditions;
@@ -27,7 +28,7 @@
2728
import com.google.devtools.build.lib.cmdline.Label;
2829
import com.google.devtools.build.lib.cmdline.TargetParsingException;
2930
import com.google.devtools.build.lib.packages.BuildSetting;
30-
import com.google.devtools.build.lib.packages.Rule;
31+
import com.google.devtools.build.lib.packages.BuildType;
3132
import com.google.devtools.build.lib.packages.Target;
3233
import com.google.devtools.build.lib.packages.Type;
3334
import com.google.devtools.build.lib.packages.Types;
@@ -38,10 +39,13 @@
3839
import java.util.ArrayList;
3940
import java.util.Collection;
4041
import java.util.HashMap;
42+
import java.util.LinkedHashSet;
4143
import java.util.List;
4244
import java.util.Map;
4345
import java.util.Objects;
46+
import java.util.SequencedSet;
4447
import java.util.TreeMap;
48+
import java.util.stream.Stream;
4549
import javax.annotation.Nullable;
4650

4751
/**
@@ -282,7 +286,7 @@ private boolean parseArg(String arg, Multimap<String, Pair<String, Target>> unpa
282286
}
283287

284288
/**
285-
* Returns the given build setting's {@link Target}.
289+
* Returns the given build setting's {@link Target}, following (unconfigured) aliases if needed.
286290
*
287291
* @return the target, or null if the {@link BuildSettingLoader} needs to do more work to retrieve
288292
* the target
@@ -293,23 +297,71 @@ private Target loadBuildSetting(String targetToBuild) throws OptionsParsingExcep
293297
return buildSettings.get(targetToBuild);
294298
}
295299

296-
Target buildSetting;
297-
try {
298-
buildSetting = buildSettingLoader.loadBuildSetting(targetToBuild);
299-
if (buildSetting == null) {
300-
return null;
300+
Target target;
301+
String targetToLoadNext = targetToBuild;
302+
SequencedSet<Label> aliasChain = new LinkedHashSet<>();
303+
while (true) {
304+
try {
305+
target = buildSettingLoader.loadBuildSetting(targetToLoadNext);
306+
if (target == null) {
307+
return null;
308+
}
309+
} catch (InterruptedException | TargetParsingException e) {
310+
Thread.currentThread().interrupt();
311+
throw new OptionsParsingException(
312+
"Error loading option " + targetToBuild + ": " + e.getMessage(), targetToBuild, e);
313+
}
314+
if (!aliasChain.add(target.getLabel())) {
315+
throw new OptionsParsingException(
316+
String.format(
317+
"Failed to load build setting '%s' due to a cycle in alias chain: %s",
318+
targetToBuild,
319+
formatAliasChain(Stream.concat(aliasChain.stream(), Stream.of(target.getLabel())))),
320+
targetToBuild);
321+
}
322+
if (target.getAssociatedRule() == null) {
323+
throw new OptionsParsingException(
324+
String.format("Unrecognized option: %s", formatAliasChain(aliasChain.stream())),
325+
targetToBuild);
326+
}
327+
if (target.getAssociatedRule().isBuildSetting()) {
328+
break;
329+
}
330+
// Follow the unconfigured values of aliases.
331+
if (target.getAssociatedRule().getRuleClass().equals("alias")) {
332+
targetToLoadNext =
333+
switch (target.getAssociatedRule().getAttr("actual")) {
334+
case Label label -> label.getUnambiguousCanonicalForm();
335+
case BuildType.SelectorList<?> ignored ->
336+
throw new OptionsParsingException(
337+
String.format(
338+
"Failed to load build setting '%s' as it resolves to an alias with an"
339+
+ " actual value that uses select(): %s. This is not supported as"
340+
+ " build settings are needed to determine the configuration the"
341+
+ " select is evaluated in.",
342+
targetToBuild, formatAliasChain(aliasChain.stream())),
343+
targetToBuild);
344+
case null, default ->
345+
throw new IllegalStateException(
346+
String.format(
347+
"Alias target '%s' with 'actual' attr value not equals to a label or a"
348+
+ " selectorlist",
349+
target.getLabel()));
350+
};
351+
continue;
301352
}
302-
} catch (InterruptedException | TargetParsingException e) {
303-
Thread.currentThread().interrupt();
304353
throw new OptionsParsingException(
305-
"Error loading option " + targetToBuild + ": " + e.getMessage(), targetToBuild, e);
306-
}
307-
Rule associatedRule = buildSetting.getAssociatedRule();
308-
if (associatedRule == null || associatedRule.getRuleClassObject().getBuildSetting() == null) {
309-
throw new OptionsParsingException("Unrecognized option: " + targetToBuild, targetToBuild);
354+
String.format("Unrecognized option: %s", formatAliasChain(aliasChain.stream())),
355+
targetToBuild);
310356
}
311-
buildSettings.put(targetToBuild, buildSetting);
312-
return buildSetting;
357+
;
358+
359+
buildSettings.put(targetToBuild, target);
360+
return target;
361+
}
362+
363+
private static String formatAliasChain(Stream<Label> aliasChain) {
364+
return aliasChain.map(Label::getCanonicalForm).collect(joining(" -> "));
313365
}
314366

315367
@VisibleForTesting

‎src/test/java/com/google/devtools/build/lib/starlark/StarlarkOptionsParsingTest.java‎

Lines changed: 142 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -567,4 +567,146 @@ public void flagReferencesExactlyOneTarget() throws Exception {
567567
.hasMessageThat()
568568
.contains("//test:all: user-defined flags must reference exactly one target");
569569
}
570+
571+
@Test
572+
public void flagIsAlias() throws Exception {
573+
scratch.file(
574+
"test/build_setting.bzl",
575+
"""
576+
string_flag = rule(
577+
implementation = lambda ctx: [],
578+
build_setting = config.string(flag = True),
579+
)
580+
""");
581+
scratch.file(
582+
"test/BUILD",
583+
"""
584+
load("//test:build_setting.bzl", "string_flag")
585+
586+
alias(
587+
name = "one",
588+
actual = "//test/pkg:two",
589+
)
590+
591+
string_flag(
592+
name = "three",
593+
build_setting_default = "",
594+
)
595+
""");
596+
scratch.file(
597+
"test/pkg/BUILD",
598+
"""
599+
alias(
600+
name = "two",
601+
actual = "//test:three",
602+
)
603+
""");
604+
605+
OptionsParsingResult result = parseStarlarkOptions("--//test:one=one --//test/pkg:two=two");
606+
607+
assertThat(result.getStarlarkOptions()).containsExactly("//test:three", "two");
608+
}
609+
610+
@Test
611+
public void flagIsAlias_cycle() throws Exception {
612+
scratch.file(
613+
"test/BUILD",
614+
"""
615+
alias(
616+
name = "one",
617+
actual = "//test/pkg:two",
618+
)
619+
620+
alias(
621+
name = "three",
622+
actual = ":one",
623+
)
624+
""");
625+
scratch.file(
626+
"test/pkg/BUILD",
627+
"""
628+
alias(
629+
name = "two",
630+
actual = "//test:three",
631+
)
632+
""");
633+
634+
OptionsParsingException e =
635+
assertThrows(OptionsParsingException.class, () -> parseStarlarkOptions("--//test:one=one"));
636+
637+
assertThat(e)
638+
.hasMessageThat()
639+
.isEqualTo(
640+
"Failed to load build setting '//test:one' due to a cycle in alias chain: //test:one"
641+
+ " -> //test/pkg:two -> //test:three -> //test:one");
642+
}
643+
644+
@Test
645+
public void flagIsAlias_usesSelect() throws Exception {
646+
scratch.file(
647+
"test/BUILD",
648+
"""
649+
alias(
650+
name = "one",
651+
actual = "//test/pkg:two",
652+
)
653+
654+
alias(
655+
name = "three",
656+
actual = ":one",
657+
)
658+
""");
659+
scratch.file(
660+
"test/pkg/BUILD",
661+
"""
662+
alias(
663+
name = "two",
664+
actual = select({"//conditions:default": "//test:three"}),
665+
)
666+
""");
667+
668+
OptionsParsingException e =
669+
assertThrows(OptionsParsingException.class, () -> parseStarlarkOptions("--//test:one=one"));
670+
671+
assertThat(e)
672+
.hasMessageThat()
673+
.isEqualTo(
674+
"Failed to load build setting '//test:one' as it resolves to an alias with an actual"
675+
+ " value that uses select(): //test:one -> //test/pkg:two. This is not supported"
676+
+ " as build settings are needed to determine the configuration the select is"
677+
+ " evaluated in.");
678+
}
679+
680+
@Test
681+
public void flagIsAlias_resolvesToNonBuildSettingTarget() throws Exception {
682+
scratch.file(
683+
"test/BUILD",
684+
"""
685+
alias(
686+
name = "one",
687+
actual = "//test/pkg:two",
688+
)
689+
690+
genrule(
691+
name = "three",
692+
outs = ["out"],
693+
cmd = "echo hello > $@",
694+
)
695+
""");
696+
scratch.file(
697+
"test/pkg/BUILD",
698+
"""
699+
alias(
700+
name = "two",
701+
actual = "//test:three",
702+
)
703+
""");
704+
705+
OptionsParsingException e =
706+
assertThrows(OptionsParsingException.class, () -> parseStarlarkOptions("--//test:one=one"));
707+
708+
assertThat(e)
709+
.hasMessageThat()
710+
.isEqualTo("Unrecognized option: //test:one -> //test/pkg:two -> //test:three");
711+
}
570712
}

0 commit comments

Comments
 (0)