-
Notifications
You must be signed in to change notification settings - Fork 4.4k
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.
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
Conversation
…brary` (bazelbuild#27927)" This reverts commit 251bfef.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code Review
This pull request reverts a previous commit that added support for multiple module interfaces per cc_library. The revert seems mostly correct and removes the feature-related code across the codebase, including logic for mandatoryInputsDigest in action caching, and related tests. However, I've found a critical issue in MetadataDigestUtils.java where the revert introduces non-determinism in action cache key generation. This could lead to incorrect caching behavior. I've left a detailed comment with a suggested fix.
| */ | ||
| public static byte[] fromMetadata(Map<String, FileArtifactValue> mdMap) { | ||
| byte[] result = emptyDigest.clone(); | ||
| byte[] result = new byte[1]; // reserve the empty string |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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].
| byte[] result = new byte[1]; // reserve the empty string | |
| byte[] result = new Fingerprint().digestAndReset(); |
meteorcloudy
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks!
|
For future reference, could you give some details on why this was reverted (ideally in the PR description pre-merge, but too late for that now)? I'm assuming some discussion took place somewhere; a link would be very useful. |
This reverts commit 251bfef. See discussion at #27492 (comment)