diff --git a/src/main/java/com/google/devtools/build/lib/actions/ActionCacheChecker.java b/src/main/java/com/google/devtools/build/lib/actions/ActionCacheChecker.java index 7d463eedc904c0..d7e3a634940722 100644 --- a/src/main/java/com/google/devtools/build/lib/actions/ActionCacheChecker.java +++ b/src/main/java/com/google/devtools/build/lib/actions/ActionCacheChecker.java @@ -19,6 +19,7 @@ import com.google.common.collect.ImmutableList; import com.google.common.collect.ImmutableMap; import com.google.common.collect.ImmutableSet; +import com.google.devtools.build.lib.actions.Artifact.ArtifactExpander; import com.google.devtools.build.lib.actions.cache.ActionCache; import com.google.devtools.build.lib.actions.cache.DigestUtils; import com.google.devtools.build.lib.actions.cache.MetadataHandler; @@ -249,6 +250,7 @@ public Token getTokenIfNeedToExecute( Map clientEnv, EventHandler handler, MetadataHandler metadataHandler, + ArtifactExpander artifactExpander, Map remoteDefaultPlatformProperties) { // TODO(bazel-team): (2010) For RunfilesAction/SymlinkAction and similar actions that // produce only symlinks we should not check whether inputs are valid at all - all that matters @@ -292,6 +294,7 @@ public Token getTokenIfNeedToExecute( entry, handler, metadataHandler, + artifactExpander, actionInputs, clientEnv, remoteDefaultPlatformProperties)) { @@ -307,11 +310,12 @@ public Token getTokenIfNeedToExecute( return null; } - protected boolean mustExecute( + private boolean mustExecute( Action action, @Nullable ActionCache.Entry entry, EventHandler handler, MetadataHandler metadataHandler, + ArtifactExpander artifactExpander, NestedSet actionInputs, Map clientEnv, Map remoteDefaultPlatformProperties) { @@ -336,9 +340,7 @@ protected boolean mustExecute( reportChanged(handler, action); actionCache.accountMiss(MissReason.DIFFERENT_FILES); return true; - } else if (!entry - .getActionKey() - .equals(action.getKey(actionKeyContext, /*artifactExpander=*/ null))) { + } else if (!entry.getActionKey().equals(action.getKey(actionKeyContext, artifactExpander))) { reportCommand(handler, action); actionCache.accountMiss(MissReason.DIFFERENT_ACTION_KEY); return true; @@ -382,6 +384,7 @@ public void updateActionCache( Action action, Token token, MetadataHandler metadataHandler, + ArtifactExpander artifactExpander, Map clientEnv, Map remoteDefaultPlatformProperties) throws IOException { @@ -397,7 +400,7 @@ public void updateActionCache( computeUsedEnv(action, clientEnv, remoteDefaultPlatformProperties); ActionCache.Entry entry = new ActionCache.Entry( - action.getKey(actionKeyContext, /*artifactExpander=*/ null), + action.getKey(actionKeyContext, artifactExpander), usedEnvironment, action.discoversInputs()); for (Artifact output : action.getOutputs()) { diff --git a/src/main/java/com/google/devtools/build/lib/actions/ActionKeyCacher.java b/src/main/java/com/google/devtools/build/lib/actions/ActionKeyCacher.java index b095660928d172..0abcd571a24829 100644 --- a/src/main/java/com/google/devtools/build/lib/actions/ActionKeyCacher.java +++ b/src/main/java/com/google/devtools/build/lib/actions/ActionKeyCacher.java @@ -28,35 +28,44 @@ public abstract class ActionKeyCacher implements ActionAnalysisMetadata { @Override public final String getKey( ActionKeyContext actionKeyContext, @Nullable ArtifactExpander artifactExpander) { + // Only cache the key when it is given all necessary information to compute a correct key. + // Practically, most of the benefit of the cache comes from execution, which does provide the + // artifactExpander. + if (artifactExpander == null) { + return computeActionKey(actionKeyContext, null); + } + if (cachedKey == null) { synchronized (this) { if (cachedKey == null) { - try { - Fingerprint fp = new Fingerprint(); - // TODO(b/153904017): Make use of the provided artifactExpander and only cache if - // present. - computeKey(actionKeyContext, /*artifactExpander=*/ null, fp); - - // Add a bool indicating whether the execution platform was set. - fp.addBoolean(getExecutionPlatform() != null); - if (getExecutionPlatform() != null) { - // Add the execution platform information. - getExecutionPlatform().addTo(fp); - } - - fp.addStringMap(getExecProperties()); - - // Compute the actual key and store it. - cachedKey = fp.hexDigestAndReset(); - } catch (CommandLineExpansionException e) { - cachedKey = KEY_ERROR; - } + cachedKey = computeActionKey(actionKeyContext, artifactExpander); } } } return cachedKey; } + private String computeActionKey( + ActionKeyContext actionKeyContext, @Nullable ArtifactExpander artifactExpander) { + try { + Fingerprint fp = new Fingerprint(); + computeKey(actionKeyContext, artifactExpander, fp); + + // Add a bool indicating whether the execution platform was set. + fp.addBoolean(getExecutionPlatform() != null); + if (getExecutionPlatform() != null) { + // Add the execution platform information. + getExecutionPlatform().addTo(fp); + } + + fp.addStringMap(getExecProperties()); + // Compute the actual key and store it. + return fp.hexDigestAndReset(); + } catch (CommandLineExpansionException e) { + return KEY_ERROR; + } + } + /** * See the javadoc for {@link Action} and {@link ActionAnalysisMetadata#getKey} for the contract * of this method. diff --git a/src/main/java/com/google/devtools/build/lib/actions/CommandLine.java b/src/main/java/com/google/devtools/build/lib/actions/CommandLine.java index 1516fb01b73cff..e369905ed156ee 100644 --- a/src/main/java/com/google/devtools/build/lib/actions/CommandLine.java +++ b/src/main/java/com/google/devtools/build/lib/actions/CommandLine.java @@ -22,6 +22,7 @@ import com.google.devtools.build.lib.skyframe.serialization.autocodec.AutoCodec; import com.google.devtools.build.lib.skyframe.serialization.autocodec.AutoCodec.VisibleForSerialization; import com.google.devtools.build.lib.util.Fingerprint; +import javax.annotation.Nullable; /** A representation of a list of arguments. */ public abstract class CommandLine { @@ -52,7 +53,18 @@ public Iterable arguments(ArtifactExpander artifactExpander) return arguments(); } - public void addToFingerprint(ActionKeyContext actionKeyContext, Fingerprint fingerprint) + /** + * Adds the command line to the provided {@link Fingerprint}. + * + *

Some of the implementations may require the to expand provided directory in order to produce + * a unique key. Consequently, the result of calling this function can be different depending on + * whether the {@link ArtifactExpander} is provided. Moreover, without it, the produced key may + * not always be unique. + */ + public void addToFingerprint( + ActionKeyContext actionKeyContext, + @Nullable ArtifactExpander artifactExpander, + Fingerprint fingerprint) throws CommandLineExpansionException { for (String s : arguments()) { fingerprint.addString(s); 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 b1923b9cca96e7..5db8d35e6c5946 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 @@ -180,19 +180,22 @@ ExpandedCommandLines expand( return new ExpandedCommandLines(arguments.build(), paramFiles); } - public void addToFingerprint(ActionKeyContext actionKeyContext, Fingerprint fingerprint) + public void addToFingerprint( + ActionKeyContext actionKeyContext, + @Nullable ArtifactExpander artifactExpander, + Fingerprint fingerprint) throws CommandLineExpansionException { // Optimize for simple case of single command line if (commandLines instanceof CommandLine) { CommandLine commandLine = (CommandLine) commandLines; - commandLine.addToFingerprint(actionKeyContext, fingerprint); + commandLine.addToFingerprint(actionKeyContext, artifactExpander, fingerprint); return; } List commandLines = getCommandLines(); for (CommandLineAndParamFileInfo pair : commandLines) { CommandLine commandLine = pair.commandLine; ParamFileInfo paramFileInfo = pair.paramFileInfo; - commandLine.addToFingerprint(actionKeyContext, fingerprint); + commandLine.addToFingerprint(actionKeyContext, artifactExpander, fingerprint); if (paramFileInfo != null) { addParamFileInfoToFingerprint(paramFileInfo, fingerprint); } 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 c0f36de199e617..293fa65ace958b 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 @@ -1288,7 +1288,10 @@ private Object substituteTreeFileArtifactArgvFragment(Object arg) { @Override @SuppressWarnings("unchecked") - public void addToFingerprint(ActionKeyContext actionKeyContext, Fingerprint fingerprint) { + public void addToFingerprint( + ActionKeyContext actionKeyContext, + @Nullable ArtifactExpander artifactExpander, + Fingerprint fingerprint) { int count = arguments.size(); for (int i = 0; i < count; ) { Object arg = arguments.get(i++); 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 226eaf4dfbc576..76a2319743261a 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 @@ -182,6 +182,6 @@ protected void computeKey( fp.addString(GUID); fp.addString(String.valueOf(makeExecutable)); fp.addString(type.toString()); - commandLine.addToFingerprint(actionKeyContext, fp); + commandLine.addToFingerprint(actionKeyContext, artifactExpander, fp); } } 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 904be146070dfa..e426d7ae917788 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 @@ -441,7 +441,7 @@ protected void computeKey( Fingerprint fp) throws CommandLineExpansionException { fp.addString(GUID); - commandLines.addToFingerprint(actionKeyContext, fp); + commandLines.addToFingerprint(actionKeyContext, artifactExpander, fp); fp.addString(getMnemonic()); // We don't need the toolManifests here, because they are a subset of the inputManifests by // definition and the output of an action shouldn't change whether something is considered a 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 ba10d0e5cd0aad..1c37360c4fe4eb 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 @@ -14,6 +14,7 @@ package com.google.devtools.build.lib.analysis.starlark; import com.google.common.base.Joiner; +import com.google.common.base.Preconditions; import com.google.common.collect.ImmutableList; import com.google.common.collect.ImmutableMap; import com.google.common.collect.Interner; @@ -189,12 +190,7 @@ private int eval( originalValues = arguments.subList(argi, argi + count); argi += count; } - List expandedValues = originalValues; - if (artifactExpander != null && (features & EXPAND_DIRECTORIES) != 0) { - if (hasDirectory(originalValues)) { - expandedValues = expandDirectories(artifactExpander, originalValues); - } - } + List expandedValues = maybeExpandDirectories(artifactExpander, originalValues); List stringValues; if (mapEach != null) { stringValues = new ArrayList<>(expandedValues.size()); @@ -278,6 +274,25 @@ private int eval( return argi; } + /** + * Expands the directories if {@code expand_directories} feature is enabled and a + * ArtifactExpander is available. + * + *

Technically, we should always expand the directories if the feature is requested, however + * we cannot do that in the absence of the {@link ArtifactExpander}. + */ + private List maybeExpandDirectories( + @Nullable ArtifactExpander artifactExpander, List originalValues) + throws CommandLineExpansionException { + if ((features & EXPAND_DIRECTORIES) == 0 + || artifactExpander == null + || !hasDirectory(originalValues)) { + return originalValues; + } + + return expandDirectories(artifactExpander, originalValues); + } + private static boolean hasDirectory(List originalValues) { int n = originalValues.size(); for (int i = 0; i < n; ++i) { @@ -336,7 +351,8 @@ private int addToFingerprint( int argi, ActionKeyContext actionKeyContext, Fingerprint fingerprint, - StarlarkSemantics starlarkSemantics) + StarlarkSemantics starlarkSemantics, + @Nullable ArtifactExpander artifactExpander) throws CommandLineExpansionException { final Location location = ((features & HAS_LOCATION) != 0) ? (Location) arguments.get(argi++) : null; @@ -345,36 +361,52 @@ private int addToFingerprint( if ((features & IS_NESTED_SET) != 0) { NestedSet values = (NestedSet) arguments.get(argi++); if (mapEach != null) { - CommandLineItem.MapFn commandLineItemMapFn = - new CommandLineItemMapEachAdaptor(mapEach, location, starlarkSemantics); + CommandLineItemMapEachAdaptor commandLineItemMapFn = + new CommandLineItemMapEachAdaptor( + mapEach, + location, + starlarkSemantics, + (features & EXPAND_DIRECTORIES) != 0 ? artifactExpander : null); try { actionKeyContext.addNestedSetToFingerprint(commandLineItemMapFn, fingerprint, values); } catch (UncheckedCommandLineExpansionException e) { // We wrap the CommandLineExpansionException below, unwrap here throw e.cause; + } finally { + // The cache holds an entry for a NestedSet for every (map_fn, hasArtifactExpanderBit). + // Clearing the artifactExpander itself saves us from storing the contents of it in the + // cache keys (it is no longer needed after we evaluate the value). + // NestedSet cache is cleared after every build, which means that the artifactExpander + // for a given action, if present, cannot change within the lifetime of the fingerprint + // cache (we call getKey with artifactExpander to check action key, when we are ready to + // execute the action in case of a cache miss). + commandLineItemMapFn.clearArtifactExpander(); } } else { actionKeyContext.addNestedSetToFingerprint(fingerprint, values); } } else { int count = (Integer) arguments.get(argi++); - final List originalValues = arguments.subList(argi, argi + count); + List maybeExpandedValues = + maybeExpandDirectories(artifactExpander, arguments.subList(argi, argi + count)); argi += count; if (mapEach != null) { - List stringValues = new ArrayList<>(count); + // TODO(b/160181927): If artifactExpander == null (which happens in the analysis phase) + // but expandDirectories is true, we run the map_each function on directory values without + // actually expanding them. This differs from the real evaluation behavior. This means + // that we can erroneously produce the same digest for two command lines that differ only + // in their directory expansion. Fortunately, this is only a problem for shared action + // conflict checking/aquery result, since at execution time we have an artifactExpander. applyMapEach( mapEach, - originalValues, - stringValues::add, + maybeExpandedValues, + fingerprint::addString, location, - /*artifactExpander=*/ null, + artifactExpander, starlarkSemantics); - for (String s : stringValues) { - fingerprint.addString(s); - } } else { - for (int i = 0; i < count; ++i) { - fingerprint.addString(CommandLineItem.expandToCommandLine(originalValues.get(i))); + for (Object value : maybeExpandedValues) { + fingerprint.addString(CommandLineItem.expandToCommandLine(value)); } } } @@ -659,7 +691,10 @@ public Iterable arguments(@Nullable ArtifactExpander artifactExpander) } @Override - public void addToFingerprint(ActionKeyContext actionKeyContext, Fingerprint fingerprint) + public void addToFingerprint( + ActionKeyContext actionKeyContext, + @Nullable ArtifactExpander artifactExpander, + Fingerprint fingerprint) throws CommandLineExpansionException { for (int argi = 0; argi < arguments.size(); ) { Object arg = arguments.get(argi++); @@ -667,7 +702,12 @@ public void addToFingerprint(ActionKeyContext actionKeyContext, Fingerprint fing argi = ((VectorArg) arg) .addToFingerprint( - arguments, argi, actionKeyContext, fingerprint, starlarkSemantics); + arguments, + argi, + actionKeyContext, + fingerprint, + starlarkSemantics, + artifactExpander); } else if (arg instanceof ScalarArg) { argi = ((ScalarArg) arg).addToFingerprint(arguments, argi, fingerprint); } else { @@ -676,7 +716,7 @@ public void addToFingerprint(ActionKeyContext actionKeyContext, Fingerprint fing } } - /** Used during action key evaluation when we don't have an artifact expander. * */ + /** Used during action key evaluation when we don't have an artifact expander. */ private static class NoopExpander implements DirectoryExpander { @Override public ImmutableList list(FileApi file) { @@ -769,23 +809,37 @@ 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 + * cleared. + */ + private final boolean hasArtifactExpander; + + @Nullable private ArtifactExpander artifactExpander; CommandLineItemMapEachAdaptor( - StarlarkCallable mapFn, Location location, StarlarkSemantics starlarkSemantics) { + StarlarkCallable mapFn, + Location location, + StarlarkSemantics starlarkSemantics, + @Nullable ArtifactExpander artifactExpander) { this.mapFn = mapFn; this.location = location; this.starlarkSemantics = starlarkSemantics; + this.hasArtifactExpander = artifactExpander != null; + this.artifactExpander = artifactExpander; } @Override public void expandToCommandLine(Object object, Consumer args) { + Preconditions.checkState(artifactExpander != null || !hasArtifactExpander); try { applyMapEach( mapFn, - ImmutableList.of(object), + maybeExpandDirectory(object), args, location, - /*artifactExpander=*/ null, + artifactExpander, starlarkSemantics); } catch (CommandLineExpansionException e) { // Rather than update CommandLineItem#expandToCommandLine and the numerous callers, @@ -794,6 +848,14 @@ public void expandToCommandLine(Object object, Consumer args) { } } + private List maybeExpandDirectory(Object object) throws CommandLineExpansionException { + if (artifactExpander == null || !VectorArg.isDirectory(object)) { + return ImmutableList.of(object); + } + + return VectorArg.expandDirectories(artifactExpander, ImmutableList.of(object)); + } + @Override public boolean equals(Object obj) { if (!(obj instanceof CommandLineItemMapEachAdaptor)) { @@ -803,13 +865,18 @@ public boolean equals(Object obj) { // Instance compare intentional // The normal implementation uses location + name of function, // which can conceivably conflict in tests - return mapFn == other.mapFn; + // We only compare presence of artifactExpander vs absence of it since the nestedset + // fingerprint cache is emptied after every build, therefore if the artifact expander is + // provided, it will be the same. + return mapFn == other.mapFn && hasArtifactExpander == other.hasArtifactExpander; } @Override public int hashCode() { - // identity hashcode intentional - return System.identityHashCode(mapFn); + // Force use of identityHashCode, in case the callable uses a custom hash function. (As of + // this writing, only providers seem to have a custom hashCode, and those shouldn't be used + // as map_each functions, but doesn't hurt to be safe...). + return 31 * System.identityHashCode(mapFn) + Boolean.hashCode(hasArtifactExpander); } @Override @@ -818,6 +885,18 @@ public int maxInstancesAllowed() { // always static return Integer.MAX_VALUE; } + + /** + * Clears the artifact expander in order not to prolong the lifetime of it unnecessarily. + * + *

Although this operation technically changes this object, it can be called after we add the + * object to a {@link HashSet}. Clearing the artifactExpander does not affect the result of + * {@link #equals} or {@link #hashCode}. Please note that once we call this function, we can no + * longer call {@link #expandToCommandLine}. + */ + void clearArtifactExpander() { + artifactExpander = null; + } } private static String errorMessage( 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 8bb9d1ad708f80..0ef18a65136cc6 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 @@ -190,8 +190,8 @@ protected void computeKey( throws CommandLineExpansionException { fp.addUUID(GUID); fp.addInt(classpathMode.ordinal()); - executableLine.addToFingerprint(actionKeyContext, fp); - flagLine.addToFingerprint(actionKeyContext, fp); + executableLine.addToFingerprint(actionKeyContext, artifactExpander, fp); + flagLine.addToFingerprint(actionKeyContext, artifactExpander, fp); // As the classpath is no longer part of commandLines implicitly, we need to explicitly add // the transitive inputs to the key here. actionKeyContext.addNestedSetToFingerprint(fp, transitiveInputs); 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 32c0060b5150f8..f859b3d8be4ec6 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 @@ -41,6 +41,7 @@ import com.google.devtools.build.lib.actions.Actions; import com.google.devtools.build.lib.actions.AlreadyReportedActionExecutionException; import com.google.devtools.build.lib.actions.Artifact; +import com.google.devtools.build.lib.actions.Artifact.ArtifactExpander; import com.google.devtools.build.lib.actions.ArtifactPathResolver; import com.google.devtools.build.lib.actions.DiscoveredInputsEvent; import com.google.devtools.build.lib.actions.FileArtifactValue; @@ -732,15 +733,12 @@ private ActionExecutionValue checkCacheAndExecuteIfNeeded( env.getListener(), state.discoveredInputs != null, action, actionLookupData)); } - ImmutableMap> expandedFilesets; - if (state.topLevelFilesets == null || state.topLevelFilesets.isEmpty()) { - expandedFilesets = state.filesetsInsideRunfiles; - } else { - Map> filesetsMap = - new HashMap<>(state.filesetsInsideRunfiles); - filesetsMap.putAll(state.topLevelFilesets); - expandedFilesets = ImmutableMap.copyOf(filesetsMap); - } + ImmutableMap> expandedFilesets = + state.getExpandedFilesets(); + + ArtifactExpander artifactExpander = + new Artifact.ArtifactExpanderImpl( + Collections.unmodifiableMap(state.expandedArtifacts), expandedFilesets); ArtifactPathResolver pathResolver = ArtifactPathResolver.createPathResolver( @@ -762,6 +760,7 @@ private ActionExecutionValue checkCacheAndExecuteIfNeeded( env.getListener(), action, metadataHandler, + artifactExpander, actionStartTime, state.allInputs.actionCacheInputs, clientEnv, @@ -842,7 +841,7 @@ private ActionExecutionValue checkCacheAndExecuteIfNeeded( metadataHandler, actionStartTime, actionLookupData, - Collections.unmodifiableMap(state.expandedArtifacts), + artifactExpander, expandedFilesets, state.topLevelFilesets, state.actionFileSystem, @@ -866,6 +865,11 @@ public void run( ActionMetadataHandler metadataHandler, Map clientEnv) throws InterruptedException, ActionExecutionException { + // TODO(b/160603797): For the sake of action key computation, we should not need + // state.filesetsInsideRunfiles. In fact, for the metadataHandler, we are guaranteed to not + // expand any filesets since we request metadata for input/output Artifacts only. + ImmutableMap> expandedFilesets = + state.getExpandedFilesets(); if (action.discoversInputs()) { state.discoveredInputs = action.getInputs(); switch (addDiscoveredInputs( @@ -887,7 +891,15 @@ public void run( } } Preconditions.checkState(!env.valuesMissing(), action); - skyframeActionExecutor.updateActionCache(action, metadataHandler, state.token, clientEnv); + skyframeActionExecutor.updateActionCache( + action, + metadataHandler, + new Artifact.ArtifactExpanderImpl( + // Skipping the filesets in runfiles since those cannot participate in command line + // creation. + Collections.unmodifiableMap(state.expandedArtifacts), expandedFilesets), + state.token, + clientEnv); } } @@ -1436,6 +1448,18 @@ Iterable filterKnownDiscoveredInputs() { discoveredInputs.toList(), input -> inputArtifactData.getMetadata(input) == null); } + ImmutableMap> getExpandedFilesets() { + if (topLevelFilesets == null || topLevelFilesets.isEmpty()) { + return filesetsInsideRunfiles; + } + + Map> filesetsMap = + Maps.newHashMapWithExpectedSize(filesetsInsideRunfiles.size() + topLevelFilesets.size()); + filesetsMap.putAll(filesetsInsideRunfiles); + filesetsMap.putAll(topLevelFilesets); + return ImmutableMap.copyOf(filesetsMap); + } + @Override public String toString() { return MoreObjects.toStringHelper(this) diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/SkyframeActionExecutor.java b/src/main/java/com/google/devtools/build/lib/skyframe/SkyframeActionExecutor.java index d16e77355888fb..542a833bac78ae 100644 --- a/src/main/java/com/google/devtools/build/lib/skyframe/SkyframeActionExecutor.java +++ b/src/main/java/com/google/devtools/build/lib/skyframe/SkyframeActionExecutor.java @@ -52,7 +52,7 @@ import com.google.devtools.build.lib.actions.Actions; import com.google.devtools.build.lib.actions.AlreadyReportedActionExecutionException; import com.google.devtools.build.lib.actions.Artifact; -import com.google.devtools.build.lib.actions.Artifact.ArtifactExpanderImpl; +import com.google.devtools.build.lib.actions.Artifact.ArtifactExpander; import com.google.devtools.build.lib.actions.Artifact.OwnerlessArtifactWrapper; import com.google.devtools.build.lib.actions.Artifact.TreeFileArtifact; import com.google.devtools.build.lib.actions.ArtifactPathResolver; @@ -111,7 +111,6 @@ import java.io.Closeable; import java.io.FileNotFoundException; import java.io.IOException; -import java.util.Collection; import java.util.Collections; import java.util.HashSet; import java.util.List; @@ -394,7 +393,7 @@ ActionExecutionValue executeAction( ActionMetadataHandler metadataHandler, long actionStartTime, ActionLookupData actionLookupData, - Map> expandedInputs, + ArtifactExpander artifactExpander, ImmutableMap> expandedFilesets, ImmutableMap> topLevelFilesets, @Nullable FileSystem actionFileSystem, @@ -416,8 +415,7 @@ ActionExecutionValue executeAction( env, action, metadataHandler, - expandedInputs, - expandedFilesets, + artifactExpander, topLevelFilesets, actionFileSystem, skyframeDepsResult); @@ -485,8 +483,7 @@ private ActionExecutionContext getContext( Environment env, Action action, MetadataHandler metadataHandler, - Map> expandedInputs, - ImmutableMap> expandedFilesets, + ArtifactExpander artifactExpander, ImmutableMap> topLevelFilesets, @Nullable FileSystem actionFileSystem, @Nullable Object skyframeDepsResult) { @@ -495,7 +492,7 @@ private ActionExecutionContext getContext( ArtifactPathResolver.createPathResolver(actionFileSystem, executorEngine.getExecRoot()); FileOutErr fileOutErr; if (replayActionOutErr) { - String actionKey = action.getKey(actionKeyContext, /*artifactExpander=*/ null); + String actionKey = action.getKey(actionKeyContext, artifactExpander); fileOutErr = actionLogBufferPathGenerator.persistent(actionKey, artifactPathResolver); try { fileOutErr.getErrorPath().delete(); @@ -520,7 +517,7 @@ private ActionExecutionContext getContext( : selectEventHandler(emitProgressEvents), clientEnv, topLevelFilesets, - new ArtifactExpanderImpl(expandedInputs, expandedFilesets), + artifactExpander, actionFileSystem, skyframeDepsResult, nestedSetExpander); @@ -552,6 +549,7 @@ Token checkActionCache( ExtendedEventHandler eventHandler, Action action, MetadataHandler metadataHandler, + ArtifactExpander artifactExpander, long actionStartTime, List resolvedCacheArtifacts, Map clientEnv, @@ -573,6 +571,7 @@ Token checkActionCache( ? reporter : null, metadataHandler, + artifactExpander, remoteDefaultProperties); } catch (UserExecException e) { throw e.toActionExecutionException(action); @@ -588,7 +587,7 @@ Token checkActionCache( if (replayActionOutErr) { // TODO(ulfjack): This assumes that the stdout/stderr files are unmodified. It would be // better to integrate them with the action cache and rerun the action when they change. - String actionKey = action.getKey(actionKeyContext, /*artifactExpander=*/ null); + String actionKey = action.getKey(actionKeyContext, artifactExpander); FileOutErr fileOutErr = actionLogBufferPathGenerator.persistent(actionKey, pathResolver); // Set the mightHaveOutput bit in FileOutErr. Otherwise hasRecordedOutput() doesn't check if // the file exists and just returns false. @@ -632,7 +631,11 @@ public T getContext(Class type) { } void updateActionCache( - Action action, MetadataHandler metadataHandler, Token token, Map clientEnv) + Action action, + MetadataHandler metadataHandler, + ArtifactExpander artifactExpander, + Token token, + Map clientEnv) throws ActionExecutionException { if (!actionCacheChecker.enabled()) { return; @@ -650,7 +653,7 @@ void updateActionCache( try { actionCacheChecker.updateActionCache( - action, token, metadataHandler, clientEnv, remoteDefaultProperties); + action, token, metadataHandler, artifactExpander, clientEnv, remoteDefaultProperties); } catch (IOException e) { // Skyframe has already done all the filesystem access needed for outputs and swallows // IOExceptions for inputs. So an IOException is impossible here. diff --git a/src/test/java/com/google/devtools/build/lib/actions/ActionCacheCheckerTest.java b/src/test/java/com/google/devtools/build/lib/actions/ActionCacheCheckerTest.java index d4f13e8e76ee09..91d2f9a7eb1c2f 100644 --- a/src/test/java/com/google/devtools/build/lib/actions/ActionCacheCheckerTest.java +++ b/src/test/java/com/google/devtools/build/lib/actions/ActionCacheCheckerTest.java @@ -118,10 +118,17 @@ private void runAction(Action action, Map clientEnv, Map digests = new HashMap<>(); for (CustomCommandLine commandLine : commandLines) { Fingerprint fingerprint = new Fingerprint(); - commandLine.addToFingerprint(actionKeyContext, fingerprint); + commandLine.addToFingerprint(actionKeyContext, /*artifactExpander=*/ null, fingerprint); String digest = fingerprint.hexDigestAndReset(); CustomCommandLine previous = digests.putIfAbsent(digest, commandLine); if (previous != null) { diff --git a/src/test/java/com/google/devtools/build/lib/skylark/BUILD b/src/test/java/com/google/devtools/build/lib/skylark/BUILD index 78aa925e432ef6..b5ceb255bca427 100644 --- a/src/test/java/com/google/devtools/build/lib/skylark/BUILD +++ b/src/test/java/com/google/devtools/build/lib/skylark/BUILD @@ -27,6 +27,7 @@ java_test( "//src/main/java/com/google/devtools/build/lib:runtime", "//src/main/java/com/google/devtools/build/lib:syntax", "//src/main/java/com/google/devtools/build/lib/actions", + "//src/main/java/com/google/devtools/build/lib/actions:action_lookup_key", "//src/main/java/com/google/devtools/build/lib/actions:artifacts", "//src/main/java/com/google/devtools/build/lib/analysis:actions/parameter_file_write_action", "//src/main/java/com/google/devtools/build/lib/analysis:actions/substitution", diff --git a/src/test/java/com/google/devtools/build/lib/skylark/StarlarkRuleImplementationFunctionsTest.java b/src/test/java/com/google/devtools/build/lib/skylark/StarlarkRuleImplementationFunctionsTest.java index ec3ff44e75eae9..ee96a1c538fb87 100644 --- a/src/test/java/com/google/devtools/build/lib/skylark/StarlarkRuleImplementationFunctionsTest.java +++ b/src/test/java/com/google/devtools/build/lib/skylark/StarlarkRuleImplementationFunctionsTest.java @@ -20,15 +20,18 @@ import static org.junit.Assert.fail; import com.google.common.base.Joiner; +import com.google.common.base.Preconditions; import com.google.common.collect.ImmutableList; import com.google.common.collect.ImmutableMap; import com.google.common.collect.Iterables; import com.google.common.collect.Lists; import com.google.devtools.build.lib.actions.Action; import com.google.devtools.build.lib.actions.ActionAnalysisMetadata; -import com.google.devtools.build.lib.actions.ActionKeyContext; +import com.google.devtools.build.lib.actions.ActionLookupKey; 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.ArtifactExpanderImpl; +import com.google.devtools.build.lib.actions.Artifact.DerivedArtifact; import com.google.devtools.build.lib.actions.CommandLine; import com.google.devtools.build.lib.actions.CommandLineExpansionException; import com.google.devtools.build.lib.actions.CompositeRunfilesSupplier; @@ -61,10 +64,10 @@ import com.google.devtools.build.lib.syntax.Starlark; import com.google.devtools.build.lib.syntax.StarlarkList; import com.google.devtools.build.lib.syntax.StarlarkThread; -import com.google.devtools.build.lib.syntax.util.EvaluationTestCase; import com.google.devtools.build.lib.testutil.MoreAsserts; import com.google.devtools.build.lib.util.Fingerprint; import com.google.devtools.build.lib.util.OsUtils; +import com.google.devtools.build.lib.vfs.PathFragment; import java.nio.charset.Charset; import java.nio.charset.StandardCharsets; import java.util.HashMap; @@ -87,7 +90,7 @@ @StarlarkGlobalLibrary // needed for CallUtils.getBuiltinCallable, sadly public class StarlarkRuleImplementationFunctionsTest extends BuildViewTestCase { - private final EvaluationTestCase ev = new BazelEvaluationTestCase(); + private final BazelEvaluationTestCase ev = new BazelEvaluationTestCase(); private StarlarkRuleContext createRuleContext(String label) throws Exception { return new StarlarkRuleContext( @@ -2864,12 +2867,9 @@ public void testStarlarkCustomCommandLineKeyComputation() throws Exception { "args.add_all(values, map_each=_map_each)")); // Ensure all these command lines have distinct keys - ActionKeyContext actionKeyContext = new ActionKeyContext(); Map digests = new HashMap<>(); for (CommandLine commandLine : commandLines.build()) { - Fingerprint fingerprint = new Fingerprint(); - commandLine.addToFingerprint(actionKeyContext, fingerprint); - String digest = fingerprint.hexDigestAndReset(); + String digest = getDigest(commandLine); CommandLine previous = digests.putIfAbsent(digest, commandLine); if (previous != null) { fail( @@ -2890,7 +2890,261 @@ public void testStarlarkCustomCommandLineKeyComputation() throws Exception { "args.add_all(values, map_each=_bad_fn)"); assertThrows( CommandLineExpansionException.class, - () -> commandLine.addToFingerprint(actionKeyContext, new Fingerprint())); + () -> + commandLine.addToFingerprint( + actionKeyContext, /*artifactExpander=*/ null, new Fingerprint())); + } + + @Test + public void skylarkCustomCommandLineKeyComputation_differentMapEach() throws Exception { + setRuleContext(createRuleContext("//foo:foo")); + + CommandLine commandLine1 = + getCommandLine( + "args = ruleContext.actions.args()", + "def _fun1(arg): return 'val1'", + "def _fun2(arg): return 'val2'", + "args.add_all(['a'], map_each=_fun1)"); + CommandLine commandLine2 = + getCommandLine( + "args = ruleContext.actions.args()", + "def _fun1(arg): return 'val1'", + "def _fun2(arg): return 'val2'", + "args.add_all(['a'], map_each=_fun2)"); + + assertThat(getDigest(commandLine1)).isNotEqualTo(getDigest(commandLine2)); + } + + @Test + public void skylarkCustomCommandLineKeyComputation_differentArg() throws Exception { + setRuleContext(createRuleContext("//foo:foo")); + + CommandLine commandLine1 = + getCommandLine( + "args = ruleContext.actions.args()", + "def _fun(arg): return arg", + "args.add_all(['a'], map_each=_fun)"); + CommandLine commandLine2 = + getCommandLine( + "args = ruleContext.actions.args()", + "def _fun(arg): return arg", + "args.add_all(['b'], map_each=_fun)"); + + assertThat(getDigest(commandLine1)).isNotEqualTo(getDigest(commandLine2)); + } + + @Test + public void skylarkCustomCommandLineKeyComputationWithExpander_equivalentMapEach_sameKey() + throws Exception { + setRuleContext(createRuleContext("//foo:foo")); + + CommandLine commandLine1 = + getCommandLine( + "args = ruleContext.actions.args()", + "directory = ruleContext.actions.declare_directory('dir')", + "args.add_joined([directory], join_with=',', map_each=str, expand_directories=True)"); + CommandLine commandLine2 = + getCommandLine( + "args = ruleContext.actions.args()", + "directory = ruleContext.actions.declare_directory('dir')", + "def mystr(file): return str(file)", + "args.add_joined([directory], join_with=',', map_each=mystr, expand_directories=True)"); + + ArtifactExpander expander = createArtifactExpander("foo/dir", "file"); + assertThat(getDigest(commandLine1, expander)).isEqualTo(getDigest(commandLine2, expander)); + } + + @Test + public void skylarkCustomCommandLineKeyComputationWithExpander_mapEachConstantForDir() + throws Exception { + setRuleContext(createRuleContext("//foo:foo")); + + CommandLine commandLine1 = + getCommandLine( + "args = ruleContext.actions.args()", + "directory = ruleContext.actions.declare_directory('dir')", + "def _constant_for_dir(f): return 'constant' if f.path.endswith('dir') else 'value1'", + "args.add_all([directory], map_each=_constant_for_dir, expand_directories=True)"); + CommandLine commandLine2 = + getCommandLine( + "args = ruleContext.actions.args()", + "directory = ruleContext.actions.declare_directory('dir')", + "def _constant_for_dir(f): return 'constant' if f.path.endswith('dir') else 'value2'", + "args.add_all([directory], map_each=_constant_for_dir, expand_directories=True)"); + + ArtifactExpander expander = createArtifactExpander("foo/dir", "file"); + assertThat(getDigest(commandLine1, expander)).isNotEqualTo(getDigest(commandLine2, expander)); + } + + @Test + public void skylarkCustomCommandLineKeyComputationWithExpander_constantForDirWithNestedSet() + throws Exception { + setRuleContext(createRuleContext("//foo:foo")); + + CommandLine commandLine1 = + getCommandLine( + "args = ruleContext.actions.args()", + "dir = ruleContext.actions.declare_directory('dir')", + "def _constant_for_dir(f): return 'constant' if f.path.endswith('dir') else 'value1'", + "args.add_all(depset([dir]), map_each=_constant_for_dir, expand_directories=True)"); + CommandLine commandLine2 = + getCommandLine( + "args = ruleContext.actions.args()", + "dir = ruleContext.actions.declare_directory('dir')", + "def _constant_for_dir(f): return 'constant' if f.path.endswith('dir') else 'value2'", + "args.add_all(depset([dir]), map_each=_constant_for_dir, expand_directories=True)"); + + ArtifactExpander expander = createArtifactExpander("foo/dir", "file"); + assertThat(getDigest(commandLine1, expander)).isNotEqualTo(getDigest(commandLine2, expander)); + } + + @Test + public void skylarkCustomCommandLineKeyComputationWithExpander_mapEachFailsForDir() + throws Exception { + setRuleContext(createRuleContext("//foo:foo")); + + CommandLine commandLine1 = + getCommandLine( + "args = ruleContext.actions.args()", + "directory = ruleContext.actions.declare_directory('dir')", + "ruleContext.actions.run_shell(outputs=[directory], command='')", + "def _fail_for_dir(file):", + " if file.path.endswith('dir'): fail('hello')", + " return 'value1'", + "args.add_all([directory], map_each=_fail_for_dir, expand_directories=True)"); + CommandLine commandLine2 = + getCommandLine( + "args = ruleContext.actions.args()", + "ruleContext.actions.run_shell(outputs=[directory], command='')", + "directory = ruleContext.actions.declare_directory('dir')", + "def _fail_for_dir(file):", + " if file.path.endswith('dir'): fail('hello')", + " return 'value2'", + "args.add_all([directory], map_each=_fail_for_dir, expand_directories=True)"); + + ArtifactExpander expander = createArtifactExpander("foo/dir", "file"); + assertThat(getDigest(commandLine1, expander)).isNotEqualTo(getDigest(commandLine2, expander)); + } + + @Test + public void skylarkCustomCommandLineKeyComputationWithExpander_differentExpansion() + throws Exception { + setRuleContext(createRuleContext("//foo:foo")); + CommandLine commandLine = + getCommandLine( + "args = ruleContext.actions.args()", + "directory = ruleContext.actions.declare_directory('dir')", + "ruleContext.actions.run_shell(outputs=[directory], command='')", + "def _get_path(file): return file.path", + "args.add_all([directory], map_each=_get_path, expand_directories=True)"); + + ArtifactExpander expander1 = createArtifactExpander("foo/dir", "file1"); + ArtifactExpander expander2 = createArtifactExpander("foo/dir", "file2"); + assertThat(getDigest(commandLine, expander1)).isNotEqualTo(getDigest(commandLine, expander2)); + } + + @Test + public void skylarkCustomCommandLineKeyComputationWithExpander_differentExpansionNoMapEach() + throws Exception { + setRuleContext(createRuleContext("//foo:foo")); + CommandLine commandLine = + getCommandLine( + "args = ruleContext.actions.args()", + "directory = ruleContext.actions.declare_directory('dir')", + "args.add_all([directory])"); + + ArtifactExpander expander1 = createArtifactExpander("foo/dir", "file1"); + ArtifactExpander expander2 = createArtifactExpander("foo/dir", "file2"); + assertThat(getDigest(commandLine, expander1)).isNotEqualTo(getDigest(commandLine, expander2)); + } + + @Test + public void skylarkCustomCommandLineKeyComputationWithExpander_extraFileInExpansionNoMapEach() + throws Exception { + setRuleContext(createRuleContext("//foo:foo")); + CommandLine commandLine = + getCommandLine( + "args = ruleContext.actions.args()", + "directory = ruleContext.actions.declare_directory('dir')", + "args.add_all([directory])"); + + ArtifactExpander expander1 = createArtifactExpander("foo/dir", "file1"); + ArtifactExpander expander2 = createArtifactExpander("foo/dir", "file1", "file2"); + assertThat(getDigest(commandLine, expander1)).isNotEqualTo(getDigest(commandLine, expander2)); + } + + @Test + public void skylarkCustomCommandLineKeyComputationWithExpander_constantForDirAddJoined() + throws Exception { + setRuleContext(createRuleContext("//foo:foo")); + + CommandLine commandLine1 = + getCommandLine( + "args = ruleContext.actions.args()", + "directory = ruleContext.actions.declare_directory('dir')", + "def _constant_for_dir(f): return 'constant' if f.path.endswith('dir') else 'value1'", + "args.add_joined([directory], join_with=',', map_each=_constant_for_dir," + + " expand_directories=True)"); + CommandLine commandLine2 = + getCommandLine( + "args = ruleContext.actions.args()", + "directory = ruleContext.actions.declare_directory('dir')", + "def _constant_for_dir(f): return 'constant' if f.path.endswith('dir') else 'value2'", + "args.add_joined([directory], join_with=',', map_each=_constant_for_dir," + + " expand_directories=True)"); + + ArtifactExpander expander = createArtifactExpander("foo/dir", "file"); + assertThat(getDigest(commandLine1, expander)).isNotEqualTo(getDigest(commandLine2, expander)); + } + + @Test + public void skylarkCustomCommandLineKeyComputation_inconsequentialChangeToStarlarkSemantics() + throws Exception { + setRuleContext(createRuleContext("//foo:foo")); + CommandLine commandLine1 = + getCommandLine( + "args = ruleContext.actions.args()", + "directory = ruleContext.actions.declare_directory('dir')", + "def _path(f): return f.path", + "args.add_all([directory], map_each=_path)"); + + ev.setSemantics("--incompatible_run_shell_command_string"); + // setStarlarkSemanticsOptions reinitializes the thread -- set the ruleContext on the new one. + setRuleContext(createRuleContext("//foo:foo")); + + CommandLine commandLine2 = + getCommandLine( + "args = ruleContext.actions.args()", + "directory = ruleContext.actions.declare_directory('dir')", + "def _path(f): return f.path", + "args.add_all([directory], map_each=_path)"); + + assertThat(getDigest(commandLine1)).isEqualTo(getDigest(commandLine2)); + } + + private static ArtifactExpander createArtifactExpander(String dirRelativePath, String... files) { + return (artifact, output) -> { + Preconditions.checkArgument( + artifact.getRootRelativePath().equals(PathFragment.create(dirRelativePath))); + for (String file : files) { + output.add( + new DerivedArtifact( + artifact.getRoot(), + artifact.getExecPath().getRelative(file), + (ActionLookupKey) artifact.getArtifactOwner())); + } + }; + } + + private String getDigest(CommandLine commandLine) throws CommandLineExpansionException { + return getDigest(commandLine, /*artifactExpander=*/ null); + } + + private String getDigest(CommandLine commandLine, ArtifactExpander artifactExpander) + throws CommandLineExpansionException { + Fingerprint fingerprint = new Fingerprint(); + commandLine.addToFingerprint(actionKeyContext, artifactExpander, fingerprint); + return fingerprint.hexDigestAndReset(); } private CommandLine getCommandLine(String... lines) throws Exception {