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 21889d84e4079f..65b26f8a8bc502 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,6 +75,7 @@ public class ActionCacheChecker { @Nullable private final ActionCache actionCache; // Null when not enabled. + /** Cache config parameters for ActionCacheChecker. */ @AutoValue public abstract static class CacheConfig { @@ -178,7 +179,6 @@ 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,7 +195,6 @@ private static boolean isUpToDate( NestedSet actionInputs, InputMetadataProvider inputMetadataProvider, OutputMetadataStore outputMetadataStore, - @Nullable byte[] mandatoryInputsDigest, @Nullable CachedOutputMetadata cachedOutputMetadata, @Nullable OutputChecker outputChecker, ImmutableMap effectiveEnvironment, @@ -210,8 +209,7 @@ private static boolean isUpToDate( effectiveEnvironment, actionExecutionSalt, outputPermissions, - useArchivedTreeArtifacts, - mandatoryInputsDigest); + useArchivedTreeArtifacts); for (Artifact artifact : action.getOutputs()) { if (artifact.isTreeArtifact()) { @@ -454,7 +452,6 @@ private TreeArtifactValue constructProxyTreeMetadata(SpecialArtifact parent) public Token getTokenIfNeedToExecute( Action action, List resolvedCacheArtifacts, - @Nullable byte[] mandatoryInputsDigest, Map clientEnv, OutputPermissions outputPermissions, EventHandler handler, @@ -511,7 +508,6 @@ public Token getTokenIfNeedToExecute( clientEnv, outputPermissions, actionExecutionSalt, - mandatoryInputsDigest, cachedOutputMetadata, outputChecker, useArchivedTreeArtifacts)) { @@ -545,7 +541,6 @@ private boolean mustExecute( Map clientEnv, OutputPermissions outputPermissions, String actionExecutionSalt, - @Nullable byte[] mandatoryInputsDigest, @Nullable CachedOutputMetadata cachedOutputMetadata, @Nullable OutputChecker outputChecker, boolean useArchivedTreeArtifacts) @@ -583,7 +578,6 @@ private boolean mustExecute( actionInputs, inputMetadataProvider, outputMetadataStore, - mandatoryInputsDigest, cachedOutputMetadata, outputChecker, effectiveEnvironment, @@ -620,7 +614,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 - public static FileArtifactValue getInputMetadataMaybe( + private static FileArtifactValue getInputMetadataMaybe( InputMetadataProvider inputMetadataProvider, Artifact artifact) { try { return getInputMetadataOrConstant(inputMetadataProvider, artifact); @@ -662,8 +656,7 @@ public void updateActionCache( Map clientEnv, OutputPermissions outputPermissions, String actionExecutionSalt, - boolean useArchivedTreeArtifacts, - @Nullable byte[] mandatoryInputsDigest) + boolean useArchivedTreeArtifacts) throws IOException, InterruptedException { checkState(cacheConfig.enabled(), "cache unexpectedly disabled, action: %s", action); Preconditions.checkArgument(token != null, "token unexpectedly null, action: %s", action); @@ -690,8 +683,7 @@ public void updateActionCache( effectiveEnvironment, actionExecutionSalt, outputPermissions, - useArchivedTreeArtifacts, - mandatoryInputsDigest); + useArchivedTreeArtifacts); for (Artifact output : action.getOutputs()) { // Remove old records from the cache if they used different key. @@ -734,14 +726,6 @@ 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 { @@ -815,7 +799,6 @@ public List getCachedInputs(Action action, PackageRootResolver resolve public Token getTokenUnconditionallyAfterFailureToRecordActionCacheHit( Action action, List resolvedCacheArtifacts, - @Nullable byte[] mandatoryInputsDigest, Map clientEnv, OutputPermissions outputPermissions, EventHandler handler, @@ -831,7 +814,6 @@ public Token getTokenUnconditionallyAfterFailureToRecordActionCacheHit( return getTokenIfNeedToExecute( action, resolvedCacheArtifacts, - mandatoryInputsDigest, clientEnv, outputPermissions, handler, @@ -894,4 +876,5 @@ 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 3d325884fd9dcd..13fcd0dbfe5a8d 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,13 +82,12 @@ public interface ActionCache { final class Entry { /** Unique instance standing for a corrupted cache entry. */ public static final ActionCache.Entry CORRUPTED = - new Entry(null, null, null, ImmutableMap.of(), ImmutableMap.of(), ImmutableList.of()); + new Entry(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; @@ -101,13 +100,11 @@ 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; @@ -131,11 +128,7 @@ public byte[] getDigest() { /** Returns whether the action discovers inputs. */ public boolean discoversInputs() { checkState(!isCorrupted()); - if (discoveredInputPaths == null) { - return false; - } - checkState(mandatoryInputsDigest != null); - return true; + return discoveredInputPaths != null; } /** @@ -147,12 +140,6 @@ 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) { @@ -200,7 +187,6 @@ 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) @@ -219,9 +205,6 @@ 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 ="); @@ -293,7 +276,6 @@ 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(); @@ -312,8 +294,6 @@ 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, @@ -321,18 +301,13 @@ public Builder( ImmutableMap clientEnv, String actionExecutionSalt, OutputPermissions outputPermissions, - boolean useArchivedTreeArtifacts, - @Nullable byte[] mandatoryInputsDigest) { + boolean useArchivedTreeArtifacts) { this.actionKey = actionKey; this.clientEnv = clientEnv; this.actionExecutionSalt = actionExecutionSalt; 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. */ @@ -416,7 +391,6 @@ public Entry build() { actionExecutionSalt, 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 fe51b7a5c8eb6f..467f991c641833 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 = 24; + private static final int VERSION = 23; /** * A timestamp, represented as the number of minutes since the Unix epoch. @@ -848,8 +848,7 @@ private byte[] encode(ActionCache.Entry entry) throws IOException { int maxDiscoveredInputsSize = 1; // presence marker if (entry.discoversInputs()) { maxDiscoveredInputsSize += - (1 + DigestUtils.ESTIMATED_SIZE) // mandatoryInputsDigest - + VarInt.MAX_VARINT_SIZE // length + VarInt.MAX_VARINT_SIZE // length + (VarInt.MAX_VARINT_SIZE // execPath * entry.getDiscoveredInputPaths().size()); } @@ -900,7 +899,6 @@ 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) { @@ -976,7 +974,6 @@ 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) { @@ -984,10 +981,6 @@ 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); @@ -1009,7 +1002,6 @@ private ActionCache.Entry decodeInternal(byte[] data) throws IOException { } return new ActionCache.Entry( digest, - mandatoryInputsDigest, discoveredInputPaths, /* outputFileMetadata= */ ImmutableMap.of(), /* outputTreeMetadata= */ ImmutableMap.of(), @@ -1090,7 +1082,6 @@ 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 d41c14179dc87f..3c55af917c2a95 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,8 +28,6 @@ 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. @@ -56,7 +54,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 = emptyDigest.clone(); + byte[] result = new byte[1]; // reserve the empty string // 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 bd76f400aed11a..7fb073bac52411 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,15 +345,6 @@ 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) @@ -367,11 +358,7 @@ public NestedSet discoverInputs(ActionExecutionContext actionExecution NestedSet inputFilesForExtraAction = shadowedActionObj.getInputFilesForExtraAction(actionExecutionContext); if (inputFilesForExtraAction == null) { - if (unusedInputsList.isPresent()) { - return allStarlarkActionInputs; - } else { - return null; - } + 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 3409ea4ffa5b48..52e537bcf44b2c 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,7 +33,6 @@ 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; @@ -53,7 +52,6 @@ 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; @@ -273,7 +271,7 @@ private SkyValue computeInternal( if (!state.hasCollectedInputs()) { try { state.allInputs = collectInputs(action, env); - } catch (ActionExecutionException e) { + } catch (AlreadyReportedActionExecutionException e) { throw new ActionExecutionFunctionException(e); } if (state.allInputs == null) { @@ -535,64 +533,6 @@ 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 @@ -601,30 +541,10 @@ private byte[] computeMandatoryInputsDigest(Action action, Environment env) */ @Nullable private AllInputs collectInputs(Action action, Environment env) - 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); - } - } - + throws InterruptedException, AlreadyReportedActionExecutionException { NestedSet allKnownInputs = action.getInputs(); if (action.inputsKnown()) { - return new AllInputs(allKnownInputs, mandatoryInputsDigest); + return new AllInputs(allKnownInputs); } checkState(action.discoversInputs(), action); @@ -635,27 +555,21 @@ private AllInputs collectInputs(Action action, Environment env) checkState(env.valuesMissing(), action); return null; } - return new AllInputs(allKnownInputs, actionCacheInputs, mandatoryInputsDigest); + return new AllInputs(allKnownInputs, actionCacheInputs); } static class AllInputs { final NestedSet defaultInputs; @Nullable final List actionCacheInputs; - @Nullable public final byte[] mandatoryInputsDigest; - AllInputs(NestedSet defaultInputs, @Nullable byte[] mandatoryInputsDigest) { + AllInputs(NestedSet defaultInputs) { this.defaultInputs = checkNotNull(defaultInputs); this.actionCacheInputs = null; - this.mandatoryInputsDigest = mandatoryInputsDigest; } - AllInputs( - NestedSet defaultInputs, - List actionCacheInputs, - @Nullable byte[] mandatoryInputsDigest) { + AllInputs(NestedSet defaultInputs, List actionCacheInputs) { this.defaultInputs = checkNotNull(defaultInputs); this.actionCacheInputs = checkNotNull(actionCacheInputs); - this.mandatoryInputsDigest = mandatoryInputsDigest; } /** Compute the inputs to request from Skyframe. */ @@ -797,7 +711,6 @@ private ActionExecutionValue checkCacheAndExecuteIfNeeded( pathResolver, actionStartTime, state.allInputs.actionCacheInputs, - state.allInputs.mandatoryInputsDigest, clientEnv); } @@ -901,12 +814,7 @@ public void run( } checkState(!env.valuesMissing(), action); skyframeActionExecutor.updateActionCache( - action, - inputMetadataProvider, - outputMetadataStore, - state.token, - state.allInputs.mandatoryInputsDigest, - clientEnv); + action, inputMetadataProvider, outputMetadataStore, state.token, clientEnv); } } @@ -1049,11 +957,30 @@ private CheckInputResults checkInputs( throws ActionExecutionException, InterruptedException, UndoneInputsException { Predicate isMandatoryInput = makeMandatoryInputPredicate(action); - Supplier> allDeps = - () -> Iterables.concat(allInputs.toList(), action.getSchedulingDependencies().toList()); - var actionExecutionFunctionExceptionHandler = - createActionExecutionFunctionExceptionHandler( - action, inputDepsResult, inputDepKeys, allDeps, isMandatoryInput); + 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); boolean hasMissingInputs = actionExecutionFunctionExceptionHandler.accumulateAndMaybeThrowExceptions(); @@ -1135,31 +1062,6 @@ 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 2d6a42a984e33b..661857bd537d81 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 @@ -736,7 +736,6 @@ Token checkActionCache( ArtifactPathResolver artifactPathResolver, long actionStartTime, List resolvedCacheArtifacts, - @Nullable byte[] mandatoryInputsDigest, Map clientEnv) throws ActionExecutionException, InterruptedException { Token token; @@ -759,7 +758,6 @@ Token checkActionCache( actionCacheChecker.getTokenIfNeedToExecute( action, resolvedCacheArtifacts, - mandatoryInputsDigest, clientEnv, getOutputPermissions(), handler, @@ -802,7 +800,6 @@ public T getContext(Class type) { actionCacheChecker.getTokenUnconditionallyAfterFailureToRecordActionCacheHit( action, resolvedCacheArtifacts, - mandatoryInputsDigest, clientEnv, getOutputPermissions(), handler, @@ -841,7 +838,6 @@ void updateActionCache( InputMetadataProvider inputMetadataProvider, OutputMetadataStore outputMetadataStore, Token token, - @Nullable byte[] mandatoryInputsDigest, Map clientEnv) throws ActionExecutionException, InterruptedException { if (!actionCacheChecker.enabled()) { @@ -857,8 +853,7 @@ void updateActionCache( clientEnv, getOutputPermissions(), actionExecutionSalt, - useArchivedTreeArtifacts(action), - mandatoryInputsDigest); + useArchivedTreeArtifacts(action)); } 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. @@ -870,10 +865,6 @@ 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 347355a302c722..09c8393c810dba 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"); @@ -221,7 +221,6 @@ private void runAction( cacheChecker.getTokenIfNeedToExecute( action, /* resolvedCacheArtifacts= */ null, - /* mandatoryInputsDigest= */ null, clientEnv, OutputPermissions.READONLY, /* handler= */ null, @@ -296,8 +295,7 @@ private void runAction( clientEnv, OutputPermissions.READONLY, actionExecutionSalt, - useArchivedTreeArtifacts, - /* mandatoryInputsDigest= */ null); + useArchivedTreeArtifacts); } } @@ -330,7 +328,9 @@ 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 @@ -489,7 +489,6 @@ public void testDeletedConstantMetadataOutputCausesReexecution() throws Exceptio cacheChecker.getTokenIfNeedToExecute( action, /* resolvedCacheArtifacts= */ null, - /* mandatoryInputsDigest= */ null, /* clientEnv= */ ImmutableMap.of(), OutputPermissions.READONLY, /* handler= */ null, @@ -559,7 +558,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)); @@ -576,7 +575,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(); @@ -592,7 +591,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) { @@ -632,7 +631,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)); @@ -643,7 +642,6 @@ public void saveOutputMetadata_remoteFileMetadataLoaded() throws Exception { cacheChecker.getTokenIfNeedToExecute( action, /* resolvedCacheArtifacts= */ null, - /* mandatoryInputsDigest= */ null, /* clientEnv= */ ImmutableMap.of(), OutputPermissions.READONLY, /* handler= */ null, @@ -678,7 +676,6 @@ public void saveOutputMetadata_remoteFileExpired_remoteFileMetadataNotLoaded() t cacheChecker.getTokenIfNeedToExecute( action, /* resolvedCacheArtifacts= */ null, - /* mandatoryInputsDigest= */ null, /* clientEnv= */ ImmutableMap.of(), OutputPermissions.READONLY, /* handler= */ null, @@ -708,7 +705,6 @@ public void saveOutputMetadata_storeOutputMetadataDisabled_remoteFileMetadataNot cacheChecker.getTokenIfNeedToExecute( action, /* resolvedCacheArtifacts= */ null, - /* mandatoryInputsDigest= */ null, /* clientEnv= */ ImmutableMap.of(), OutputPermissions.READONLY, /* handler= */ null, @@ -727,7 +723,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 = @@ -750,7 +746,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"; @@ -770,7 +766,6 @@ public void saveOutputMetadata_localMetadataIsDifferentFromRemoteMetadata_notCac cacheChecker.getTokenIfNeedToExecute( action, /* resolvedCacheArtifacts= */ null, - /* mandatoryInputsDigest= */ null, /* clientEnv= */ ImmutableMap.of(), OutputPermissions.READONLY, /* handler= */ null, @@ -862,7 +857,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 = @@ -894,7 +889,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 = @@ -922,7 +917,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 = @@ -967,7 +962,6 @@ public void saveOutputMetadata_emptyTreeMetadata_saved() throws Exception { cacheChecker.getTokenIfNeedToExecute( action, /* resolvedCacheArtifacts= */ null, - /* mandatoryInputsDigest= */ null, /* clientEnv= */ ImmutableMap.of(), OutputPermissions.READONLY, /* handler= */ null, @@ -992,7 +986,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"), ""); @@ -1026,7 +1020,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"), ""); @@ -1051,7 +1045,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 = @@ -1073,7 +1067,6 @@ public void saveOutputMetadata_treeMetadata_remoteFileMetadataLoaded() throws Ex cacheChecker.getTokenIfNeedToExecute( action, /* resolvedCacheArtifacts= */ null, - /* mandatoryInputsDigest= */ null, /* clientEnv= */ ImmutableMap.of(), OutputPermissions.READONLY, /* handler= */ null, @@ -1100,7 +1093,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,7 +1127,6 @@ public void saveOutputMetadata_treeMetadata_localFileMetadataLoaded() throws Exc cacheChecker.getTokenIfNeedToExecute( action, /* resolvedCacheArtifacts= */ null, - /* mandatoryInputsDigest= */ null, /* clientEnv= */ ImmutableMap.of(), OutputPermissions.READONLY, /* handler= */ null, @@ -1180,7 +1172,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 = @@ -1208,7 +1200,6 @@ public void saveOutputMetadata_treeMetadata_localArchivedArtifactLoaded() throws cacheChecker.getTokenIfNeedToExecute( action, /* resolvedCacheArtifacts= */ null, - /* mandatoryInputsDigest= */ null, /* clientEnv= */ ImmutableMap.of(), OutputPermissions.READONLY, /* handler= */ null, @@ -1274,7 +1265,6 @@ public void saveOutputMetadata_treeFileExpired_treeMetadataNotLoaded() throws Ex cacheChecker.getTokenIfNeedToExecute( action, /* resolvedCacheArtifacts= */ null, - /* mandatoryInputsDigest= */ null, /* clientEnv= */ ImmutableMap.of(), OutputPermissions.READONLY, /* handler= */ null, @@ -1319,7 +1309,6 @@ public void saveOutputMetadata_archivedRepresentationExpired_treeMetadataNotLoad cacheChecker.getTokenIfNeedToExecute( action, /* resolvedCacheArtifacts= */ null, - /* mandatoryInputsDigest= */ null, /* clientEnv= */ ImmutableMap.of(), OutputPermissions.READONLY, /* handler= */ null, @@ -1372,7 +1361,6 @@ public void saveOutputMetadata_toggleArchivedTreeArtifacts_notLoaded( cacheChecker.getTokenIfNeedToExecute( action, /* resolvedCacheArtifacts= */ null, - /* mandatoryInputsDigest= */ null, /* clientEnv= */ ImmutableMap.of(), OutputPermissions.READONLY, /* handler= */ null, @@ -1396,7 +1384,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 = @@ -1417,7 +1405,6 @@ public void saveOutputMetadata_treeMetadataWithSameLocalFileMetadata_cached() th cacheChecker.getTokenIfNeedToExecute( action, /* resolvedCacheArtifacts= */ null, - /* mandatoryInputsDigest= */ null, /* clientEnv= */ ImmutableMap.of(), OutputPermissions.READONLY, /* handler= */ null, @@ -1453,7 +1440,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 edf4677ff8b648..8d2a82d8ff67a9 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,7 +37,6 @@ 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; @@ -79,7 +78,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"); @@ -232,7 +231,7 @@ public void testRemoveIf() throws IOException { // Remove entries that discover inputs and flush the journal. cache.removeIf(Entry::discoversInputs); - assertIncrementalSave(cache); + assertFullSave(); // Check that the entries that discover inputs are gone, and the rest are still there. for (int i = 0; i < 100; i++) { @@ -771,7 +770,6 @@ private static ActionCache.Entry.Builder builder(String actionKey, boolean disco /* clientEnv= */ ImmutableMap.of(), /* actionExecutionSalt= */ "", OutputPermissions.READONLY, - /* useArchivedTreeArtifacts= */ false, - discoversInputs ? new byte[DigestUtils.ESTIMATED_SIZE] : null); + /* useArchivedTreeArtifacts= */ false); } } 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 7a46bd629981ad..36f0ebc98d37ea 100644 --- a/src/test/java/com/google/devtools/build/lib/skyframe/BUILD +++ b/src/test/java/com/google/devtools/build/lib/skyframe/BUILD @@ -1629,7 +1629,6 @@ 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 880723325714ca..0e9a918cdee931 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,7 +41,6 @@ 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; @@ -112,10 +111,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 hash of the empty byte array. + // the digest [0] assertThat(value.getMetadata().getDigest()).isEqualTo(value.getDigest()); // Java zero-fills arrays. - assertThat(value.getDigest()).isEqualTo(new Fingerprint().digestAndReset()); + assertThat(value.getDigest()).isEqualTo(new byte[1]); } @Test diff --git a/src/test/shell/bazel/BUILD b/src/test/shell/bazel/BUILD index 3a067c7a65ae51..d1a7723aea97c8 100644 --- a/src/test/shell/bazel/BUILD +++ b/src/test/shell/bazel/BUILD @@ -1030,7 +1030,6 @@ 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 4d1fa2f133068c..00a722afdc3b51 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 - local -r clang_version=$(clang --version | head -n1 | grep -oE '[0-9]+\.[0-9]+' | head -n1) + 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) + major_version=$(echo "$clang_version" | cut -d. -f1) if [[ "$major_version" -lt 17 ]]; then return 0 fi fi - if is_darwin; then + if [[ "$(uname -s)" == "Darwin" ]]; then return 0 fi @@ -2285,7 +2285,6 @@ 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. @@ -2294,152 +2293,6 @@ 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