diff --git a/src/main/java/com/google/devtools/build/lib/actions/CommandLines.java b/src/main/java/com/google/devtools/build/lib/actions/CommandLines.java index 6f40a936f817e3..2678b055f1c6dc 100644 --- a/src/main/java/com/google/devtools/build/lib/actions/CommandLines.java +++ b/src/main/java/com/google/devtools/build/lib/actions/CommandLines.java @@ -99,14 +99,20 @@ private CommandLines(Object commandLines) { * is expected to write these param files prior to execution of an action. * * @param artifactExpander The artifact expander to use. - * @param paramFileBasePath Used to derive param file names. Often the first output of an action. + * @param paramFileBasePath Used to derive param file names. Often the first output of an action + * @param stripPaths function to strip configuration prefixes from output paths, in accordance + * with the logic in {@link PathStripper} * @param limits The command line limits the host OS can support. * @return The expanded command line and its param files (if any). */ public ExpandedCommandLines expand( - ArtifactExpander artifactExpander, PathFragment paramFileBasePath, CommandLineLimits limits) + ArtifactExpander artifactExpander, + PathFragment paramFileBasePath, + Function stripPaths, + CommandLineLimits limits) throws CommandLineExpansionException, InterruptedException { - return expand(artifactExpander, paramFileBasePath, limits, PARAM_FILE_ARG_LENGTH_ESTIMATE); + return expand( + artifactExpander, paramFileBasePath, limits, stripPaths, PARAM_FILE_ARG_LENGTH_ESTIMATE); } @VisibleForTesting @@ -114,6 +120,7 @@ ExpandedCommandLines expand( ArtifactExpander artifactExpander, PathFragment paramFileBasePath, CommandLineLimits limits, + Function stripPaths, int paramFileArgLengthEstimate) throws CommandLineExpansionException, InterruptedException { // Optimize for simple case of single command line @@ -155,7 +162,8 @@ ExpandedCommandLines expand( String paramArg = SingleStringArgFormatter.format( - paramFileInfo.getFlagFormatString(), paramFileExecPath.getPathString()); + paramFileInfo.getFlagFormatString(), + stripPaths.apply(paramFileExecPath).getPathString()); arguments.addElement(paramArg); cmdLineLength += paramArg.length() + 1; diff --git a/src/main/java/com/google/devtools/build/lib/actions/PathStripper.java b/src/main/java/com/google/devtools/build/lib/actions/PathStripper.java index 1029b974cb7b7a..100506cd2de4c4 100644 --- a/src/main/java/com/google/devtools/build/lib/actions/PathStripper.java +++ b/src/main/java/com/google/devtools/build/lib/actions/PathStripper.java @@ -18,7 +18,9 @@ import com.google.common.base.Preconditions; import com.google.common.collect.ImmutableList; +import com.google.devtools.build.lib.actions.Artifact.DerivedArtifact; import com.google.devtools.build.lib.actions.CommandLines.ParamFileActionInput; +import com.google.devtools.build.lib.collect.nestedset.NestedSet; import com.google.devtools.build.lib.vfs.PathFragment; import java.util.Collection; import java.util.HashSet; @@ -47,9 +49,19 @@ * *

As an experimental feature, most logic is centralized here to provide easy hooks into executor * code and avoid complicating large swaths of the code base. "Qualifying" actions are determined by - * {@code --experimental_path_agnostic_action}. As this feature stabilizes, rule definitions can - * more directly own their integration, particularly declaring which of their actions qualify and - * where paths appear in their command lines. + * + *

*/ public interface PathStripper { /** @@ -102,13 +114,32 @@ public List processInputs(List inputs) { }; } + /** + * Returns the regex to strip output paths from a string. + * + *

Supports strings with multiple output paths in arbitrary places. For example + * "/path/to/compiler bazel-out/x86-fastbuild/foo src/my.src -Dbazel-out/arm-opt/bar". + * + *

Doesn't strip paths that would be non-existent without config prefixes. For example, these + * are unchanged: "bazel-out/x86-fastbuild", "bazel-out;foo", "/path/to/compiler bazel-out". + * + * @param outputRoot root segment of output paths (i.e. "bazel-out") + */ + private static Pattern stripPathsPattern(String outputRoot) { + // Match "bazel-out" followed by a slash followed by any combination of word characters, "_", + // and "-", followed by another slash. This would miss substrings like "bazel-out/k8-fastbuild". + // But those don't represent actual outputs (all outputs would have to have names beneath that + // path). So we're not trying to replace those. + return Pattern.compile(outputRoot + "/[\\w_-]+/"); + } + /** * Instantiates a {@link PathStripper} for a spawn action. * * @param spawn the action to support * @param pathAgnosticActions mnemonics of actions that "qualify" for path stripping. Just because * an action type qualifies doesn't mean its paths are stripped. See {@link - * #shouldStripPaths}. + * #isPathStrippable}. * @param outputRoot root of the output tree ("bazel-out"). */ static PathStripper create( @@ -118,9 +149,7 @@ static PathStripper create( if (!shouldStripPaths(spawn, pathAgnosticActions, outputRoot)) { return noop(); } - Pattern stripPathsPattern = - Pattern.compile("\\Q" + outputRoot.getPathString() + "\\E/[\\w-_]+(/|$)"); - + Pattern stripPathsPattern = stripPathsPattern(outputRoot.getPathString()); return new PathStripper() { @Override public String getExecPathString(ActionInput artifact) { @@ -144,11 +173,19 @@ public String processCmdArg(String arg) { // Then lazily instantiate their paths in the executor client. That's roughly what // processInputs(), below, models for ParamFileActionInputs. But that involves more API // modeling and rule logic cleanup. So we take the path of least resistance for now. + if (spawn.stripOutputPaths()) { + // Command line paths were already stripped during action construction. + return arg; + } return stripPathsPattern.matcher(arg).replaceAll(outputRoot.getPathString() + "/"); } @Override public ImmutableList processInputs(List inputs) { + if (spawn.stripOutputPaths()) { + // .param file paths were already stripped during action construction. + return ImmutableList.copyOf(inputs); + } return inputs.stream() .map( input -> @@ -163,16 +200,31 @@ public ImmutableList processInputs(List inputs) { /** * Should this action have its paths stripped for execution? * - *

Only true for actions that a) qualify, b) don't have distinct paths that would become - * duplicates if their config prefixes were removed. + *

Only true for actions that are strippable according to {@link #isPathStrippable} and are + * triggered via {@code --experimental_path_agnostic_action} or {@code + * --experimental_output_paths=strip}. */ - static boolean shouldStripPaths( + private static boolean shouldStripPaths( Spawn spawn, Collection pathAgnosticActions, PathFragment outputRoot) { String actionMnemonic = spawn.getMnemonic(); - if (!pathAgnosticActions.contains(actionMnemonic)) { + // If an action is triggered via --experimental_output_paths=strip, that means its owning rule + // opted it in by setting spawn.stripOutputPaths(). + if (!pathAgnosticActions.contains(actionMnemonic) && !spawn.stripOutputPaths()) { return false; } + return isPathStrippable(spawn.getInputFiles(), outputRoot); + } + /** + * Is this action safe to strip? + * + *

This is distinct from whether we should strip it. An action is stripped if a) the + * build requests it to be stripped ({@link #shouldStripPaths}) and b) it's safe to do so. + * + *

This method only checks b). + */ + static boolean isPathStrippable( + NestedSet actionInputs, PathFragment outputRoot) { // For qualifying action types, check that no inputs or outputs would clash if paths were // removed, e.g. "bazel-out/k8-fastbuild/foo" and "bazel-out/host/foo". // @@ -184,22 +236,76 @@ static boolean shouldStripPaths( // caching the same action in host and target configurations. This could be mitigated by // stripping the host prefix *only* when the entire action is in the host configuration. HashSet rootRelativePaths = new HashSet<>(); - for (ActionInput input : spawn.getInputFiles().toList()) { + for (ActionInput input : actionInputs.toList()) { if (!isOutputPath(input, outputRoot)) { continue; } // For "bazel-out/k8-fastbuild/foo/bar", get "foo/bar". if (!rootRelativePaths.add(input.getExecPath().subFragment(2))) { + // TODO(bazel-team): don't fail on duplicate inputs, i.e. when the same exact exec path + // (including config prefix) is included twice. return false; } } return true; } - /** Is this a strippable path? */ + /** + * Is this a strippable path? + * + * @param artifact artifact whose path to check + * @param outputRoot - the output tree's execPath-relative root (e.g. "bazel-out") + */ static boolean isOutputPath(ActionInput artifact, PathFragment outputRoot) { // We can't simply check for DerivedArtifact. Output paths can also appear, for example, in // ParamFileActionInput and ActionInputHelper.BasicActionInput. - return artifact.getExecPath().startsWith(outputRoot); + return isOutputPath(artifact.getExecPath(), outputRoot); + } + + /** + * Is this a strippable path? + * + * @param pathFragment path to check + * @param outputRoot - the output tree's execPath-relative root (e.g. "bazel-out") + */ + static boolean isOutputPath(PathFragment pathFragment, PathFragment outputRoot) { + return pathFragment.startsWith(outputRoot); + } + + /* + * Utility method: strips the configuration prefix from an output artifact's exec path. + * + *

Rules that support path stripping can use this to help their implementation logic. + */ + static PathFragment strip(PathFragment execPath) { + return execPath.subFragment(0, 1).getRelative(execPath.subFragment(2)); + } + + /** + * Utility method: returns an output artifact's exec path with its configuration prefix stripped. + * + *

Rules that support path stripping can use this to help their implementation logic. + */ + static String strip(DerivedArtifact artifact) { + return strip(artifact.getExecPath()).getPathString(); + } + + /** + * Utility class to strip output path configuration prefixes from arbitrary strings. + * + *

Rules that support path stripping can use this to help their implementation logic. + */ + class StringStripper { + private final Pattern pattern; + private final String outputRoot; + + public StringStripper(String outputRoot) { + this.outputRoot = outputRoot; + this.pattern = stripPathsPattern(outputRoot); + } + + public String strip(String str) { + return pattern.matcher(str).replaceAll(outputRoot + "/"); + } } } diff --git a/src/main/java/com/google/devtools/build/lib/actions/Spawn.java b/src/main/java/com/google/devtools/build/lib/actions/Spawn.java index 20fcdf3ccc57f9..5a8ff0b16de045 100644 --- a/src/main/java/com/google/devtools/build/lib/actions/Spawn.java +++ b/src/main/java/com/google/devtools/build/lib/actions/Spawn.java @@ -177,4 +177,14 @@ default String getTargetLabel() { Label label = getResourceOwner().getOwner().getLabel(); return label == null ? null : label.toString(); } + + /** + * If true, this spawn strips output path config prefixes from its inputs, outputs, and command + * line (including .params files) before running on an executor. + * + *

See {@link PathStripper}. + */ + default boolean stripOutputPaths() { + return false; + } } 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 9f6e4209171f2f..590b8f09ab4bb2 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 @@ -18,16 +18,19 @@ import com.google.common.base.Joiner; import com.google.common.base.Objects; import com.google.common.base.Preconditions; +import com.google.common.base.Verify; import com.google.common.collect.ImmutableList; import com.google.common.collect.ImmutableMap; import com.google.common.collect.Interner; import com.google.devtools.build.lib.actions.ActionKeyContext; 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.DerivedArtifact; import com.google.devtools.build.lib.actions.Artifact.TreeFileArtifact; import com.google.devtools.build.lib.actions.CommandLine; import com.google.devtools.build.lib.actions.CommandLineExpansionException; import com.google.devtools.build.lib.actions.CommandLineItem; +import com.google.devtools.build.lib.actions.PathStripper; import com.google.devtools.build.lib.actions.SingleStringArgFormatter; import com.google.devtools.build.lib.cmdline.Label; import com.google.devtools.build.lib.collect.nestedset.NestedSet; @@ -57,8 +60,7 @@ /** A customizable, serializable class for building memory efficient command lines. */ @Immutable -public final class CustomCommandLine extends CommandLine { - +public class CustomCommandLine extends CommandLine { private interface ArgvFragment { /** * Expands this fragment into the passed command line vector. @@ -66,10 +68,15 @@ private interface ArgvFragment { * @param arguments The command line's argument vector. * @param argi The index of the next available argument. * @param builder The command line builder to which we should add arguments. + * @param stripOutputPaths Strip output path config prefixes? See {@link PathStripper}. * @return The index of the next argument, after the ArgvFragment has consumed its args. If the * ArgvFragment doesn't have any args, it should return {@code argi} unmodified. */ - int eval(List arguments, int argi, ImmutableList.Builder builder) + int eval( + List arguments, + int argi, + ImmutableList.Builder builder, + boolean stripOutputPaths) throws CommandLineExpansionException, InterruptedException; int addToFingerprint( @@ -87,7 +94,11 @@ int addToFingerprint( */ private abstract static class StandardArgvFragment implements ArgvFragment { @Override - public final int eval(List arguments, int argi, ImmutableList.Builder builder) { + public final int eval( + List arguments, + int argi, + ImmutableList.Builder builder, + boolean stripOutputPaths) { eval(builder); return argi; // Doesn't consume any arguments, so return argi unmodified } @@ -360,9 +371,13 @@ private static final class VectorArgFragment implements ArgvFragment { this.hasJoinWith = hasJoinWith; } - @SuppressWarnings("unchecked") @Override - public int eval(List arguments, int argi, ImmutableList.Builder builder) + @SuppressWarnings("unchecked") + public int eval( + List arguments, + int argi, + ImmutableList.Builder builder, + boolean stripOutputPaths) throws CommandLineExpansionException, InterruptedException { final List mutatedValues; CommandLineItem.MapFn mapFn = @@ -391,7 +406,13 @@ public int eval(List arguments, int argi, ImmutableList.Builder } } else { for (int i = 0; i < count; ++i) { - mutatedValues.add(CommandLineItem.expandToCommandLine(arguments.get(argi++))); + Object arg = arguments.get(argi++); + if (arg instanceof DerivedArtifact && stripOutputPaths) { + // This argument is an output file and the command line creator strips output paths. + mutatedValues.add(PathStripper.strip((DerivedArtifact) arg)); + } else { + mutatedValues.add(CommandLineItem.expandToCommandLine(arg)); + } } } } @@ -500,7 +521,11 @@ private static void push(List arguments, String formatStr, Object... arg } @Override - public int eval(List arguments, int argi, ImmutableList.Builder builder) { + public int eval( + List arguments, + int argi, + ImmutableList.Builder builder, + boolean stripOutputPaths) { int argCount = (Integer) arguments.get(argi++); String formatStr = (String) arguments.get(argi++); Object[] args = new Object[argCount]; @@ -541,7 +566,11 @@ private static void push(List arguments, String before, Object arg) { } @Override - public int eval(List arguments, int argi, ImmutableList.Builder builder) { + public int eval( + List arguments, + int argi, + ImmutableList.Builder builder, + boolean stripOutputPaths) { String before = (String) arguments.get(argi++); Object arg = arguments.get(argi++); builder.add(before + CommandLineItem.expandToCommandLine(arg)); @@ -715,6 +744,10 @@ public static final class Builder { // toString() results. private final List arguments = new ArrayList<>(); + private boolean stripOutputPaths = false; + + private PathFragment outputRoot = null; + public boolean isEmpty() { return arguments.isEmpty(); } @@ -723,6 +756,27 @@ public boolean isEmpty() { private boolean treeArtifactsRequested = false; + /** + * Strip output path config prefixes from the command line. + * + *

This offers better executor caching. But it's only safe for actions that don't vary when + * {@code /x86-fastbuild/} (or equivalent) changes in the executor's action key. This only + * affects {@link #addExecPath} and {@link #addPath(PathFragment)} entries. Output paths + * embedded in larger strings and added via {@link #add(String)} or other variants must be + * handled separately. + * + *

See {@link PathStripper} for details. + * + * @param outputRoot the output tree's root fragment (i.e. "bazel-out") + */ + public Builder stripOutputPaths(PathFragment outputRoot) { + Preconditions.checkArgument(!stripOutputPaths); + Preconditions.checkArgument(this.outputRoot == null); + this.stripOutputPaths = true; + this.outputRoot = outputRoot; + return this; + } + /** * Adds a constant-value string. * @@ -1113,7 +1167,14 @@ public NestedSet getTreeArtifactInputs() { } public CustomCommandLine build() { - return new CustomCommandLine(arguments); + return stripOutputPaths + ? new PathStrippingCustomCommandline( + arguments, + /*substitutionMap=*/ null, + Verify.verifyNotNull( + outputRoot, + "path stripping needs an output root ('bazel-out') to identify output paths")) + : new CustomCommandLine(arguments, /*substitutionMap=*/ null); } private Builder addObjectInternal(@Nullable Object value) { @@ -1213,16 +1274,55 @@ public static Builder builder(Builder other) { */ private final Map substitutionMap; - private CustomCommandLine(List arguments) { - this(arguments, null); - } - private CustomCommandLine( List arguments, Map substitutionMap) { this.arguments = ImmutableList.copyOf(arguments); this.substitutionMap = substitutionMap == null ? null : ImmutableMap.copyOf(substitutionMap); } + protected boolean stripOutputPaths() { + return false; + } + + protected PathFragment getOutputRoot() { + throw new UnsupportedOperationException( + "This should only be called for PathStrippingCustomCommandLine"); + } + + /** + * {@link CustomCommandLine} that strips config prefixes from output paths. See {@link + * PathStripper}. + * + *

We use inheritance vs. a {@code stripOutputPaths} field in {@link CustomCommandLine} because + * Java-heavy builds keep many {@link CustomCommandLine} objects in memory. So we need to minimize + * each one's memory footprint. + */ + private static final class PathStrippingCustomCommandline extends CustomCommandLine { + // TODO(https://github.com/bazelbuild/bazel/issues/6526): outputRoot is just an indirect + // reference to "bazel-out". Java-heavy builds keep enough CustomCommandLine objects in memory + // such that each additional reference contributes observable extra memory on the host machine. + // Find a way to consolidate this into a single global reference. + private final PathFragment outputRoot; + + private PathStrippingCustomCommandline( + List arguments, + Map substitutionMap, + @Nullable PathFragment outputRoot) { + super(arguments, substitutionMap); + this.outputRoot = outputRoot; + } + + @Override + protected boolean stripOutputPaths() { + return true; + } + + @Override + protected PathFragment getOutputRoot() { + return outputRoot; + } + } + /** * Given the list of {@link TreeFileArtifact}s, returns another CustomCommandLine that replaces * their parent TreeArtifacts with the TreeFileArtifacts in all {@link @@ -1235,7 +1335,7 @@ public CustomCommandLine evaluateTreeFileArtifacts(Iterable tr substitutionMap.put(treeFileArtifact.getParent(), treeFileArtifact); } - return new CustomCommandLine(arguments, substitutionMap.build()); + return new CustomCommandLine(arguments, substitutionMap.buildOrThrow()); } @Override @@ -1268,8 +1368,14 @@ private ImmutableList argumentsInternal(@Nullable ArtifactExpander artif (TreeArtifactExpansionArgvFragment) substitutedArg; expansionArg.eval(builder, artifactExpander); } else { - i = ((ArgvFragment) substitutedArg).eval(arguments, i, builder); + i = ((ArgvFragment) substitutedArg).eval(arguments, i, builder, stripOutputPaths()); } + } else if (substitutedArg instanceof DerivedArtifact && stripOutputPaths()) { + builder.add(PathStripper.strip((DerivedArtifact) substitutedArg)); + } else if (substitutedArg instanceof PathFragment + && stripOutputPaths() + && PathStripper.isOutputPath((PathFragment) substitutedArg, getOutputRoot())) { + builder.add(PathStripper.strip(((PathFragment) substitutedArg)).getPathString()); } else { builder.add(CommandLineItem.expandToCommandLine(substitutedArg)); } @@ -1279,7 +1385,10 @@ private ImmutableList argumentsInternal(@Nullable ArtifactExpander artif private void evalSimpleVectorArg(Iterable arg, ImmutableList.Builder builder) { for (Object value : arg) { - builder.add(CommandLineItem.expandToCommandLine(value)); + builder.add( + value instanceof DerivedArtifact && stripOutputPaths() + ? PathStripper.strip((DerivedArtifact) value) + : CommandLineItem.expandToCommandLine(value)); } } diff --git a/src/main/java/com/google/devtools/build/lib/analysis/actions/SpawnAction.java b/src/main/java/com/google/devtools/build/lib/analysis/actions/SpawnAction.java index a7041752c41ae6..7c4ca3f08b1e0b 100644 --- a/src/main/java/com/google/devtools/build/lib/analysis/actions/SpawnAction.java +++ b/src/main/java/com/google/devtools/build/lib/analysis/actions/SpawnAction.java @@ -51,6 +51,7 @@ import com.google.devtools.build.lib.actions.ExecException; import com.google.devtools.build.lib.actions.FilesetOutputSymlink; import com.google.devtools.build.lib.actions.ParamFileInfo; +import com.google.devtools.build.lib.actions.PathStripper; import com.google.devtools.build.lib.actions.ResourceSetOrBuilder; import com.google.devtools.build.lib.actions.RunfilesSupplier; import com.google.devtools.build.lib.actions.Spawn; @@ -114,6 +115,7 @@ public interface ExtraActionInfoSupplier { private final ExtraActionInfoSupplier extraActionInfoSupplier; private final Artifact primaryOutput; private final Consumer>> resultConsumer; + private final boolean stripOutputPaths; /** * Constructs a SpawnAction using direct initialization arguments. @@ -166,7 +168,8 @@ public SpawnAction( mnemonic, false, null, - null); + null, + /*stripOutputPaths=*/ false); } /** @@ -210,7 +213,8 @@ public SpawnAction( String mnemonic, boolean executeUnconditionally, ExtraActionInfoSupplier extraActionInfoSupplier, - Consumer>> resultConsumer) { + Consumer>> resultConsumer, + boolean stripOutputPaths) { super(owner, tools, inputs, runfilesSupplier, outputs, env); this.primaryOutput = primaryOutput; this.resourceSetOrBuilder = resourceSetOrBuilder; @@ -223,6 +227,7 @@ public SpawnAction( this.executeUnconditionally = executeUnconditionally; this.extraActionInfoSupplier = extraActionInfoSupplier; this.resultConsumer = resultConsumer; + this.stripOutputPaths = stripOutputPaths; } @Override @@ -369,7 +374,8 @@ final Spawn getSpawn(NestedSet inputs) inputs, /*additionalInputs=*/ ImmutableList.of(), /*filesetMappings=*/ ImmutableMap.of(), - /*reportOutputs=*/ true); + /*reportOutputs=*/ true, + stripOutputPaths); } /** @@ -402,7 +408,11 @@ protected Spawn getSpawn( boolean reportOutputs) throws CommandLineExpansionException, InterruptedException { ExpandedCommandLines expandedCommandLines = - commandLines.expand(artifactExpander, getPrimaryOutput().getExecPath(), commandLineLimits); + commandLines.expand( + artifactExpander, + getPrimaryOutput().getExecPath(), + stripOutputPaths ? PathStripper::strip : (origPath) -> origPath, + commandLineLimits); return new ActionSpawn( ImmutableList.copyOf(expandedCommandLines.arguments()), this, @@ -411,7 +421,8 @@ protected Spawn getSpawn( getInputs(), expandedCommandLines.getParamFiles(), filesetMappings, - reportOutputs); + reportOutputs, + stripOutputPaths); } Spawn getSpawnForExtraAction() throws CommandLineExpansionException, InterruptedException { @@ -569,6 +580,7 @@ private static final class ActionSpawn extends BaseSpawn { private final Map> filesetMappings; private final ImmutableMap effectiveEnvironment; private final boolean reportOutputs; + private final boolean stripOutputPaths; /** * Creates an ActionSpawn with the given environment variables. @@ -584,7 +596,8 @@ private ActionSpawn( NestedSet inputs, Iterable additionalInputs, Map> filesetMappings, - boolean reportOutputs) + boolean reportOutputs, + boolean stripOutputPaths) throws CommandLineExpansionException { super( arguments, @@ -603,6 +616,7 @@ private ActionSpawn( inputsBuilder.addAll(additionalInputs); this.inputs = inputsBuilder.build(); this.filesetMappings = filesetMappings; + this.stripOutputPaths = stripOutputPaths; // If the action environment is already resolved using the client environment, the given // environment variables are used as they are. Otherwise, they are used as clientEnv to @@ -615,6 +629,11 @@ private ActionSpawn( this.reportOutputs = reportOutputs; } + @Override + public boolean stripOutputPaths() { + return stripOutputPaths; + } + @Override public ImmutableMap getEnvironment() { return effectiveEnvironment; @@ -834,7 +853,8 @@ protected SpawnAction createSpawnAction( mnemonic, executeUnconditionally, extraActionInfoSupplier, - resultConsumer); + resultConsumer, + /*stripOutputPaths=*/ false); } /** diff --git a/src/main/java/com/google/devtools/build/lib/analysis/actions/StarlarkAction.java b/src/main/java/com/google/devtools/build/lib/analysis/actions/StarlarkAction.java index 5371908a2837ba..8b99d5d10bdb48 100644 --- a/src/main/java/com/google/devtools/build/lib/analysis/actions/StarlarkAction.java +++ b/src/main/java/com/google/devtools/build/lib/analysis/actions/StarlarkAction.java @@ -128,7 +128,8 @@ public StarlarkAction( mnemonic, /* executeUnconditionally */ false, /* extraActionInfoSupplier */ null, - /* resultConsumer */ null); + /* resultConsumer */ null, + /* stripOutputPaths */ false); this.allStarlarkActionInputs = inputs; this.unusedInputsList = unusedInputsList; diff --git a/src/main/java/com/google/devtools/build/lib/analysis/config/CoreOptions.java b/src/main/java/com/google/devtools/build/lib/analysis/config/CoreOptions.java index 8d0cc8190817c5..6e6cb6b83ba6cc 100644 --- a/src/main/java/com/google/devtools/build/lib/analysis/config/CoreOptions.java +++ b/src/main/java/com/google/devtools/build/lib/analysis/config/CoreOptions.java @@ -586,6 +586,19 @@ public String getTypeDescription() { + "target_environment values.") public Label autoCpuEnvironmentGroup; + @Option( + name = "experimental_allow_unresolved_symlinks", + defaultValue = "false", + documentationCategory = OptionDocumentationCategory.UNDOCUMENTED, + effectTags = { + OptionEffectTag.LOSES_INCREMENTAL_STATE, + OptionEffectTag.LOADING_AND_ANALYSIS, + }, + help = + "If enabled, Bazel allows the use of ctx.action.{declare_symlink,symlink}, thus " + + "allowing the user to create symlinks (resolved and unresolved)") + public boolean allowUnresolvedSymlinks; + /** Values for --experimental_output_paths. */ public enum OutputPathsMode { /** Use the production output path model. */ @@ -600,6 +613,22 @@ public enum OutputPathsMode { *

Follow the above link for latest details on exact scope. */ CONTENT, + /** + * Strip the config prefix (i.e. {@code /x86-fastbuild/} from output paths for actions that are + * registered to support this feature. + * + *

This works independently of {@code --experimental_path_agnostic_action} ({@link + * com.google.devtools.build.lib.exec.ExecutionOptions#pathAgnosticActions}). This flag enables + * actions that know how to strip output paths from their command lines, which requires custom + * code in the action creation logic. {@code --experimental_path_agnostic_action} is a catch-all + * that automatically strips command lines after actions have constructed them. The latter is + * suitable for experimentation but not as robust since it's essentially a textual replacement + * postprocessor. That may miss subtleties in the command line's structure, isn't particularly + * efficient, and isn't safe for actions with special dependencies on their output paths. + * + *

See {@link com.google.devtools.build.lib.actions.PathStripper} for details. + */ + STRIP, } /** Converter for --experimental_output_paths. */ @@ -609,19 +638,6 @@ public OutputPathsConverter() { } } - @Option( - name = "experimental_allow_unresolved_symlinks", - defaultValue = "false", - documentationCategory = OptionDocumentationCategory.UNDOCUMENTED, - effectTags = { - OptionEffectTag.LOSES_INCREMENTAL_STATE, - OptionEffectTag.LOADING_AND_ANALYSIS, - }, - help = - "If enabled, Bazel allows the use of ctx.action.{declare_symlink,symlink}, thus " - + "allowing the user to create symlinks (resolved and unresolved)") - public boolean allowUnresolvedSymlinks; - @Option( name = "experimental_output_paths", converter = OutputPathsConverter.class, diff --git a/src/main/java/com/google/devtools/build/lib/analysis/extra/ExtraAction.java b/src/main/java/com/google/devtools/build/lib/analysis/extra/ExtraAction.java index f6e6d061e4e154..28ef8c35003fa0 100644 --- a/src/main/java/com/google/devtools/build/lib/analysis/extra/ExtraAction.java +++ b/src/main/java/com/google/devtools/build/lib/analysis/extra/ExtraAction.java @@ -90,7 +90,8 @@ public final class ExtraAction extends SpawnAction { mnemonic, false, null, - null); + null, + /*stripOutputPaths=*/ false); this.shadowedAction = shadowedAction; this.createDummyOutput = createDummyOutput; diff --git a/src/main/java/com/google/devtools/build/lib/bazel/rules/ninja/actions/NinjaAction.java b/src/main/java/com/google/devtools/build/lib/bazel/rules/ninja/actions/NinjaAction.java index b79fbc5eafeb39..811c71a1740cfd 100644 --- a/src/main/java/com/google/devtools/build/lib/bazel/rules/ninja/actions/NinjaAction.java +++ b/src/main/java/com/google/devtools/build/lib/bazel/rules/ninja/actions/NinjaAction.java @@ -94,7 +94,8 @@ public NinjaAction( /* mnemonic= */ MNEMONIC, /* executeUnconditionally= */ executeUnconditionally, /* extraActionInfoSupplier= */ null, - /* resultConsumer= */ null); + /* resultConsumer= */ null, + /*stripOutputPaths=*/ false); this.sourceRoot = sourceRoot; this.depFile = depFile; diff --git a/src/main/java/com/google/devtools/build/lib/rules/cpp/LtoBackendAction.java b/src/main/java/com/google/devtools/build/lib/rules/cpp/LtoBackendAction.java index 6acc1e61d2139b..025302deac0e2e 100644 --- a/src/main/java/com/google/devtools/build/lib/rules/cpp/LtoBackendAction.java +++ b/src/main/java/com/google/devtools/build/lib/rules/cpp/LtoBackendAction.java @@ -104,7 +104,8 @@ public LtoBackendAction( mnemonic, false, null, - null); + null, + /*stripOutputPaths=*/ false); mandatoryInputs = inputs; Preconditions.checkState( (allBitcodeFiles == null) == (importsFile == null), diff --git a/src/main/java/com/google/devtools/build/lib/rules/genrule/GenRuleAction.java b/src/main/java/com/google/devtools/build/lib/rules/genrule/GenRuleAction.java index 0853fa2fa36ca8..e56f470774da7e 100644 --- a/src/main/java/com/google/devtools/build/lib/rules/genrule/GenRuleAction.java +++ b/src/main/java/com/google/devtools/build/lib/rules/genrule/GenRuleAction.java @@ -67,7 +67,8 @@ public GenRuleAction( MNEMONIC, false, null, - null); + null, + /*stripOutputPaths=*/ false); } @Override diff --git a/src/main/java/com/google/devtools/build/lib/rules/java/BUILD b/src/main/java/com/google/devtools/build/lib/rules/java/BUILD index 18e9a68511fe17..1622ae20c9d358 100644 --- a/src/main/java/com/google/devtools/build/lib/rules/java/BUILD +++ b/src/main/java/com/google/devtools/build/lib/rules/java/BUILD @@ -172,6 +172,7 @@ java_library( "//src/main/java/com/google/devtools/build/lib/analysis:config/build_configuration", "//src/main/java/com/google/devtools/build/lib/analysis:config/build_options", "//src/main/java/com/google/devtools/build/lib/analysis:config/core_option_converters", + "//src/main/java/com/google/devtools/build/lib/analysis:config/core_options", "//src/main/java/com/google/devtools/build/lib/analysis:config/fragment", "//src/main/java/com/google/devtools/build/lib/analysis:config/fragment_options", "//src/main/java/com/google/devtools/build/lib/analysis:config/invalid_configuration_exception", diff --git a/src/main/java/com/google/devtools/build/lib/rules/java/JavaCompilationHelper.java b/src/main/java/com/google/devtools/build/lib/rules/java/JavaCompilationHelper.java index 3ef98d011fc6a7..a4906dc62152f8 100644 --- a/src/main/java/com/google/devtools/build/lib/rules/java/JavaCompilationHelper.java +++ b/src/main/java/com/google/devtools/build/lib/rules/java/JavaCompilationHelper.java @@ -22,6 +22,7 @@ import com.google.common.collect.Iterables; import com.google.devtools.build.lib.actions.Artifact; import com.google.devtools.build.lib.actions.ExecutionRequirements; +import com.google.devtools.build.lib.actions.PathStripper; import com.google.devtools.build.lib.analysis.AnalysisEnvironment; import com.google.devtools.build.lib.analysis.FilesToRunProvider; import com.google.devtools.build.lib.analysis.RuleContext; @@ -32,6 +33,8 @@ import com.google.devtools.build.lib.analysis.actions.SpawnAction; import com.google.devtools.build.lib.analysis.config.BuildConfigurationValue; import com.google.devtools.build.lib.analysis.config.CoreOptionConverters.StrictDepsMode; +import com.google.devtools.build.lib.analysis.config.CoreOptions; +import com.google.devtools.build.lib.analysis.config.CoreOptions.OutputPathsMode; import com.google.devtools.build.lib.analysis.test.InstrumentedFilesCollector; import com.google.devtools.build.lib.cmdline.Label; import com.google.devtools.build.lib.collect.nestedset.NestedSet; @@ -367,7 +370,7 @@ private boolean separateResourceJar( || !getTranslations().isEmpty(); } - private ImmutableMap getExecutionInfo() throws InterruptedException { + private ImmutableMap getExecutionInfo() { ImmutableMap.Builder executionInfo = ImmutableMap.builder(); ImmutableMap.Builder workerInfo = ImmutableMap.builder(); if (javaToolchain.getJavacSupportsWorkers()) { @@ -877,4 +880,24 @@ static Artifact derivedArtifact( path = path.replaceName(prefix + basename); return context.getDerivedArtifact(path, artifact.getRoot()); } + + /** + * Canonical place to determine if a Java action should strip config prefixes from its output + * paths. + * + *

See {@link PathStripper}. + */ + static boolean stripOutputPaths( + NestedSet actionInputs, BuildConfigurationValue configuration) { + CoreOptions coreOptions = configuration.getOptions().get(CoreOptions.class); + return coreOptions.outputPathsMode == OutputPathsMode.STRIP + && PathStripper.isPathStrippable( + actionInputs, + PathFragment.create(configuration.getDirectories().getRelativeOutputPath())); + } + + /** The output path under the exec root (i.e. "bazel-out"). */ + static PathFragment outputBase(Artifact anyGeneratedArtifact) { + return anyGeneratedArtifact.getExecPath().subFragment(0, 1); + } } diff --git a/src/main/java/com/google/devtools/build/lib/rules/java/JavaCompileAction.java b/src/main/java/com/google/devtools/build/lib/rules/java/JavaCompileAction.java index 2c748cb649f156..82cb5a85658075 100644 --- a/src/main/java/com/google/devtools/build/lib/rules/java/JavaCompileAction.java +++ b/src/main/java/com/google/devtools/build/lib/rules/java/JavaCompileAction.java @@ -52,6 +52,7 @@ import com.google.devtools.build.lib.actions.ExecException; import com.google.devtools.build.lib.actions.ParamFileInfo; import com.google.devtools.build.lib.actions.ParameterFile; +import com.google.devtools.build.lib.actions.PathStripper; import com.google.devtools.build.lib.actions.ResourceSet; import com.google.devtools.build.lib.actions.RunfilesSupplier; import com.google.devtools.build.lib.actions.Spawn; @@ -90,6 +91,7 @@ import java.util.List; import java.util.Map; import java.util.UUID; +import java.util.function.Function; import javax.annotation.Nullable; import net.starlark.java.eval.EvalException; import net.starlark.java.eval.Sequence; @@ -161,11 +163,7 @@ public JavaCompileAction( super( owner, tools, - NestedSetBuilder.stableOrder() - .addTransitive(mandatoryInputs) - .addTransitive(transitiveInputs) - .addTransitive(dependencyArtifacts) - .build(), + allInputs(mandatoryInputs, transitiveInputs, dependencyArtifacts), runfilesSupplier, outputs, env); @@ -201,6 +199,18 @@ public JavaCompileAction( describe()); } + /** Computes all of a {@link JavaCompileAction}'s inputs. */ + static NestedSet allInputs( + NestedSet mandatoryInputs, + NestedSet transitiveInputs, + NestedSet dependencyArtifacts) { + return NestedSetBuilder.stableOrder() + .addTransitive(mandatoryInputs) + .addTransitive(transitiveInputs) + .addTransitive(dependencyArtifacts) + .build(); + } + @Override public String getMnemonic() { return compilationType.mnemonic; @@ -282,6 +292,12 @@ static class ReducedClasspath { } } + /** Should we strip config prefixes from executor output paths for this compilation? */ + private boolean stripOutputPaths() { + return JavaCompilationHelper.stripOutputPaths( + allInputs(mandatoryInputs, transitiveInputs, dependencyArtifacts), configuration); + } + @VisibleForTesting JavaSpawn getReducedSpawn( ActionExecutionContext actionExecutionContext, @@ -289,6 +305,11 @@ JavaSpawn getReducedSpawn( boolean fallback) throws CommandLineExpansionException, InterruptedException { CustomCommandLine.Builder classpathLine = CustomCommandLine.builder(); + boolean stripOutputPaths = stripOutputPaths(); + if (stripOutputPaths) { + classpathLine.stripOutputPaths(JavaCompilationHelper.outputBase(getPrimaryOutput())); + } + if (fallback) { classpathLine.addExecPaths("--classpath", transitiveInputs); } else { @@ -312,6 +333,7 @@ JavaSpawn getReducedSpawn( reducedCommandLine.expand( actionExecutionContext.getArtifactExpander(), getPrimaryOutput().getExecPath(), + stripOutputPaths ? PathStripper::strip : Function.identity(), configuration.getCommandLineLimits()); NestedSet inputs = NestedSetBuilder.stableOrder() @@ -323,16 +345,19 @@ JavaSpawn getReducedSpawn( getEffectiveEnvironment(actionExecutionContext.getClientEnv()), getExecutionInfo(), inputs, - /*onlyMandatoryOutput=*/ fallback ? null : outputDepsProto); + /*onlyMandatoryOutput=*/ fallback ? null : outputDepsProto, + stripOutputPaths); } private JavaSpawn getFullSpawn(ActionExecutionContext actionExecutionContext) throws CommandLineExpansionException, InterruptedException { + boolean stripOutputPaths = stripOutputPaths(); CommandLines.ExpandedCommandLines expandedCommandLines = getCommandLines() .expand( actionExecutionContext.getArtifactExpander(), getPrimaryOutput().getExecPath(), + stripOutputPaths ? PathStripper::strip : Function.identity(), configuration.getCommandLineLimits()); return new JavaSpawn( expandedCommandLines, @@ -351,7 +376,8 @@ private JavaSpawn getFullSpawn(ActionExecutionContext actionExecutionContext) // possible by stripping prefixes on the executor. .addTransitive(dependencyArtifacts) .build(), - /*onlyMandatoryOutput=*/ null); + /*onlyMandatoryOutput=*/ null, + stripOutputPaths); } @Override @@ -486,13 +512,15 @@ public ExtraActionInfo.Builder getExtraActionInfo(ActionKeyContext actionKeyCont private final class JavaSpawn extends BaseSpawn { private final NestedSet inputs; private final Artifact onlyMandatoryOutput; + private final boolean stripOutputPaths; JavaSpawn( CommandLines.ExpandedCommandLines expandedCommandLines, Map environment, Map executionInfo, NestedSet inputs, - @Nullable Artifact onlyMandatoryOutput) { + @Nullable Artifact onlyMandatoryOutput, + boolean stripOutputPaths) { super( ImmutableList.copyOf(expandedCommandLines.arguments()), environment, @@ -505,6 +533,7 @@ private final class JavaSpawn extends BaseSpawn { NestedSetBuilder.fromNestedSet(inputs) .addAll(expandedCommandLines.getParamFiles()) .build(); + this.stripOutputPaths = stripOutputPaths; } @Override @@ -516,6 +545,10 @@ public NestedSet getInputFiles() { public boolean isMandatoryOutput(ActionInput output) { return onlyMandatoryOutput == null || onlyMandatoryOutput.equals(output); } + + public boolean stripOutputPaths() { + return stripOutputPaths; + } } @VisibleForTesting @@ -530,6 +563,9 @@ public CommandLines getCommandLines() { private CommandLine getFullClasspathLine() { CustomCommandLine.Builder classpathLine = CustomCommandLine.builder().addExecPaths("--classpath", transitiveInputs); + if (stripOutputPaths()) { + classpathLine.stripOutputPaths(JavaCompilationHelper.outputBase(getPrimaryOutput())); + } if (classpathMode == JavaClasspathMode.JAVABUILDER) { classpathLine.add("--reduce_classpath_mode", "JAVABUILDER_REDUCED"); if (!dependencyArtifacts.isEmpty()) { @@ -605,11 +641,18 @@ public NestedSet getPossibleInputsForTesting() { *

If the executor doesn't strip config prefixes (i.e. config stripping isn't turned on as a * feature), this is a trivial copy. * + *

If config stripping is on, this method won't work with {@link + * JavaConfiguration.JavaClasspathMode#JAVABUILDER}. That mode causes downstream Java compilations + * to read this .jdeps on the executor. Since this method replaces config prefixes, the .jdeps + * entries won't match the executor's stripped paths. This works best with {@link + * JavaConfiguration.JavaClasspathMode#BAZEL}, where Bazel directly processes the .jdeps on the + * local filesystem. Those paths match. + * * @param spawnResult the executor action that created the possibly stripped .jdeps output * @param outputDepsProto path to the .jdeps output * @param actionInputs all inputs to the current action * @param actionExecutionContext the action execution context - * @return the full deps proto (also wdritten to disk to satisfy the action's declared output) + * @return the full deps proto (also written to disk to satisfy the action's declared output) */ static Deps.Dependencies createFullOutputDeps( SpawnResult spawnResult, diff --git a/src/main/java/com/google/devtools/build/lib/rules/java/JavaCompileActionBuilder.java b/src/main/java/com/google/devtools/build/lib/rules/java/JavaCompileActionBuilder.java index 64ec799a725e1d..214cc1060c7b8e 100644 --- a/src/main/java/com/google/devtools/build/lib/rules/java/JavaCompileActionBuilder.java +++ b/src/main/java/com/google/devtools/build/lib/rules/java/JavaCompileActionBuilder.java @@ -26,9 +26,9 @@ import com.google.devtools.build.lib.actions.ActionAnalysisMetadata; import com.google.devtools.build.lib.actions.ActionEnvironment; import com.google.devtools.build.lib.actions.Artifact; -import com.google.devtools.build.lib.actions.CommandLine; import com.google.devtools.build.lib.actions.EmptyRunfilesSupplier; import com.google.devtools.build.lib.actions.ExecutionRequirements; +import com.google.devtools.build.lib.actions.PathStripper; import com.google.devtools.build.lib.actions.extra.ExtraActionInfo; import com.google.devtools.build.lib.actions.extra.JavaCompileInfo; import com.google.devtools.build.lib.analysis.RuleContext; @@ -48,6 +48,7 @@ import java.util.Collections; import java.util.Objects; import java.util.Optional; +import java.util.stream.Collectors; import java.util.stream.Stream; import javax.annotation.Nullable; @@ -199,15 +200,15 @@ public JavaCompileAction build() { NestedSetBuilder toolsBuilder = NestedSetBuilder.compileOrder(); - CommandLine executableLine = javaBuilder.buildCommandLine(toolchain, toolsBuilder); - + CustomCommandLine.Builder executableLine = + javaBuilder.buildCommandLine(toolchain, toolsBuilder); toolsBuilder.addTransitive(toolsJars); ActionEnvironment actionEnvironment = ruleContext.getConfiguration().getActionEnvironment().addFixedVariables(UTF8_ENVIRONMENT); - NestedSetBuilder mandatoryInputs = NestedSetBuilder.stableOrder(); - mandatoryInputs + NestedSetBuilder mandatoryInputsBuilder = NestedSetBuilder.stableOrder(); + mandatoryInputsBuilder .addTransitive(plugins.processorClasspath()) .addTransitive(plugins.data()) .addTransitive(extraData) @@ -219,7 +220,7 @@ public JavaCompileAction build() { .addAll(additionalInputs) .addTransitive(bootClassPath.systemInputs()); if (coverageArtifact != null) { - mandatoryInputs.add(coverageArtifact); + mandatoryInputsBuilder.add(coverageArtifact); } JavaCompileExtraActionInfoSupplier extraActionInfoSupplier = @@ -254,7 +255,18 @@ public JavaCompileAction build() { } NestedSet tools = toolsBuilder.build(); - mandatoryInputs.addTransitive(tools); + mandatoryInputsBuilder.addTransitive(tools); + NestedSet mandatoryInputs = mandatoryInputsBuilder.build(); + + boolean stripOutputPaths = + JavaCompilationHelper.stripOutputPaths( + JavaCompileAction.allInputs( + mandatoryInputs, classpathEntries, compileTimeDependencyArtifacts), + ruleContext.getConfiguration()); + if (stripOutputPaths) { + executableLine.stripOutputPaths(JavaCompilationHelper.outputBase(outputs.output())); + } + return new JavaCompileAction( /* compilationType= */ JavaCompileAction.CompilationType.JAVAC, /* owner= */ ruleContext.getActionOwner(), @@ -267,14 +279,14 @@ public JavaCompileAction build() { /* sourceFiles= */ sourceFiles, /* sourceJars= */ sourceJars, /* plugins= */ plugins), - /* mandatoryInputs= */ mandatoryInputs.build(), + /* mandatoryInputs= */ mandatoryInputs, /* transitiveInputs= */ classpathEntries, /* directJars= */ directJars, /* outputs= */ allOutputs(), /* executionInfo= */ executionInfo, /* extraActionInfoSupplier= */ extraActionInfoSupplier, - /* executableLine= */ executableLine, - /* flagLine= */ buildParamFileContents(internedJcopts), + /* executableLine= */ executableLine.build(), + /* flagLine= */ buildParamFileContents(internedJcopts, stripOutputPaths), /* configuration= */ ruleContext.getConfiguration(), /* dependencyArtifacts= */ compileTimeDependencyArtifacts, /* outputDepsProto= */ outputs.depsProto(), @@ -292,9 +304,13 @@ private ImmutableSet allOutputs() { return result.build(); } - private CustomCommandLine buildParamFileContents(ImmutableList javacOpts) { + private CustomCommandLine buildParamFileContents( + ImmutableList javacOpts, boolean stripOutputPaths) { CustomCommandLine.Builder result = CustomCommandLine.builder(); + if (stripOutputPaths) { + result.stripOutputPaths(JavaCompilationHelper.outputBase(outputs.output())); + } result.addExecPath("--output", outputs.output()); result.addExecPath("--native_header_output", outputs.nativeHeader()); @@ -317,7 +333,21 @@ private CustomCommandLine buildParamFileContents(ImmutableList javacOpts result.addExecPaths("--source_jars", ImmutableList.copyOf(sourceJars)); result.addExecPaths("--sources", sourceFiles); if (!javacOpts.isEmpty()) { - result.addAll("--javacopts", javacOpts); + if (stripOutputPaths) { + // Use textual search/replace to remove configuration prefixes from javacopts. This should + // ideally be constructed directly from artifact exec paths. In this case, targets with + // make variables like $(execpath ...)" stringify those paths before we get to this class. + // TODO(https://github.com/bazelbuild/bazel/issues/6526): provide direct path stripping + // support for make variable expansion. + PathStripper.StringStripper coptStripper = + new PathStripper.StringStripper( + ruleContext.getConfiguration().getDirectories().getRelativeOutputPath()); + result.addAll( + "--javacopts", + javacOpts.stream().map(coptStripper::strip).collect(Collectors.toList())); + } else { + result.addAll("--javacopts", javacOpts); + } // terminate --javacopts with `--` to support javac flags that start with `--` result.add("--"); } diff --git a/src/main/java/com/google/devtools/build/lib/rules/java/JavaHeaderCompileActionBuilder.java b/src/main/java/com/google/devtools/build/lib/rules/java/JavaHeaderCompileActionBuilder.java index c322839b70f8f2..0838f8667e1c75 100644 --- a/src/main/java/com/google/devtools/build/lib/rules/java/JavaHeaderCompileActionBuilder.java +++ b/src/main/java/com/google/devtools/build/lib/rules/java/JavaHeaderCompileActionBuilder.java @@ -29,7 +29,6 @@ import com.google.devtools.build.lib.actions.ActionEnvironment; import com.google.devtools.build.lib.actions.ActionExecutionContext; import com.google.devtools.build.lib.actions.Artifact; -import com.google.devtools.build.lib.actions.CommandLine; import com.google.devtools.build.lib.actions.CommandLines; import com.google.devtools.build.lib.actions.EmptyRunfilesSupplier; import com.google.devtools.build.lib.actions.ExecutionRequirements; @@ -49,6 +48,7 @@ import com.google.devtools.build.lib.rules.java.JavaPluginInfo.JavaPluginData; import com.google.devtools.build.lib.util.OnDemandString; import com.google.devtools.build.lib.util.Pair; +import com.google.devtools.build.lib.vfs.PathFragment; import com.google.devtools.build.lib.view.proto.Deps; import java.io.IOException; import java.io.Serializable; @@ -315,7 +315,7 @@ public void build(JavaToolchainProvider javaToolchain) throws InterruptedExcepti .filter(x -> x != null) .forEachOrdered(outputs::add); - NestedSetBuilder mandatoryInputs = + NestedSetBuilder mandatoryInputsBuilder = NestedSetBuilder.stableOrder() .addAll(additionalInputs) .addTransitive(bootclasspathEntries) @@ -329,8 +329,8 @@ public void build(JavaToolchainProvider javaToolchain) throws InterruptedExcepti : javaToolchain.getHeaderCompiler(); // The header compiler is either a jar file that needs to be executed using // `java -jar `, or an executable that can be run directly. - CommandLine executableLine = headerCompiler.buildCommandLine(javaToolchain, mandatoryInputs); - + CustomCommandLine.Builder executableLine = + headerCompiler.buildCommandLine(javaToolchain, mandatoryInputsBuilder); CustomCommandLine.Builder commandLine = CustomCommandLine.builder() .addExecPath("--output", outputJar) @@ -375,12 +375,12 @@ public void build(JavaToolchainProvider javaToolchain) throws InterruptedExcepti } else { classpath = classpathEntries; } - mandatoryInputs.addTransitive(classpath); + mandatoryInputsBuilder.addTransitive(classpath); commandLine.addExecPaths("--classpath", classpath); commandLine.add("--reduce_classpath_mode", "NONE"); - NestedSet allInputs = mandatoryInputs.build(); + NestedSet allInputs = mandatoryInputsBuilder.build(); Consumer>> resultConsumer = createResultConsumer( outputDepsProto, @@ -390,6 +390,14 @@ public void build(JavaToolchainProvider javaToolchain) throws InterruptedExcepti // full .jdeps from the .stripped .jdeps produced on the executor. /*insertDependencies=*/ classpathMode == JavaClasspathMode.BAZEL); + boolean stripOutputPaths = + JavaCompilationHelper.stripOutputPaths(allInputs, ruleContext.getConfiguration()); + if (stripOutputPaths) { + PathFragment outputBase = JavaCompilationHelper.outputBase(outputJar); + commandLine.stripOutputPaths(outputBase); + executableLine.stripOutputPaths(outputBase); + } + ruleContext.registerAction( new SpawnAction( /* owner= */ ruleContext.getActionOwner(), @@ -399,7 +407,7 @@ public void build(JavaToolchainProvider javaToolchain) throws InterruptedExcepti /* primaryOutput= */ outputJar, /* resourceSetOrBuilder= */ AbstractAction.DEFAULT_RESOURCE_SET, /* commandLines= */ CommandLines.builder() - .addCommandLine(executableLine) + .addCommandLine(executableLine.build()) .addCommandLine(commandLine.build(), PARAM_FILE_INFO) .build(), /* commandLineLimits= */ ruleContext.getConfiguration().getCommandLineLimits(), @@ -413,7 +421,8 @@ public void build(JavaToolchainProvider javaToolchain) throws InterruptedExcepti /* mnemonic= */ "Turbine", /* executeUnconditionally= */ false, /* extraActionInfoSupplier= */ null, - /* resultConsumer= */ resultConsumer)); + /* resultConsumer= */ resultConsumer, + /*stripOutputPaths= */ stripOutputPaths)); return; } @@ -422,8 +431,8 @@ public void build(JavaToolchainProvider javaToolchain) throws InterruptedExcepti // annotation processing. if (!useHeaderCompilerDirect) { - mandatoryInputs.addTransitive(plugins.processorClasspath()); - mandatoryInputs.addTransitive(plugins.data()); + mandatoryInputsBuilder.addTransitive(plugins.processorClasspath()); + mandatoryInputsBuilder.addTransitive(plugins.data()); } commandLine.addAll( @@ -439,6 +448,21 @@ public void build(JavaToolchainProvider javaToolchain) throws InterruptedExcepti commandLine.addExecPaths("--direct_dependencies", directJars); } + NestedSet mandatoryInputs = mandatoryInputsBuilder.build(); + boolean stripOutputPaths = + JavaCompilationHelper.stripOutputPaths( + NestedSetBuilder.stableOrder() + .addTransitive(mandatoryInputs) + .addTransitive(classpathEntries) + .addTransitive(compileTimeDependencyArtifacts) + .build(), + ruleContext.getConfiguration()); + if (stripOutputPaths) { + PathFragment outputBase = JavaCompilationHelper.outputBase(outputJar); + commandLine.stripOutputPaths(outputBase); + executableLine.stripOutputPaths(outputBase); + } + ruleContext.registerAction( new JavaCompileAction( /* compilationType= */ JavaCompileAction.CompilationType.TURBINE, @@ -447,13 +471,13 @@ public void build(JavaToolchainProvider javaToolchain) throws InterruptedExcepti /* tools= */ toolsJars, /* runfilesSupplier= */ EmptyRunfilesSupplier.INSTANCE, /* progressMessage= */ progressMessage, - /* mandatoryInputs= */ mandatoryInputs.build(), + /* mandatoryInputs= */ mandatoryInputs, /* transitiveInputs= */ classpathEntries, /* directJars= */ directJars, /* outputs= */ outputs.build(), /* executionInfo= */ executionInfo, /* extraActionInfoSupplier= */ null, - /* executableLine= */ executableLine, + /* executableLine= */ executableLine.build(), /* flagLine= */ commandLine.build(), /* configuration= */ ruleContext.getConfiguration(), /* dependencyArtifacts= */ compileTimeDependencyArtifacts, diff --git a/src/main/java/com/google/devtools/build/lib/rules/java/JavaToolchainTool.java b/src/main/java/com/google/devtools/build/lib/rules/java/JavaToolchainTool.java index 405fe58f89a269..48353556053a80 100644 --- a/src/main/java/com/google/devtools/build/lib/rules/java/JavaToolchainTool.java +++ b/src/main/java/com/google/devtools/build/lib/rules/java/JavaToolchainTool.java @@ -20,7 +20,6 @@ import com.google.common.collect.ImmutableCollection; import com.google.common.collect.ImmutableMap; import com.google.devtools.build.lib.actions.Artifact; -import com.google.devtools.build.lib.actions.CommandLine; import com.google.devtools.build.lib.analysis.AliasProvider; import com.google.devtools.build.lib.analysis.FileProvider; import com.google.devtools.build.lib.analysis.FilesToRunProvider; @@ -127,9 +126,10 @@ void buildCommandLine( * also {@link #buildCommandLine(CustomCommandLine.Builder, JavaToolchainProvider, * NestedSetBuilder)}. */ - CommandLine buildCommandLine(JavaToolchainProvider toolchain, NestedSetBuilder inputs) { + CustomCommandLine.Builder buildCommandLine( + JavaToolchainProvider toolchain, NestedSetBuilder inputs) { CustomCommandLine.Builder command = CustomCommandLine.builder(); buildCommandLine(command, toolchain, inputs); - return command.build(); + return command; } } diff --git a/src/test/java/com/google/devtools/build/lib/actions/CommandLinesTest.java b/src/test/java/com/google/devtools/build/lib/actions/CommandLinesTest.java index ca9f6539aebb59..c5a517b384b1d3 100644 --- a/src/test/java/com/google/devtools/build/lib/actions/CommandLinesTest.java +++ b/src/test/java/com/google/devtools/build/lib/actions/CommandLinesTest.java @@ -21,6 +21,7 @@ import com.google.devtools.build.lib.actions.CommandLines.ExpandedCommandLines; import com.google.devtools.build.lib.actions.ParameterFile.ParameterFileType; import com.google.devtools.build.lib.vfs.PathFragment; +import java.util.function.Function; import org.junit.Test; import org.junit.runner.RunWith; import org.junit.runners.JUnit4; @@ -39,7 +40,8 @@ public void testSimpleCommandLine() throws Exception { CommandLines.builder() .addCommandLine(CommandLine.of(ImmutableList.of("--foo", "--bar"))) .build(); - ExpandedCommandLines expanded = commandLines.expand(artifactExpander, execPath, NO_LIMIT, 0); + ExpandedCommandLines expanded = + commandLines.expand(artifactExpander, execPath, NO_LIMIT, Function.identity(), 0); assertThat(commandLines.allArguments()).containsExactly("--foo", "--bar"); assertThat(expanded.arguments()).containsExactly("--foo", "--bar"); assertThat(expanded.getParamFiles()).isEmpty(); @@ -48,7 +50,8 @@ public void testSimpleCommandLine() throws Exception { @Test public void testFromArguments() throws Exception { CommandLines commandLines = CommandLines.of(ImmutableList.of("--foo", "--bar")); - ExpandedCommandLines expanded = commandLines.expand(artifactExpander, execPath, NO_LIMIT, 0); + ExpandedCommandLines expanded = + commandLines.expand(artifactExpander, execPath, NO_LIMIT, Function.identity(), 0); assertThat(commandLines.allArguments()).containsExactly("--foo", "--bar"); assertThat(expanded.arguments()).containsExactly("--foo", "--bar"); assertThat(expanded.getParamFiles()).isEmpty(); @@ -60,7 +63,8 @@ public void testConcat() throws Exception { CommandLines.concat( CommandLine.of(ImmutableList.of("--before")), CommandLines.of(ImmutableList.of("--foo", "--bar"))); - ExpandedCommandLines expanded = commandLines.expand(artifactExpander, execPath, NO_LIMIT, 0); + ExpandedCommandLines expanded = + commandLines.expand(artifactExpander, execPath, NO_LIMIT, Function.identity(), 0); assertThat(commandLines.allArguments()).containsExactly("--before", "--foo", "--bar"); assertThat(expanded.arguments()).containsExactly("--before", "--foo", "--bar"); assertThat(expanded.getParamFiles()).isEmpty(); @@ -74,7 +78,8 @@ public void testSimpleParamFileUseAlways() throws Exception { CommandLine.of(ImmutableList.of("--foo", "--bar")), ParamFileInfo.builder(ParameterFileType.UNQUOTED).setUseAlways(true).build()) .build(); - ExpandedCommandLines expanded = commandLines.expand(artifactExpander, execPath, NO_LIMIT, 0); + ExpandedCommandLines expanded = + commandLines.expand(artifactExpander, execPath, NO_LIMIT, Function.identity(), 0); assertThat(commandLines.allArguments()).containsExactly("--foo", "--bar"); assertThat(expanded.arguments()).containsExactly("@output.txt-0.params"); assertThat(expanded.getParamFiles()).hasSize(1); @@ -90,12 +95,15 @@ public void testMaybeUseParamsFiles() throws Exception { ParamFileInfo.builder(ParameterFileType.UNQUOTED).setUseAlways(false).build()) .build(); // Set max length to longer than command line, no param file needed - ExpandedCommandLines expanded = commandLines.expand(artifactExpander, execPath, NO_LIMIT, 0); + ExpandedCommandLines expanded = + commandLines.expand(artifactExpander, execPath, NO_LIMIT, Function.identity(), 0); assertThat(expanded.arguments()).containsExactly("--foo", "--bar"); assertThat(expanded.getParamFiles()).isEmpty(); // Set max length to 0, spill to param file is forced - expanded = commandLines.expand(artifactExpander, execPath, new CommandLineLimits(0), 0); + expanded = + commandLines.expand( + artifactExpander, execPath, new CommandLineLimits(0), Function.identity(), 0); assertThat(expanded.arguments()).containsExactly("@output.txt-0.params"); assertThat(expanded.getParamFiles()).hasSize(1); assertThat(expanded.getParamFiles().get(0).arguments).containsExactly("--foo", "--bar"); @@ -114,7 +122,8 @@ public void testMixOfCommandLinesAndParamFiles() throws Exception { CommandLine.of(ImmutableList.of("g", "h")), ParamFileInfo.builder(ParameterFileType.UNQUOTED).setUseAlways(true).build()) .build(); - ExpandedCommandLines expanded = commandLines.expand(artifactExpander, execPath, NO_LIMIT, 0); + ExpandedCommandLines expanded = + commandLines.expand(artifactExpander, execPath, NO_LIMIT, Function.identity(), 0); assertThat(commandLines.allArguments()).containsExactly("a", "b", "c", "d", "e", "f", "g", "h"); assertThat(expanded.arguments()) .containsExactly("a", "b", "@output.txt-0.params", "e", "f", "@output.txt-1.params"); @@ -139,7 +148,8 @@ public void testFirstParamFilePassesButSecondFailsLengthTest() throws Exception ParamFileInfo.builder(ParameterFileType.UNQUOTED).setUseAlways(false).build()) .build(); ExpandedCommandLines expanded = - commandLines.expand(artifactExpander, execPath, new CommandLineLimits(4), 0); + commandLines.expand( + artifactExpander, execPath, new CommandLineLimits(4), Function.identity(), 0); assertThat(commandLines.allArguments()).containsExactly("a", "b", "c", "d"); assertThat(expanded.arguments()).containsExactly("a", "b", "@output.txt-0.params"); assertThat(expanded.getParamFiles()).hasSize(1); @@ -159,7 +169,8 @@ public void flagsOnly() throws Exception { .build()) .build(); ExpandedCommandLines expanded = - commandLines.expand(artifactExpander, execPath, new CommandLineLimits(4), 0); + commandLines.expand( + artifactExpander, execPath, new CommandLineLimits(4), Function.identity(), 0); assertThat(commandLines.allArguments()).containsExactly("--a", "1", "--b=c", "2"); assertThat(expanded.arguments()).containsExactly("1", "2", "@output.txt-0.params"); assertThat(expanded.getParamFiles()).hasSize(1);