Skip to content

Commit 98c77ea

Browse files
fmeumfmeum
authored
[8.5.0] Avoid action key change with expand_directories=False and no manual expansion (#27258)
Since 2a3191b, the action key was sensitive to changes of the expansion of a tree artifact even if expansion is disabled and the custom map_each callback cannot perform manual expansion. Work towards aspect-build/rules_js#2379 Closes #27163. PiperOrigin-RevId: 816229224 Change-Id: If56a9d54e52556e8bef4cabc5c6be3b2c8e150d9 (cherry picked from commit 5b68c32)
1 parent 5a00100 commit 98c77ea

File tree

2 files changed

+123
-6
lines changed

2 files changed

+123
-6
lines changed

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

Lines changed: 14 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -550,7 +550,13 @@ private int addToFingerprint(
550550
// fingerprinting stringificationType here.
551551
CommandLineItemMapEachAdaptor commandLineItemMapFn =
552552
new CommandLineItemMapEachAdaptor(
553-
mapEach, location, starlarkSemantics, artifactExpander, outputPathsMode);
553+
mapEach,
554+
location,
555+
starlarkSemantics,
556+
(features & EXPAND_DIRECTORIES) != 0 || wantsDirectoryExpander(mapEach)
557+
? artifactExpander
558+
: null,
559+
outputPathsMode);
554560
try {
555561
actionKeyContext.addNestedSetToFingerprint(commandLineItemMapFn, fingerprint, values);
556562
} finally {
@@ -1129,14 +1135,11 @@ private static void applyMapEach(
11291135
// TODO(b/77140311): Error if we issue print statements.
11301136
thread.setPrintHandler((th, msg) -> {});
11311137
int count = originalValues.size();
1132-
// map_each can accept either each object, or each object + a directory expander.
1133-
boolean wantsDirectoryExpander =
1134-
mapFn instanceof StarlarkFunction starlarkFunction
1135-
&& starlarkFunction.getParameterNames().size() >= 2;
11361138
// We create a list that we reuse for the args to map_each
11371139
List<Object> args = new ArrayList<>(2);
11381140
args.add(null); // This will be overwritten each iteration.
1139-
if (wantsDirectoryExpander) {
1141+
// map_each can accept either each object, or each object + a directory expander.
1142+
if (wantsDirectoryExpander(mapFn)) {
11401143
DirectoryExpander expander;
11411144
if (artifactExpander != null) {
11421145
expander = new FullExpander(artifactExpander);
@@ -1175,6 +1178,11 @@ private static void applyMapEach(
11751178
}
11761179
}
11771180

1181+
private static boolean wantsDirectoryExpander(StarlarkCallable mapFn) {
1182+
return mapFn instanceof StarlarkFunction starlarkFunction
1183+
&& starlarkFunction.getParameterNames().size() >= 2;
1184+
}
1185+
11781186
private static class CommandLineItemMapEachAdaptor
11791187
extends CommandLineItem.ParametrizedMapFn<Object> {
11801188
private final StarlarkCallable mapFn;

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

Lines changed: 109 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -654,6 +654,115 @@ def map_each(x, expander):
654654
.isNotEqualTo(fingerprintAfter.hexDigestAndReset());
655655
}
656656

657+
@Test
658+
public void
659+
vectorArgArguments_expandDirectoriesDisabled_noMapEach_expansionDoesNotAffectActionKey(
660+
@TestParameter boolean useNestedSet) throws Exception {
661+
SpecialArtifact tree = createTreeArtifact("tree");
662+
TreeFileArtifact child1 = TreeFileArtifact.createTreeOutput(tree, "child1");
663+
TreeFileArtifact child2 = TreeFileArtifact.createTreeOutput(tree, "child2");
664+
// The files won't be read so MISSING_FILE_MARKER will do
665+
TreeArtifactValue treeArtifactValueBefore =
666+
TreeArtifactValue.newBuilder(tree)
667+
.putChild(child1, FileArtifactValue.MISSING_FILE_MARKER)
668+
.putChild(child2, FileArtifactValue.MISSING_FILE_MARKER)
669+
.build();
670+
TreeArtifactValue treeArtifactValueAfter =
671+
TreeArtifactValue.newBuilder(tree)
672+
.putChild(child1, FileArtifactValue.MISSING_FILE_MARKER)
673+
.build();
674+
var vectorArg =
675+
useNestedSet
676+
? new VectorArg.Builder(
677+
NestedSetBuilder.wrap(Order.STABLE_ORDER, ImmutableList.of(tree)), Artifact.class)
678+
: new VectorArg.Builder(Tuple.of(tree));
679+
CommandLine commandLine =
680+
builder
681+
.add(vectorArg.setExpandDirectories(false).setLocation(Location.BUILTIN))
682+
.build(/* flagPerLine= */ false, RepositoryMapping.ALWAYS_FALLBACK);
683+
684+
var expanderBefore =
685+
createArtifactExpander(
686+
ImmutableMap.of(tree, treeArtifactValueBefore.getChildren()), ImmutableMap.of());
687+
var argumentsBefore = commandLine.arguments(expanderBefore, PathMapper.NOOP);
688+
var fingerprintBefore = new Fingerprint();
689+
commandLine.addToFingerprint(
690+
new ActionKeyContext(), expanderBefore, CoreOptions.OutputPathsMode.OFF, fingerprintBefore);
691+
assertThat(argumentsBefore).containsExactly("bin/tree");
692+
693+
var expanderAfter =
694+
createArtifactExpander(
695+
ImmutableMap.of(tree, treeArtifactValueAfter.getChildren()), ImmutableMap.of());
696+
var argumentsAfter = commandLine.arguments(expanderAfter, PathMapper.NOOP);
697+
var fingerprintAfter = new Fingerprint();
698+
commandLine.addToFingerprint(
699+
new ActionKeyContext(), expanderAfter, CoreOptions.OutputPathsMode.OFF, fingerprintAfter);
700+
assertThat(argumentsAfter).containsExactly("bin/tree");
701+
702+
assertThat(fingerprintBefore.hexDigestAndReset())
703+
.isEqualTo(fingerprintAfter.hexDigestAndReset());
704+
}
705+
706+
@Test
707+
public void
708+
vectorArgArguments_expandDirectoriesDisabled_noManualExpansion_expansionDoesNotAffectActionKey(
709+
@TestParameter boolean useNestedSet) throws Exception {
710+
SpecialArtifact tree = createTreeArtifact("tree");
711+
TreeFileArtifact child1 = TreeFileArtifact.createTreeOutput(tree, "child1");
712+
TreeFileArtifact child2 = TreeFileArtifact.createTreeOutput(tree, "child2");
713+
// The files won't be read so MISSING_FILE_MARKER will do
714+
TreeArtifactValue treeArtifactValueBefore =
715+
TreeArtifactValue.newBuilder(tree)
716+
.putChild(child1, FileArtifactValue.MISSING_FILE_MARKER)
717+
.putChild(child2, FileArtifactValue.MISSING_FILE_MARKER)
718+
.build();
719+
TreeArtifactValue treeArtifactValueAfter =
720+
TreeArtifactValue.newBuilder(tree)
721+
.putChild(child1, FileArtifactValue.MISSING_FILE_MARKER)
722+
.build();
723+
var vectorArg =
724+
useNestedSet
725+
? new VectorArg.Builder(
726+
NestedSetBuilder.wrap(Order.STABLE_ORDER, ImmutableList.of(tree)), Artifact.class)
727+
: new VectorArg.Builder(Tuple.of(tree));
728+
CommandLine commandLine =
729+
builder
730+
.add(
731+
vectorArg
732+
.setExpandDirectories(false)
733+
.setLocation(Location.BUILTIN)
734+
.setMapEach(
735+
(StarlarkFunction)
736+
execStarlark(
737+
"""
738+
def map_each(x):
739+
return x.path
740+
map_each
741+
""")))
742+
.build(/* flagPerLine= */ false, RepositoryMapping.ALWAYS_FALLBACK);
743+
744+
var expanderBefore =
745+
createArtifactExpander(
746+
ImmutableMap.of(tree, treeArtifactValueBefore.getChildren()), ImmutableMap.of());
747+
var argumentsBefore = commandLine.arguments(expanderBefore, PathMapper.NOOP);
748+
var fingerprintBefore = new Fingerprint();
749+
commandLine.addToFingerprint(
750+
new ActionKeyContext(), expanderBefore, CoreOptions.OutputPathsMode.OFF, fingerprintBefore);
751+
assertThat(argumentsBefore).containsExactly("bin/tree");
752+
753+
var expanderAfter =
754+
createArtifactExpander(
755+
ImmutableMap.of(tree, treeArtifactValueAfter.getChildren()), ImmutableMap.of());
756+
var argumentsAfter = commandLine.arguments(expanderAfter, PathMapper.NOOP);
757+
var fingerprintAfter = new Fingerprint();
758+
commandLine.addToFingerprint(
759+
new ActionKeyContext(), expanderAfter, CoreOptions.OutputPathsMode.OFF, fingerprintAfter);
760+
assertThat(argumentsAfter).containsExactly("bin/tree");
761+
762+
assertThat(fingerprintBefore.hexDigestAndReset())
763+
.isEqualTo(fingerprintAfter.hexDigestAndReset());
764+
}
765+
657766
private static VectorArg.Builder vectorArg(Object... elems) {
658767
return new VectorArg.Builder(Tuple.of(elems)).setLocation(Location.BUILTIN);
659768
}

0 commit comments

Comments
 (0)