diff --git a/src/main/java/com/google/devtools/build/lib/actions/ActionInputHelper.java b/src/main/java/com/google/devtools/build/lib/actions/ActionInputHelper.java index bd3ab97d84eb49..96f9d6ae7b3081 100644 --- a/src/main/java/com/google/devtools/build/lib/actions/ActionInputHelper.java +++ b/src/main/java/com/google/devtools/build/lib/actions/ActionInputHelper.java @@ -139,7 +139,7 @@ public static List expandArtifacts( } } else if (artifact.isTreeArtifact()) { ImmutableSortedSet children = - artifactExpander.expandTreeArtifact(artifact); + artifactExpander.tryExpandTreeArtifact(artifact); if (children.isEmpty()) { emptyTreeArtifacts.add(artifact); } else { diff --git a/src/main/java/com/google/devtools/build/lib/actions/ArtifactExpander.java b/src/main/java/com/google/devtools/build/lib/actions/ArtifactExpander.java index b4d22fa8f83d05..c7dadf232de972 100644 --- a/src/main/java/com/google/devtools/build/lib/actions/ArtifactExpander.java +++ b/src/main/java/com/google/devtools/build/lib/actions/ArtifactExpander.java @@ -24,12 +24,27 @@ /** Expands tree artifacts and filesets. */ public interface ArtifactExpander { + /** + * Returns the expansion of the given {@linkplain SpecialArtifactType#TREE tree artifact}. + * + * @throws MissingExpansionException if this expander does not have data for the given tree + * artifact + */ + ImmutableSortedSet expandTreeArtifact(Artifact treeArtifact) + throws MissingExpansionException; + /** * Returns the expansion of the given {@linkplain SpecialArtifactType#TREE tree artifact}. * *

If this expander does not have data for the given tree artifact, returns an empty set. */ - ImmutableSortedSet expandTreeArtifact(Artifact treeArtifact); + default ImmutableSortedSet tryExpandTreeArtifact(Artifact treeArtifact) { + try { + return expandTreeArtifact(treeArtifact); + } catch (MissingExpansionException e) { + return ImmutableSortedSet.of(); + } + } /** * Returns the expansion of the given {@linkplain SpecialArtifactType#FILESET fileset artifact}. diff --git a/src/main/java/com/google/devtools/build/lib/analysis/actions/CustomCommandLine.java b/src/main/java/com/google/devtools/build/lib/analysis/actions/CustomCommandLine.java index cec01d75d6e8b3..33dcd3ed7c3d5a 100644 --- a/src/main/java/com/google/devtools/build/lib/analysis/actions/CustomCommandLine.java +++ b/src/main/java/com/google/devtools/build/lib/analysis/actions/CustomCommandLine.java @@ -692,7 +692,7 @@ private static final class ExpandedTreeArtifactArg extends TreeArtifactExpansion @Override void eval(ImmutableList.Builder builder, ArtifactExpander artifactExpander) { - for (TreeFileArtifact child : artifactExpander.expandTreeArtifact(treeArtifact)) { + for (TreeFileArtifact child : artifactExpander.tryExpandTreeArtifact(treeArtifact)) { builder.add(child.getExecPathString()); } } diff --git a/src/main/java/com/google/devtools/build/lib/analysis/starlark/StarlarkCustomCommandLine.java b/src/main/java/com/google/devtools/build/lib/analysis/starlark/StarlarkCustomCommandLine.java index 0b83542089c251..d36daad0865b49 100644 --- a/src/main/java/com/google/devtools/build/lib/analysis/starlark/StarlarkCustomCommandLine.java +++ b/src/main/java/com/google/devtools/build/lib/analysis/starlark/StarlarkCustomCommandLine.java @@ -406,7 +406,7 @@ private static boolean hasDirectory(List originalValues) { } private static boolean isDirectory(Object object) { - return object instanceof Artifact && ((Artifact) object).isDirectory(); + return object instanceof Artifact artifact && artifact.isDirectory(); } private static List expandDirectories( @@ -417,7 +417,18 @@ private static List expandDirectories( if (isDirectory(object)) { Artifact artifact = (Artifact) object; if (artifact.isTreeArtifact()) { - expandedValues.addAll(artifactExpander.expandTreeArtifact(artifact)); + try { + expandedValues.addAll(artifactExpander.expandTreeArtifact(artifact)); + } catch (MissingExpansionException e) { + throw new CommandLineExpansionException( + String.format( + "Failed to expand directory %s. Either add the directory as an input of the" + + " action or set 'expand_directories = False' in the 'add_all' or" + + " 'add_joined' call to have the path of the directory added to the" + + " command line instead of its contents.", + Starlark.repr(artifact)), + e); + } } else if (artifact.isFileset()) { expandFileset(artifactExpander, artifact, expandedValues, pathMapper); } else { @@ -1024,10 +1035,17 @@ private static final class FullExpander implements DirectoryExpander { } @Override - public ImmutableList list(FileApi file) { + public ImmutableList list(FileApi file) throws EvalException { Artifact artifact = (Artifact) file; if (artifact.isTreeArtifact()) { - return ImmutableList.copyOf(expander.expandTreeArtifact(artifact)); + try { + return ImmutableList.copyOf(expander.expandTreeArtifact(artifact)); + } catch (MissingExpansionException unused) { + throw Starlark.errorf( + "Failed to expand directory %s. Only directories that are action inputs can be" + + " expanded.", + Starlark.repr(artifact)); + } } else { return ImmutableList.of(file); } @@ -1069,7 +1087,7 @@ private static void applyMapEach( } for (int i = 0; i < count; ++i) { args.set(0, originalValues.get(i)); - Object ret = Starlark.call(thread, mapFn, args, /*kwargs=*/ ImmutableMap.of()); + Object ret = Starlark.call(thread, mapFn, args, /* kwargs= */ ImmutableMap.of()); if (ret instanceof String string) { consumer.accept(string); } else if (ret instanceof Sequence sequence) { @@ -1102,6 +1120,7 @@ private static class CommandLineItemMapEachAdaptor private final StarlarkCallable mapFn; private final Location location; private final StarlarkSemantics starlarkSemantics; + /** * Indicates whether artifactExpander was provided on construction. This is used to distinguish * the case where it's not provided from the case where it was provided but subsequently diff --git a/src/main/java/com/google/devtools/build/lib/rules/cpp/CcToolchainVariables.java b/src/main/java/com/google/devtools/build/lib/rules/cpp/CcToolchainVariables.java index 3eb08a3f0cbda4..7150b1d792460b 100644 --- a/src/main/java/com/google/devtools/build/lib/rules/cpp/CcToolchainVariables.java +++ b/src/main/java/com/google/devtools/build/lib/rules/cpp/CcToolchainVariables.java @@ -926,7 +926,7 @@ public VariableValue getFieldValue( if (objectFile.isTreeArtifact() && expander != null) { expandedObjectFiles.addAll( Collections2.transform( - expander.expandTreeArtifact(objectFile), Artifact::getExecPathString)); + expander.tryExpandTreeArtifact(objectFile), Artifact::getExecPathString)); } else { expandedObjectFiles.add(objectFile.getExecPathString()); } diff --git a/src/main/java/com/google/devtools/build/lib/rules/cpp/CppCompileAction.java b/src/main/java/com/google/devtools/build/lib/rules/cpp/CppCompileAction.java index 1a18a979960955..859daa1f8f2f45 100644 --- a/src/main/java/com/google/devtools/build/lib/rules/cpp/CppCompileAction.java +++ b/src/main/java/com/google/devtools/build/lib/rules/cpp/CppCompileAction.java @@ -1026,7 +1026,7 @@ public void validateInclusions( for (Artifact input : set.toList()) { if (input.isTreeArtifact()) { allowedIncludes.addAll( - actionExecutionContext.getArtifactExpander().expandTreeArtifact(input)); + actionExecutionContext.getArtifactExpander().tryExpandTreeArtifact(input)); } allowedIncludes.add(input); } diff --git a/src/main/java/com/google/devtools/build/lib/rules/cpp/CppModuleMapAction.java b/src/main/java/com/google/devtools/build/lib/rules/cpp/CppModuleMapAction.java index c571215fd6d8a1..bc370934c2186c 100644 --- a/src/main/java/com/google/devtools/build/lib/rules/cpp/CppModuleMapAction.java +++ b/src/main/java/com/google/devtools/build/lib/rules/cpp/CppModuleMapAction.java @@ -224,7 +224,7 @@ private static Iterable expandedHeaders(ArtifactExpander artifactExpan List expandedHeaders = new ArrayList<>(); for (Artifact unexpandedHeader : unexpandedHeaders) { if (unexpandedHeader.isTreeArtifact()) { - expandedHeaders.addAll(artifactExpander.expandTreeArtifact(unexpandedHeader)); + expandedHeaders.addAll(artifactExpander.tryExpandTreeArtifact(unexpandedHeader)); } else { expandedHeaders.add(unexpandedHeader); } diff --git a/src/main/java/com/google/devtools/build/lib/rules/cpp/UmbrellaHeaderAction.java b/src/main/java/com/google/devtools/build/lib/rules/cpp/UmbrellaHeaderAction.java index c13306eb28ec1a..5e507c3d5b3174 100644 --- a/src/main/java/com/google/devtools/build/lib/rules/cpp/UmbrellaHeaderAction.java +++ b/src/main/java/com/google/devtools/build/lib/rules/cpp/UmbrellaHeaderAction.java @@ -83,7 +83,7 @@ private static Iterable expandedHeaders(ArtifactExpander artifactExpan List expandedHeaders = new ArrayList<>(); for (Artifact unexpandedHeader : unexpandedHeaders) { if (unexpandedHeader.isTreeArtifact()) { - expandedHeaders.addAll(artifactExpander.expandTreeArtifact(unexpandedHeader)); + expandedHeaders.addAll(artifactExpander.tryExpandTreeArtifact(unexpandedHeader)); } else { expandedHeaders.add(unexpandedHeader); } diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/ActionExecutionFunction.java b/src/main/java/com/google/devtools/build/lib/skyframe/ActionExecutionFunction.java index 4d3e6b2e1b8194..48d9022211f882 100644 --- a/src/main/java/com/google/devtools/build/lib/skyframe/ActionExecutionFunction.java +++ b/src/main/java/com/google/devtools/build/lib/skyframe/ActionExecutionFunction.java @@ -970,10 +970,14 @@ private static final class ActionInputArtifactExpander implements ArtifactExpand } @Override - public ImmutableSortedSet expandTreeArtifact(Artifact treeArtifact) { + public ImmutableSortedSet expandTreeArtifact(Artifact treeArtifact) + throws MissingExpansionException { checkArgument(treeArtifact.isTreeArtifact(), treeArtifact); TreeArtifactValue tree = inputArtifactData.getTreeMetadata(treeArtifact.getExecPath()); - return tree == null ? ImmutableSortedSet.of() : tree.getChildren(); + if (tree == null) { + throw new MissingExpansionException("Missing expansion for tree artifact: " + treeArtifact); + } + return tree.getChildren(); } @Override diff --git a/src/main/java/com/google/devtools/build/lib/starlarkbuildapi/DirectoryExpander.java b/src/main/java/com/google/devtools/build/lib/starlarkbuildapi/DirectoryExpander.java index 63f42ab710f6d3..8cc87317a8bed8 100644 --- a/src/main/java/com/google/devtools/build/lib/starlarkbuildapi/DirectoryExpander.java +++ b/src/main/java/com/google/devtools/build/lib/starlarkbuildapi/DirectoryExpander.java @@ -19,6 +19,7 @@ import net.starlark.java.annot.Param; import net.starlark.java.annot.StarlarkBuiltin; import net.starlark.java.annot.StarlarkMethod; +import net.starlark.java.eval.EvalException; import net.starlark.java.eval.StarlarkValue; /** A class that can be used to expand directories at execution time. */ @@ -44,5 +45,5 @@ public interface DirectoryExpander extends StarlarkValue { named = false, doc = "The directory or file to expand."), }) - ImmutableList list(FileApi artifact); + ImmutableList list(FileApi artifact) throws EvalException; } diff --git a/src/test/java/com/google/devtools/build/lib/analysis/starlark/StarlarkCustomCommandLineTest.java b/src/test/java/com/google/devtools/build/lib/analysis/starlark/StarlarkCustomCommandLineTest.java index 987db7c126234e..15a339f92f987a 100644 --- a/src/test/java/com/google/devtools/build/lib/analysis/starlark/StarlarkCustomCommandLineTest.java +++ b/src/test/java/com/google/devtools/build/lib/analysis/starlark/StarlarkCustomCommandLineTest.java @@ -37,15 +37,24 @@ import com.google.devtools.build.lib.actions.util.ActionsTestUtil; import com.google.devtools.build.lib.analysis.config.CoreOptions; import com.google.devtools.build.lib.analysis.starlark.StarlarkCustomCommandLine.VectorArg; +import com.google.devtools.build.lib.cmdline.BazelModuleContext; +import com.google.devtools.build.lib.cmdline.Label; import com.google.devtools.build.lib.cmdline.RepositoryMapping; import com.google.devtools.build.lib.testutil.Scratch; import com.google.devtools.build.lib.util.Fingerprint; import com.google.devtools.build.lib.vfs.Path; import com.google.devtools.build.lib.vfs.PathFragment; import java.io.IOException; +import net.starlark.java.eval.Module; +import net.starlark.java.eval.Mutability; +import net.starlark.java.eval.Starlark; +import net.starlark.java.eval.StarlarkFunction; import net.starlark.java.eval.StarlarkSemantics; +import net.starlark.java.eval.StarlarkThread; import net.starlark.java.eval.Tuple; +import net.starlark.java.syntax.FileOptions; import net.starlark.java.syntax.Location; +import net.starlark.java.syntax.ParserInput; import org.junit.Before; import org.junit.Test; import org.junit.runner.RunWith; @@ -54,8 +63,10 @@ /** Tests for {@link StarlarkCustomCommandLine}. */ @RunWith(JUnit4.class) public final class StarlarkCustomCommandLineTest { - - private static final ArtifactExpander EMPTY_EXPANDER = artifact -> ImmutableSortedSet.of(); + private static final ArtifactExpander EMPTY_EXPANDER = + artifact -> { + throw new ArtifactExpander.MissingExpansionException("Missing expansion for " + artifact); + }; private final Scratch scratch = new Scratch(); private Path execRoot; @@ -197,21 +208,18 @@ public void flagPerLine() throws Exception { } @Test - public void vectorArgAddToFingerprint_treeArtifactMissingExpansion_returnsDigest() - throws Exception { + public void vectorArgAddToFingerprint_treeArtifactMissingExpansion_fails() { SpecialArtifact tree = createTreeArtifact("tree"); CommandLine commandLine = builder .add(vectorArg(tree).setExpandDirectories(true)) .build(/* flagPerLine= */ false, RepositoryMapping.ALWAYS_FALLBACK); - ActionKeyContext actionKeyContext = new ActionKeyContext(); - Fingerprint fingerprint = new Fingerprint(); - - // TODO(b/167696101): Fail arguments computation when we are missing the directory from inputs. - commandLine.addToFingerprint( - actionKeyContext, EMPTY_EXPANDER, CoreOptions.OutputPathsMode.OFF, fingerprint); - assertThat(fingerprint.digestAndReset()).isNotEmpty(); + CommandLineExpansionException e = + assertThrows( + CommandLineExpansionException.class, + () -> commandLine.arguments(EMPTY_EXPANDER, PathMapper.NOOP)); + assertThat(e).hasMessageThat().contains("Failed to expand directory "); } @Test @@ -227,7 +235,7 @@ public void vectorArgAddToFingerprint_expandFileset_includesInDigest() throws Ex Fingerprint fingerprint = new Fingerprint(); ArtifactExpander artifactExpander = createArtifactExpander( - /*treeExpansions=*/ ImmutableMap.of(), + /* treeExpansions= */ ImmutableMap.of(), ImmutableMap.of(fileset, ImmutableList.of(symlink1, symlink2))); commandLine.addToFingerprint( @@ -304,7 +312,7 @@ public void vectorArgArguments_expandsFileset() throws Exception { FilesetOutputSymlink symlink2 = createFilesetSymlink("file2"); ArtifactExpander artifactExpander = createArtifactExpander( - /*treeExpansions=*/ ImmutableMap.of(), + /* treeExpansions= */ ImmutableMap.of(), ImmutableMap.of(fileset, ImmutableList.of(symlink1, symlink2))); Iterable arguments = commandLine.arguments(artifactExpander, PathMapper.NOOP); @@ -313,15 +321,42 @@ public void vectorArgArguments_expandsFileset() throws Exception { } @Test - public void vectorArgArguments_treeArtifactMissingExpansion_returnsEmptyList() throws Exception { + public void vectorArgArguments_treeArtifactMissingExpansion_fails() { SpecialArtifact tree = createTreeArtifact("tree"); CommandLine commandLine = builder .add(vectorArg(tree).setExpandDirectories(true)) .build(/* flagPerLine= */ false, RepositoryMapping.ALWAYS_FALLBACK); - // TODO(b/167696101): Fail arguments computation when we are missing the directory from inputs. - assertThat(commandLine.arguments(EMPTY_EXPANDER, PathMapper.NOOP)).isEmpty(); + assertThrows( + CommandLineExpansionException.class, + () -> commandLine.arguments(EMPTY_EXPANDER, PathMapper.NOOP)); + } + + @Test + public void vectorArgArguments_manuallyExpandedTreeArtifactMissingExpansion_fails() + throws Exception { + SpecialArtifact tree = createTreeArtifact("tree"); + CommandLine commandLine = + builder + .add( + vectorArg(tree) + .setExpandDirectories(false) + .setMapEach( + (StarlarkFunction) + execStarlark( + """ + def map_each(x, expander): + expander.expand(x) + map_each + """))) + .build(/* flagPerLine= */ false, RepositoryMapping.ALWAYS_FALLBACK); + + CommandLineExpansionException e = + assertThrows( + CommandLineExpansionException.class, + () -> commandLine.arguments(EMPTY_EXPANDER, PathMapper.NOOP)); + assertThat(e).hasMessageThat().contains("Failed to expand directory "); } @Test @@ -379,9 +414,14 @@ private static ArtifactExpander createArtifactExpander( ImmutableMap> filesetExpansions) { return new ArtifactExpander() { @Override - public ImmutableSortedSet expandTreeArtifact(Artifact treeArtifact) { + public ImmutableSortedSet expandTreeArtifact(Artifact treeArtifact) + throws MissingExpansionException { //noinspection SuspiciousMethodCalls - return treeExpansions.getOrDefault(treeArtifact, ImmutableSortedSet.of()); + ImmutableSortedSet expansion = treeExpansions.get(treeArtifact); + if (expansion == null) { + throw new MissingExpansionException("Cannot expand " + treeArtifact); + } + return expansion; } @Override @@ -396,4 +436,23 @@ public ImmutableList expandFileset(Artifact artifact) } }; } + + private static Object execStarlark(String code) throws Exception { + try (Mutability mutability = Mutability.create("test")) { + StarlarkThread thread = StarlarkThread.createTransient(mutability, StarlarkSemantics.DEFAULT); + return Starlark.execFile( + ParserInput.fromString(code, "test/label.bzl"), + FileOptions.DEFAULT, + Module.withPredeclaredAndData( + StarlarkSemantics.DEFAULT, + ImmutableMap.of(), + BazelModuleContext.create( + Label.parseCanonicalUnchecked("//test:label"), + RepositoryMapping.ALWAYS_FALLBACK, + "test/label.bzl", + /* loads= */ ImmutableList.of(), + /* bzlTransitiveDigest= */ new byte[0])), + thread); + } + } } diff --git a/src/test/java/com/google/devtools/build/lib/skyframe/TreeArtifactBuildTest.java b/src/test/java/com/google/devtools/build/lib/skyframe/TreeArtifactBuildTest.java index e1def1226e43dc..ca59878caedbfe 100644 --- a/src/test/java/com/google/devtools/build/lib/skyframe/TreeArtifactBuildTest.java +++ b/src/test/java/com/google/devtools/build/lib/skyframe/TreeArtifactBuildTest.java @@ -1020,7 +1020,8 @@ private static final class CopyTreeAction extends SimpleTestAction { @Override void run(ActionExecutionContext context) throws IOException { - for (Artifact child : context.getArtifactExpander().expandTreeArtifact(getPrimaryInput())) { + for (Artifact child : + context.getArtifactExpander().tryExpandTreeArtifact(getPrimaryInput())) { Path newOutput = getPrimaryOutput().getPath().getRelative(child.getParentRelativePath()); newOutput.createDirectoryAndParents(); FileSystemUtils.copyFile(child.getPath(), newOutput); diff --git a/src/test/java/com/google/devtools/build/lib/skyframe/rewinding/LostImportantOutputHandlerModule.java b/src/test/java/com/google/devtools/build/lib/skyframe/rewinding/LostImportantOutputHandlerModule.java index dd855571c35fe0..ff374ad21133a1 100644 --- a/src/test/java/com/google/devtools/build/lib/skyframe/rewinding/LostImportantOutputHandlerModule.java +++ b/src/test/java/com/google/devtools/build/lib/skyframe/rewinding/LostImportantOutputHandlerModule.java @@ -163,7 +163,7 @@ private ImmutableList expand( private Stream expand(Artifact output, ArtifactExpander expander) { if (output.isTreeArtifact()) { - var children = expander.expandTreeArtifact(output).stream(); + var children = expander.tryExpandTreeArtifact(output).stream(); var archivedTreeArtifact = expander.getArchivedTreeArtifact(output); var expansion = archivedTreeArtifact == null diff --git a/src/test/java/com/google/devtools/build/lib/testutil/SpawnInputUtils.java b/src/test/java/com/google/devtools/build/lib/testutil/SpawnInputUtils.java index 49bd5a86dcca7b..ca82f1f86f3626 100644 --- a/src/test/java/com/google/devtools/build/lib/testutil/SpawnInputUtils.java +++ b/src/test/java/com/google/devtools/build/lib/testutil/SpawnInputUtils.java @@ -99,7 +99,7 @@ public static SpecialArtifact getTreeArtifactWithName(Spawn spawn, String name) public static Artifact getExpandedToArtifact( String name, Artifact expandableArtifact, Spawn spawn, ActionExecutionContext context) { - return context.getArtifactExpander().expandTreeArtifact(expandableArtifact).stream() + return context.getArtifactExpander().tryExpandTreeArtifact(expandableArtifact).stream() .filter(artifact -> artifact.getExecPathString().contains(name)) .findFirst() .orElseThrow(