Move stateful logic from local metadata to state #29190

jvandort · 2024-05-16T15:42:53Z

Prior changes have attempted to add a distinction between immutable component metadata and mutable component state.
Currently, the component metadata is mutable, and calling methods on it may cause user code to run. The metadata caches its computed values.
The state which calls those metadata methods transforms the metadata's computed values and then also caches its transformations.

This double layer of caching makes it very difficult to properly handle project lock synchronization. This change
consolidates the logic of computing variants and configurations into the component state. All stateful logic from the
component metadata has been moved into the component state, and the component metadata type is now fully immutable.

The most significant change here is that a ComponentGraphResolveMetadata instance no longer has methods to expose
the component's variants or configrations. For local components, this logic is now completely handled by the component
state. For external components, the metadata still has the responsibility of computing variants and configurations. So,
a new interface for external metadata has been introduced to expose this functionality, ExternalComponentGraphResolveMetadata.

Note that this commit does not actually solve the project locking issues present in the code. Its sole purpose is to collapse
the double-caching layers into a single class, so that the project locking can be handled much more easily in a separate change.

Reviewing cheatsheet

Before merging the PR, comments starting with

❌ ❓must be fixed
🤔 💅 should be fixed
💭 may be fixed
🎉 celebrate happy things

jvandort · 2024-05-16T16:07:45Z

...t-suite/src/integTest/groovy/org/gradle/api/plugins/JvmTestSuitePluginIntegrationTest.groovy

@@ -110,76 +110,6 @@ Artifacts
        hasIncubatingLegend()
    }

