CommandLine: Add support for tree artifact expansions. ParameterFileWriteAction: Add support to write out CommandLine with tree artifact expansions. -- MOS_MIGRATED_REVID=122734422
diff --git a/src/main/java/com/google/devtools/build/lib/analysis/actions/CommandLine.java b/src/main/java/com/google/devtools/build/lib/analysis/actions/CommandLine.java index 0eb8c4b..53adca3 100644 --- a/src/main/java/com/google/devtools/build/lib/analysis/actions/CommandLine.java +++ b/src/main/java/com/google/devtools/build/lib/analysis/actions/CommandLine.java
@@ -17,6 +17,7 @@ import com.google.common.base.Joiner; import com.google.common.collect.ImmutableList; import com.google.common.collect.Iterables; +import com.google.devtools.build.lib.actions.Artifact.ArtifactExpander; import com.google.devtools.build.lib.collect.CollectionUtils; import com.google.devtools.build.lib.util.Preconditions; @@ -30,6 +31,18 @@ public abstract Iterable<String> arguments(); /** + * Returns the evaluated command line with enclosed artifacts expanded by {@code artifactExpander} + * at execution time. + * + * <p>By default, this method just delegates to {@link #arguments()}, without performing any + * artifact expansion. Subclasses should override this method if they contain TreeArtifacts and + * need to expand them for proper argument evaluation. + */ + public Iterable<String> arguments(ArtifactExpander artifactExpander) { + return arguments(); + } + + /** * Returns whether the command line represents a shell command with the given shell executable. * This is used to give better error messages. * @@ -89,6 +102,11 @@ } @Override + public Iterable<String> arguments(ArtifactExpander artifactExpander) { + return Iterables.concat(executableArgs, commandLine.arguments(artifactExpander)); + } + + @Override public boolean isShellCommand() { return isShellCommand; }
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 3ee8946..2af93fe 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
@@ -19,6 +19,7 @@ import com.google.common.collect.ImmutableList; import com.google.common.collect.ImmutableMap; import com.google.devtools.build.lib.actions.Artifact; +import com.google.devtools.build.lib.actions.Artifact.ArtifactExpander; import com.google.devtools.build.lib.actions.Artifact.TreeFileArtifact; import com.google.devtools.build.lib.cmdline.Label; import com.google.devtools.build.lib.collect.CollectionUtils; @@ -30,6 +31,10 @@ import java.util.ArrayList; import java.util.List; import java.util.Map; +import java.util.Set; +import java.util.TreeSet; + +import javax.annotation.Nullable; /** * A customizable, serializable class for building memory efficient command lines. @@ -41,7 +46,6 @@ abstract void eval(ImmutableList.Builder<String> builder); } - /** * A command line argument for {@link TreeFileArtifact}. * @@ -61,6 +65,43 @@ abstract Object substituteTreeArtifact(Map<Artifact, TreeFileArtifact> substitutionMap); } + /** + * A command line argument that can expand enclosed TreeArtifacts into a list of child + * {@link TreeFileArtifact}s at execution time before argument evaluation. + * + * <p>The main difference between this class and {@link TreeFileArtifactArgvFragment} is that + * {@link TreeFileArtifactArgvFragment} is used in {@link SpawnActionTemplate} to substitutes a + * TreeArtifact with *one* of its child TreeFileArtifacts, while this class expands a TreeArtifact + * into *all* of its child TreeFileArtifacts. + * + */ + private abstract static class TreeArtifactExpansionArgvFragment extends ArgvFragment { + /** + * Evaluates this argument fragment into an argument string and adds it into {@code builder}. + * The enclosed TreeArtifact will be expanded using {@code artifactExpander}. + */ + abstract void eval(ImmutableList.Builder<String> builder, ArtifactExpander artifactExpander); + + /** + * Returns a string that describes this argument fragment. The string can be used as part of + * an action key for the command line at analysis time. + */ + abstract String describe(); + + /** + * Evaluates this argument fragment by serializing it into a string. Note that the returned + * argument is not suitable to be used as part of an actual command line. The purpose of this + * method is to provide a unique command line argument string to be used as part of an action + * key at analysis time. + * + * <p>Internally this method just calls {@link #describe}. + */ + @Override + void eval(ImmutableList.Builder<String> builder) { + builder.add(describe()); + } + } + // It's better to avoid anonymous classes if we want to serialize command lines private static final class JoinExecPathsArg extends ArgvFragment { @@ -78,6 +119,40 @@ } } + private static final class JoinExpandedTreeArtifactExecPathsArg + extends TreeArtifactExpansionArgvFragment { + + private final String delimiter; + private final Artifact treeArtifact; + + private JoinExpandedTreeArtifactExecPathsArg(String delimiter, Artifact treeArtifact) { + Preconditions.checkArgument( + treeArtifact.isTreeArtifact(), "%s is not a TreeArtifact", treeArtifact); + this.delimiter = delimiter; + this.treeArtifact = treeArtifact; + } + + @Override + void eval(ImmutableList.Builder<String> builder, ArtifactExpander artifactExpander) { + Set<Artifact> expandedArtifacts = new TreeSet<>(); + artifactExpander.expand(treeArtifact, expandedArtifacts); + Preconditions.checkState( + !expandedArtifacts.isEmpty(), + "%s expanded into nothing, maybe it's not added as a input for the associated action?", + treeArtifact); + + builder.add(Artifact.joinExecPaths(delimiter, expandedArtifacts)); + } + + @Override + public String describe() { + return String.format( + "JoinExpandedTreeArtifactExecPathsArg{ delimiter: %s, treeArtifact: %s}", + delimiter, + treeArtifact.getExecPathString()); + } + } + private static final class PathWithTemplateArg extends ArgvFragment { private final String template; @@ -424,6 +499,18 @@ return this; } + /** + * Adds a string joined together by the exec paths of all {@link TreeFileArtifact}s under + * {@code treeArtifact}. + * + * @param delimiter the delimiter used to join the artifact exec paths. + * @param treeArtifact the TreeArtifact containing the {@link TreeFileArtifact}s to join. + */ + public Builder addJoinExpandedTreeArtifactExecPath(String delimiter, Artifact treeArtifact) { + arguments.add(new JoinExpandedTreeArtifactExecPathsArg(delimiter, treeArtifact)); + return this; + } + public Builder addBeforeEachPath(String repeated, Iterable<PathFragment> paths) { if (repeated != null && paths != null) { arguments.add(InterspersingArgs.fromStrings(paths, repeated, "%s")); @@ -515,11 +602,27 @@ @Override public Iterable<String> arguments() { + return argumentsInternal(null); + } + + @Override + public Iterable<String> arguments(ArtifactExpander artifactExpander) { + return argumentsInternal(Preconditions.checkNotNull(artifactExpander)); + } + + private Iterable<String> argumentsInternal(@Nullable ArtifactExpander artifactExpander) { ImmutableList.Builder<String> builder = ImmutableList.builder(); for (Object arg : arguments) { Object substitutedArg = substituteTreeFileArtifactArgvFragment(arg); if (substitutedArg instanceof ArgvFragment) { - ((ArgvFragment) substitutedArg).eval(builder); + if (artifactExpander != null + && substitutedArg instanceof TreeArtifactExpansionArgvFragment) { + TreeArtifactExpansionArgvFragment expansionArg = + (TreeArtifactExpansionArgvFragment) substitutedArg; + expansionArg.eval(builder, artifactExpander); + } else { + ((ArgvFragment) substitutedArg).eval(builder); + } } else { builder.add(substitutedArg.toString()); }
diff --git a/src/main/java/com/google/devtools/build/lib/analysis/actions/ParameterFileWriteAction.java b/src/main/java/com/google/devtools/build/lib/analysis/actions/ParameterFileWriteAction.java index 2a1c9b8..659dae6 100644 --- a/src/main/java/com/google/devtools/build/lib/analysis/actions/ParameterFileWriteAction.java +++ b/src/main/java/com/google/devtools/build/lib/analysis/actions/ParameterFileWriteAction.java
@@ -16,12 +16,16 @@ import com.google.common.annotations.VisibleForTesting; import com.google.common.collect.ImmutableList; +import com.google.common.collect.Iterables; import com.google.devtools.build.lib.actions.ActionExecutionContext; import com.google.devtools.build.lib.actions.ActionOwner; import com.google.devtools.build.lib.actions.Artifact; +import com.google.devtools.build.lib.actions.Artifact.ArtifactExpander; import com.google.devtools.build.lib.actions.ParameterFile.ParameterFileType; +import com.google.devtools.build.lib.analysis.actions.AbstractFileWriteAction.DeterministicWriter; import com.google.devtools.build.lib.concurrent.ThreadSafety.Immutable; import com.google.devtools.build.lib.util.Fingerprint; +import com.google.devtools.build.lib.util.Preconditions; import com.google.devtools.build.lib.util.ShellEscaper; import java.io.IOException; @@ -40,6 +44,7 @@ private final CommandLine commandLine; private final ParameterFileType type; private final Charset charset; + private final boolean hasInputArtifactToExpand; /** * Creates a new instance. @@ -52,10 +57,27 @@ */ public ParameterFileWriteAction(ActionOwner owner, Artifact output, CommandLine commandLine, ParameterFileType type, Charset charset) { - super(owner, ImmutableList.<Artifact>of(), output, false); + this(owner, ImmutableList.<Artifact>of(), output, commandLine, type, charset); + } + + /** + * Creates a new instance. + * + * @param owner the action owner + * @param inputs the list of TreeArtifacts that must be resolved and expanded before evaluating + * the contents of {@link commandLine}. + * @param output the Artifact that will be created by executing this Action + * @param commandLine the contents to be written to the file + * @param type the type of the file + * @param charset the charset of the file + */ + public ParameterFileWriteAction(ActionOwner owner, Iterable<Artifact> inputs, Artifact output, + CommandLine commandLine, ParameterFileType type, Charset charset) { + super(owner, ImmutableList.copyOf(inputs), output, false); this.commandLine = commandLine; this.type = type; this.charset = charset; + this.hasInputArtifactToExpand = !Iterables.isEmpty(inputs); } /** @@ -65,51 +87,73 @@ */ @VisibleForTesting public Iterable<String> getContents() { + Preconditions.checkState( + !hasInputArtifactToExpand, + "This action contains a CommandLine with TreeArtifacts: %s, which must be expanded using " + + "ArtifactExpander first before we can evaluate the CommandLine.", + getInputs()); return commandLine.arguments(); } + @VisibleForTesting + public Iterable<String> getContents(ArtifactExpander artifactExpander) { + return commandLine.arguments(artifactExpander); + } + @Override public DeterministicWriter newDeterministicWriter(ActionExecutionContext ctx) { - return new DeterministicWriter() { - @Override - public void writeOutputFile(OutputStream out) throws IOException { - switch (type) { - case SHELL_QUOTED : - writeContentQuoted(out); - break; - case UNQUOTED : - writeContentUnquoted(out); - break; - default : - throw new AssertionError(); - } + return new ParamFileWriter(Preconditions.checkNotNull(ctx.getArtifactExpander())); + } + + private class ParamFileWriter implements DeterministicWriter { + private final ArtifactExpander artifactExpander; + + ParamFileWriter(ArtifactExpander artifactExpander) { + this.artifactExpander = artifactExpander; + } + + @Override + public void writeOutputFile(OutputStream out) throws IOException { + Iterable<String> arguments = commandLine.arguments(artifactExpander); + + switch (type) { + case SHELL_QUOTED : + writeContentQuoted(out, arguments); + break; + case UNQUOTED : + writeContentUnquoted(out, arguments); + break; + default : + throw new AssertionError(); } - }; - } - - /** - * Writes the arguments from the list into the parameter file. - */ - private void writeContentUnquoted(OutputStream outputStream) throws IOException { - OutputStreamWriter out = new OutputStreamWriter(outputStream, charset); - for (String line : commandLine.arguments()) { - out.write(line); - out.write('\n'); } - out.flush(); - } - /** - * Writes the arguments from the list into the parameter file with shell - * quoting (if required). - */ - private void writeContentQuoted(OutputStream outputStream) throws IOException { - OutputStreamWriter out = new OutputStreamWriter(outputStream, charset); - for (String line : ShellEscaper.escapeAll(commandLine.arguments())) { - out.write(line); - out.write('\n'); + /** + * Writes the arguments from the list into the parameter file. + */ + private void writeContentUnquoted(OutputStream outputStream, Iterable<String> arguments) + throws IOException { + OutputStreamWriter out = new OutputStreamWriter(outputStream, charset); + for (String line : arguments) { + out.write(line); + out.write('\n'); + } + out.flush(); } - out.flush(); + + /** + * Writes the arguments from the list into the parameter file with shell + * quoting (if required). + */ + private void writeContentQuoted(OutputStream outputStream, Iterable<String> arguments) + throws IOException { + OutputStreamWriter out = new OutputStreamWriter(outputStream, charset); + for (String line : ShellEscaper.escapeAll(arguments)) { + out.write(line); + out.write('\n'); + } + out.flush(); + } } @Override
diff --git a/src/test/java/com/google/devtools/build/lib/actions/CustomCommandLineTest.java b/src/test/java/com/google/devtools/build/lib/actions/CustomCommandLineTest.java index 2af98f4b..531baef 100644 --- a/src/test/java/com/google/devtools/build/lib/actions/CustomCommandLineTest.java +++ b/src/test/java/com/google/devtools/build/lib/actions/CustomCommandLineTest.java
@@ -18,9 +18,11 @@ import static org.junit.Assert.fail; import com.google.common.collect.ImmutableList; +import com.google.devtools.build.lib.actions.Artifact.ArtifactExpander; import com.google.devtools.build.lib.actions.Artifact.SpecialArtifact; import com.google.devtools.build.lib.actions.Artifact.SpecialArtifactType; import com.google.devtools.build.lib.actions.Artifact.TreeFileArtifact; +import com.google.devtools.build.lib.analysis.actions.CommandLine; import com.google.devtools.build.lib.analysis.actions.CustomCommandLine; import com.google.devtools.build.lib.analysis.actions.CustomCommandLine.CustomArgv; import com.google.devtools.build.lib.analysis.actions.CustomCommandLine.CustomMultiArgv; @@ -35,6 +37,8 @@ import org.junit.runner.RunWith; import org.junit.runners.JUnit4; +import java.util.Collection; + /** * Tests for CustomCommandLine. */ @@ -238,6 +242,39 @@ } + @Test + public void testJoinExpandedTreeArtifactExecPath() { + Artifact treeArtifact = createTreeArtifact("myTreeArtifact"); + + CommandLine commandLine = CustomCommandLine.builder() + .add("hello") + .addJoinExpandedTreeArtifactExecPath(":", treeArtifact) + .build(); + + assertThat(commandLine.arguments()).containsExactly( + "hello", + "JoinExpandedTreeArtifactExecPathsArg{ delimiter: :, treeArtifact: myTreeArtifact}"); + + final Iterable<TreeFileArtifact> treeFileArtifacts = ImmutableList.of( + createTreeFileArtifact(treeArtifact, "children/child1"), + createTreeFileArtifact(treeArtifact, "children/child2")); + + ArtifactExpander artifactExpander = new ArtifactExpander() { + @Override + public void expand(Artifact artifact, Collection<? super Artifact> output) { + for (TreeFileArtifact treeFileArtifact : treeFileArtifacts) { + if (treeFileArtifact.getParent().equals(artifact)) { + output.add(treeFileArtifact); + } + } + } + }; + + assertThat(commandLine.arguments(artifactExpander)).containsExactly( + "hello", + "myTreeArtifact/children/child1:myTreeArtifact/children/child2"); + } + private Artifact createTreeArtifact(String rootRelativePath) { PathFragment relpath = new PathFragment(rootRelativePath); return new SpecialArtifact(
diff --git a/src/test/java/com/google/devtools/build/lib/actions/util/ActionsTestUtil.java b/src/test/java/com/google/devtools/build/lib/actions/util/ActionsTestUtil.java index aae837f..0ba8f7a 100644 --- a/src/test/java/com/google/devtools/build/lib/actions/util/ActionsTestUtil.java +++ b/src/test/java/com/google/devtools/build/lib/actions/util/ActionsTestUtil.java
@@ -31,6 +31,7 @@ import com.google.devtools.build.lib.actions.ActionInputHelper; import com.google.devtools.build.lib.actions.ActionOwner; import com.google.devtools.build.lib.actions.Artifact; +import com.google.devtools.build.lib.actions.Artifact.ArtifactExpander; import com.google.devtools.build.lib.actions.ArtifactOwner; import com.google.devtools.build.lib.actions.Executor; import com.google.devtools.build.lib.actions.MutableActionGraph; @@ -91,7 +92,7 @@ metadataHandler, fileOutErr, actionGraph == null - ? null + ? createDummyArtifactExpander() : ActionInputHelper.actionGraphArtifactExpander(actionGraph)); } @@ -108,7 +109,17 @@ public static ActionExecutionContext createContext(EventHandler eventHandler) { DummyExecutor dummyExecutor = new DummyExecutor(eventHandler); - return new ActionExecutionContext(dummyExecutor, null, null, null, null); + return new ActionExecutionContext( + dummyExecutor, null, null, null, createDummyArtifactExpander()); + } + + private static ArtifactExpander createDummyArtifactExpander() { + return new ArtifactExpander() { + @Override + public void expand(Artifact artifact, Collection<? super Artifact> output) { + return; + } + }; }
diff --git a/src/test/java/com/google/devtools/build/lib/analysis/actions/ParamFileWriteActionTest.java b/src/test/java/com/google/devtools/build/lib/analysis/actions/ParamFileWriteActionTest.java new file mode 100644 index 0000000..21b7975 --- /dev/null +++ b/src/test/java/com/google/devtools/build/lib/analysis/actions/ParamFileWriteActionTest.java
@@ -0,0 +1,168 @@ +// Copyright 2015 The Bazel Authors. All rights reserved. +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. +package com.google.devtools.build.lib.analysis.actions; + +import static com.google.common.truth.Truth.assertThat; +import static org.junit.Assert.assertEquals; + +import com.google.common.collect.ImmutableList; +import com.google.devtools.build.lib.actions.Action; +import com.google.devtools.build.lib.actions.ActionExecutionContext; +import com.google.devtools.build.lib.actions.ActionInputHelper; +import com.google.devtools.build.lib.actions.Artifact; +import com.google.devtools.build.lib.actions.Artifact.ArtifactExpander; +import com.google.devtools.build.lib.actions.Artifact.SpecialArtifact; +import com.google.devtools.build.lib.actions.Artifact.SpecialArtifactType; +import com.google.devtools.build.lib.actions.Artifact.TreeFileArtifact; +import com.google.devtools.build.lib.actions.ArtifactOwner; +import com.google.devtools.build.lib.actions.Executor; +import com.google.devtools.build.lib.actions.ParameterFile.ParameterFileType; +import com.google.devtools.build.lib.actions.Root; +import com.google.devtools.build.lib.actions.util.ActionsTestUtil; +import com.google.devtools.build.lib.analysis.util.BuildViewTestCase; +import com.google.devtools.build.lib.exec.util.TestExecutorBuilder; +import com.google.devtools.build.lib.util.io.FileOutErr; +import com.google.devtools.build.lib.vfs.FileSystemUtils; +import com.google.devtools.build.lib.vfs.PathFragment; + +import org.junit.Before; +import org.junit.Test; +import org.junit.runner.RunWith; +import org.junit.runners.JUnit4; + +import java.nio.charset.StandardCharsets; +import java.util.Collection; + +/** Tests for ParamFileWriteAction. */ +@RunWith(JUnit4.class) +public class ParamFileWriteActionTest extends BuildViewTestCase { + private Root rootDir; + private Artifact outputArtifact; + private Artifact treeArtifact; + + @Before + public void createArtifacts() throws Exception { + rootDir = Root.asDerivedRoot(scratch.dir("/exec/root")); + outputArtifact = getBinArtifactWithNoOwner("destination.txt"); + FileSystemUtils.createDirectoryAndParents(outputArtifact.getPath().getParentDirectory()); + treeArtifact = createTreeArtifact("artifact/myTreeFileArtifact"); + } + + + @Test + public void testOutputs() { + Action action = createParameterFileWriteAction( + ImmutableList.<Artifact>of(), createNormalCommandLine()); + assertThat(Artifact.toRootRelativePaths(action.getOutputs())).containsExactly( + "destination.txt"); + } + + @Test + public void testInputs() { + Action action = createParameterFileWriteAction( + ImmutableList.of(treeArtifact), + createTreeArtifactExpansionCommandLine()); + assertThat(Artifact.toExecPaths(action.getInputs())).containsExactly( + "artifact/myTreeFileArtifact"); + } + + @Test + public void testWriteCommandLineWithoutTreeArtifactExpansion() throws Exception { + Action action = createParameterFileWriteAction( + ImmutableList.<Artifact>of(), createNormalCommandLine()); + ActionExecutionContext context = actionExecutionContext(); + action.execute(context); + String content = new String(FileSystemUtils.readContentAsLatin1(outputArtifact.getPath())); + assertEquals("--flag1\n--flag2\n--flag3\nvalue1\nvalue2", content.trim()); + } + + @Test + public void testWriteCommandLineWithTreeArtifactExpansion() throws Exception { + Action action = createParameterFileWriteAction( + ImmutableList.of(treeArtifact), + createTreeArtifactExpansionCommandLine()); + ActionExecutionContext context = actionExecutionContext(); + action.execute(context); + String content = new String(FileSystemUtils.readContentAsLatin1(outputArtifact.getPath())); + assertEquals( + "--flag1\n" + + "artifact/myTreeFileArtifact/artifacts/treeFileArtifact1:" + + "artifact/myTreeFileArtifact/artifacts/treeFileArtifact2", + content.trim()); + } + + private Artifact createTreeArtifact(String rootRelativePath) { + PathFragment relpath = new PathFragment(rootRelativePath); + return new SpecialArtifact( + rootDir.getPath().getRelative(relpath), + rootDir, + rootDir.getExecPath().getRelative(relpath), + ArtifactOwner.NULL_OWNER, + SpecialArtifactType.TREE); + } + + private TreeFileArtifact createTreeFileArtifact( + Artifact inputTreeArtifact, String parentRelativePath) { + return ActionInputHelper.treeFileArtifact( + inputTreeArtifact, + new PathFragment(parentRelativePath)); + } + + private ParameterFileWriteAction createParameterFileWriteAction( + Iterable<Artifact> inputTreeArtifacts, CommandLine commandLine) { + return new ParameterFileWriteAction( + ActionsTestUtil.NULL_ACTION_OWNER, + inputTreeArtifacts, + outputArtifact, + commandLine, + ParameterFileType.UNQUOTED, + StandardCharsets.ISO_8859_1); + } + + private CommandLine createNormalCommandLine() { + return CustomCommandLine.builder() + .add("--flag1") + .add("--flag2") + .add("--flag3", ImmutableList.of("value1", "value2")) + .build(); + } + + private CommandLine createTreeArtifactExpansionCommandLine() { + return CustomCommandLine.builder() + .add("--flag1") + .addJoinExpandedTreeArtifactExecPath(":", treeArtifact) + .build(); + } + + private ActionExecutionContext actionExecutionContext() throws Exception { + final Iterable<TreeFileArtifact> treeFileArtifacts = ImmutableList.of( + createTreeFileArtifact(treeArtifact, "artifacts/treeFileArtifact1"), + createTreeFileArtifact(treeArtifact, "artifacts/treeFileArtifact2")); + + ArtifactExpander artifactExpander = new ArtifactExpander() { + @Override + public void expand(Artifact artifact, Collection<? super Artifact> output) { + for (TreeFileArtifact treeFileArtifact : treeFileArtifacts) { + if (treeFileArtifact.getParent().equals(artifact)) { + output.add(treeFileArtifact); + } + } + } + }; + + Executor executor = new TestExecutorBuilder(directories, binTools).build(); + return new ActionExecutionContext( + executor, null, null, new FileOutErr(), artifactExpander); + } +}