Skip to content

Commit

Permalink
Register implicit capabilities for conflict detection in more cases (#…
Browse files Browse the repository at this point in the history
…11334)

In general conflict detection for implicit capabilities is skipped
for performance optimization. However, if the corresponding capability
is explicitly declared by another component that was visited *before*,
we need to do the conflict detection between the component with
the implicit capability and the one visited earlier.

See also: #11300
  • Loading branch information
jjohannes committed Nov 13, 2019
1 parent fe767c9 commit da50a17
Show file tree
Hide file tree
Showing 5 changed files with 80 additions and 5 deletions.
Expand Up @@ -77,6 +77,65 @@ class CapabilitiesRulesIntegrationTest extends AbstractModuleDependencyResolveTe
Cannot select module with conflict on capability 'cglib:cglib:3.2.5' also provided by [cglib:cglib-nodep:3.2.5($variant)]""")
}

def "implicit capability conflict is detected if implicit capability is discovered late"() {
given:
repository {
'cglib:cglib:3.2.5'()
'cglib:cglib-nodep:3.2.5'()
'org:lib:1.0' {
dependsOn 'cglib:cglib:3.2.5'
}
}

buildFile << """
class CapabilityRule implements ComponentMetadataRule {
@Override
void execute(ComponentMetadataContext context) {
def details = context.details
details.allVariants {
withCapabilities {
addCapability('cglib', 'cglib', details.id.version)
}
}
}
}
dependencies {
conf "cglib:cglib-nodep:3.2.5"
conf "org:lib:1.0"
components {
withModule('cglib:cglib-nodep', CapabilityRule)
}
}
"""

when:
repositoryInteractions {
'cglib:cglib-nodep:3.2.5' {
expectGetMetadata()
}
'cglib:cglib:3.2.5' {
expectGetMetadata()
}
'org:lib:1.0' {
expectGetMetadata()
}
}
fails ':checkDeps'

then:
def variant = 'runtime'
if (!isGradleMetadataPublished() && useIvy()) {
variant = 'default'
}
failure.assertHasCause("""Module 'cglib:cglib-nodep' has been rejected:
Cannot select module with conflict on capability 'cglib:cglib:3.2.5' also provided by [cglib:cglib:3.2.5($variant)]""")
failure.assertHasCause("""Module 'cglib:cglib' has been rejected:
Cannot select module with conflict on capability 'cglib:cglib:3.2.5' also provided by [cglib:cglib-nodep:3.2.5($variant)]""")
}

@Unroll
def "can detect conflict with capability in different versions (#rule)"() {
given:
Expand Down
Expand Up @@ -197,7 +197,7 @@ private void traverseGraph(final ResolveState resolveState, final Map<ModuleVers
}

private void registerCapabilities(final ResolveState resolveState, final NodeState node) {
node.forEachCapability(new Action<Capability>() {
node.forEachCapability(capabilitiesConflictHandler, new Action<Capability>() {
@Override
public void execute(Capability capability) {
// This is a performance optimization. Most modules do not declare capabilities. So, instead of systematically registering
Expand Down
Expand Up @@ -38,6 +38,7 @@
import org.gradle.api.internal.artifacts.ivyservice.resolveengine.excludes.ModuleExclusions;
import org.gradle.api.internal.artifacts.ivyservice.resolveengine.excludes.specs.ExcludeSpec;
import org.gradle.api.internal.artifacts.ivyservice.resolveengine.graph.DependencyGraphNode;
import org.gradle.api.internal.artifacts.ivyservice.resolveengine.graph.conflicts.CapabilitiesConflictHandler;
import org.gradle.api.internal.artifacts.ivyservice.resolveengine.strict.StrictVersionConstraints;
import org.gradle.api.internal.artifacts.result.DefaultResolvedVariantResult;
import org.gradle.api.internal.attributes.ImmutableAttributes;
Expand Down Expand Up @@ -1037,13 +1038,16 @@ public void clearConstraintEdges(PendingDependencies pendingDependencies, NodeSt
clearIncomingEdges();
}

void forEachCapability(Action<? super Capability> action) {
void forEachCapability(CapabilitiesConflictHandler capabilitiesConflictHandler, Action<? super Capability> action) {
List<? extends Capability> capabilities = metaData.getCapabilities().getCapabilities();
// If there's more than one node selected for the same component, we need to add
// the implicit capability to the list, in order to make sure we can discover conflicts
// between variants of the same module. Note that the fact the implicit capability is
// in general not included is not a bug but a performance optimization
if (capabilities.isEmpty() && component.hasMoreThanOneSelectedNodeUsingVariantAwareResolution()) {
// between variants of the same module.
// We also need too add the implicit capability if it was seen before as an explicit
// capability in order to detect the conflict between the two.
// Note that the fact that the implicit capability is not included in other cases
// is not a bug but a performance optimization.
if (capabilities.isEmpty() && (component.hasMoreThanOneSelectedNodeUsingVariantAwareResolution() || capabilitiesConflictHandler.hasSeenCapability(component.getImplicitCapability()))) {
action.execute(component.getImplicitCapability());
} else {
// The isEmpty check is not required, might look innocent, but Guava's performance bad for an empty immutable list
Expand Down
Expand Up @@ -23,6 +23,13 @@
import java.util.Collection;

public interface CapabilitiesConflictHandler extends ConflictHandler<CapabilitiesConflictHandler.Candidate, ConflictResolutionResult, CapabilitiesConflictHandler.Resolver> {

/**
* Was the given capability already seen which might require a conflict check later?
* This is needed to determine if also implicit capabilities need to enter conflict detection.
*/
boolean hasSeenCapability(Capability capability);

interface Candidate {
NodeState getNode();
Capability getCapability();
Expand Down
Expand Up @@ -139,6 +139,11 @@ public void registerResolver(Resolver conflictResolver) {
resolvers.add(conflictResolver);
}

@Override
public boolean hasSeenCapability(Capability capability) {
return capabilityWithoutVersionToNodes.containsKey(((CapabilityInternal) capability).getCapabilityId());
}

public static CapabilitiesConflictHandler.Candidate candidate(NodeState node, Capability capability, Collection<NodeState> implicitCapabilityProviders) {
return new Candidate(node, capability, implicitCapabilityProviders);
}
Expand Down

0 comments on commit da50a17

Please sign in to comment.