Skip to content

Commit

Permalink
Fix source of consistent resolution being eagerly resolved
Browse files Browse the repository at this point in the history
This commit fixes an issue with consistent resolution, which caused
the consistent resolution source to be fully resolved (including
external dependencies) during task dependencies computation.

To fix this problem, we reworked how "synthetic" dependencies are
added to the resolution process. Synthetic dependencies include
dependency locking constraints, as well as constraints for
dependency resolution consistency. They would now only be added
to the resolution graph if we're actually at the execution phase,
not during task dependency computation.

Fixes #15588
  • Loading branch information
melix committed Dec 15, 2020
1 parent bf2d5bf commit 37decb5
Show file tree
Hide file tree
Showing 14 changed files with 148 additions and 25 deletions.
Expand Up @@ -21,6 +21,7 @@ import org.gradle.integtests.fixtures.RequiredFeature
import org.gradle.integtests.fixtures.ToBeFixedForConfigurationCache
import org.gradle.integtests.fixtures.resolve.ResolveTestFixture
import org.gradle.integtests.resolve.AbstractModuleDependencyResolveTest
import spock.lang.Issue

// Limit the combinations of tests since we're only interested in the consistency
// behavior, not actual metadata
Expand Down Expand Up @@ -296,4 +297,57 @@ class ProjectLocalDependencyResolutionConsistencyIntegrationTest extends Abstrac
}
}

@Issue("https://github.com/gradle/gradle/issues/15588")
def "shouldn't resolve source configuration during task dependency resolution phase"() {
repository {
'org:foo:1.1'()
}

buildFile << """
configurations {
other
conf.shouldResolveConsistentlyWith(other)
}
dependencies {
conf 'org:foo:1.0'
other 'org:foo:1.1'
}
"""
withEagerResolutionPrevention()

when:
repositoryInteractions {
'org:foo:1.1' {
expectResolve()
}
}
run 'checkDeps'

then:
resolve.expectGraph {
root(':', ':test:') {
edge('org:foo:1.0', 'org:foo:1.1') {
byConsistentResolution('other')
}
constraint("org:foo:{strictly 1.1}", "org:foo:1.1")
}
}
}

void withEagerResolutionPrevention() {
buildFile << """
def beforeTaskExecutionPhase = true
gradle.taskGraph.whenReady {
beforeTaskExecutionPhase = false
}
project.configurations.all {
it.incoming.beforeResolve { configuration ->
if (beforeTaskExecutionPhase) {
throw new RuntimeException("Configuration \${configuration.name} is being resolved before task execution phase.")
}
}
}
"""
}
}
Expand Up @@ -33,5 +33,6 @@ void resolve(ResolveContext resolveContext,
DependencyGraphVisitor graphVisitor,
DependencyArtifactsVisitor artifactsVisitor,
AttributesSchemaInternal consumerSchema,
ArtifactTypeRegistry artifactTypeRegistry);
ArtifactTypeRegistry artifactTypeRegistry,
boolean includeSyntheticDependencies);
}
Expand Up @@ -133,7 +133,7 @@ public void resolveBuildDependencies(ConfigurationInternal configuration, Resolv
InMemoryResolutionResultBuilder resolutionResultBuilder = new InMemoryResolutionResultBuilder();
CompositeDependencyGraphVisitor graphVisitor = new CompositeDependencyGraphVisitor(failureCollector, resolutionResultBuilder);
DefaultResolvedArtifactsBuilder artifactsVisitor = new DefaultResolvedArtifactsBuilder(buildProjectDependencies, resolutionStrategy.getSortOrder());
resolver.resolve(configuration, ImmutableList.of(), metadataHandler, IS_LOCAL_EDGE, graphVisitor, artifactsVisitor, attributesSchema, artifactTypeRegistry);
resolver.resolve(configuration, ImmutableList.of(), metadataHandler, IS_LOCAL_EDGE, graphVisitor, artifactsVisitor, attributesSchema, artifactTypeRegistry, false);
result.graphResolved(resolutionResultBuilder.getResolutionResult(), new ResolvedLocalComponentsResultGraphVisitor(currentBuild), new BuildDependenciesOnlyVisitedArtifactSet(failureCollector.complete(Collections.emptySet()), artifactsVisitor.complete(), artifactTransforms, configuration.getDependenciesResolver()));
}

