Fix inconsistent resolution ordering #13580
Merged
Conversation
This file contains 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
|
If we do a 6.5.1 this is a serious candidate. |
|
@bot-gradle test this |
|
OK, I've already triggered ReadyForMerge build for you. |
ljacomet
reviewed
Jun 24, 2020
...t/src/main/java/org/gradle/api/internal/artifacts/dsl/DefaultComponentMetadataProcessor.java
Outdated
Show resolved
Hide resolved
.../java/org/gradle/internal/component/external/model/ivy/RealisedIvyModuleResolveMetadata.java
Show resolved
Hide resolved
...agement/src/main/java/org/gradle/internal/resolve/caching/ComponentMetadataRuleExecutor.java
Show resolved
Hide resolved
...ava/org/gradle/internal/component/external/model/AbstractModuleComponentResolveMetadata.java
Outdated
Show resolved
Hide resolved
ljacomet
approved these changes
Jun 25, 2020
|
@ljacomet PTAL |
|
Sorry some internal error occurs, please contact the administrator @blindpirate |
|
@bot-gradle test RFM |
|
Sorry some internal error occurs, please contact the administrator @blindpirate |
|
@bot-gradle test this |
|
Sorry some internal error occurs, please contact the administrator @blindpirate |
|
@bot-gradle test this |
|
Sorry some internal error occurs, please contact the administrator @blindpirate |
The fix for #12951 introduced by #12957 worked but made a number of other bugs much more likely to happen: by using the right derivation strategy in each project, we were properly fetching the component metadata we wanted to process from the cache. However, this component metadata was mutable, because of laziness. In particular, derivation of variants require to set the derivation strategy on "deemed immutable" metadata. The problem is that when several configurations are resolved concurrently, we ended up mutating the same component from different threads, with different derivation strategies. The ideal fix would be to have real immutable component metadata from the cache. However, because of laziness (required for performance), we can't do that. The fix in this PR is therefore to create a copy of the module metadata whenever a different derivation strategy needs to be used. It also highlighted another bug, which is that when we use cached component metadata rules, the derivation strategy wasn't part of the cache key, meaning that we would get whatever was first stored in the binary cache. We now properly separate the entries in the cache by amending the cache key with the name of the strategy. This isn't ideal as we could potentially have _stateful_ strategies, but for now there are no such things in the wild. Again in reality we'd like to get rid of the "magic snapshotting" of rules and do something similar to what transforms do, with proper declared input types. Fixes #13555
ljacomet
approved these changes
Jun 25, 2020
...nt/src/main/java/org/gradle/api/internal/artifacts/dsl/WrappingComponentMetadataContext.java
Show resolved
Hide resolved
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
The fix for #12951 introduced by #12957 worked but made a number
of other bugs much more likely to happen: by using the right derivation
strategy in each project, we were properly fetching the component
metadata we wanted to process from the cache.
However, this component metadata was mutable, because of laziness.
In particular, derivation of variants require to set the derivation
strategy on "deemed immutable" metadata. The problem is that when
several configurations are resolved concurrently, we ended up mutating
the same component from different threads, with different derivation
strategies.
The ideal fix would be to have real immutable component metadata from
the cache. However, because of laziness (required for performance), we
can't do that.
The fix in this PR is therefore to create a copy of the module metadata
whenever a different derivation strategy needs to be used.
It also highlighted another bug, which is that when we use cached
component metadata rules, the derivation strategy wasn't part of the
cache key, meaning that we would get whatever was first stored in the
binary cache.
We now properly separate the entries in the cache by amending the cache
key with the name of the strategy. This isn't ideal as we could potentially
have stateful strategies, but for now there are no such things in the
wild. Again in reality we'd like to get rid of the "magic snapshotting"
of rules and do something similar to what transforms do, with proper
declared input types.
Fixes #13555
Fixes #?
Context
Contributor Checklist
<subproject>/src/integTest) to verify changes from a user perspective<subproject>/src/test) to verify logic./gradlew <changed-subproject>:checkGradle Core Team Checklist