-    def "Only one suite with a given test type allowed per project"() {


These tests were moved to TestReportAggregationPluginIntegrationTest.groovy. See the commit message for more details.

jvandort · 2024-05-16T16:09:32Z

...java/org/gradle/internal/component/external/model/ExternalComponentGraphResolveMetadata.java

+     * May be empty, in which case selection falls back to the legacy configurations available via {@link #getConfiguration(String)}.
+     * The component should provide a configuration called {@value Dependency#DEFAULT_CONFIGURATION}.
+     */
+    List<? extends VariantGraphResolveMetadata> getVariantsForGraphTraversal();


These methods were previously defined in ComponentGraphResolveMetadata.java (the super-interface to this interface), but have been moved here since local component metadata is now not responsible for configurations and variants (since the logic required to support these methods is stateful)

jvandort · 2024-05-16T16:12:30Z

...main/java/org/gradle/internal/component/model/DefaultExternalComponentGraphResolveState.java

@@ -231,4 +237,38 @@ public AttributesSchemaInternal getAttributesSchema() {
            return metadata.getAttributesSchema();
        }
    }
+
+    private static class ExternalGraphSelectionCandidates implements GraphSelectionCandidates {


Copied from AbstractComponentGraphResolveState.java, but with the explicit check for isCanBeConsumed replaced with an assert since all external components always return consumable configurations.

jvandort · 2024-05-16T16:13:37Z

...n/java/org/gradle/internal/component/local/model/LocalComponentGraphResolveStateFactory.java

+     * A {@link ConfigurationMetadataFactory} which uses a list of pre-constructed configuration
+     * metadata as its data source.
+     */
+    public static class ConfigurationsListMetadataFactory implements ConfigurationMetadataFactory {


This factory and the one below have been migrated from the now removed DefaultLocalComponentGraphResolveMetadata.java

...n/java/org/gradle/internal/component/local/model/DefaultLocalComponentGraphResolveState.java

jvandort · 2024-05-16T16:24:57Z

...ava/org/gradle/internal/component/local/model/DefaultLocalComponentGraphResolveMetadata.java

This is the old stateful component metadata this PR intends to make stateless. It has been replaced by LocalComponentGraphResolveMetadata.java

jvandort · 2024-05-16T16:25:35Z

...g/gradle/internal/component/local/model/DefaultLocalComponentGraphResolveMetadataTest.groovy

The tests in this file were moved to LocalComponentGraphResolveStateFactoryTest. A lot of changes had to be made, so git did not recognize the move.

jvandort · 2024-05-16T16:28:27Z

@bot-gradle test apt

bot-gradle · 2024-05-16T16:28:33Z

I've triggered the following builds for you. Click here to see all build failures.

AllPerformanceTestsReadyForRelease build

tresat

As a part of all this work, the ConfigurationNotConsumableFailure should be handled more similarly to other resolution failures. Other than that it looks like a good change.

.../main/java/org/gradle/api/internal/artifacts/ivyservice/InMemoryResolutionResultBuilder.java

...le/api/internal/artifacts/ivyservice/projectmodule/DefaultProjectLocalComponentProvider.java

...java/org/gradle/internal/component/external/model/ExternalComponentGraphResolveMetadata.java

.../test/groovy/org/gradle/internal/component/external/model/IvyDependencyDescriptorTest.groovy

...management/src/main/java/org/gradle/internal/component/model/ComponentGraphResolveState.java

...n/java/org/gradle/internal/component/local/model/DefaultLocalComponentGraphResolveState.java

Prior changes have attempted to add a distinction between immutable component metadata and mutable component state. Currently, the component metadata is mutable, and calling methods on it may cause user code to run. The metadata caches its computed values. The state which calls those metadata methods transforms the metadata's computed values and then also caches its transformations. This double layer of caching makes it very difficult to properly handle project lock synchronization. This change consolidates the logic of computing variants and configurations into the component state. All stateful logic from the component metadata has been moved into the component state, and the component metadata type is now fully immutable. The most significant change here is that a `ComponentGraphResolveMetadata` instance no longer has methods to expose the component's variants or configrations. For local components, this logic is now completely handled by the component state. For external components, the metadata still has the responsibility of computing variants and configurations. So, a new interface for external metadata has been introduced to expose this functionality, `ExternalComponentGraphResolveMetadata`. Note that this commit does not actually solve the project locking issues present in the code. Its sole purpose is to collapse the double-caching layers into a single class, so that the project locking can be handled much more easily in a separate change.

…encies This data is only used for reporting tasks and is almost never needed. Only capture the data if it is used. (and in reality, this data isn't even needed in the in-memory resolution result builder either, since it is captured and sourced from the full resolution result anyways) Also move tests that verify tests cannot share a test type into the test report aggregation integ test. We no longer realize all variants in the current project's component when we resolve a configuration in that component. This means we do not call the code that detects duplicate test types when we run a test. This is a known problem, and the solution is to build a more robust artifact reselection mechanism that can support multiple tests with the same test type. For now, we move these tests into a test that does trigger the error.

- Rename `returnAllVariants` to `includeAllSelectableVariantResults` for better clarity and consistency with naming on the state type - Add javadoc - Deprecate `getLegacyMetadata` on the state type and add suppressions where it is still being used - Rename `DefaultResolutionResultBuilder` to `ResolutionResultGraphBuilder` to better differentiate it from the related but different StreamingResolutinoResultBuilder and InMemoryResolutionResultBuilder that delegate to it - Remove `supportsAttributeMatching` method from `GraphSelectionCandiates` in favor of checking if the variants for attribute matching is empty - Move `ConfigurationMetadtaFactory` method to top-level file - Use `ResolutionFailureHandler` to handle configuration not consumable failures. We hard-code the resolution instead of using a failure dsecriber since at the moment there is no dynamic way to introduce new desribers for this type of failures. We should migrate to this pattern for other failures that do not have dynamic describer interfaces in order to avoid complexity and indirection.

jvandort · 2024-05-22T19:04:22Z

@bot-gradle test this

bot-gradle · 2024-05-22T19:04:27Z

I've triggered the following builds for you. Click here to see all build failures.

PullRequestFeedback build

gitstream-cm bot added the 30 min review label May 16, 2024

jvandort changed the title ~~Mmove stateful logic from local metadata to state~~ Move stateful logic from local metadata to state May 16, 2024

jvandort added this to the 8.9 RC1 milestone May 16, 2024

jvandort self-assigned this May 16, 2024

jvandort commented May 16, 2024

View reviewed changes

...n/java/org/gradle/internal/component/local/model/DefaultLocalComponentGraphResolveState.java Outdated Show resolved Hide resolved

jvandort force-pushed the jvandort/dm/move-stateful-logic-from-local-metadata-to-state branch from b017cfc to 77190ce Compare May 16, 2024 16:18

jvandort commented May 16, 2024

View reviewed changes

gradle deleted a comment from jvandort May 16, 2024

gradle deleted a comment from bot-gradle May 16, 2024

jvandort marked this pull request as ready for review May 16, 2024 17:32

jvandort requested review from a team as code owners May 16, 2024 17:32

jvandort requested review from octylFractal, bamboo, mlopatkin and tresat and removed request for a team and octylFractal May 16, 2024 17:32

tresat requested changes May 20, 2024

View reviewed changes

jvandort added 3 commits May 22, 2024 12:00

Add additional javadoc

7881df0

Update javadoc, clean up test mocking

1ac7e5e

jvandort force-pushed the jvandort/dm/move-stateful-logic-from-local-metadata-to-state branch from 4491d91 to 1ac7e5e Compare May 22, 2024 16:01

jvandort force-pushed the jvandort/dm/move-stateful-logic-from-local-metadata-to-state branch from 439be1c to 0f58327 Compare May 22, 2024 19:04

jvandort added this pull request to the merge queue May 22, 2024

Merged via the queue into master with commit 0f789f5 May 22, 2024
52 of 56 checks passed

jvandort deleted the jvandort/dm/move-stateful-logic-from-local-metadata-to-state branch May 22, 2024 20:43

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Move stateful logic from local metadata to state #29190

Move stateful logic from local metadata to state #29190

jvandort commented May 16, 2024

jvandort May 16, 2024 •

edited

jvandort May 16, 2024

jvandort May 16, 2024

jvandort May 16, 2024

jvandort May 16, 2024

jvandort May 16, 2024 •

edited

jvandort commented May 16, 2024

bot-gradle commented May 16, 2024

tresat left a comment

jvandort commented May 22, 2024

bot-gradle commented May 22, 2024

Move stateful logic from local metadata to state #29190

Move stateful logic from local metadata to state #29190

Conversation

jvandort commented May 16, 2024

Reviewing cheatsheet

jvandort May 16, 2024 • edited

Choose a reason for hiding this comment

jvandort May 16, 2024

Choose a reason for hiding this comment

jvandort May 16, 2024

Choose a reason for hiding this comment

jvandort May 16, 2024

Choose a reason for hiding this comment

jvandort May 16, 2024

Choose a reason for hiding this comment

jvandort May 16, 2024 • edited

Choose a reason for hiding this comment

jvandort commented May 16, 2024

bot-gradle commented May 16, 2024

tresat left a comment

Choose a reason for hiding this comment

jvandort commented May 22, 2024

bot-gradle commented May 22, 2024

jvandort May 16, 2024 •

edited

jvandort May 16, 2024 •

edited