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 0584da78330a04..c41828df85edde 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, remoteDefaultExecProperties, + mandatoryInputsDigest, cachedOutputMetadata, outputChecker, useArchivedTreeArtifacts)) { @@ -548,6 +552,7 @@ private boolean mustExecute( Map clientEnv, OutputPermissions outputPermissions, ImmutableMap remoteDefaultExecProperties, + @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 remoteDefaultExecProperties, - 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 920809175af89a..a33f7e84600608 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 @@ -734,6 +734,7 @@ Token checkActionCache( ArtifactPathResolver artifactPathResolver, long actionStartTime, List resolvedCacheArtifacts, + @Nullable byte[] mandatoryInputsDigest, Map clientEnv) throws ActionExecutionException, InterruptedException { Token token; @@ -763,6 +764,7 @@ Token checkActionCache( actionCacheChecker.getTokenIfNeedToExecute( action, resolvedCacheArtifacts, + mandatoryInputsDigest, clientEnv, getOutputPermissions(), handler, @@ -805,6 +807,7 @@ public T getContext(Class type) { actionCacheChecker.getTokenUnconditionallyAfterFailureToRecordActionCacheHit( action, resolvedCacheArtifacts, + mandatoryInputsDigest, clientEnv, getOutputPermissions(), handler, @@ -843,6 +846,7 @@ void updateActionCache( InputMetadataProvider inputMetadataProvider, OutputMetadataStore outputMetadataStore, Token token, + @Nullable byte[] mandatoryInputsDigest, Map clientEnv) throws ActionExecutionException, InterruptedException { if (!actionCacheChecker.enabled()) { @@ -864,7 +868,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. @@ -876,6 +881,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 f32641d88375ad..1cbe896841787b 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..4d1fa2f133068c 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 > b.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