Re-evaluate component configuration metadata if the underlying config…

…uration may have changed

subprojects/dependency-management/src/main/java/org/gradle/api/internal/artifacts/ivyservice/moduleconverter/DefaultRootComponentMetadataBuilder.java

@@ -137,7 +137,9 @@ private static class MetadataHolder implements MutationValidator {
         @Override
         public void validateMutation(MutationType type) {
             if (type == MutationType.DEPENDENCIES || type == MutationType.ARTIFACTS || type == MutationType.DEPENDENCY_ATTRIBUTES) {
-                cachedValue = null;
+                if (cachedValue != null) {
+                    cachedValue.reevaluate();
+                }
             }
         }
 

subprojects/dependency-management/src/main/java/org/gradle/internal/component/local/model/DefaultLocalComponentMetadata.java

@@ -269,6 +269,24 @@ public ImmutableAttributes getAttributes() {
         return ImmutableAttributes.EMPTY;
     }
 
+    /**
+     * We currently allow a configuration that has been partially observed for resolution to be modified
+     * in a beforeResolve callback.
+     *
+     * To reduce the number of instances of root component metadata we create, we mark all configurations
+     * as dirty and in need of re-evaluation when we see certain types of modifications to a configuration.
+     *
+     * In the future, we could narrow the number of configurations that need to be re-evaluated, but it would
+     * be better to get of the behavior that allows configurations to be modified once they've been observed.
+     *
+     * @see org.gradle.api.internal.artifacts.ivyservice.moduleconverter.DefaultRootComponentMetadataBuilder.MetadataHolder#tryCached(ComponentIdentifier)
+     */
+    public void reevaluate() {
+        for (DefaultLocalConfigurationMetadata conf : allConfigurations.values()) {
+            conf.reevaluate();
+        }
+    }
+
     private class LocalVariantMetadata extends DefaultVariantMetadata {
         private final CalculatedValueContainer<ImmutableList<LocalComponentArtifactMetadata>, ?> artifacts;
 
@@ -325,6 +343,7 @@ protected class DefaultLocalConfigurationMetadata implements LocalConfigurationM
         private final CalculatedValueContainerFactory factory;
 
         private ConfigurationInternal backingConfiguration;
+        private boolean reevaluate = true;
         private LocalConfigurationMetadataBuilder configurationMetadataBuilder;
 
         private final List<LocalOriginDependencyMetadata> definedDependencies = Lists.newArrayList();
@@ -605,12 +624,30 @@ public void addVariant(String name, VariantResolveMetadata.Identifier identifier
         }
 
         synchronized void realizeDependencies() {
-            if (backingConfiguration != null) {
+            if (reevaluate && backingConfiguration != null) {
                 backingConfiguration.runDependencyActions();
                 configurationMetadataBuilder.addDependenciesAndExcludes(this, backingConfiguration);
-                backingConfiguration = null;
             }
+            reevaluate = false;
         }
 
+        /**
+         * When the backing configuration could have been modified, we need to clear our retained cache/state,
+         * so that the next evaluation is clean.
+         */
+        synchronized void reevaluate() {
+            definedDependencies.clear();
+            definedFiles.clear();
+            definedExcludes.clear();
+            configurationDependencies = null;
+            configurationExcludes = null;
+            configurationFileDependencies = null;
+            reevaluate = true;
+        }
+
+        @Override
+        public boolean needsReevaluate() {
+            return reevaluate;
+        }
     }
 }

subprojects/dependency-management/src/main/java/org/gradle/internal/component/local/model/LocalConfigurationGraphResolveMetadata.java

@@ -33,4 +33,6 @@ public interface LocalConfigurationGraphResolveMetadata extends ConfigurationGra
      * <p>Note that this may be expensive, and should be called only when required.</p>
      */
     LocalConfigurationMetadata prepareToResolveArtifacts();
+
+    boolean needsReevaluate();
 }

subprojects/dependency-management/src/test/groovy/org/gradle/api/internal/artifacts/ivyservice/moduleconverter/DefaultRootComponentMetadataBuilderTest.groovy

@@ -16,16 +16,23 @@
 
 package org.gradle.api.internal.artifacts.ivyservice.moduleconverter
 
+import com.google.common.collect.ImmutableSet
 import org.gradle.api.internal.artifacts.DefaultModuleIdentifier
 import org.gradle.api.internal.artifacts.ImmutableModuleIdentifierFactory
 import org.gradle.api.internal.artifacts.Module
 import org.gradle.api.internal.artifacts.component.ComponentIdentifierFactory
+import org.gradle.api.internal.artifacts.configurations.ConfigurationInternal
 import org.gradle.api.internal.artifacts.configurations.ConfigurationsProvider
 import org.gradle.api.internal.artifacts.configurations.DependencyMetaDataProvider
 import org.gradle.api.internal.artifacts.configurations.MutationValidator