Expand Down Expand Up @@ -175,7 +175,7 @@ public void resolveGraph(ConfigurationInternal configuration, ResolverResults re
ImmutableList<DependencyArtifactsVisitor> allVisitors = visitors.build();
CompositeDependencyArtifactsVisitor artifactsVisitor = new CompositeDependencyArtifactsVisitor(allVisitors);

resolver.resolve(configuration, resolutionAwareRepositories, metadataHandler, Specs.satisfyAll(), graphVisitor, artifactsVisitor, attributesSchema, artifactTypeRegistry);
resolver.resolve(configuration, resolutionAwareRepositories, metadataHandler, Specs.satisfyAll(), graphVisitor, artifactsVisitor, attributesSchema, artifactTypeRegistry, true);

VisitedArtifactsResults artifactsResults = artifactsBuilder.complete();
VisitedFileDependencyResults fileDependencyResults = fileDependencyVisitor.complete();
Expand Down
Expand Up @@ -48,7 +48,9 @@ public DefaultRootComponentMetadataBuilder(DependencyMetaDataProvider metadataPr
ComponentIdentifierFactory componentIdentifierFactory,
ImmutableModuleIdentifierFactory moduleIdentifierFactory,
LocalComponentMetadataBuilder localComponentMetadataBuilder,
ConfigurationsProvider configurationsProvider, ProjectStateRegistry projectStateRegistry, DependencyLockingProvider dependencyLockingProvider) {
ConfigurationsProvider configurationsProvider,
ProjectStateRegistry projectStateRegistry,
DependencyLockingProvider dependencyLockingProvider) {
this.metadataProvider = metadataProvider;
this.componentIdentifierFactory = componentIdentifierFactory;
this.moduleIdentifierFactory = moduleIdentifierFactory;
Expand Down
Expand Up @@ -131,7 +131,7 @@ public DefaultArtifactDependencyResolver(BuildOperationExecutor buildOperationEx
}

@Override
public void resolve(ResolveContext resolveContext, List<? extends ResolutionAwareRepository> repositories, GlobalDependencyResolutionRules metadataHandler, Spec<? super DependencyMetadata> edgeFilter, DependencyGraphVisitor graphVisitor, DependencyArtifactsVisitor artifactsVisitor, AttributesSchemaInternal consumerSchema, ArtifactTypeRegistry artifactTypeRegistry) {
public void resolve(ResolveContext resolveContext, List<? extends ResolutionAwareRepository> repositories, GlobalDependencyResolutionRules metadataHandler, Spec<? super DependencyMetadata> edgeFilter, DependencyGraphVisitor graphVisitor, DependencyArtifactsVisitor artifactsVisitor, AttributesSchemaInternal consumerSchema, ArtifactTypeRegistry artifactTypeRegistry, boolean includeSyntheticDependencies) {
LOGGER.debug("Resolving {}", resolveContext);

validateResolutionStrategy(resolveContext.getResolutionStrategy());
Expand All @@ -142,7 +142,7 @@ public void resolve(ResolveContext resolveContext, List<? extends ResolutionAwar
DependencyGraphVisitor artifactsGraphVisitor = new ResolvedArtifactsGraphVisitor(artifactsVisitor, resolvers.getArtifactSelector());

// Resolve the dependency graph
builder.resolve(resolveContext, new CompositeDependencyGraphVisitor(graphVisitor, artifactsGraphVisitor));
builder.resolve(resolveContext, new CompositeDependencyGraphVisitor(graphVisitor, artifactsGraphVisitor), includeSyntheticDependencies);
}

private static void validateResolutionStrategy(ResolutionStrategyInternal resolutionStrategy) {
Expand Down
Expand Up @@ -51,6 +51,7 @@
import org.gradle.api.specs.Spec;
import org.gradle.internal.component.IncompatibleVariantsSelectionException;
import org.gradle.internal.component.external.model.DefaultModuleComponentIdentifier;
import org.gradle.internal.component.local.model.RootLocalComponentMetadata;
import org.gradle.internal.component.model.ComponentResolveMetadata;
import org.gradle.internal.component.model.DefaultCompatibilityCheckResult;
import org.gradle.internal.component.model.DependencyMetadata;
Expand Down Expand Up @@ -130,7 +131,7 @@ public DependencyGraphBuilder(DependencyToComponentIdResolver componentIdResolve
this.versionParser = versionParser;
}

public void resolve(final ResolveContext resolveContext, final DependencyGraphVisitor modelVisitor) {
public void resolve(final ResolveContext resolveContext, final DependencyGraphVisitor modelVisitor, boolean includeSyntheticDependencies) {

IdGenerator<Long> idGenerator = new LongIdGenerator();
DefaultBuildableComponentResolveResult rootModule = new DefaultBuildableComponentResolveResult();
Expand All @@ -139,7 +140,9 @@ public void resolve(final ResolveContext resolveContext, final DependencyGraphVi
int graphSize = estimateSize(resolveContext);
ResolutionStrategyInternal resolutionStrategy = resolveContext.getResolutionStrategy();

final ResolveState resolveState = new ResolveState(idGenerator, rootModule, resolveContext.getName(), idResolver, metaDataResolver, edgeFilter, attributesSchema, moduleExclusions, componentSelectorConverter, attributesFactory, dependencySubstitutionApplicator, versionSelectorScheme, versionComparator, versionParser, moduleConflictHandler.getResolver(), graphSize, resolveContext.getResolutionStrategy().getConflictResolution());
List<? extends DependencyMetadata> syntheticDependencies = includeSyntheticDependencies ? syntheticDependenciesOf(rootModule, resolveContext.getName()) : Collections.emptyList();

final ResolveState resolveState = new ResolveState(idGenerator, rootModule, resolveContext.getName(), idResolver, metaDataResolver, edgeFilter, attributesSchema, moduleExclusions, componentSelectorConverter, attributesFactory, dependencySubstitutionApplicator, versionSelectorScheme, versionComparator, versionParser, moduleConflictHandler.getResolver(), graphSize, resolveContext.getResolutionStrategy().getConflictResolution(), syntheticDependencies);

Map<ModuleVersionIdentifier, ComponentIdentifier> componentIdentifierCache = Maps.newHashMapWithExpectedSize(graphSize / 2);
traverseGraph(resolveState, componentIdentifierCache);
Expand All @@ -150,6 +153,14 @@ public void resolve(final ResolveContext resolveContext, final DependencyGraphVi

}

private static List<? extends DependencyMetadata> syntheticDependenciesOf(DefaultBuildableComponentResolveResult rootModule, String name) {
ComponentResolveMetadata metadata = rootModule.getMetadata();
if (metadata instanceof RootLocalComponentMetadata) {
return ((RootLocalComponentMetadata)metadata).getSyntheticDependencies(name);
}
return Collections.emptyList();
}

/**
* This method is a heuristic that gives an idea of the "size" of the graph. The larger
* the graph is, the higher the risk of internal resizes exists, so we try to estimate
Expand Down
Expand Up @@ -466,7 +466,7 @@ private List<? extends DependencyMetadata> dependencies() {
cachedDependencyStates = null;
cachedFilteredDependencyStates = null;
}
List<? extends DependencyMetadata> dependencies = metaData.getDependencies();
List<? extends DependencyMetadata> dependencies = getAllDependencies();
if (transitiveEdgeCount == 0 && metaData.isExternalVariant()) {
// there must be a single dependency state because this variant is an "available-at"
// variant and here we are in the case the "including" component said that transitive
Expand All @@ -478,6 +478,10 @@ private List<? extends DependencyMetadata> dependencies() {
return dependencies;
}

protected List<? extends DependencyMetadata> getAllDependencies() {
return metaData.getDependencies();
}

private static DependencyMetadata makeNonTransitive(DependencyMetadata dependencyMetadata) {
return new NonTransitiveVariantDependencyMetadata(dependencyMetadata);
}
Expand Down
Expand Up @@ -55,6 +55,7 @@
import java.util.Comparator;
import java.util.Deque;
import java.util.LinkedHashMap;
import java.util.List;
import java.util.Map;
import java.util.Objects;

Expand Down Expand Up @@ -86,6 +87,7 @@ class ResolveState implements ComponentStateFactory<ComponentState> {
private final ResolveOptimizations resolveOptimizations;
private final Map<VersionConstraint, ResolvedVersionConstraint> resolvedVersionConstraints = Maps.newHashMap();
private final AttributeDesugaring attributeDesugaring;
private final List<? extends DependencyMetadata> generatedRootDependencies;

public ResolveState(IdGenerator<Long> idGenerator,
ComponentResolveResult rootResult,
Expand All @@ -103,7 +105,8 @@ public ResolveState(IdGenerator<Long> idGenerator,
VersionParser versionParser,
ModuleConflictResolver<ComponentState> conflictResolver,
int graphSize,
ConflictResolution conflictResolution) {
ConflictResolution conflictResolution,
List<? extends DependencyMetadata> generatedRootDependencies) {
this.idGenerator = idGenerator;
this.idResolver = idResolver;
this.metaDataResolver = metaDataResolver;
Expand All @@ -121,6 +124,7 @@ public ResolveState(IdGenerator<Long> idGenerator,
this.selectors = new LinkedHashMap<>(5 * graphSize / 2);
this.queue = new ArrayDeque<>(graphSize);
this.conflictResolution = conflictResolution;
this.generatedRootDependencies = generatedRootDependencies;
this.resolveOptimizations = new ResolveOptimizations();
this.attributeDesugaring = new AttributeDesugaring(attributesFactory);
// Create root module
Expand Down Expand Up @@ -156,6 +160,10 @@ private ModuleResolveState getModule(ModuleIdentifier id, boolean rootModule) {
return modules.computeIfAbsent(id, mid -> new ModuleResolveState(idGenerator, id, metaDataResolver, attributesFactory, versionComparator, versionParser, selectorStateResolver, resolveOptimizations, rootModule, conflictResolution));
}

List<? extends DependencyMetadata> getGeneratedRootDependencies() {
return generatedRootDependencies;
}

@Override
public ComponentState getRevision(ComponentIdentifier componentIdentifier, ModuleVersionIdentifier id, ComponentResolveMetadata metadata) {
ComponentState componentState = getModule(id.getModule()).getVersion(id, componentIdentifier);
Expand Down
Expand Up @@ -16,21 +16,26 @@

package org.gradle.api.internal.artifacts.ivyservice.resolveengine.graph.builder;

import com.google.common.collect.ImmutableList;
import org.gradle.api.internal.artifacts.ResolvedConfigurationIdentifier;
import org.gradle.api.internal.artifacts.ivyservice.resolveengine.graph.RootGraphNode;
import org.gradle.internal.component.local.model.LocalFileDependencyMetadata;
import org.gradle.internal.component.local.model.RootConfigurationMetadata;
import org.gradle.internal.component.model.ConfigurationMetadata;
import org.gradle.internal.component.model.DependencyMetadata;

import java.util.List;
import java.util.Set;

class RootNode extends NodeState implements RootGraphNode {
private final ResolveOptimizations resolveOptimizations;
private final List<? extends DependencyMetadata> generatedRootDependencies;

RootNode(Long resultId, ComponentState moduleRevision, ResolvedConfigurationIdentifier id, ResolveState resolveState, ConfigurationMetadata configuration) {
super(resultId, id, moduleRevision, resolveState, configuration);
moduleRevision.setRoot();
this.resolveOptimizations = resolveState.getResolveOptimizations();
this.generatedRootDependencies = resolveState.getGeneratedRootDependencies();
}

@Override
Expand Down Expand Up @@ -61,4 +66,17 @@ public RootConfigurationMetadata getMetadata() {
public ResolveOptimizations getResolveOptimizations() {
return resolveOptimizations;
}

@Override
protected List<? extends DependencyMetadata> getAllDependencies() {
List<? extends DependencyMetadata> superDependencies = super.getAllDependencies();
if (generatedRootDependencies.isEmpty()) {
return superDependencies;
}
int expectedSize = superDependencies.size() + generatedRootDependencies.size();
ImmutableList.Builder<DependencyMetadata> allDependencies = ImmutableList.builderWithExpectedSize(expectedSize);
allDependencies.addAll(superDependencies);
allDependencies.addAll(generatedRootDependencies);
return allDependencies.build();
}
}
Expand Up @@ -421,7 +421,6 @@ public List<? extends LocalOriginDependencyMetadata> getDependencies() {
configuration.addDefinedDependencies(result);
}
}
maybeAddGeneratedDependencies(result);
AttributeValue<Category> attributeValue = this.getAttributes().findEntry(Category.CATEGORY_ATTRIBUTE);
if (attributeValue.isPresent() && attributeValue.get().getName().equals(Category.ENFORCED_PLATFORM)) {
// need to wrap all dependencies to force them
Expand All @@ -436,8 +435,8 @@ public List<? extends LocalOriginDependencyMetadata> getDependencies() {
return configurationDependencies;
}

void maybeAddGeneratedDependencies(ImmutableList.Builder<LocalOriginDependencyMetadata> result) {
// No-op
List<LocalOriginDependencyMetadata> getSyntheticDependencies() {
return Collections.emptyList();
}

void addDefinedDependencies(ImmutableList.Builder<LocalOriginDependencyMetadata> result) {
Expand Down

0 comments on commit 37decb5

Please sign in to comment.