Skip to content

Commit fe3f455

Browse files
fmeumcopybara-github
fmeum
authored and
copybara-github
committed
Improve action key computation for command lines with Labels
This avoids rerunning an action when the main repo mapping changes in unrelated ways, for example because a new repo is added. Clean up one unnecessary `null` check on `mainRepositoryMapping`. Fixes #27061 Closes #27081. PiperOrigin-RevId: 818857131 Change-Id: I5036444dd6b39b12063f927ad807e2a93aeaf69f
1 parent 1241b55 commit fe3f455

File tree

2 files changed

+174
-17
lines changed

2 files changed

+174
-17
lines changed

‎src/main/java/com/google/devtools/build/lib/analysis/starlark/StarlarkCustomCommandLine.java‎

Lines changed: 29 additions & 17 deletions
Original file line numberDiff line numberDiff line change
@@ -512,7 +512,8 @@ private int addToFingerprint(
512512
ActionKeyContext actionKeyContext,
513513
Fingerprint fingerprint,
514514
@Nullable InputMetadataProvider inputMetadataProvider,
515-
CoreOptions.OutputPathsMode outputPathsMode)
515+
CoreOptions.OutputPathsMode outputPathsMode,
516+
RepositoryMapping mainRepoMapping)
516517
throws CommandLineExpansionException, InterruptedException {
517518
StarlarkCallable mapEach = null;
518519
Location location = null;
@@ -569,6 +570,10 @@ private int addToFingerprint(
569570
}
570571
} else {
571572
fingerprint.addInt(stringificationType.ordinal());
573+
if (stringificationType == StringificationType.LABEL) {
574+
fingerprint.addStringMap(
575+
Maps.transformValues(mainRepoMapping.entries(), RepositoryName::getName));
576+
}
572577
actionKeyContext.addNestedSetToFingerprint(fingerprint, values);
573578
}
574579
} else {
@@ -597,7 +602,7 @@ private int addToFingerprint(
597602
starlarkSemantics);
598603
} else {
599604
for (Object value : maybeExpandedValues) {
600-
addSingleObjectToFingerprint(fingerprint, value);
605+
addSingleObjectToFingerprint(fingerprint, value, mainRepoMapping);
601606
}
602607
}
603608
}
@@ -822,9 +827,13 @@ static int preprocess(
822827
return argi;
823828
}
824829

