Remove problematic ClasspathFingerprintingStrategy and ZipHasher fallback strategy#34150
Merged
Conversation
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
d674eb0 to
4513448
Compare
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
4513448 to
dab88f2
Compare
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
1660e8f to
edf433d
Compare
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
abstratt
reviewed
Jul 8, 2025
Member
There was a problem hiding this comment.
❓
Is this debug message still accurate?
Member
Author
There was a problem hiding this comment.
Yes, this message is still ok. We still fallback to full zip hash, we just don't use a fallback ZipHasher in some cases
abstratt
reviewed
Jul 8, 2025
Member
There was a problem hiding this comment.
❓
Do you intend to call this fixture setup method it from other tests? Otherwise, I would inline it/merge with the setup code below
Collaborator
|
The following builds have passed: |
Member
Author
|
@bot-gradle test this |
This comment has been minimized.
This comment has been minimized.
Collaborator
|
The following builds have failed: |
abstratt
reviewed
Jul 14, 2025
Member
There was a problem hiding this comment.
📝
This was unused, apparently since this PR got merged:
abstratt
approved these changes
Jul 14, 2025
abstratt
left a comment
Member
There was a problem hiding this comment.
LGTM, awesome job creating the reproducer
octylFractal
approved these changes
Jul 14, 2025
Comment on lines
129
to
132
Member
There was a problem hiding this comment.
💅 This seems like it was moved for no reason, to keep diffs clean I would prefer to keep it where it was. But it's not a huge deal.
Due to a bug in ZipHasher when we use a fallback we could potentially poison fingerprint cache. We could fix it, but since this method is not used anymore we can just remove it.
8fedd8f to
98dc6ee
Compare
auto-merge was automatically disabled
July 15, 2025 14:04
Pull Request is not mergeable
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
5 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Relates to #32464
Due to a bug in ZipHasher it can happen that when we use a fallback ZipHasher we could potentially poison fingerprint cache. See #32464 (comment).
We could fix it, but since fallback is not used anymore we can just remove it.
This happens only with remote Kotlin libraries with inline function that are present on build script and Java compile classpath.
This PR also adds a test that fails with 8.14.3.