From b88a5d3b14e0bc2661e9171790259ecc14c8145b Mon Sep 17 00:00:00 2001 From: Fabian Meumertzheim Date: Wed, 10 Dec 2025 08:12:00 -0800 Subject: [PATCH 1/2] [9.0.0] Add support for multiple module interfaces per `cc_library` When multiple `module_interfaces` are specified on a single `cc_library`, the individual compilation actions form a DAG based on `import`s between these modules. Consider the following situation: * `a.cppm` imports `b.cppm`, both of which are in the `module_interfaces` of a single `cc_library`. * Building the target populates the action cache with an entry for `a.pcm` that stores `b.pcm` as a discovered input. * Now edit `a.cppm` and `b.cppm` so that `b.cppm` imports `a.cppm` and `a.cppm` no longer imports `b.cppm`. * Build again (optionally after a shutdown). Before this commit, this resulted in an action cycle since during action cache checking, Bazel would reuse or look up the inputs discovered in the previous build, thus introducing an edge from `a.pcm` to `b.pcm`. Together with the newly discovered edge from `b.pcm` to `a.pcm`, this resulted in a cycle. This is fixed by not requesting the previously discovered inputs (either retained in memory or in the action cache) if the mandatory inputs changed. In the case of C++20 modules, this is sufficient since the modmap file, which lists all transitive `.pcm` files required for compilation, is a mandatory input. As part of this change, `MetadataDigestUtils.fromMetadata` had to be modified to always return a byte array of proper digest length, even if called with an empty map, to match the assumptions of the action cache. This change is pretty much Fabian's PR https://github.com/bazelbuild/bazel/pull/27492 with a tiny fix added on top (not returning from computeMandatoryInputsDigest() early on valuesMissing() if inErrorBubbling() is true) Closes #27492. PiperOrigin-RevId: 842733471 Change-Id: I48fa2c0bceb888dcb58db29d50c30719b2122c5d (cherry picked from commit cb9bd8615210dda2104f79d281938e47187dc2de) --- .../build/lib/actions/ActionCacheChecker.java | 29 +++- .../build/lib/actions/cache/ActionCache.java | 32 +++- .../cache/CompactPersistentActionCache.java | 13 +- .../actions/cache/MetadataDigestUtils.java | 4 +- .../lib/analysis/actions/StarlarkAction.java | 15 +- .../lib/skyframe/ActionExecutionFunction.java | 160 ++++++++++++++---- .../lib/skyframe/SkyframeActionExecutor.java | 11 +- .../lib/actions/ActionCacheCheckerTest.java | 55 +++--- .../CompactPersistentActionCacheTest.java | 8 +- .../google/devtools/build/lib/skyframe/BUILD | 1 + .../skyframe/TreeArtifactMetadataTest.java | 5 +- src/test/shell/bazel/BUILD | 1 + src/test/shell/bazel/cc_integration_test.sh | 153 ++++++++++++++++- 13 files changed, 413 insertions(+), 74 deletions(-) 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 91dfcf1197b723..15911a78e34518 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 @@ -75,7 +75,6 @@ public class ActionCacheChecker { @Nullable private final ActionCache actionCache; // Null when not enabled. - /** Cache config parameters for ActionCacheChecker. */ @AutoValue public abstract static class CacheConfig { @@ -179,6 +178,7 @@ private static TreeArtifactValue getCachedTreeMetadata( * @param actionInputs the action inputs; usually action.getInputs(), but might be a previously * cached set of discovered inputs for actions that discover them. * @param outputMetadataStore metadata provider for action outputs. + * @param mandatoryInputsDigest the digest of mandatory inputs, or null if not discovering inputs. * @param cachedOutputMetadata cached metadata that should be used instead of {@code * outputMetadataStore}. * @param outputChecker used to check whether remote metadata should be trusted. @@ -195,6 +195,7 @@ private static boolean isUpToDate( NestedSet actionInputs, InputMetadataProvider inputMetadataProvider, OutputMetadataStore outputMetadataStore, + @Nullable byte[] mandatoryInputsDigest, @Nullable CachedOutputMetadata cachedOutputMetadata, @Nullable OutputChecker outputChecker, ImmutableMap effectiveEnvironment, @@ -209,7 +210,8 @@ private static boolean isUpToDate( effectiveEnvironment, effectiveExecProperties, outputPermissions, - useArchivedTreeArtifacts); + useArchivedTreeArtifacts, + mandatoryInputsDigest); for (Artifact artifact : action.getOutputs()) { if (artifact.isTreeArtifact()) { @@ -459,6 +461,7 @@ private TreeArtifactValue constructProxyTreeMetadata(SpecialArtifact parent) public Token getTokenIfNeedToExecute( Action action, List resolvedCacheArtifacts, + @Nullable byte[] mandatoryInputsDigest, Map clientEnv, OutputPermissions outputPermissions, EventHandler handler, @@ -515,6 +518,7 @@ public Token getTokenIfNeedToExecute( clientEnv, outputPermissions, remoteDefaultPlatformProperties, + mandatoryInputsDigest, cachedOutputMetadata, outputChecker, useArchivedTreeArtifacts)) { @@ -548,6 +552,7 @@ private boolean mustExecute( Map clientEnv, OutputPermissions outputPermissions, ImmutableMap remoteDefaultPlatformProperties, + @Nullable byte[] mandatoryInputsDigest, @Nullable CachedOutputMetadata cachedOutputMetadata, @Nullable OutputChecker outputChecker, boolean useArchivedTreeArtifacts) @@ -587,6 +592,7 @@ private boolean mustExecute( actionInputs, inputMetadataProvider, outputMetadataStore, + mandatoryInputsDigest, cachedOutputMetadata, outputChecker, effectiveEnvironment, @@ -623,7 +629,7 @@ private static FileArtifactValue getOutputMetadataOrConstant( // to trigger a re-execution, so we should catch the IOException explicitly there. In others, we // should propagate the exception, because it is unexpected (e.g., bad file system state). @Nullable - private static FileArtifactValue getInputMetadataMaybe( + public static FileArtifactValue getInputMetadataMaybe( InputMetadataProvider inputMetadataProvider, Artifact artifact) { try { return getInputMetadataOrConstant(inputMetadataProvider, artifact); @@ -665,7 +671,8 @@ public void updateActionCache( Map clientEnv, OutputPermissions outputPermissions, ImmutableMap remoteDefaultPlatformProperties, - boolean useArchivedTreeArtifacts) + boolean useArchivedTreeArtifacts, + @Nullable byte[] mandatoryInputsDigest) throws IOException, InterruptedException { checkState(cacheConfig.enabled(), "cache unexpectedly disabled, action: %s", action); Preconditions.checkArgument(token != null, "token unexpectedly null, action: %s", action); @@ -694,7 +701,8 @@ public void updateActionCache( effectiveEnvironment, effectiveExecProperties, outputPermissions, - useArchivedTreeArtifacts); + useArchivedTreeArtifacts, + mandatoryInputsDigest); for (Artifact output : action.getOutputs()) { // Remove old records from the cache if they used different key. @@ -737,6 +745,14 @@ public void updateActionCache( actionCache.put(key, builder.build()); } + public boolean mandatoryInputsMatch(Action action, byte[] mandatoryInputsDigest) { + checkArgument(action.discoversInputs()); + ActionCache.Entry entry = getCacheEntry(action); + return entry != null + && !entry.isCorrupted() + && Arrays.equals(entry.getMandatoryInputsDigest(), mandatoryInputsDigest); + } + @Nullable public List getCachedInputs(Action action, PackageRootResolver resolver) throws PackageRootResolver.PackageRootException, InterruptedException { @@ -810,6 +826,7 @@ public List getCachedInputs(Action action, PackageRootResolver resolve public Token getTokenUnconditionallyAfterFailureToRecordActionCacheHit( Action action, List resolvedCacheArtifacts, + @Nullable byte[] mandatoryInputsDigest, Map clientEnv, OutputPermissions outputPermissions, EventHandler handler, @@ -825,6 +842,7 @@ public Token getTokenUnconditionallyAfterFailureToRecordActionCacheHit( return getTokenIfNeedToExecute( action, resolvedCacheArtifacts, + mandatoryInputsDigest, clientEnv, outputPermissions, handler, @@ -887,5 +905,4 @@ private Token(Action action) { this.cacheKey = action.getPrimaryOutput().getExecPathString(); } } - } diff --git a/src/main/java/com/google/devtools/build/lib/actions/cache/ActionCache.java b/src/main/java/com/google/devtools/build/lib/actions/cache/ActionCache.java index 1249c94a8e7e25..6deb3925544190 100644 --- a/src/main/java/com/google/devtools/build/lib/actions/cache/ActionCache.java +++ b/src/main/java/com/google/devtools/build/lib/actions/cache/ActionCache.java @@ -82,12 +82,13 @@ public interface ActionCache { final class Entry { /** Unique instance standing for a corrupted cache entry. */ public static final ActionCache.Entry CORRUPTED = - new Entry(null, null, ImmutableMap.of(), ImmutableMap.of(), ImmutableList.of()); + new Entry(null, null, null, ImmutableMap.of(), ImmutableMap.of(), ImmutableList.of()); // Digest of all relevant properties of the action for cache invalidation purposes. // Null if the entry is corrupted. @Nullable private final byte[] digest; + @Nullable private final byte[] mandatoryInputsDigest; // List of input paths discovered by the action. // Null if the action does not discover inputs. @Nullable private final ImmutableList discoveredInputPaths; @@ -100,11 +101,13 @@ final class Entry { Entry( @Nullable byte[] digest, + @Nullable byte[] mandatoryInputsDigest, @Nullable ImmutableList discoveredInputPaths, ImmutableMap outputFileMetadata, ImmutableMap outputTreeMetadata, ImmutableList proxyOutputs) { this.digest = digest; + this.mandatoryInputsDigest = mandatoryInputsDigest; this.discoveredInputPaths = discoveredInputPaths; this.outputFileMetadata = outputFileMetadata; this.outputTreeMetadata = outputTreeMetadata; @@ -128,7 +131,11 @@ public byte[] getDigest() { /** Returns whether the action discovers inputs. */ public boolean discoversInputs() { checkState(!isCorrupted()); - return discoveredInputPaths != null; + if (discoveredInputPaths == null) { + return false; + } + checkState(mandatoryInputsDigest != null); + return true; } /** @@ -140,6 +147,12 @@ public ImmutableList getDiscoveredInputPaths() { return discoveredInputPaths; } + @Nullable + public byte[] getMandatoryInputsDigest() { + checkState(discoversInputs()); + return mandatoryInputsDigest; + } + /** Gets the metadata of an output file. */ @Nullable public FileArtifactValue getOutputFile(Artifact output) { @@ -187,6 +200,7 @@ public String toString() { return MoreObjects.toStringHelper(this) .add("digest", digest) .add("discoveredInputPaths", discoveredInputPaths) + .add("mandatoryInputsDigest", mandatoryInputsDigest) .add("outputFileMetadata", outputFileMetadata) .add("outputTreeMetadata", outputTreeMetadata) .add("proxyOutputs", proxyOutputs) @@ -205,6 +219,9 @@ void dump(PrintStream out) { out.format(" %s\n", path); } } + if (mandatoryInputsDigest != null) { + out.format(" mandatoryInputsDigest = %s\n", formatDigest(mandatoryInputsDigest)); + } if (!outputFileMetadata.isEmpty()) { out.println(" outputFileMetadata ="); @@ -275,6 +292,7 @@ public static final class Builder { // Discovered inputs. // Null if the action does not discover inputs. @Nullable private final ImmutableList.Builder discoveredInputPaths; + @Nullable private final byte[] mandatoryInputsDigest; private final ImmutableMap.Builder outputFileMetadata = ImmutableMap.builder(); @@ -293,6 +311,8 @@ public static final class Builder { * @param discoversInputs whether the action discovers inputs. * @param outputPermissions the requested output permissions. * @param useArchivedTreeArtifacts whether archived tree artifacts are enabled. + * @param mandatoryInputsDigest the digest of the mandatory inputs, or null if the action + * doesn't discover inputs. */ public Builder( String actionKey, @@ -300,13 +320,18 @@ public Builder( ImmutableMap clientEnv, ImmutableMap execProperties, OutputPermissions outputPermissions, - boolean useArchivedTreeArtifacts) { + boolean useArchivedTreeArtifacts, + @Nullable byte[] mandatoryInputsDigest) { this.actionKey = actionKey; this.clientEnv = clientEnv; this.execProperties = execProperties; this.discoveredInputPaths = discoversInputs ? ImmutableList.builder() : null; this.outputPermissions = outputPermissions; this.useArchivedTreeArtifacts = useArchivedTreeArtifacts; + checkArgument( + (mandatoryInputsDigest != null) == discoversInputs, + "mandatoryInputsDigest must be set iff the action discovers inputs"); + this.mandatoryInputsDigest = mandatoryInputsDigest; } /** Adds metadata of an input file. */ @@ -390,6 +415,7 @@ public Entry build() { execProperties, outputPermissions, useArchivedTreeArtifacts), + mandatoryInputsDigest, discoveredInputPaths != null ? discoveredInputPaths.build() : null, outputFileMetadata.buildOrThrow(), outputTreeMetadata.buildOrThrow(), diff --git a/src/main/java/com/google/devtools/build/lib/actions/cache/CompactPersistentActionCache.java b/src/main/java/com/google/devtools/build/lib/actions/cache/CompactPersistentActionCache.java index 467f991c641833..fe51b7a5c8eb6f 100644 --- a/src/main/java/com/google/devtools/build/lib/actions/cache/CompactPersistentActionCache.java +++ b/src/main/java/com/google/devtools/build/lib/actions/cache/CompactPersistentActionCache.java @@ -75,7 +75,7 @@ public class CompactPersistentActionCache implements ActionCache { // cache records. private static final int VALIDATION_KEY = -10; - private static final int VERSION = 23; + private static final int VERSION = 24; /** * A timestamp, represented as the number of minutes since the Unix epoch. @@ -848,7 +848,8 @@ private byte[] encode(ActionCache.Entry entry) throws IOException { int maxDiscoveredInputsSize = 1; // presence marker if (entry.discoversInputs()) { maxDiscoveredInputsSize += - VarInt.MAX_VARINT_SIZE // length + (1 + DigestUtils.ESTIMATED_SIZE) // mandatoryInputsDigest + + VarInt.MAX_VARINT_SIZE // length + (VarInt.MAX_VARINT_SIZE // execPath * entry.getDiscoveredInputPaths().size()); } @@ -899,6 +900,7 @@ private byte[] encode(ActionCache.Entry entry) throws IOException { VarInt.putVarInt(entry.discoversInputs() ? 1 : 0, sink); if (entry.discoversInputs()) { + MetadataDigestUtils.write(entry.getMandatoryInputsDigest(), sink); ImmutableList discoveredInputPaths = entry.getDiscoveredInputPaths(); VarInt.putVarInt(discoveredInputPaths.size(), sink); for (String discoveredInputPath : discoveredInputPaths) { @@ -974,6 +976,7 @@ private ActionCache.Entry decodeInternal(byte[] data) throws IOException { byte[] digest = MetadataDigestUtils.read(source); + byte[] mandatoryInputsDigest = null; ImmutableList discoveredInputPaths = null; int discoveredInputsPresenceMarker = VarInt.getVarInt(source); if (discoveredInputsPresenceMarker != 0) { @@ -981,6 +984,10 @@ private ActionCache.Entry decodeInternal(byte[] data) throws IOException { throw new IOException( "Invalid presence marker for discovered inputs: " + discoveredInputsPresenceMarker); } + mandatoryInputsDigest = MetadataDigestUtils.read(source); + if (mandatoryInputsDigest.length != digest.length) { + throw new IOException("Corrupted mandatory inputs digest"); + } int numDiscoveredInputs = VarInt.getVarInt(source); if (numDiscoveredInputs < 0) { throw new IOException("Invalid discovered input count: " + numDiscoveredInputs); @@ -1002,6 +1009,7 @@ private ActionCache.Entry decodeInternal(byte[] data) throws IOException { } return new ActionCache.Entry( digest, + mandatoryInputsDigest, discoveredInputPaths, /* outputFileMetadata= */ ImmutableMap.of(), /* outputTreeMetadata= */ ImmutableMap.of(), @@ -1082,6 +1090,7 @@ private ActionCache.Entry decodeInternal(byte[] data) throws IOException { } return new ActionCache.Entry( digest, + mandatoryInputsDigest, discoveredInputPaths, outputFiles.buildOrThrow(), outputTrees.buildOrThrow(), diff --git a/src/main/java/com/google/devtools/build/lib/actions/cache/MetadataDigestUtils.java b/src/main/java/com/google/devtools/build/lib/actions/cache/MetadataDigestUtils.java index 3c55af917c2a95..d41c14179dc87f 100644 --- a/src/main/java/com/google/devtools/build/lib/actions/cache/MetadataDigestUtils.java +++ b/src/main/java/com/google/devtools/build/lib/actions/cache/MetadataDigestUtils.java @@ -28,6 +28,8 @@ public final class MetadataDigestUtils { private MetadataDigestUtils() {} + private static final byte[] emptyDigest = new Fingerprint().digestAndReset(); + /** * @param source the byte buffer source. * @return the digest from the given buffer. @@ -54,7 +56,7 @@ public static void write(byte[] digest, OutputStream sink) throws IOException { * @param mdMap A collection of (execPath, FileArtifactValue) pairs. Values may be null. */ public static byte[] fromMetadata(Map mdMap) { - byte[] result = new byte[1]; // reserve the empty string + byte[] result = emptyDigest.clone(); // Profiling showed that MessageDigest engine instantiation was a hotspot, so create one // instance for this computation to amortize its cost. Fingerprint fp = new Fingerprint(); 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 7fb073bac52411..bd76f400aed11a 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 @@ -345,6 +345,15 @@ public NestedSet getAllowedDerivedInputs() { return getInputs(); } + @Override + public NestedSet getMandatoryInputs() { + if (unusedInputsList.isPresent()) { + return NestedSetBuilder.emptySet(Order.STABLE_ORDER); + } else { + return getInputs(); + } + } + @Nullable @Override public NestedSet discoverInputs(ActionExecutionContext actionExecutionContext) @@ -358,7 +367,11 @@ public NestedSet discoverInputs(ActionExecutionContext actionExecution NestedSet inputFilesForExtraAction = shadowedActionObj.getInputFilesForExtraAction(actionExecutionContext); if (inputFilesForExtraAction == null) { - return null; + if (unusedInputsList.isPresent()) { + return allStarlarkActionInputs; + } else { + return null; + } } updateInputs( createInputs( 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 52e537bcf44b2c..3409ea4ffa5b48 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 @@ -33,6 +33,7 @@ import com.google.common.collect.SetMultimap; import com.google.common.flogger.GoogleLogger; import com.google.devtools.build.lib.actions.Action; +import com.google.devtools.build.lib.actions.ActionCacheChecker; import com.google.devtools.build.lib.actions.ActionCacheChecker.Token; import com.google.devtools.build.lib.actions.ActionCompletionEvent; import com.google.devtools.build.lib.actions.ActionExecutedEvent.ErrorTiming; @@ -52,6 +53,7 @@ import com.google.devtools.build.lib.actions.RichArtifactData; import com.google.devtools.build.lib.actions.RichDataProducingAction; import com.google.devtools.build.lib.actions.SpawnMetrics; +import com.google.devtools.build.lib.actions.cache.MetadataDigestUtils; import com.google.devtools.build.lib.actions.cache.OutputMetadataStore; import com.google.devtools.build.lib.analysis.BlazeDirectories; import com.google.devtools.build.lib.bugreport.BugReport; @@ -271,7 +273,7 @@ private SkyValue computeInternal( if (!state.hasCollectedInputs()) { try { state.allInputs = collectInputs(action, env); - } catch (AlreadyReportedActionExecutionException e) { + } catch (ActionExecutionException e) { throw new ActionExecutionFunctionException(e); } if (state.allInputs == null) { @@ -533,6 +535,64 @@ private Reset handleLostInputs( return rewindPlanResult.toNullIfMissingDependenciesElseReset(); } + /** + * Computes the digest of the action's mandatory inputs. Callers should check for missing values + * before using the result, which may be null in error cases. + */ + @Nullable + private byte[] computeMandatoryInputsDigest(Action action, Environment env) + throws InterruptedException, ActionExecutionException { + NestedSet mandatoryInputsSet = action.getMandatoryInputs(); + var mandatoryInputsKeysBuilder = ImmutableSet.builder(); + for (Artifact leaf : mandatoryInputsSet.getLeaves()) { + mandatoryInputsKeysBuilder.add(Artifact.key(leaf)); + } + for (NestedSet nonLeaf : mandatoryInputsSet.getNonLeaves()) { + mandatoryInputsKeysBuilder.add(ArtifactNestedSetKey.create(nonLeaf)); + } + var mandatoryInputsKeys = mandatoryInputsKeysBuilder.build(); + var lookupResult = env.getValuesAndExceptions(mandatoryInputsKeys); + if (env.valuesMissing() && !env.inErrorBubbling()) { + return null; + } + var mandatoryInputs = mandatoryInputsSet.toList(); + var inputArtifactData = new ActionInputMap(mandatoryInputs.size()); + var inputMap = new HashMap(mandatoryInputs.size()); + var actionExecutionFunctionExceptionHandler = + createActionExecutionFunctionExceptionHandler( + action, + lookupResult, + mandatoryInputsKeys, + () -> mandatoryInputs, + /* isMandatoryInput= */ Predicates.alwaysTrue()); + var unused = actionExecutionFunctionExceptionHandler.accumulateAndMaybeThrowExceptions(); + if (env.valuesMissing() && !env.inErrorBubbling()) { + return null; + } + for (var artifact : mandatoryInputs) { + SkyValue value = + getAndCheckInputSkyValue( + env, + action, + artifact, + mandatoryInputsKeys, + /* isMandatoryInput= */ Predicates.alwaysTrue(), + actionExecutionFunctionExceptionHandler); + if (value == null || value instanceof MissingArtifactValue) { + // This can happen with rewinding or in keep-going builds and is always an indication to + // halt the current action execution attempt - we do not have to compute a digest. + return null; + } + ActionInputMapHelper.addToMap( + inputArtifactData, artifact, value, MetadataConsumerForMetrics.NO_OP); + inputMap.put( + artifact.getExecPathString(), + ActionCacheChecker.getInputMetadataMaybe(inputArtifactData, artifact)); + } + actionExecutionFunctionExceptionHandler.maybeThrowException(); + return MetadataDigestUtils.fromMetadata(inputMap); + } + /** * An action's inputs needed for execution. May not just be the result of Action#getInputs(). If * the action cache's view of this action contains additional inputs, it will request metadata for @@ -541,10 +601,30 @@ private Reset handleLostInputs( */ @Nullable private AllInputs collectInputs(Action action, Environment env) - throws InterruptedException, AlreadyReportedActionExecutionException { + throws InterruptedException, ActionExecutionException { + byte[] mandatoryInputsDigest = null; + if (action.discoversInputs()) { + // When the mandatory inputs of an action have changed, it will certainly have to be + // reexecuted. It is important that we detect this before requesting the previously discovered + // inputs from Skyframe as that could result in cycles that otherwise would not occur. + // Note that this approach does not guarantee the absence of such "phantom" cycles in general, + // it just happens to work for all current use cases in Bazel. A theoretically sound solution + // would require checking the discovered inputs for changes one by one, in the order in which + // they were originally discovered. + mandatoryInputsDigest = computeMandatoryInputsDigest(action, env); + if (env.valuesMissing()) { + return null; + } + if (mandatoryInputsDigest == null + || !skyframeActionExecutor.mandatoryInputsMatch(action, mandatoryInputsDigest)) { + action.resetDiscoveredInputs(); + return new AllInputs(action.getInputs(), mandatoryInputsDigest); + } + } + NestedSet allKnownInputs = action.getInputs(); if (action.inputsKnown()) { - return new AllInputs(allKnownInputs); + return new AllInputs(allKnownInputs, mandatoryInputsDigest); } checkState(action.discoversInputs(), action); @@ -555,21 +635,27 @@ private AllInputs collectInputs(Action action, Environment env) checkState(env.valuesMissing(), action); return null; } - return new AllInputs(allKnownInputs, actionCacheInputs); + return new AllInputs(allKnownInputs, actionCacheInputs, mandatoryInputsDigest); } static class AllInputs { final NestedSet defaultInputs; @Nullable final List actionCacheInputs; + @Nullable public final byte[] mandatoryInputsDigest; - AllInputs(NestedSet defaultInputs) { + AllInputs(NestedSet defaultInputs, @Nullable byte[] mandatoryInputsDigest) { this.defaultInputs = checkNotNull(defaultInputs); this.actionCacheInputs = null; + this.mandatoryInputsDigest = mandatoryInputsDigest; } - AllInputs(NestedSet defaultInputs, List actionCacheInputs) { + AllInputs( + NestedSet defaultInputs, + List actionCacheInputs, + @Nullable byte[] mandatoryInputsDigest) { this.defaultInputs = checkNotNull(defaultInputs); this.actionCacheInputs = checkNotNull(actionCacheInputs); + this.mandatoryInputsDigest = mandatoryInputsDigest; } /** Compute the inputs to request from Skyframe. */ @@ -711,6 +797,7 @@ private ActionExecutionValue checkCacheAndExecuteIfNeeded( pathResolver, actionStartTime, state.allInputs.actionCacheInputs, + state.allInputs.mandatoryInputsDigest, clientEnv); } @@ -814,7 +901,12 @@ public void run( } checkState(!env.valuesMissing(), action); skyframeActionExecutor.updateActionCache( - action, inputMetadataProvider, outputMetadataStore, state.token, clientEnv); + action, + inputMetadataProvider, + outputMetadataStore, + state.token, + state.allInputs.mandatoryInputsDigest, + clientEnv); } } @@ -957,30 +1049,11 @@ private CheckInputResults checkInputs( throws ActionExecutionException, InterruptedException, UndoneInputsException { Predicate isMandatoryInput = makeMandatoryInputPredicate(action); - ActionExecutionFunctionExceptionHandler actionExecutionFunctionExceptionHandler = - new ActionExecutionFunctionExceptionHandler( - Suppliers.memoize( - () -> { - ImmutableSet allInputsSet = - ImmutableSet.builder() - .addAll(allInputs.toList()) - .addAll(action.getSchedulingDependencies().toList()) - .build(); - SetMultimap skyKeyToArtifactSet = - MultimapBuilder.hashKeys().hashSetValues().build(); - allInputsSet.forEach( - input -> { - SkyKey key = Artifact.key(input); - if (key != input) { - skyKeyToArtifactSet.put(key, input); - } - }); - return skyKeyToArtifactSet; - }), - inputDepsResult, - action, - isMandatoryInput, - inputDepKeys); + Supplier> allDeps = + () -> Iterables.concat(allInputs.toList(), action.getSchedulingDependencies().toList()); + var actionExecutionFunctionExceptionHandler = + createActionExecutionFunctionExceptionHandler( + action, inputDepsResult, inputDepKeys, allDeps, isMandatoryInput); boolean hasMissingInputs = actionExecutionFunctionExceptionHandler.accumulateAndMaybeThrowExceptions(); @@ -1062,6 +1135,31 @@ private CheckInputResults checkInputs( return new CheckInputResults(inputArtifactData); } + private ActionExecutionFunctionExceptionHandler createActionExecutionFunctionExceptionHandler( + Action action, + SkyframeLookupResult inputDepsResult, + ImmutableSet inputDepKeys, + Supplier> allDeps, + Predicate isMandatoryInput) { + return new ActionExecutionFunctionExceptionHandler( + Suppliers.memoize( + () -> { + SetMultimap skyKeyToArtifactSet = + MultimapBuilder.hashKeys().hashSetValues().build(); + for (Artifact input : allDeps.get()) { + SkyKey key = Artifact.key(input); + if (key != input) { + skyKeyToArtifactSet.put(key, input); + } + } + return skyKeyToArtifactSet; + }), + inputDepsResult, + action, + isMandatoryInput, + inputDepKeys); + } + @CanIgnoreReturnValue @Nullable private SkyValue getAndCheckInputSkyValue( 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 c0852d040fcd52..67c2ea40a5c170 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 @@ -735,6 +735,7 @@ Token checkActionCache( ArtifactPathResolver artifactPathResolver, long actionStartTime, List resolvedCacheArtifacts, + @Nullable byte[] mandatoryInputsDigest, Map clientEnv) throws ActionExecutionException, InterruptedException { Token token; @@ -764,6 +765,7 @@ Token checkActionCache( actionCacheChecker.getTokenIfNeedToExecute( action, resolvedCacheArtifacts, + mandatoryInputsDigest, clientEnv, getOutputPermissions(), handler, @@ -806,6 +808,7 @@ public T getContext(Class type) { actionCacheChecker.getTokenUnconditionallyAfterFailureToRecordActionCacheHit( action, resolvedCacheArtifacts, + mandatoryInputsDigest, clientEnv, getOutputPermissions(), handler, @@ -846,6 +849,7 @@ void updateActionCache( InputMetadataProvider inputMetadataProvider, OutputMetadataStore outputMetadataStore, Token token, + @Nullable byte[] mandatoryInputsDigest, Map clientEnv) throws ActionExecutionException, InterruptedException { if (!actionCacheChecker.enabled()) { @@ -871,7 +875,8 @@ void updateActionCache( clientEnv, getOutputPermissions(), remoteDefaultProperties, - useArchivedTreeArtifacts(action)); + useArchivedTreeArtifacts(action), + mandatoryInputsDigest); } 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. @@ -883,6 +888,10 @@ void updateActionCache( } } + boolean mandatoryInputsMatch(Action action, byte[] mandatoryInputsDigest) { + return actionCacheChecker.mandatoryInputsMatch(action, mandatoryInputsDigest); + } + @Nullable List getActionCachedInputs(Action action, PackageRootResolver resolver) throws AlreadyReportedActionExecutionException, InterruptedException { 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 d64eaba1a57456..90e80793840d23 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 @@ -107,7 +107,7 @@ public void setupCache() throws Exception { execRoot = scratch.resolve("/output"); cache = new CorruptibleActionCache(cacheRoot, corruptedCacheRoot, tmpDir, clock); - cacheChecker = createActionCacheChecker(/*storeOutputMetadata=*/ false); + cacheChecker = createActionCacheChecker(/* storeOutputMetadata= */ false); digestHashFunction = DigestHashFunction.SHA256; fileSystem = new InMemoryFileSystem(digestHashFunction); artifactRoot = ArtifactRoot.asDerivedRoot(execRoot, RootType.OUTPUT, "bin"); @@ -222,6 +222,7 @@ private void runAction( cacheChecker.getTokenIfNeedToExecute( action, /* resolvedCacheArtifacts= */ null, + /* mandatoryInputsDigest= */ null, clientEnv, OutputPermissions.READONLY, /* handler= */ null, @@ -296,7 +297,8 @@ private void runAction( clientEnv, OutputPermissions.READONLY, platform, - useArchivedTreeArtifacts); + useArchivedTreeArtifacts, + /* mandatoryInputsDigest= */ null); } } @@ -329,9 +331,7 @@ private void doTestCorruptedCacheEntry(Action action) throws Exception { cache.corruptAllEntries(); runAction(action); - assertStatistics( - 0, - new MissDetailsBuilder().set(MissReason.CORRUPTED_CACHE_ENTRY, 1).build()); + assertStatistics(0, new MissDetailsBuilder().set(MissReason.CORRUPTED_CACHE_ENTRY, 1).build()); } @Test @@ -496,6 +496,7 @@ public void testDeletedConstantMetadataOutputCausesReexecution() throws Exceptio cacheChecker.getTokenIfNeedToExecute( action, /* resolvedCacheArtifacts= */ null, + /* mandatoryInputsDigest= */ null, /* clientEnv= */ ImmutableMap.of(), OutputPermissions.READONLY, /* handler= */ null, @@ -565,7 +566,7 @@ private static TreeArtifactValue createTreeMetadata( @Test public void saveOutputMetadata_remoteFileMetadataSaved() throws Exception { - cacheChecker = createActionCacheChecker(/*storeOutputMetadata=*/ true); + cacheChecker = createActionCacheChecker(/* storeOutputMetadata= */ true); Artifact output = createArtifact(artifactRoot, "bin/dummy"); String content = "content"; Action action = new InjectOutputFileMetadataAction(output, createRemoteMetadata(content)); @@ -582,7 +583,7 @@ public void saveOutputMetadata_remoteFileMetadataSaved() throws Exception { @Test public void saveOutputMetadata_localFileMetadataNotSaved() throws Exception { - cacheChecker = createActionCacheChecker(/*storeOutputMetadata=*/ true); + cacheChecker = createActionCacheChecker(/* storeOutputMetadata= */ true); Artifact output = createArtifact(artifactRoot, "bin/dummy"); Action action = new WriteEmptyOutputAction(output); output.getPath().delete(); @@ -598,7 +599,7 @@ public void saveOutputMetadata_localFileMetadataNotSaved() throws Exception { @Test public void saveOutputMetadata_remoteMetadataInjectedAndLocalFilesStored() throws Exception { - cacheChecker = createActionCacheChecker(/*storeOutputMetadata=*/ true); + cacheChecker = createActionCacheChecker(/* storeOutputMetadata= */ true); Artifact output = createArtifact(artifactRoot, "bin/dummy"); Action action = new WriteEmptyOutputAction(output) { @@ -638,7 +639,7 @@ public void saveOutputMetadata_notSavedIfDisabled() throws Exception { @Test public void saveOutputMetadata_remoteFileMetadataLoaded() throws Exception { - cacheChecker = createActionCacheChecker(/*storeOutputMetadata=*/ true); + cacheChecker = createActionCacheChecker(/* storeOutputMetadata= */ true); Artifact output = createArtifact(artifactRoot, "bin/dummy"); String content = "content"; Action action = new InjectOutputFileMetadataAction(output, createRemoteMetadata(content)); @@ -649,6 +650,7 @@ public void saveOutputMetadata_remoteFileMetadataLoaded() throws Exception { cacheChecker.getTokenIfNeedToExecute( action, /* resolvedCacheArtifacts= */ null, + /* mandatoryInputsDigest= */ null, /* clientEnv= */ ImmutableMap.of(), OutputPermissions.READONLY, /* handler= */ null, @@ -683,6 +685,7 @@ public void saveOutputMetadata_remoteFileExpired_remoteFileMetadataNotLoaded() t cacheChecker.getTokenIfNeedToExecute( action, /* resolvedCacheArtifacts= */ null, + /* mandatoryInputsDigest= */ null, /* clientEnv= */ ImmutableMap.of(), OutputPermissions.READONLY, /* handler= */ null, @@ -712,6 +715,7 @@ public void saveOutputMetadata_storeOutputMetadataDisabled_remoteFileMetadataNot cacheChecker.getTokenIfNeedToExecute( action, /* resolvedCacheArtifacts= */ null, + /* mandatoryInputsDigest= */ null, /* clientEnv= */ ImmutableMap.of(), OutputPermissions.READONLY, /* handler= */ null, @@ -730,7 +734,7 @@ public void saveOutputMetadata_storeOutputMetadataDisabled_remoteFileMetadataNot @Test public void saveOutputMetadata_localMetadataIsSameAsRemoteMetadata_cached( @TestParameter boolean hasResolvedPath) throws Exception { - cacheChecker = createActionCacheChecker(/*storeOutputMetadata=*/ true); + cacheChecker = createActionCacheChecker(/* storeOutputMetadata= */ true); Artifact output = createArtifact(artifactRoot, "bin/dummy"); String content = "content"; PathFragment resolvedPath = @@ -753,7 +757,7 @@ public void saveOutputMetadata_localMetadataIsSameAsRemoteMetadata_cached( @Test public void saveOutputMetadata_localMetadataIsDifferentFromRemoteMetadata_notCached() throws Exception { - cacheChecker = createActionCacheChecker(/*storeOutputMetadata=*/ true); + cacheChecker = createActionCacheChecker(/* storeOutputMetadata= */ true); Artifact output = createArtifact(artifactRoot, "bin/dummy"); String content1 = "content1"; String content2 = "content2"; @@ -773,6 +777,7 @@ public void saveOutputMetadata_localMetadataIsDifferentFromRemoteMetadata_notCac cacheChecker.getTokenIfNeedToExecute( action, /* resolvedCacheArtifacts= */ null, + /* mandatoryInputsDigest= */ null, /* clientEnv= */ ImmutableMap.of(), OutputPermissions.READONLY, /* handler= */ null, @@ -864,7 +869,7 @@ public void saveOutputMetadata_untrustedRemoteMetadataFromOutputStore_notCached( @Test public void saveOutputMetadata_treeMetadata_remoteFileMetadataSaved() throws Exception { - cacheChecker = createActionCacheChecker(/*storeOutputMetadata=*/ true); + cacheChecker = createActionCacheChecker(/* storeOutputMetadata= */ true); SpecialArtifact output = createTreeArtifactWithGeneratingAction(artifactRoot, PathFragment.create("bin/dummy")); ImmutableMap children = @@ -896,7 +901,7 @@ public void saveOutputMetadata_treeMetadata_remoteFileMetadataSaved() throws Exc @Test public void saveOutputMetadata_treeMetadata_remoteArchivedArtifactSaved() throws Exception { - cacheChecker = createActionCacheChecker(/*storeOutputMetadata=*/ true); + cacheChecker = createActionCacheChecker(/* storeOutputMetadata= */ true); SpecialArtifact output = createTreeArtifactWithGeneratingAction(artifactRoot, PathFragment.create("bin/dummy")); Action action = @@ -924,7 +929,7 @@ public void saveOutputMetadata_treeMetadata_remoteArchivedArtifactSaved() throws @Test public void saveOutputMetadata_treeMetadata_resolvedPathSaved() throws Exception { - cacheChecker = createActionCacheChecker(/*storeOutputMetadata=*/ true); + cacheChecker = createActionCacheChecker(/* storeOutputMetadata= */ true); SpecialArtifact output = createTreeArtifactWithGeneratingAction(artifactRoot, PathFragment.create("bin/dummy")); Action action = @@ -969,6 +974,7 @@ public void saveOutputMetadata_emptyTreeMetadata_saved() throws Exception { cacheChecker.getTokenIfNeedToExecute( action, /* resolvedCacheArtifacts= */ null, + /* mandatoryInputsDigest= */ null, /* clientEnv= */ ImmutableMap.of(), OutputPermissions.READONLY, /* handler= */ null, @@ -993,7 +999,7 @@ public void saveOutputMetadata_emptyTreeMetadata_saved() throws Exception { @Test public void saveOutputMetadata_treeMetadata_localFileMetadataNotSaved() throws Exception { - cacheChecker = createActionCacheChecker(/*storeOutputMetadata=*/ true); + cacheChecker = createActionCacheChecker(/* storeOutputMetadata= */ true); SpecialArtifact output = createTreeArtifactWithGeneratingAction(artifactRoot, PathFragment.create("bin/dummy")); writeIsoLatin1(fileSystem.getPath("/file2"), ""); @@ -1027,7 +1033,7 @@ public void saveOutputMetadata_treeMetadata_localFileMetadataNotSaved() throws E @Test public void saveOutputMetadata_treeMetadata_localArchivedArtifactNotSaved() throws Exception { - cacheChecker = createActionCacheChecker(/*storeOutputMetadata=*/ true); + cacheChecker = createActionCacheChecker(/* storeOutputMetadata= */ true); SpecialArtifact output = createTreeArtifactWithGeneratingAction(artifactRoot, PathFragment.create("bin/dummy")); writeIsoLatin1(fileSystem.getPath("/archive"), ""); @@ -1052,7 +1058,7 @@ public void saveOutputMetadata_treeMetadata_localArchivedArtifactNotSaved() thro @Test public void saveOutputMetadata_treeMetadata_remoteFileMetadataLoaded() throws Exception { - cacheChecker = createActionCacheChecker(/*storeOutputMetadata=*/ true); + cacheChecker = createActionCacheChecker(/* storeOutputMetadata= */ true); SpecialArtifact output = createTreeArtifactWithGeneratingAction(artifactRoot, PathFragment.create("bin/dummy")); ImmutableMap children = @@ -1074,6 +1080,7 @@ public void saveOutputMetadata_treeMetadata_remoteFileMetadataLoaded() throws Ex cacheChecker.getTokenIfNeedToExecute( action, /* resolvedCacheArtifacts= */ null, + /* mandatoryInputsDigest= */ null, /* clientEnv= */ ImmutableMap.of(), OutputPermissions.READONLY, /* handler= */ null, @@ -1100,7 +1107,7 @@ public void saveOutputMetadata_treeMetadata_remoteFileMetadataLoaded() throws Ex @Test public void saveOutputMetadata_treeMetadata_localFileMetadataLoaded() throws Exception { - cacheChecker = createActionCacheChecker(/*storeOutputMetadata=*/ true); + cacheChecker = createActionCacheChecker(/* storeOutputMetadata= */ true); SpecialArtifact output = createTreeArtifactWithGeneratingAction(artifactRoot, PathFragment.create("bin/dummy")); ImmutableMap children1 = @@ -1134,6 +1141,7 @@ public void saveOutputMetadata_treeMetadata_localFileMetadataLoaded() throws Exc cacheChecker.getTokenIfNeedToExecute( action, /* resolvedCacheArtifacts= */ null, + /* mandatoryInputsDigest= */ null, /* clientEnv= */ ImmutableMap.of(), OutputPermissions.READONLY, /* handler= */ null, @@ -1179,7 +1187,7 @@ public void saveOutputMetadata_treeMetadata_localFileMetadataLoaded() throws Exc @Test public void saveOutputMetadata_treeMetadata_localArchivedArtifactLoaded() throws Exception { - cacheChecker = createActionCacheChecker(/*storeOutputMetadata=*/ true); + cacheChecker = createActionCacheChecker(/* storeOutputMetadata= */ true); SpecialArtifact output = createTreeArtifactWithGeneratingAction(artifactRoot, PathFragment.create("bin/dummy")); Action action = @@ -1207,6 +1215,7 @@ public void saveOutputMetadata_treeMetadata_localArchivedArtifactLoaded() throws cacheChecker.getTokenIfNeedToExecute( action, /* resolvedCacheArtifacts= */ null, + /* mandatoryInputsDigest= */ null, /* clientEnv= */ ImmutableMap.of(), OutputPermissions.READONLY, /* handler= */ null, @@ -1272,6 +1281,7 @@ public void saveOutputMetadata_treeFileExpired_treeMetadataNotLoaded() throws Ex cacheChecker.getTokenIfNeedToExecute( action, /* resolvedCacheArtifacts= */ null, + /* mandatoryInputsDigest= */ null, /* clientEnv= */ ImmutableMap.of(), OutputPermissions.READONLY, /* handler= */ null, @@ -1316,6 +1326,7 @@ public void saveOutputMetadata_archivedRepresentationExpired_treeMetadataNotLoad cacheChecker.getTokenIfNeedToExecute( action, /* resolvedCacheArtifacts= */ null, + /* mandatoryInputsDigest= */ null, /* clientEnv= */ ImmutableMap.of(), OutputPermissions.READONLY, /* handler= */ null, @@ -1368,6 +1379,7 @@ public void saveOutputMetadata_toggleArchivedTreeArtifacts_notLoaded( cacheChecker.getTokenIfNeedToExecute( action, /* resolvedCacheArtifacts= */ null, + /* mandatoryInputsDigest= */ null, /* clientEnv= */ ImmutableMap.of(), OutputPermissions.READONLY, /* handler= */ null, @@ -1391,7 +1403,7 @@ private static void writeContentAsLatin1(Path path, String content) throws IOExc @Test public void saveOutputMetadata_treeMetadataWithSameLocalFileMetadata_cached() throws Exception { - cacheChecker = createActionCacheChecker(/*storeOutputMetadata=*/ true); + cacheChecker = createActionCacheChecker(/* storeOutputMetadata= */ true); SpecialArtifact output = createTreeArtifactWithGeneratingAction(artifactRoot, PathFragment.create("bin/dummy")); ImmutableMap children = @@ -1412,6 +1424,7 @@ public void saveOutputMetadata_treeMetadataWithSameLocalFileMetadata_cached() th cacheChecker.getTokenIfNeedToExecute( action, /* resolvedCacheArtifacts= */ null, + /* mandatoryInputsDigest= */ null, /* clientEnv= */ ImmutableMap.of(), OutputPermissions.READONLY, /* handler= */ null, @@ -1447,7 +1460,7 @@ public void saveOutputMetadata_treeMetadataWithSameLocalFileMetadata_cached() th @Test public void saveOutputMetadata_treeMetadataWithSameLocalArchivedArtifact_cached() throws Exception { - cacheChecker = createActionCacheChecker(/*storeOutputMetadata=*/ true); + cacheChecker = createActionCacheChecker(/* storeOutputMetadata= */ true); SpecialArtifact output = createTreeArtifactWithGeneratingAction(artifactRoot, PathFragment.create("bin/dummy")); Action action = diff --git a/src/test/java/com/google/devtools/build/lib/actions/cache/CompactPersistentActionCacheTest.java b/src/test/java/com/google/devtools/build/lib/actions/cache/CompactPersistentActionCacheTest.java index ade4c0a5f47cff..3d796fdeccda18 100644 --- a/src/test/java/com/google/devtools/build/lib/actions/cache/CompactPersistentActionCacheTest.java +++ b/src/test/java/com/google/devtools/build/lib/actions/cache/CompactPersistentActionCacheTest.java @@ -37,6 +37,7 @@ import com.google.devtools.build.lib.skyframe.TreeArtifactValue; import com.google.devtools.build.lib.testutil.ManualClock; import com.google.devtools.build.lib.testutil.Scratch; +import com.google.devtools.build.lib.vfs.DigestUtils; import com.google.devtools.build.lib.vfs.FileSystemUtils; import com.google.devtools.build.lib.vfs.OutputPermissions; import com.google.devtools.build.lib.vfs.Path; @@ -78,7 +79,7 @@ public class CompactPersistentActionCacheTest { private final EventHandler eventHandler = spy(EventHandler.class); @Before - public final void createFiles() throws Exception { + public final void createFiles() throws Exception { execRoot = scratch.resolve("/output"); cacheRoot = scratch.resolve("/cache_root"); corruptedCacheRoot = scratch.resolve("/corrupted_cache_root"); @@ -231,7 +232,7 @@ public void testRemoveIf() throws IOException { // Remove entries that discover inputs and flush the journal. cache.removeIf(Entry::discoversInputs); - assertFullSave(); + assertIncrementalSave(cache); // Check that the entries that discover inputs are gone, and the rest are still there. for (int i = 0; i < 100; i++) { @@ -770,6 +771,7 @@ private static ActionCache.Entry.Builder builder(String actionKey, boolean disco /* clientEnv= */ ImmutableMap.of(), /* execProperties= */ ImmutableMap.of(), OutputPermissions.READONLY, - /* useArchivedTreeArtifacts= */ false); + /* useArchivedTreeArtifacts= */ false, + discoversInputs ? new byte[DigestUtils.ESTIMATED_SIZE] : null); } } diff --git a/src/test/java/com/google/devtools/build/lib/skyframe/BUILD b/src/test/java/com/google/devtools/build/lib/skyframe/BUILD index 36f0ebc98d37ea..7a46bd629981ad 100644 --- a/src/test/java/com/google/devtools/build/lib/skyframe/BUILD +++ b/src/test/java/com/google/devtools/build/lib/skyframe/BUILD @@ -1629,6 +1629,7 @@ java_test( "//src/main/java/com/google/devtools/build/lib/skyframe:action_output_metadata_store", "//src/main/java/com/google/devtools/build/lib/skyframe:skyframe_cluster", "//src/main/java/com/google/devtools/build/lib/skyframe:tree_artifact_value", + "//src/main/java/com/google/devtools/build/lib/util", "//src/main/java/com/google/devtools/build/lib/vfs", "//src/main/java/com/google/devtools/build/lib/vfs:pathfragment", "//src/main/java/com/google/devtools/build/skyframe", diff --git a/src/test/java/com/google/devtools/build/lib/skyframe/TreeArtifactMetadataTest.java b/src/test/java/com/google/devtools/build/lib/skyframe/TreeArtifactMetadataTest.java index 0e9a918cdee931..880723325714ca 100644 --- a/src/test/java/com/google/devtools/build/lib/skyframe/TreeArtifactMetadataTest.java +++ b/src/test/java/com/google/devtools/build/lib/skyframe/TreeArtifactMetadataTest.java @@ -41,6 +41,7 @@ import com.google.devtools.build.lib.collect.nestedset.NestedSetBuilder; import com.google.devtools.build.lib.collect.nestedset.Order; import com.google.devtools.build.lib.events.NullEventHandler; +import com.google.devtools.build.lib.util.Fingerprint; import com.google.devtools.build.lib.vfs.FileStatus; import com.google.devtools.build.lib.vfs.FileStatusWithDigestAdapter; import com.google.devtools.build.lib.vfs.Path; @@ -111,10 +112,10 @@ private TreeArtifactValue doTestTreeArtifacts( public void testEmptyTreeArtifacts() throws Exception { TreeArtifactValue value = doTestTreeArtifacts(ImmutableList.of()); // Additional test, only for this test method: we expect the FileArtifactValue is equal to - // the digest [0] + // the hash of the empty byte array. assertThat(value.getMetadata().getDigest()).isEqualTo(value.getDigest()); // Java zero-fills arrays. - assertThat(value.getDigest()).isEqualTo(new byte[1]); + assertThat(value.getDigest()).isEqualTo(new Fingerprint().digestAndReset()); } @Test diff --git a/src/test/shell/bazel/BUILD b/src/test/shell/bazel/BUILD index d1a7723aea97c8..3a067c7a65ae51 100644 --- a/src/test/shell/bazel/BUILD +++ b/src/test/shell/bazel/BUILD @@ -1030,6 +1030,7 @@ sh_test( shard_count = 10, tags = [ "no_windows", + "requires-network", ], ) diff --git a/src/test/shell/bazel/cc_integration_test.sh b/src/test/shell/bazel/cc_integration_test.sh index 00a722afdc3b51..4a4131ea7c6f30 100755 --- a/src/test/shell/bazel/cc_integration_test.sh +++ b/src/test/shell/bazel/cc_integration_test.sh @@ -2210,14 +2210,14 @@ EOF function test_cpp20_modules_with_clang() { type -P clang || return 0 # Check if clang version is less than 17 - clang_version=$(clang --version | head -n1 | grep -oE '[0-9]+\.[0-9]+' | head -n1) + local -r clang_version=$(clang --version | head -n1 | grep -oE '[0-9]+\.[0-9]+' | head -n1) if [[ -n "$clang_version" ]]; then - major_version=$(echo "$clang_version" | cut -d. -f1) + local -r major_version=$(echo "$clang_version" | cut -d. -f1) if [[ "$major_version" -lt 17 ]]; then return 0 fi fi - if [[ "$(uname -s)" == "Darwin" ]]; then + if is_darwin; then return 0 fi @@ -2285,6 +2285,7 @@ export void f_base() { } EOF + # TODO: Make it so that --cxxopt applies to module_interfaces as well. bazel build //:main --experimental_cpp_modules --repo_env=CC=clang --copt=-std=c++20 --disk_cache=disk &> $TEST_log || fail "Expected build C++20 Modules success with compiler 'clang'" # Verify that the build can hit the cache without action cycles. @@ -2293,6 +2294,152 @@ EOF expect_log "17 disk cache hit" } +function test_cpp20_modules_change_ab_to_ba_no_cycle() { + type -P clang || return 0 + # Check if clang version is less than 17 + local -r clang_version=$(clang --version | head -n1 | grep -oE '[0-9]+\.[0-9]+' | head -n1) + if [[ -n "$clang_version" ]]; then + local -r major_version=$(echo "$clang_version" | cut -d. -f1) + if [[ "$major_version" -lt 17 ]]; then + return 0 + fi + fi + + if is_darwin; then + return 0 + fi + + add_rules_cc "MODULE.bazel" + # TODO: Drop this after the next rules_cc release. + cat >> MODULE.bazel <<'EOF' +single_version_override( + module_name = "rules_cc", + patches = ["//:rules_cc.patch"], +) +EOF + cat > rules_cc.patch <<'EOF' +--- cc/private/compile/compile.bzl ++++ cc/private/compile/compile.bzl +@@ -244,9 +244,6 @@ def compile( + + if module_interfaces and not feature_configuration.is_enabled("cpp_modules"): + fail("to use C++20 Modules, the feature cpp_modules must be enabled") +- if module_interfaces and len(module_interfaces) > 1: +- fail("module_interfaces must be a list of files with exactly one file " + +- "due to implementation limitation. see https://github.com/bazelbuild/bazel/pull/22553") + + language_normalized = "c++" if language == None else language + language_normalized = language_normalized.replace("+", "p").upper() +EOF + + cat > BUILD.bazel <<'EOF' +load("@rules_cc//cc:defs.bzl", "cc_library", "cc_binary") + +package(features = ["cpp_modules"]) + +cc_library( + name = "lib", + module_interfaces = ["a.cppm", "b.cppm"], +) +EOF + cat > a.cppm <<'EOF' +export module a; +import b; +EOF + cat > b.cppm <<'EOF' +export module b; +EOF + + bazel build //:lib --experimental_cpp_modules --repo_env=CC=clang --copt=-std=c++20 &> $TEST_log || fail "Expected build C++20 Modules success with compiler 'clang'" + + cat > a.cppm <<'EOF' +export module a; +EOF + cat > bar.cppm <<'EOF' +export module b; +import a; +EOF + + bazel build //:lib --experimental_cpp_modules --repo_env=CC=clang --copt=-std=c++20 &> $TEST_log || fail "Expected build C++20 Modules success with compiler 'clang'" +} + +function test_cpp20_modules_change_abc_to_acb_no_cycle() { + type -P clang || return 0 + # Check if clang version is less than 17 + local -r clang_version=$(clang --version | head -n1 | grep -oE '[0-9]+\.[0-9]+' | head -n1) + if [[ -n "$clang_version" ]]; then + local -r major_version=$(echo "$clang_version" | cut -d. -f1) + if [[ "$major_version" -lt 17 ]]; then + return 0 + fi + fi + + if is_darwin; then + return 0 + fi + + add_rules_cc "MODULE.bazel" + # TODO: Drop this after the next rules_cc release. + cat >> MODULE.bazel <<'EOF' +single_version_override( + module_name = "rules_cc", + patches = ["//:rules_cc.patch"], +) +EOF + cat > rules_cc.patch <<'EOF' +--- cc/private/compile/compile.bzl ++++ cc/private/compile/compile.bzl +@@ -244,9 +244,6 @@ def compile( + + if module_interfaces and not feature_configuration.is_enabled("cpp_modules"): + fail("to use C++20 Modules, the feature cpp_modules must be enabled") +- if module_interfaces and len(module_interfaces) > 1: +- fail("module_interfaces must be a list of files with exactly one file " + +- "due to implementation limitation. see https://github.com/bazelbuild/bazel/pull/22553") + + language_normalized = "c++" if language == None else language + language_normalized = language_normalized.replace("+", "p").upper() +EOF + + cat > BUILD.bazel <<'EOF' +load("@rules_cc//cc:defs.bzl", "cc_library", "cc_binary") + +package(features = ["cpp_modules"]) + +cc_library( + name = "lib", + module_interfaces = ["a.cppm", "b.cppm", "c.cppm"], +) +EOF + cat > a.cppm <<'EOF' +export module a; +import b; +EOF + cat > b.cppm <<'EOF' +export module b; +import c; +EOF + cat > c.cppm <<'EOF' +export module c; +EOF + + bazel build //:lib --experimental_cpp_modules --repo_env=CC=clang --copt=-std=c++20 &> $TEST_log || fail "Expected build C++20 Modules success with compiler 'clang'" + + cat > a.cppm <<'EOF' +export module a; +import c; +EOF + cat > b.cppm <<'EOF' +export module b; +EOF + cat > c.cppm <<'EOF' +export module c; +import b; +EOF + + bazel build //:lib --experimental_cpp_modules --repo_env=CC=clang --copt=-std=c++20 &> $TEST_log || fail "Expected build C++20 Modules success with compiler 'clang'" +} + function test_external_repo_lto() { add_rules_cc "MODULE.bazel" REPO_PATH=$TEST_TMPDIR/repo From 705ab3eb73b9bf429b1c524e3e57c6bbebf29f5c Mon Sep 17 00:00:00 2001 From: Fabian Meumertzheim Date: Wed, 10 Dec 2025 17:39:14 +0100 Subject: [PATCH 2/2] Apply suggestion from @fmeum --- src/test/shell/bazel/cc_integration_test.sh | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/test/shell/bazel/cc_integration_test.sh b/src/test/shell/bazel/cc_integration_test.sh index 4a4131ea7c6f30..4d1fa2f133068c 100755 --- a/src/test/shell/bazel/cc_integration_test.sh +++ b/src/test/shell/bazel/cc_integration_test.sh @@ -2355,7 +2355,7 @@ EOF cat > a.cppm <<'EOF' export module a; EOF - cat > bar.cppm <<'EOF' + cat > b.cppm <<'EOF' export module b; import a; EOF