825-
static int addToFingerprint(List<Object> arguments, int argi, Fingerprint fingerprint) {
830+
static int addToFingerprint(
831+
List<Object> arguments,
832+
int argi,
833+
Fingerprint fingerprint,
834+
@Nullable RepositoryMapping mainRepoMapping) {
826835
Object object = arguments.get(argi++);
827-
addSingleObjectToFingerprint(fingerprint, object);
836+
addSingleObjectToFingerprint(fingerprint, object, mainRepoMapping);
828837
String formatStr = (String) arguments.get(argi++);
829838
fingerprint.addString(formatStr);
830839
fingerprint.addUUID(SINGLE_FORMATTED_ARG_UUID);
@@ -958,7 +967,7 @@ public final Iterable<String> arguments(
958967
private static Object /* String | DerivedArtifact */ expandToCommandLine(
959968
Object object, @Nullable RepositoryMapping mainRepoMapping) {
960969
// Label arguments are rare, so we don't bother rendering them lazily.
961-
if (mainRepoMapping != null && object instanceof Label label) {
970+
if (object instanceof Label label) {
962971
return label.getDisplayForm(mainRepoMapping);
963972
}
964973

@@ -968,13 +977,14 @@ public final Iterable<String> arguments(
968977
: CommandLineItem.expandToCommandLine(object);
969978
}
970979

971-
private static void addSingleObjectToFingerprint(Fingerprint fingerprint, Object object) {
980+
private static void addSingleObjectToFingerprint(
981+
Fingerprint fingerprint, Object object, @Nullable RepositoryMapping mainRepoMapping) {
982+
if (object instanceof Label label) {
983+
fingerprint.addString(label.getDisplayForm(mainRepoMapping));
984+
return;
985+
}
972986
StringificationType stringificationType =
973-
switch (object) {
974-
case FileApi ignored -> StringificationType.FILE;
975-
case Label ignored -> StringificationType.LABEL;
976-
default -> StringificationType.DEFAULT;
977-
};
987+
object instanceof FileApi ? StringificationType.FILE : StringificationType.DEFAULT;
978988
fingerprint.addInt(stringificationType.ordinal());
979989
fingerprint.addString(CommandLineItem.expandToCommandLine(object));
980990
}
@@ -1047,11 +1057,12 @@ public void addToFingerprint(
10471057
throws CommandLineExpansionException, InterruptedException {
10481058
List<Object> arguments = rawArgsAsList();
10491059
int size;
1050-
if (arguments.getLast() instanceof RepositoryMapping mainRepoMapping) {
1051-
fingerprint.addStringMap(
1052-
Maps.transformValues(mainRepoMapping.entries(), RepositoryName::getName));
1060+
RepositoryMapping mainRepoMapping;
1061+
if (arguments.getLast() instanceof RepositoryMapping mapping) {
1062+
mainRepoMapping = mapping;
10531063
size = arguments.size() - 1;
10541064
} else {
1065+
mainRepoMapping = null;
10551066
size = arguments.size();
10561067
}
10571068
for (int argi = 0; argi < size; ) {
@@ -1065,11 +1076,12 @@ public void addToFingerprint(
10651076
actionKeyContext,
10661077
fingerprint,
10671078
inputMetadataProvider,
1068-
effectiveOutputPathsMode);
1079+
effectiveOutputPathsMode,
1080+
mainRepoMapping);
10691081
} else if (arg == SINGLE_FORMATTED_ARG_MARKER) {
1070-
argi = SingleFormattedArg.addToFingerprint(arguments, argi, fingerprint);
1082+
argi = SingleFormattedArg.addToFingerprint(arguments, argi, fingerprint, mainRepoMapping);
10711083
} else {
1072-
addSingleObjectToFingerprint(fingerprint, arg);
1084+
addSingleObjectToFingerprint(fingerprint, arg, mainRepoMapping);
10731085
}
10741086
}
10751087
}

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

Lines changed: 145 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -3546,6 +3546,151 @@ public void starlarkCustomCommandLineKeyComputation_inconsequentialChangeToStarl
35463546
assertThat(getDigest(commandLine1)).isEqualTo(getDigest(commandLine2));
35473547
}
35483548

3549+
@Test
3550+
public void starlarkCustomCommandLineKeyComputation_singleLabel_repoMappingChanges_relevant()
3551+
throws Exception {
3552+
setRuleContext(createRuleContext("//foo:foo"));
3553+
3554+
var mainRepoMapping1 =
3555+
RepositoryMapping.create(
3556+
ImmutableMap.of("apparent1", RepositoryName.createUnvalidated("canonical+")),
3557+
RepositoryName.MAIN);
3558+
var mainRepoMapping2 =
3559+
RepositoryMapping.create(
3560+
ImmutableMap.of("apparent2", RepositoryName.createUnvalidated("canonical+")),
3561+
RepositoryName.MAIN);
3562+
var args =
3563+
"""
3564+
args = ruleContext.actions.args()
3565+
args.add(Label("@@canonical+//foo:bar"))
3566+
""";
3567+
var commandLine1 = getCommandLine(mainRepoMapping1, args);
3568+
var commandLine2 = getCommandLine(mainRepoMapping2, args);
3569+
3570+
assertThat(ImmutableList.copyOf(commandLine1.arguments()))
3571+
.isNotEqualTo(ImmutableList.copyOf(commandLine2.arguments()));
3572+
assertThat(getDigest(commandLine1)).isNotEqualTo(getDigest(commandLine2));
3573+
}
3574+
3575+
@Test
3576+
public void starlarkCustomCommandLineKeyComputation_singleLabel_repoMappingChanges_notRelevant()
3577+
throws Exception {
3578+
setRuleContext(createRuleContext("//foo:foo"));
3579+
3580+
var mainRepoMapping1 =
3581+
RepositoryMapping.create(
3582+
ImmutableMap.of(
3583+
"apparent", RepositoryName.createUnvalidated("canonical+"),
3584+
"other_repo1", RepositoryName.createUnvalidated("other_repo+")),
3585+
RepositoryName.MAIN);
3586+
var mainRepoMapping2 =
3587+
RepositoryMapping.create(
3588+
ImmutableMap.of(
3589+
"apparent", RepositoryName.createUnvalidated("canonical+"),
3590+
"other_repo2", RepositoryName.createUnvalidated("other_repo+")),
3591+
RepositoryName.MAIN);
3592+
var args =
3593+
"""
3594+
args = ruleContext.actions.args()
3595+
args.add(Label("@@canonical+//foo:bar"))
3596+
""";
3597+
var commandLine1 = getCommandLine(mainRepoMapping1, args);
3598+
var commandLine2 = getCommandLine(mainRepoMapping2, args);
3599+
3600+
assertThat(commandLine1.arguments())
3601+
.containsExactlyElementsIn(commandLine2.arguments())
3602+
.inOrder();
3603+
assertThat(getDigest(commandLine1)).isEqualTo(getDigest(commandLine2));
3604+
}
3605+
3606+
@Test
3607+
public void starlarkCustomCommandLineKeyComputation_listOfLabels_repoMappingChanges_relevant()
3608+
throws Exception {
3609+
setRuleContext(createRuleContext("//foo:foo"));
3610+
3611+
var mainRepoMapping1 =
3612+
RepositoryMapping.create(
3613+
ImmutableMap.of("apparent1", RepositoryName.createUnvalidated("canonical+")),
3614+
RepositoryName.MAIN);
3615+
var mainRepoMapping2 =
3616+
RepositoryMapping.create(
3617+
ImmutableMap.of("apparent2", RepositoryName.createUnvalidated("canonical+")),
3618+
RepositoryName.MAIN);
3619+
var args =
3620+
"""
3621+
args = ruleContext.actions.args()
3622+
args.add_all([Label("@@canonical+//foo:bar"), Label("@@canonical+//foo:baz")])
3623+
""";
3624+
var commandLine1 = getCommandLine(mainRepoMapping1, args);
3625+
var commandLine2 = getCommandLine(mainRepoMapping2, args);
3626+
3627+
assertThat(ImmutableList.copyOf(commandLine1.arguments()))
3628+
.isNotEqualTo(ImmutableList.copyOf(commandLine2.arguments()));
3629+
assertThat(getDigest(commandLine1)).isNotEqualTo(getDigest(commandLine2));
3630+
}
3631+
3632+
@Test
3633+
public void starlarkCustomCommandLineKeyComputation_listOfLabels_repoMappingChanges_notRelevant()
3634+
throws Exception {
3635+
setRuleContext(createRuleContext("//foo:foo"));
3636+
3637+
var mainRepoMapping1 =
3638+
RepositoryMapping.create(
3639+
ImmutableMap.of(
3640+
"apparent",
3641+
RepositoryName.createUnvalidated("canonical+"),
3642+
"other_repo1",
3643+
RepositoryName.createUnvalidated("other_repo+")),
3644+
RepositoryName.MAIN);
3645+
var mainRepoMapping2 =
3646+
RepositoryMapping.create(
3647+
ImmutableMap.of(
3648+
"apparent",
3649+
RepositoryName.createUnvalidated("canonical+"),
3650+
"other_repo2",
3651+
RepositoryName.createUnvalidated("other_repo+")),
3652+
RepositoryName.MAIN);
3653+
var args =
3654+
"""
3655+
args = ruleContext.actions.args()
3656+
args.add_all([Label("@@canonical+//foo:bar"), Label("@@canonical+//foo:baz")])
3657+
""";
3658+
var commandLine1 = getCommandLine(mainRepoMapping1, args);
3659+
var commandLine2 = getCommandLine(mainRepoMapping2, args);
3660+
3661+
assertThat(commandLine1.arguments())
3662+
.containsExactlyElementsIn(commandLine2.arguments())
3663+
.inOrder();
3664+
assertThat(getDigest(commandLine1)).isEqualTo(getDigest(commandLine2));
3665+
}
3666+
3667+
@Test
3668+
public void
3669+
starlarkCustomCommandLineKeyComputation_nestedSetOfLabels_repoMappingChanges_relevant()
3670+
throws Exception {
3671+
setRuleContext(createRuleContext("//foo:foo"));
3672+
3673+
var mainRepoMapping1 =
3674+
RepositoryMapping.create(
3675+
ImmutableMap.of("apparent1", RepositoryName.createUnvalidated("canonical+")),
3676+
RepositoryName.MAIN);
3677+
var mainRepoMapping2 =
3678+
RepositoryMapping.create(
3679+
ImmutableMap.of("apparent2", RepositoryName.createUnvalidated("canonical+")),
3680+
RepositoryName.MAIN);
3681+
var args =
3682+
"""
3683+
args = ruleContext.actions.args()
3684+
args.add_all(depset([Label("@@canonical+//foo:bar"), Label("@@canonical+//foo:baz")]))
3685+
""";
3686+
var commandLine1 = getCommandLine(mainRepoMapping1, args);
3687+
var commandLine2 = getCommandLine(mainRepoMapping2, args);
3688+
3689+
assertThat(ImmutableList.copyOf(commandLine1.arguments()))
3690+
.isNotEqualTo(ImmutableList.copyOf(commandLine2.arguments()));
3691+
assertThat(getDigest(commandLine1)).isNotEqualTo(getDigest(commandLine2));
3692+
}
3693+
35493694
@Test
35503695
public void starlarkCustomCommandLineKeyComputation_labelVsString() throws Exception {
35513696
setRuleContext(createRuleContext("//foo:foo"));

0 commit comments

Comments
 (0)