Skip to content

Revert "[9.0.0] Add support for multiple module interfaces per cc_library (#27927)" #28190

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
meteorcloudy merged 1 commit into from
Jan 8, 2026
Merged

Revert "[9.0.0] Add support for multiple module interfaces per cc_library (#27927)" #28190

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
29 changes: 6 additions & 23 deletions src/main/java/com/google/devtools/build/lib/actions/ActionCacheChecker.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand Down Expand Up @@ -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.
Expand All @@ -195,7 +195,6 @@ private static boolean isUpToDate(
NestedSet<Artifact> actionInputs,
InputMetadataProvider inputMetadataProvider,
OutputMetadataStore outputMetadataStore,
@Nullable byte[] mandatoryInputsDigest,
@Nullable CachedOutputMetadata cachedOutputMetadata,
@Nullable OutputChecker outputChecker,
ImmutableMap<String, String> effectiveEnvironment,
Expand All @@ -210,8 +209,7 @@ private static boolean isUpToDate(
effectiveEnvironment,
actionExecutionSalt,
outputPermissions,
useArchivedTreeArtifacts,
mandatoryInputsDigest);
useArchivedTreeArtifacts);

for (Artifact artifact : action.getOutputs()) {
if (artifact.isTreeArtifact()) {
Expand Down Expand Up @@ -454,7 +452,6 @@ private TreeArtifactValue constructProxyTreeMetadata(SpecialArtifact parent)
public Token getTokenIfNeedToExecute(
Action action,
List<Artifact> resolvedCacheArtifacts,
@Nullable byte[] mandatoryInputsDigest,
Map<String, String> clientEnv,
OutputPermissions outputPermissions,
EventHandler handler,
Expand Down Expand Up @@ -511,7 +508,6 @@ public Token getTokenIfNeedToExecute(
clientEnv,
outputPermissions,
actionExecutionSalt,
mandatoryInputsDigest,
cachedOutputMetadata,
outputChecker,
useArchivedTreeArtifacts)) {
Expand Down Expand Up @@ -545,7 +541,6 @@ private boolean mustExecute(
Map<String, String> clientEnv,
OutputPermissions outputPermissions,
String actionExecutionSalt,
@Nullable byte[] mandatoryInputsDigest,
@Nullable CachedOutputMetadata cachedOutputMetadata,
@Nullable OutputChecker outputChecker,
boolean useArchivedTreeArtifacts)
Expand Down Expand Up @@ -583,7 +578,6 @@ private boolean mustExecute(
actionInputs,
inputMetadataProvider,
outputMetadataStore,
mandatoryInputsDigest,
cachedOutputMetadata,
outputChecker,
effectiveEnvironment,
Expand Down Expand Up @@ -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);
Expand Down Expand Up @@ -662,8 +656,7 @@ public void updateActionCache(
Map<String, String> 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);
Expand All @@ -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.
Expand Down Expand Up @@ -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<Artifact> getCachedInputs(Action action, PackageRootResolver resolver)
throws PackageRootResolver.PackageRootException, InterruptedException {
Expand Down Expand Up @@ -815,7 +799,6 @@ public List<Artifact> getCachedInputs(Action action, PackageRootResolver resolve
public Token getTokenUnconditionallyAfterFailureToRecordActionCacheHit(
Action action,
List<Artifact> resolvedCacheArtifacts,
@Nullable byte[] mandatoryInputsDigest,
Map<String, String> clientEnv,
OutputPermissions outputPermissions,
EventHandler handler,
Expand All @@ -831,7 +814,6 @@ public Token getTokenUnconditionallyAfterFailureToRecordActionCacheHit(
return getTokenIfNeedToExecute(
action,
resolvedCacheArtifacts,
mandatoryInputsDigest,
clientEnv,
outputPermissions,
handler,
Expand Down Expand Up @@ -894,4 +876,5 @@ private Token(Action action) {
this.cacheKey = action.getPrimaryOutput().getExecPathString();
}
}

}
32 changes: 3 additions & 29 deletions src/main/java/com/google/devtools/build/lib/actions/cache/ActionCache.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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<String> discoveredInputPaths;
Expand All @@ -101,13 +100,11 @@ final class Entry {

Entry(
@Nullable byte[] digest,
@Nullable byte[] mandatoryInputsDigest,
@Nullable ImmutableList<String> discoveredInputPaths,
ImmutableMap<String, FileArtifactValue> outputFileMetadata,
ImmutableMap<String, SerializableTreeArtifactValue> outputTreeMetadata,
ImmutableList<String> proxyOutputs) {
this.digest = digest;
this.mandatoryInputsDigest = mandatoryInputsDigest;
this.discoveredInputPaths = discoveredInputPaths;
this.outputFileMetadata = outputFileMetadata;
this.outputTreeMetadata = outputTreeMetadata;
Expand All @@ -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;
}

/**
Expand All @@ -147,12 +140,6 @@ public ImmutableList<String> getDiscoveredInputPaths() {
return discoveredInputPaths;
}

@Nullable
public byte[] getMandatoryInputsDigest() {
checkState(discoversInputs());
return mandatoryInputsDigest;
}

/** Gets the metadata of an output file. */
@Nullable
public FileArtifactValue getOutputFile(Artifact output) {
Expand Down Expand Up @@ -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)
Expand All @@ -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 =");
Expand Down Expand Up @@ -293,7 +276,6 @@ public static final class Builder {
// Discovered inputs.
// Null if the action does not discover inputs.
@Nullable private final ImmutableList.Builder<String> discoveredInputPaths;
@Nullable private final byte[] mandatoryInputsDigest;

private final ImmutableMap.Builder<String, FileArtifactValue> outputFileMetadata =
ImmutableMap.builder();
Expand All @@ -312,27 +294,20 @@ 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,
boolean discoversInputs,
ImmutableMap<String, String> 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. */
Expand Down Expand Up @@ -416,7 +391,6 @@ public Entry build() {
actionExecutionSalt,
outputPermissions,
useArchivedTreeArtifacts),
mandatoryInputsDigest,
discoveredInputPaths != null ? discoveredInputPaths.build() : null,
outputFileMetadata.buildOrThrow(),
outputTreeMetadata.buildOrThrow(),
Expand Down
13 changes: 2 additions & 11 deletions src/main/java/com/google/devtools/build/lib/actions/cache/CompactPersistentActionCache.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand Down Expand Up @@ -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());
}
Expand Down Expand Up @@ -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<String> discoveredInputPaths = entry.getDiscoveredInputPaths();
VarInt.putVarInt(discoveredInputPaths.size(), sink);
for (String discoveredInputPath : discoveredInputPaths) {
Expand Down Expand Up @@ -976,18 +974,13 @@ private ActionCache.Entry decodeInternal(byte[] data) throws IOException {

byte[] digest = MetadataDigestUtils.read(source);

byte[] mandatoryInputsDigest = null;
ImmutableList<String> discoveredInputPaths = null;
int discoveredInputsPresenceMarker = VarInt.getVarInt(source);
if (discoveredInputsPresenceMarker != 0) {
if (discoveredInputsPresenceMarker != 1) {
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);
Expand All @@ -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(),
Expand Down Expand Up @@ -1090,7 +1082,6 @@ private ActionCache.Entry decodeInternal(byte[] data) throws IOException {
}
return new ActionCache.Entry(
digest,
mandatoryInputsDigest,
discoveredInputPaths,
outputFiles.buildOrThrow(),
outputTrees.buildOrThrow(),
Expand Down
4 changes: 1 addition & 3 deletions src/main/java/com/google/devtools/build/lib/actions/cache/MetadataDigestUtils.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand All @@ -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<String, FileArtifactValue> mdMap) {
byte[] result = emptyDigest.clone();
byte[] result = new byte[1]; // reserve the empty string
Copy link

@gemini-code-assist gemini-code-assist bot Jan 8, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

critical

This change introduces non-determinism in action cache keys. DigestUtils.combineUnordered is only order-independent when all digests have the same length, which triggers an XOR-based combination. By initializing result to a 1-byte array, while getDigest returns a 16-byte MD5 digest, the first combination will use combineTwo. combineTwo is not commutative, so the final digest will depend on the iteration order of the input map, which is not guaranteed. This can lead to incorrect cache hits or misses.

To fix this, result should be initialized with a digest of the same length, for example, the digest of an empty string, to ensure the commutative XOR logic is always used.

Note: This change will require updating TreeArtifactMetadataTest.testEmptyTreeArtifacts to expect the correct digest for an empty tree artifact, instead of new byte[1].

Suggested change
byte[] result = new byte[1]; // reserve the empty string
byte[] result = new Fingerprint().digestAndReset();

// Profiling showed that MessageDigest engine instantiation was a hotspot, so create one
// instance for this computation to amortize its cost.
Fingerprint fp = new Fingerprint();
Expand Down
15 changes: 1 addition & 14 deletions src/main/java/com/google/devtools/build/lib/analysis/actions/StarlarkAction.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -345,15 +345,6 @@ public NestedSet<Artifact> getAllowedDerivedInputs() {
return getInputs();
}

@Override
public NestedSet<Artifact> getMandatoryInputs() {
if (unusedInputsList.isPresent()) {
return NestedSetBuilder.emptySet(Order.STABLE_ORDER);
} else {
return getInputs();
}
}

@Nullable
@Override
public NestedSet<Artifact> discoverInputs(ActionExecutionContext actionExecutionContext)
Expand All @@ -367,11 +358,7 @@ public NestedSet<Artifact> discoverInputs(ActionExecutionContext actionExecution
NestedSet<Artifact> inputFilesForExtraAction =
shadowedActionObj.getInputFilesForExtraAction(actionExecutionContext);
if (inputFilesForExtraAction == null) {
if (unusedInputsList.isPresent()) {
return allStarlarkActionInputs;
} else {
return null;
}
return null;
}
updateInputs(
createInputs(
Expand Down
Loading
Loading