Skip to content

Commit

Permalink
Properly clean up constraints when node leaves graph
Browse files Browse the repository at this point in the history 
Before this change, some node that carried constraints would remain
registered as interested in pending dependencies. This could cause
invalid deferred selection.
With this set of changes, such state is cleaned up when a node that
carries constraints no longer participates in the graph, either because
it is not transitive or because it is no longer selected.

Issue #13329
  • Loading branch information
@ljacomet
ljacomet committed Jun 16, 2020
1 parent cd88f6f commit c46e7d1
Show file tree
Hide file tree
Showing 5 changed files with 183 additions and 19 deletions.
126 changes: 126 additions & 0 deletions .../groovy/org/gradle/integtests/resolve/platforms/JavaPlatformResolveIntegrationTest.groovy
View file
Expand Up @@ -717,6 +717,132 @@ class JavaPlatformResolveIntegrationTest extends AbstractHttpDependencyResolutio
}
}
def 'platform deselection does not cause orphan edges'() {
given:
def platform = mavenHttpRepo.module('org.test', 'platform', '1.0').withModuleMetadata().withoutDefaultVariants()
.withVariant('apiElements') {
useDefaultArtifacts = false
attribute(Usage.USAGE_ATTRIBUTE.name, Usage.JAVA_API)
attribute(Category.CATEGORY_ATTRIBUTE.name, Category.REGULAR_PLATFORM)
constraint('org.test', 'depA', '1.1')
constraint('org.test', 'depB', '1.0')
}
.withVariant('runtimeElements') {
useDefaultArtifacts = false
attribute(Usage.USAGE_ATTRIBUTE.name, Usage.JAVA_RUNTIME)
attribute(Category.CATEGORY_ATTRIBUTE.name, Category.REGULAR_PLATFORM)
constraint('org.test', 'depA', '1.1')
constraint('org.test', 'depB', '1.0')
}.publish()
def depA = mavenHttpRepo.module('org.test', 'depA', '1.0').withModuleMetadata()
.withVariant('runtime') {
dependsOn('org.test', 'platform', '1.0', null, [(Category.CATEGORY_ATTRIBUTE.name): Category.REGULAR_PLATFORM])
dependsOn('org.test', 'depB', '1.0')
}.publish()
def depA11 = mavenHttpRepo.module('org.test', 'depA', '1.1').withModuleMetadata()
.withVariant('runtime') {
dependsOn('org.test', 'platform', '1.0', null, [(Category.CATEGORY_ATTRIBUTE.name): Category.REGULAR_PLATFORM])
dependsOn('org.test', 'depB', '1.0')
}.publish()
def depB = mavenHttpRepo.module('org.test', 'depB', '1.0').withModuleMetadata()
.withVariant('runtime') {
dependsOn('org.test', 'platform', '1.0', null, [(Category.CATEGORY_ATTRIBUTE.name): Category.REGULAR_PLATFORM])
}.publish()
def otherPlatform10 = mavenHttpRepo.module('org.test', 'otherPlatform', '1.0').withModuleMetadata().withoutDefaultVariants()
.withVariant('apiElements') {
useDefaultArtifacts = false
attribute(Usage.USAGE_ATTRIBUTE.name, Usage.JAVA_API)
attribute(Category.CATEGORY_ATTRIBUTE.name, Category.REGULAR_PLATFORM)
constraint('org.test', 'depD', '1.0')
constraint('org.test', 'test', '1.9')
}
.withVariant('runtimeElements') {
useDefaultArtifacts = false
attribute(Usage.USAGE_ATTRIBUTE.name, Usage.JAVA_RUNTIME)
attribute(Category.CATEGORY_ATTRIBUTE.name, Category.REGULAR_PLATFORM)
constraint('org.test', 'depD', '1.0')
constraint('org.test', 'test', '1.9')
}.publish()
def otherPlatform11 = mavenHttpRepo.module('org.test', 'otherPlatform', '1.1').withModuleMetadata().withoutDefaultVariants()
.withVariant('apiElements') {
useDefaultArtifacts = false
attribute(Usage.USAGE_ATTRIBUTE.name, Usage.JAVA_API)
attribute(Category.CATEGORY_ATTRIBUTE.name, Category.REGULAR_PLATFORM)
constraint('org.test', 'depD', '1.0')
constraint('org.test', 'test', '1.9')
}
.withVariant('runtimeElements') {
useDefaultArtifacts = false
attribute(Usage.USAGE_ATTRIBUTE.name, Usage.JAVA_RUNTIME)
attribute(Category.CATEGORY_ATTRIBUTE.name, Category.REGULAR_PLATFORM)
constraint('org.test', 'depD', '1.0')
constraint('org.test', 'test', '1.9')
}.publish()
def depE = mavenHttpRepo.module('org.test', 'depE', '1.0').withModuleMetadata()
.withVariant('runtime') {
dependsOn('org.test', 'otherPlatform', '1.1', null, [(Category.CATEGORY_ATTRIBUTE.name): Category.REGULAR_PLATFORM])
}.publish()
def depD = mavenHttpRepo.module('org.test', 'depD', '1.0').dependsOn(depE).publish()
def depC = mavenHttpRepo.module('org.test', 'depC', '1.0').dependsOn(depD).publish()
depA.allowAll()
depA11.allowAll()
depB.allowAll()
depC.allowAll()
depD.allowAll()
depE.allowAll()
platform.allowAll()
otherPlatform10.allowAll()
otherPlatform11.allowAll()
buildFile << """
configurations {
conf.dependencies.clear()
}
dependencies {
conf 'org.test:depA:1.0'
conf platform('org.test:otherPlatform:1.0')
conf 'org.test:depC:1.0'
}
"""
checkConfiguration("conf")
resolve.expectDefaultConfiguration("runtime")
when:
succeeds 'checkDeps'
then:
resolve.expectGraph {
root(":", "org.test:test:1.9") {
edge('org.test:depA:1.0', 'org.test:depA:1.1') {
module('org.test:platform:1.0') {
noArtifacts()
constraint('org.test:depA:1.1')
constraint('org.test:depB:1.0')
}
module('org.test:depB:1.0') {
module('org.test:platform:1.0')
}
}
edge("org.test:otherPlatform:1.0", "org.test:otherPlatform:1.1") {
noArtifacts()
constraint('org.test:depD:1.0')
}
module("org.test:depC:1.0") {
module('org.test:depD:1.0') {
module('org.test:depE:1.0') {
module('org.test:otherPlatform:1.1') {
noArtifacts()
}
}
}
}
}
}
}
private void checkConfiguration(String configuration) {
resolve = new ResolveTestFixture(buildFile, configuration)
Expand Down
2 changes: 1 addition & 1 deletion ...l/artifacts/ivyservice/resolveengine/graph/builder/DefaultPendingDependenciesVisitor.java
View file
Expand Up @@ -65,7 +65,7 @@ private boolean markNoLongerPending(PendingDependencies pendingDependencies) {
boolean activatedPending = false;
if (pendingDependencies.hasPendingComponents()) {
if (noLongerPending == null) {
noLongerPending = Lists.newLinkedList();
noLongerPending = Lists.newArrayList();
}
noLongerPending.add(pendingDependencies);
activatedPending = pendingDependencies.shouldReportActivatePending();
Expand Down
4 changes: 4 additions & 0 deletions ...dle/api/internal/artifacts/ivyservice/resolveengine/graph/builder/ModuleResolveState.java
View file
Expand Up @@ -391,6 +391,10 @@ void addPendingNode(NodeState node) {
pendingDependencies.addNode(node);
}

void removePendingNode(NodeState nodeState) {
pendingDependencies.removeNode(nodeState);
}

public void maybeUpdateSelection() {
if (replaced) {
// Never update selection for a replaced module
Expand Down
55 changes: 40 additions & 15 deletions ...a/org/gradle/api/internal/artifacts/ivyservice/resolveengine/graph/builder/NodeState.java
View file
Expand Up @@ -235,23 +235,11 @@ public void visitOutgoingDependencies(Collection<EdgeState> discoveredEdges) {

if (!component.isSelected()) {
LOGGER.debug("version for {} is not selected. ignoring.", this);
if (upcomingNoLongerPendingConstraints != null) {
for (ModuleIdentifier identifier : upcomingNoLongerPendingConstraints) {
ModuleResolveState module = resolveState.getModule(identifier);
for (EdgeState unattachedDependency : module.getUnattachedDependencies()) {
if (!unattachedDependency.getSelector().isResolved()) {
// Unresolved - we have a selector that was deferred but the constraint has been removed in between
NodeState from = unattachedDependency.getFrom();
from.prepareToRecomputeEdge(unattachedDependency);
}
}
}
upcomingNoLongerPendingConstraints = null;
}
cleanupConstraints();
return;
}

// Check if there are any transitive incoming edges at all. Don't traverse if not.
// Check if there are any transitive incoming edges at all. Don't traverse if not.
if (transitiveEdgeCount == 0 && !isRoot()) {
handleNonTransitiveNode(discoveredEdges);
return;
Expand Down Expand Up @@ -289,6 +277,42 @@ public void visitOutgoingDependencies(Collection<EdgeState> discoveredEdges) {
visitOwners(discoveredEdges);
}

/*
* When a node exits the graph, its constraints need to be cleaned up.
* This means:
* * Rescheduling any deferred selection impacted by a constraint coming from this node
* * Making sure we no longer are registered as pending interest on nodes pointed by constraints
*/
private void cleanupConstraints() {
// This part covers constraint that were taken into account between a selection being deferred and this node being scheduled for traversal
if (upcomingNoLongerPendingConstraints != null) {
for (ModuleIdentifier identifier : upcomingNoLongerPendingConstraints) {
ModuleResolveState module = resolveState.getModule(identifier);
for (EdgeState unattachedDependency : module.getUnattachedDependencies()) {
if (!unattachedDependency.getSelector().isResolved()) {
// Unresolved - we have a selector that was deferred but the constraint has been removed in between
NodeState from = unattachedDependency.getFrom();
from.prepareToRecomputeEdge(unattachedDependency);
}
}
}
upcomingNoLongerPendingConstraints = null;
}
// This part covers constraint that might be triggered in the future if the node they point gains a real edge
if (cachedFilteredDependencyStates != null && !cachedFilteredDependencyStates.isEmpty()) {
// We may have registered this node as pending if it had constraints.
// Let's clear that state since it is no longer part of selection
for (DependencyState dependencyState : cachedFilteredDependencyStates) {
if (dependencyState.getDependency().isConstraint()) {
ModuleResolveState targetModule = resolveState.getModule(dependencyState.getModuleIdentifier());
if (targetModule.isPending()) {
targetModule.removePendingNode(this);
}
}
}
}
}

private boolean excludesSameDependenciesAsPreviousTraversal(ExcludeSpec newResolutionFilter) {
List<DependencyState> oldStates = cachedFilteredDependencyStates;
if (previousTraversalExclusions == null || oldStates == null) {
Expand Down Expand Up @@ -361,6 +385,7 @@ private boolean isVirtualPlatformNeedsRefresh() {
* @param discoveredEdges In/Out parameter collecting dependencies or platforms
*/
private void handleNonTransitiveNode(Collection<EdgeState> discoveredEdges) {
cleanupConstraints();
// If node was previously traversed, need to remove outgoing edges.
if (previousTraversalExclusions != null) {
removeOutgoingEdges();
Expand Down Expand Up @@ -601,10 +626,10 @@ void addIncomingEdge(EdgeState dependencyEdge) {
void removeIncomingEdge(EdgeState dependencyEdge) {
if (incomingEdges.remove(dependencyEdge)) {
incomingHash -= dependencyEdge.hashCode();
resolveState.onFewerSelected(this);
if (dependencyEdge.isTransitive()) {
transitiveEdgeCount--;
}
resolveState.onFewerSelected(this);
}
}

Expand Down
15 changes: 12 additions & 3 deletions ...le/api/internal/artifacts/ivyservice/resolveengine/graph/builder/PendingDependencies.java
View file
Expand Up @@ -33,16 +33,24 @@ public class PendingDependencies {
this.reportActivePending = true;
}

void addNode(NodeState state) {
void addNode(NodeState nodeState) {
if (hardEdges != 0) {
throw new IllegalStateException("Cannot add a pending node for a dependency which is not pending");
}
affectedComponents.add(state);
if (state.getComponent().getModule().isVirtualPlatform()) {
affectedComponents.add(nodeState);
if (nodeState.getComponent().getModule().isVirtualPlatform()) {
reportActivePending = false;
}
}

public void removeNode(NodeState nodeState) {
if (hardEdges != 0) {
throw new IllegalStateException("Cannot remove a pending node for a dependency which is not pending");
}
boolean removed = affectedComponents.remove(nodeState);
assert removed : "Removed a pending node that was not registered";
}

void turnIntoHardDependencies() {
for (NodeState affectedComponent : affectedComponents) {
affectedComponent.prepareForConstraintNoLongerPending(moduleIdentifier);
Expand Down Expand Up @@ -71,4 +79,5 @@ void decreaseHardEdgeCount() {
public boolean shouldReportActivatePending() {
return reportActivePending;
}

}

0 comments on commit c46e7d1

Please sign in to comment.