+import org.gradle.api.internal.artifacts.configurations.ResolutionStrategyInternal
 import org.gradle.api.internal.artifacts.dsl.dependencies.DependencyLockingProvider
+import org.gradle.api.internal.attributes.ImmutableAttributes
 import org.gradle.api.internal.project.ProjectStateRegistry
 import org.gradle.internal.component.external.model.DefaultModuleComponentIdentifier
+import org.gradle.internal.component.external.model.ImmutableCapabilities
+import org.gradle.internal.component.local.model.BuildableLocalComponentMetadata
+import org.gradle.internal.component.local.model.DefaultLocalComponentMetadata
 import org.gradle.util.TestUtil
 import spock.lang.Specification
 
@@ -37,12 +44,15 @@ class DefaultRootComponentMetadataBuilderTest extends Specification {
     ComponentIdentifierFactory componentIdentifierFactory = Mock()
     ImmutableModuleIdentifierFactory moduleIdentifierFactory = Mock()
     LocalComponentMetadataBuilder configurationComponentMetaDataBuilder = Mock()
+    ConfigurationInternal configuration = Mock()
     def configurationsProvider = Stub(ConfigurationsProvider) {
-        getAll() >> ([] as Set)
+        getAll() >> ([configuration] as Set)
     }
     ProjectStateRegistry projectStateRegistry = Mock()
     DependencyLockingProvider dependencyLockingProvider = Mock()
 
+    DefaultLocalComponentMetadata.DefaultLocalConfigurationMetadata configurationMetadata = Mock()
+
     def mid = DefaultModuleIdentifier.newId('foo', 'bar')
 
     def builderFactory = new DefaultRootComponentMetadataBuilder.Factory(
@@ -57,6 +67,19 @@ class DefaultRootComponentMetadataBuilderTest extends Specification {
 
     def builder = builderFactory.create(configurationsProvider)
 
+    def setup() {
+        ResolutionStrategyInternal resolutionStrategy = Mock()
+        resolutionStrategy.isDependencyLockingEnabled() >> false
+        configuration.getResolutionStrategy() >> resolutionStrategy
+        configuration.getName() >> "conf"
+
+        configurationComponentMetaDataBuilder.addConfiguration(_, _) >> { args ->
+            BuildableLocalComponentMetadata componentMetadata = args[0]
+            componentMetadata.addConfiguration("conf", "", ImmutableSet.of(), ImmutableSet.of("conf"), true, true, ImmutableAttributes.EMPTY, true, null, false, ImmutableCapabilities.EMPTY, null)
+            configurationMetadata
+        }
+    }
+
     def "caches root component metadata"() {
         componentIdentifierFactory.createComponentIdentifier(_) >> {
             new DefaultModuleComponentIdentifier(mid, '1.0')
@@ -87,25 +110,55 @@ class DefaultRootComponentMetadataBuilderTest extends Specification {
         !otherRoot.is(root)
     }
 
-    def "caching of component metadata when #mutationType change"() {
+    def "reevaluates component metadata when #mutationType change"() {
         componentIdentifierFactory.createComponentIdentifier(_) >> {
             new DefaultModuleComponentIdentifier(mid, '1.0')
         }
         def root = builder.toRootComponentMetaData()
 
+        def conf = root.getConfiguration("conf")
+        assert conf.needsReevaluate()
+        conf.realizeDependencies()
+        assert !conf.needsReevaluate()
+
         when:
         builder.validator.validateMutation(mutationType)
         def otherRoot = builder.toRootComponentMetaData()
 
         then:
-        otherRoot.is(root) == cached
+        root == otherRoot
+        conf.needsReevaluate()
 
         where:
-        mutationType                                | cached
-        MutationValidator.MutationType.DEPENDENCIES | false
-        MutationValidator.MutationType.ARTIFACTS    | false
-        MutationValidator.MutationType.ROLE         | true
-        MutationValidator.MutationType.STRATEGY     | true
+        mutationType << [
+            MutationValidator.MutationType.DEPENDENCIES,
+            MutationValidator.MutationType.ARTIFACTS,
+        ]
     }
 
+    def "does not reevaluate component metadata when #mutationType change"() {
+        componentIdentifierFactory.createComponentIdentifier(_) >> {
+            new DefaultModuleComponentIdentifier(mid, '1.0')
+        }
+        def root = builder.toRootComponentMetaData()
+
+        def conf = root.getConfiguration("conf")
+        assert conf.needsReevaluate()
+        conf.realizeDependencies()
+        assert !conf.needsReevaluate()
+
+        when:
+        builder.validator.validateMutation(mutationType)
+        def otherRoot = builder.toRootComponentMetaData()
+
+        then:
+        root == otherRoot
+        !conf.needsReevaluate()
+
+        where:
+        mutationType << [
+                MutationValidator.MutationType.ROLE,
+                MutationValidator.MutationType.STRATEGY,
+        ]
+    }
 }