Skip to content
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

Jump to bottom

Fix issue with file hash cache during runtime classpath normalization #16145

Merged
merged 2 commits into from Feb 17, 2021
Merged

Fix issue with file hash cache during runtime classpath normalization #16145

merged 2 commits into from Feb 17, 2021

Conversation

ghale
Copy link
Member

@ghale ghale commented Feb 13, 2021 •
edited

Normalization rules were not being properly captured when calculating the cache key for file hashes during runtime classpath normalization. This was causing incorrect build cache misses under the right sequence of events.

Fixes #16144

@ghale ghale added this to the 6.8.3 milestone Feb 13, 2021
@ghale ghale self-assigned this Feb 13, 2021
@ghale ghale requested a review from a team as a code owner February 13, 2021 16:19
@ghale ghale changed the title Fixes issue with file hashing cache Fix issue with file hash cache during runtime classpath normalization Feb 13, 2021
@ghale ghale force-pushed the gh/issues/16144 branch 3 times, most recently from a67f468 to 4d16964 Compare February 13, 2021 16:36
@ghale ghale changed the base branch from master to release February 13, 2021 16:41
@gradle gradle deleted a comment from ghale Feb 13, 2021
@gradle gradle deleted a comment from ghale Feb 13, 2021
@gradle gradle deleted a comment from ghale Feb 13, 2021
ghale
ghale commented Feb 14, 2021
View reviewed changes
...groovy/org/gradle/normalization/ConfigureRuntimeClasspathNormalizationIntegrationTest.groovy Outdated
@@ -80,6 +85,7 @@ class ConfigureRuntimeClasspathNormalizationIntegrationTest extends AbstractInte
'nested in dir jars' | 'ignoredResourceInNestedInDirJar' | 'notIgnoredResourceInNestedInDirJar' | Api.ANNOTATION
}

@ToBeFixedForConfigurationCache(because = "classpath normalization")
Copy link
Member Author

Choose a reason for hiding this comment

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

This always should have been failing with config cache because we use classpath normalization in the test. But there was an issue with how we were formatting the manifest which was causing it to pass incorrectly in that case. Fixing the manifest caused the test to fail (as it should) so we need the annotation here.

Copy link
Member

Choose a reason for hiding this comment

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

Can we point those tests to #13706, so it is clear there is an issue on the configuration cache side?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yep - done.

wolfs
wolfs reviewed Feb 15, 2021
View reviewed changes
Copy link
Member

@wolfs wolfs left a comment

Choose a reason for hiding this comment

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

I think the change looks good. Does the new test fail without the fix of the production code? Can we do something to make sure we don't run into the same problem again?

...groovy/org/gradle/normalization/ConfigureRuntimeClasspathNormalizationIntegrationTest.groovy Outdated
@@ -80,6 +85,7 @@ class ConfigureRuntimeClasspathNormalizationIntegrationTest extends AbstractInte
'nested in dir jars' | 'ignoredResourceInNestedInDirJar' | 'notIgnoredResourceInNestedInDirJar' | Api.ANNOTATION
}

@ToBeFixedForConfigurationCache(because = "classpath normalization")
Copy link
Member

Choose a reason for hiding this comment

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

Can we point those tests to #13706, so it is clear there is an issue on the configuration cache side?

...groovy/org/gradle/normalization/ConfigureRuntimeClasspathNormalizationIntegrationTest.groovy Outdated
@@ -235,8 +241,8 @@ class ConfigureRuntimeClasspathNormalizationIntegrationTest extends AbstractInte
change | config | api
'ignore file' | "ignore '**/some-other-file.txt'" | Api.RUNTIME
'ignore file' | "ignore '**/some-other-file.txt'" | Api.ANNOTATION
'ignore manifest attribute' | "metaInf { ignoreAttribute 'Implementation-version' }" | Api.RUNTIME
'ignore manifest attribute' | "metaInf { ignoreAttribute 'Implementation-version' }" | Api.ANNOTATION
'ignore manifest attribute' | "metaInf { ignoreAttribute '${IMPLEMENTATION_VERSION}' }" | Api.RUNTIME
Copy link
Member

Choose a reason for hiding this comment

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

Are the | now not aligned any more?

...groovy/org/gradle/normalization/ConfigureRuntimeClasspathNormalizationIntegrationTest.groovy Show resolved Hide resolved
@ghale
Copy link
Member Author

ghale commented Feb 15, 2021

@wolfs Yes, this test fails without the fix. It should, at the least, guard against regressions for the normalization settings we test against. I'm not 100% convinced it's the best way to test for this problem, but I couldn't figure out a better way to test for it.

@ghale
Copy link
Member Author

ghale commented Feb 15, 2021

@bot-gradle test RfM please

@gradle gradle deleted a comment from ghale Feb 15, 2021
@bot-gradle
Copy link
Collaborator

OK, I've already triggered ReadyForMerge build for you.

@ghale ghale changed the base branch from release to master February 16, 2021 18:32
@ghale
Fixes issue with file hashing cache
5312ef5 
Normalization rules were not being properly captured when calculating the
cache key for file hashes during runtime classpath normalization.  This was
causing incorrect build cache misses under the right sequence of events.

Fixes #16144
@ghale
Add some further detail in classpath normalization test
1ae5eca
@ghale
Copy link
Member Author

ghale commented Feb 16, 2021

@bot-gradle test RfM please

@gradle gradle deleted a comment from ghale Feb 16, 2021
@bot-gradle
Copy link
Collaborator

OK, I've already triggered ReadyForMerge build for you.

wolfs
wolfs approved these changes Feb 17, 2021
View reviewed changes
Copy link
Member

@wolfs wolfs left a comment

Choose a reason for hiding this comment

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

LGTM!

@ghale ghale merged commit 8458b09 into master Feb 17, 2021
@wolfs wolfs modified the milestones: 6.8.3, 7.0 RC1 Feb 17, 2021
@Sinapse87
Copy link

We encountered this issue, thanks a lot for fixing. Can't wait to test it

wolfs pushed a commit that referenced this pull request Feb 19, 2021
@ghale @wolfs
Merge pull request #16145 from gradle/gh/issues/16144
35da400 
Fix issue with file hash cache during runtime classpath normalization
@wolfs wolfs mentioned this pull request Feb 19, 2021
Merged
@wolfs
Copy link
Member

wolfs commented Feb 19, 2021

@Sinapse87 Please try out 6.8.3-20210219125813+0000, which is a nightly for the upcoming 6.8.3 release, which contains a fix for the issue.

@ghale ghale deleted the gh/issues/16144 branch June 16, 2021 19:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@wolfs wolfs wolfs approved these changes

Assignees

@ghale ghale

Labels
a:bug in:core DO NOT USE
Projects
None yet
Milestone
7.0 RC1
Development

Successfully merging this pull request may close these issues.

Build cache miss when jars have been hashed with different normalization configuration
5 participants
@ghale @bot-gradle @Sinapse87 @wolfs @lptr