From 24ad3996e87de12320c1a59b9edf6e18c091f50f Mon Sep 17 00:00:00 2001 From: Tomo Suzuki Date: Fri, 13 Aug 2021 10:16:16 -0400 Subject: [PATCH 1/6] incorporated changes --- .../dependencies/gradle/LinkageCheckTask.java | 36 +++++++++++++++++-- 1 file changed, 33 insertions(+), 3 deletions(-) diff --git a/gradle-plugin/src/main/java/com/google/cloud/tools/dependencies/gradle/LinkageCheckTask.java b/gradle-plugin/src/main/java/com/google/cloud/tools/dependencies/gradle/LinkageCheckTask.java index f174ccaee8..02b6d25c2c 100644 --- a/gradle-plugin/src/main/java/com/google/cloud/tools/dependencies/gradle/LinkageCheckTask.java +++ b/gradle-plugin/src/main/java/com/google/cloud/tools/dependencies/gradle/LinkageCheckTask.java @@ -33,6 +33,8 @@ import com.google.common.collect.ImmutableList; import com.google.common.collect.ImmutableListMultimap; import com.google.common.collect.ImmutableSet; +import com.google.common.collect.Multimap; +import com.google.common.collect.MultimapBuilder; import java.io.IOException; import java.nio.file.Path; import java.nio.file.Paths; @@ -200,7 +202,8 @@ private void recordDependencyPaths( ImmutableListMultimap.Builder output, ArrayDeque stack, ImmutableSet targetCoordinates, - Set checkedCircularDependency) { + Set checkedCircularDependency, + Multimap checkedCoordinates) { ResolvedComponentResult item = stack.getLast(); ModuleVersionIdentifier identifier = item.getModuleVersion(); String coordinates = @@ -210,6 +213,12 @@ private void recordDependencyPaths( String dependencyPath = stack.stream().map(this::formatComponentResult).collect(Collectors.joining(" / ")); output.put(coordinates, dependencyPath); + for (ResolvedComponentResult result : stack) { + if (result.equals(stack.peekLast())) { + continue; + } + checkedCoordinates.put(result, coordinates); + } } for (DependencyResult dependencyResult : item.getDependencies()) { @@ -228,9 +237,23 @@ private void recordDependencyPaths( + "\n The stack is: " + stack); } + } else if (checkedCoordinates.containsKey(child)) { + for (String matchedCoordinates : checkedCoordinates.get(child)) { + if (matchedCoordinates.isEmpty()) { + continue; + } + + stack.add(child); + String dependencyPath = + stack.stream().map(this::formatComponentResult).collect(Collectors.joining(" / ")) + + " (*)"; // (*) is to mark that this was already seen + stack.removeLast(); + output.put(matchedCoordinates, dependencyPath); + } } else { stack.add(child); - recordDependencyPaths(output, stack, targetCoordinates, checkedCircularDependency); + recordDependencyPaths( + output, stack, targetCoordinates, checkedCircularDependency, checkedCoordinates); } } else if (dependencyResult instanceof UnresolvedDependencyResult) { UnresolvedDependencyResult unresolvedResult = (UnresolvedDependencyResult) dependencyResult; @@ -243,6 +266,8 @@ private void recordDependencyPaths( } } + // Hacky approach to mark it as already checked even without a match + checkedCoordinates.put(stack.peekLast(), ""); stack.removeLast(); } @@ -263,7 +288,12 @@ private String dependencyPathToArtifacts( ImmutableListMultimap.Builder coordinatesToDependencyPaths = ImmutableListMultimap.builder(); - recordDependencyPaths(coordinatesToDependencyPaths, stack, targetCoordinates, new HashSet<>()); + recordDependencyPaths( + coordinatesToDependencyPaths, + stack, + targetCoordinates, + new HashSet<>(), + MultimapBuilder.hashKeys().arrayListValues().build()); ImmutableListMultimap dependencyPaths = coordinatesToDependencyPaths.build(); for (String coordinates : dependencyPaths.keySet()) { From d38c8dbbe447f6ab74473c4be261869b6ee358c3 Mon Sep 17 00:00:00 2001 From: Tomo Suzuki Date: Fri, 13 Aug 2021 15:07:08 -0400 Subject: [PATCH 2/6] Level-order to traverse dependency tree --- .../gradle/BuildStatusFunctionalTest.groovy | 11 + .../dependencies/gradle/LinkageCheckTask.java | 236 ++++++++++++------ 2 files changed, 168 insertions(+), 79 deletions(-) diff --git a/gradle-plugin/src/functionalTest/groovy/com/google/cloud/tools/dependencies/gradle/BuildStatusFunctionalTest.groovy b/gradle-plugin/src/functionalTest/groovy/com/google/cloud/tools/dependencies/gradle/BuildStatusFunctionalTest.groovy index c01ec0b51f..b9e7d05991 100644 --- a/gradle-plugin/src/functionalTest/groovy/com/google/cloud/tools/dependencies/gradle/BuildStatusFunctionalTest.groovy +++ b/gradle-plugin/src/functionalTest/groovy/com/google/cloud/tools/dependencies/gradle/BuildStatusFunctionalTest.groovy @@ -111,6 +111,17 @@ class BuildStatusFunctionalTest extends Specification { |io.grpc:grpc-grpclb:1.28.1 is at: | g:test-123:0.1.0-SNAPSHOT / com.google.cloud:google-cloud-logging:1.101.1 / com.google.api:gax-grpc:1.56.0 / io.grpc:grpc-alts:1.28.1 / io.grpc:grpc-grpclb:1.28.1 |""".stripMargin()) + + // "(omitted for duplicate)" should come after the non-omitted items + result.output.contains(""" + |io.grpc:grpc-alts:1.28.1 is at: + | g:test-123:0.1.0-SNAPSHOT / com.google.cloud:google-cloud-logging:1.101.1 / com.google.api:gax-grpc:1.56.0 / io.grpc:grpc-alts:1.28.1 + |""".stripMargin()) + + // Ensure the node closest to the root is printed + result.output.contains("g:test-123:0.1.0-SNAPSHOT / io.grpc:grpc-core:1.29.0") + // Ensure that the relationship between grpc-netty-shaded to grpc-core is only printed once + result.output.count("io.grpc:grpc-netty-shaded:1.28.1 / io.grpc:grpc-core:1.29.0") == 1 result.task(":linkageCheck").outcome == TaskOutcome.FAILED } diff --git a/gradle-plugin/src/main/java/com/google/cloud/tools/dependencies/gradle/LinkageCheckTask.java b/gradle-plugin/src/main/java/com/google/cloud/tools/dependencies/gradle/LinkageCheckTask.java index 02b6d25c2c..f505f87112 100644 --- a/gradle-plugin/src/main/java/com/google/cloud/tools/dependencies/gradle/LinkageCheckTask.java +++ b/gradle-plugin/src/main/java/com/google/cloud/tools/dependencies/gradle/LinkageCheckTask.java @@ -30,18 +30,22 @@ import com.google.cloud.tools.opensource.dependencies.DependencyGraph; import com.google.cloud.tools.opensource.dependencies.DependencyPath; import com.google.cloud.tools.opensource.dependencies.PathToNode; +import com.google.common.base.Joiner; +import com.google.common.base.MoreObjects; import com.google.common.collect.ImmutableList; -import com.google.common.collect.ImmutableListMultimap; import com.google.common.collect.ImmutableSet; -import com.google.common.collect.Multimap; +import com.google.common.collect.ListMultimap; import com.google.common.collect.MultimapBuilder; import java.io.IOException; import java.nio.file.Path; import java.nio.file.Paths; import java.util.ArrayDeque; +import java.util.ArrayList; +import java.util.Collections; import java.util.HashSet; +import java.util.List; +import java.util.Objects; import java.util.Set; -import java.util.stream.Collectors; import org.eclipse.aether.artifact.Artifact; import org.eclipse.aether.artifact.DefaultArtifact; import org.eclipse.aether.graph.Dependency; @@ -193,82 +197,168 @@ private String dependencyPathsOfProblematicJars( + dependencyPathToArtifacts(componentResult, problematicJars.build()); } - String formatComponentResult(ResolvedComponentResult componentResult) { + static String formatComponentResult(ResolvedComponentResult componentResult) { ModuleVersionIdentifier identifier = componentResult.getModuleVersion(); return identifier.toString(); } - private void recordDependencyPaths( - ImmutableListMultimap.Builder output, - ArrayDeque stack, - ImmutableSet targetCoordinates, - Set checkedCircularDependency, - Multimap checkedCoordinates) { - ResolvedComponentResult item = stack.getLast(); - ModuleVersionIdentifier identifier = item.getModuleVersion(); - String coordinates = - String.format( - "%s:%s:%s", identifier.getGroup(), identifier.getName(), identifier.getVersion()); - if (targetCoordinates.contains(coordinates)) { - String dependencyPath = - stack.stream().map(this::formatComponentResult).collect(Collectors.joining(" / ")); - output.put(coordinates, dependencyPath); - for (ResolvedComponentResult result : stack) { - if (result.equals(stack.peekLast())) { - continue; - } - checkedCoordinates.put(result, coordinates); + /** Dependency nodes to record dependency paths while traversing the dependency tree */ + private static class ResolvedComponentResultNode { + ResolvedComponentResult componentResult; + + ResolvedComponentResultNode parent; + + ResolvedComponentResultNode( + ResolvedComponentResult componentResult, ResolvedComponentResultNode parent) { + this.componentResult = componentResult; + this.parent = parent; + } + + boolean hasParent(ResolvedComponentResult other) { + if (componentResult.equals(other)) { + return true; + } + if (parent == null) { + return false; + } + return parent.hasParent(other); + } + + public String pathFromRoot() { + ResolvedComponentResultNode iter = this; + List dependencyPathElementsReversed = new ArrayList<>(); + while (iter != null) { + dependencyPathElementsReversed.add(formatComponentResult(iter.componentResult)); + iter = iter.parent; } + Collections.reverse(dependencyPathElementsReversed); + String dependencyPath = Joiner.on(" / ").join(dependencyPathElementsReversed); + return dependencyPath; } - for (DependencyResult dependencyResult : item.getDependencies()) { - if (dependencyResult instanceof ResolvedDependencyResult) { - ResolvedDependencyResult resolvedDependencyResult = - (ResolvedDependencyResult) dependencyResult; - ResolvedComponentResult child = resolvedDependencyResult.getSelected(); + @Override + public String toString() { + return MoreObjects.toStringHelper(this) + .add("componentResult", componentResult) + .add("parent", parent) + .toString(); + } - if (stack.contains(child)) { - // Circular dependency check - if (checkedCircularDependency.add(child)) { - getLogger() - .error( - "Circular dependency for: " - + resolvedDependencyResult - + "\n The stack is: " - + stack); + @Override + public boolean equals(Object other) { + if (this == other) { + return true; + } + if (other == null || getClass() != other.getClass()) { + return false; + } + ResolvedComponentResultNode that = (ResolvedComponentResultNode) other; + return Objects.equals(componentResult, that.componentResult) + && Objects.equals(parent, that.parent); + } + + @Override + public int hashCode() { + return Objects.hash(componentResult, parent); + } + } + + /** + * Returns mapping from Maven coordinates to their dependency paths appearing in the dependency + * graph + * + * @param componentResult The root project in the dependency graph + * @param targetCoordinatesSet The Maven coordinates to check their dependency paths + */ + private ListMultimap groupCoordinatesToDependencyPaths( + ResolvedComponentResult componentResult, Set targetCoordinatesSet) { + + ListMultimap coordinatesToDependencyPaths = + MultimapBuilder.hashKeys().arrayListValues().build(); + // No need to print the same circular dependency multiple times + Set checkedCircularDependency = new HashSet<>(); + + for (String targetCoordinates : targetCoordinatesSet) { + // Queue of dependnecy nodes. Each node knows its parent. + ArrayDeque queue = new ArrayDeque<>(); + ResolvedComponentResultNode firstItem = + new ResolvedComponentResultNode(componentResult, null); + queue.add(firstItem); + + // Mapping to omit duplicate dependency paths in the output. This is a mapping from Maven + // coordinates to dependency nodes that has the dependency of the coordinate in their direct + // or transitive dependencies. + Set nodesDependOnTarget = new HashSet<>(); + + while (!queue.isEmpty()) { + ResolvedComponentResultNode node = queue.poll(); + ResolvedComponentResult item = node.componentResult; + + ModuleVersionIdentifier identifier = item.getModuleVersion(); + String coordinates = + String.format( + "%s:%s:%s", identifier.getGroup(), identifier.getName(), identifier.getVersion()); + if (targetCoordinates.equals(coordinates)) { + String dependencyPath = node.pathFromRoot(); + coordinatesToDependencyPaths.put(targetCoordinates, dependencyPath); + + ResolvedComponentResultNode iter = node.parent; + while (iter != null) { + nodesDependOnTarget.add(iter.componentResult); + iter = iter.parent; } - } else if (checkedCoordinates.containsKey(child)) { - for (String matchedCoordinates : checkedCoordinates.get(child)) { - if (matchedCoordinates.isEmpty()) { - continue; - } + } - stack.add(child); - String dependencyPath = - stack.stream().map(this::formatComponentResult).collect(Collectors.joining(" / ")) - + " (*)"; // (*) is to mark that this was already seen - stack.removeLast(); - output.put(matchedCoordinates, dependencyPath); + if (nodesDependOnTarget.contains(item)) { + // Do not show duplicate dependency paths. If we know that this node contains dependency + // having targetCoordinates in its direct/transitive dependencies, there is no need to + // print the dependency paths again. + String dependencyPath = node.pathFromRoot() + " (omitted for duplicate)"; + coordinatesToDependencyPaths.put(targetCoordinates, dependencyPath); + + ResolvedComponentResultNode iter = node; + while (iter != null) { + nodesDependOnTarget.add(iter.componentResult); + iter = iter.parent; + } + continue; + } + + for (DependencyResult dependencyResult : item.getDependencies()) { + if (dependencyResult instanceof ResolvedDependencyResult) { + ResolvedDependencyResult resolvedDependencyResult = + (ResolvedDependencyResult) dependencyResult; + ResolvedComponentResult child = resolvedDependencyResult.getSelected(); + + if (node.hasParent(child)) { + // Circular dependency check + if (checkedCircularDependency.add(child)) { + getLogger() + .error( + "Circular dependency for: " + + resolvedDependencyResult + + "\n The stack is: " + + node.pathFromRoot()); + } + } else { + ResolvedComponentResultNode childNode = new ResolvedComponentResultNode(child, node); + queue.add(childNode); + } + } else if (dependencyResult instanceof UnresolvedDependencyResult) { + UnresolvedDependencyResult unresolvedResult = + (UnresolvedDependencyResult) dependencyResult; + getLogger() + .error( + "Could not resolve dependency: " + + unresolvedResult.getAttempted().getDisplayName()); + } else { + getLogger().error("Unexpected dependency result type: " + dependencyResult); } - } else { - stack.add(child); - recordDependencyPaths( - output, stack, targetCoordinates, checkedCircularDependency, checkedCoordinates); } - } else if (dependencyResult instanceof UnresolvedDependencyResult) { - UnresolvedDependencyResult unresolvedResult = (UnresolvedDependencyResult) dependencyResult; - getLogger() - .error( - "Could not resolve dependency: " - + unresolvedResult.getAttempted().getDisplayName()); - } else { - getLogger().error("Unexpected dependency result type: " + dependencyResult); } } - // Hacky approach to mark it as already checked even without a match - checkedCoordinates.put(stack.peekLast(), ""); - stack.removeLast(); + return coordinatesToDependencyPaths; } private String dependencyPathToArtifacts( @@ -280,22 +370,10 @@ private String dependencyPathToArtifacts( .map(Artifacts::toCoordinates) .collect(toImmutableSet()); - StringBuilder output = new StringBuilder(); + ListMultimap dependencyPaths = + groupCoordinatesToDependencyPaths(componentResult, targetCoordinates); - ArrayDeque stack = new ArrayDeque<>(); - stack.add(componentResult); - - ImmutableListMultimap.Builder coordinatesToDependencyPaths = - ImmutableListMultimap.builder(); - - recordDependencyPaths( - coordinatesToDependencyPaths, - stack, - targetCoordinates, - new HashSet<>(), - MultimapBuilder.hashKeys().arrayListValues().build()); - - ImmutableListMultimap dependencyPaths = coordinatesToDependencyPaths.build(); + StringBuilder output = new StringBuilder(); for (String coordinates : dependencyPaths.keySet()) { output.append(coordinates + " is at:\n"); for (String dependencyPath : dependencyPaths.get(coordinates)) { From e052b56d9a7f7c3940bcab6bf1442bf9c2efa114 Mon Sep 17 00:00:00 2001 From: Tomo Suzuki Date: Fri, 13 Aug 2021 18:10:39 -0400 Subject: [PATCH 3/6] Use for-loop to iterate nodes to root --- .../tools/dependencies/gradle/LinkageCheckTask.java | 12 +++--------- 1 file changed, 3 insertions(+), 9 deletions(-) diff --git a/gradle-plugin/src/main/java/com/google/cloud/tools/dependencies/gradle/LinkageCheckTask.java b/gradle-plugin/src/main/java/com/google/cloud/tools/dependencies/gradle/LinkageCheckTask.java index f505f87112..5eaee6f09e 100644 --- a/gradle-plugin/src/main/java/com/google/cloud/tools/dependencies/gradle/LinkageCheckTask.java +++ b/gradle-plugin/src/main/java/com/google/cloud/tools/dependencies/gradle/LinkageCheckTask.java @@ -225,11 +225,9 @@ boolean hasParent(ResolvedComponentResult other) { } public String pathFromRoot() { - ResolvedComponentResultNode iter = this; List dependencyPathElementsReversed = new ArrayList<>(); - while (iter != null) { + for (ResolvedComponentResultNode iter = this; iter != null; iter = iter.parent) { dependencyPathElementsReversed.add(formatComponentResult(iter.componentResult)); - iter = iter.parent; } Collections.reverse(dependencyPathElementsReversed); String dependencyPath = Joiner.on(" / ").join(dependencyPathElementsReversed); @@ -302,10 +300,8 @@ private ListMultimap groupCoordinatesToDependencyPaths( String dependencyPath = node.pathFromRoot(); coordinatesToDependencyPaths.put(targetCoordinates, dependencyPath); - ResolvedComponentResultNode iter = node.parent; - while (iter != null) { + for (ResolvedComponentResultNode iter = node.parent; iter != null; iter = iter.parent) { nodesDependOnTarget.add(iter.componentResult); - iter = iter.parent; } } @@ -316,10 +312,8 @@ private ListMultimap groupCoordinatesToDependencyPaths( String dependencyPath = node.pathFromRoot() + " (omitted for duplicate)"; coordinatesToDependencyPaths.put(targetCoordinates, dependencyPath); - ResolvedComponentResultNode iter = node; - while (iter != null) { + for (ResolvedComponentResultNode iter = node; iter != null; iter = iter.parent) { nodesDependOnTarget.add(iter.componentResult); - iter = iter.parent; } continue; } From d6106de235be38ff3ec45f76bc5dfde87c0c3536 Mon Sep 17 00:00:00 2001 From: Tomo Suzuki Date: Mon, 16 Aug 2021 16:50:01 -0400 Subject: [PATCH 4/6] Applied review --- .../dependencies/gradle/DependencyNode.java | 88 +++++++++ .../dependencies/gradle/LinkageCheckTask.java | 169 ++++++------------ 2 files changed, 144 insertions(+), 113 deletions(-) create mode 100644 gradle-plugin/src/main/java/com/google/cloud/tools/dependencies/gradle/DependencyNode.java diff --git a/gradle-plugin/src/main/java/com/google/cloud/tools/dependencies/gradle/DependencyNode.java b/gradle-plugin/src/main/java/com/google/cloud/tools/dependencies/gradle/DependencyNode.java new file mode 100644 index 0000000000..a8db93ae82 --- /dev/null +++ b/gradle-plugin/src/main/java/com/google/cloud/tools/dependencies/gradle/DependencyNode.java @@ -0,0 +1,88 @@ +/* + * Copyright 2021 Google LLC. + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +package com.google.cloud.tools.dependencies.gradle; + +import com.google.common.base.Joiner; +import com.google.common.base.MoreObjects; +import com.google.common.collect.ImmutableList; +import java.util.ArrayDeque; +import java.util.List; +import java.util.Objects; +import java.util.stream.Collectors; +import org.gradle.api.artifacts.result.ResolvedComponentResult; + +/** Dependency nodes to record dependency paths while traversing the dependency tree */ +class DependencyNode { + ResolvedComponentResult componentResult; + + DependencyNode parent; + + DependencyNode( + ResolvedComponentResult componentResult, DependencyNode parent) { + this.componentResult = componentResult; + this.parent = parent; + } + + boolean isDescendantOf(ResolvedComponentResult other) { + if (componentResult.equals(other)) { + return true; + } + if (parent == null) { + return false; + } + return parent.isDescendantOf(other); + } + + String pathFromRoot() { + return rootToNode().stream().map(LinkageCheckTask::formatComponentResult) + .collect(Collectors.joining(" / ")); + } + + ImmutableList rootToNode() { + ArrayDeque nodes = new ArrayDeque<>(); + for (DependencyNode iter = this; iter != null; iter = iter.parent) { + nodes.addFirst(iter.componentResult); + } + return ImmutableList.copyOf(nodes); + } + + @Override + public String toString() { + return MoreObjects.toStringHelper(this) + .add("componentResult", componentResult) + .add("parent", parent) + .toString(); + } + + @Override + public boolean equals(Object other) { + if (this == other) { + return true; + } + if (other == null || getClass() != other.getClass()) { + return false; + } + DependencyNode that = (DependencyNode) other; + return Objects.equals(componentResult, that.componentResult) + && Objects.equals(parent, that.parent); + } + + @Override + public int hashCode() { + return Objects.hash(componentResult, parent); + } +} diff --git a/gradle-plugin/src/main/java/com/google/cloud/tools/dependencies/gradle/LinkageCheckTask.java b/gradle-plugin/src/main/java/com/google/cloud/tools/dependencies/gradle/LinkageCheckTask.java index 5eaee6f09e..0b912979a0 100644 --- a/gradle-plugin/src/main/java/com/google/cloud/tools/dependencies/gradle/LinkageCheckTask.java +++ b/gradle-plugin/src/main/java/com/google/cloud/tools/dependencies/gradle/LinkageCheckTask.java @@ -30,8 +30,6 @@ import com.google.cloud.tools.opensource.dependencies.DependencyGraph; import com.google.cloud.tools.opensource.dependencies.DependencyPath; import com.google.cloud.tools.opensource.dependencies.PathToNode; -import com.google.common.base.Joiner; -import com.google.common.base.MoreObjects; import com.google.common.collect.ImmutableList; import com.google.common.collect.ImmutableSet; import com.google.common.collect.ListMultimap; @@ -41,10 +39,8 @@ import java.nio.file.Paths; import java.util.ArrayDeque; import java.util.ArrayList; -import java.util.Collections; import java.util.HashSet; import java.util.List; -import java.util.Objects; import java.util.Set; import org.eclipse.aether.artifact.Artifact; import org.eclipse.aether.artifact.DefaultArtifact; @@ -70,6 +66,9 @@ public class LinkageCheckTask extends DefaultTask { private LinkageCheckerPluginExtension extension; + // A set to avoid printing the same circular dependency multiple times + private Set checkedCircularDependency = new HashSet<>(); + @TaskAction public void run() throws IOException { extension = getProject().getExtensions().findByType(LinkageCheckerPluginExtension.class); @@ -202,94 +201,34 @@ static String formatComponentResult(ResolvedComponentResult componentResult) { return identifier.toString(); } - /** Dependency nodes to record dependency paths while traversing the dependency tree */ - private static class ResolvedComponentResultNode { - ResolvedComponentResult componentResult; - - ResolvedComponentResultNode parent; - - ResolvedComponentResultNode( - ResolvedComponentResult componentResult, ResolvedComponentResultNode parent) { - this.componentResult = componentResult; - this.parent = parent; - } - - boolean hasParent(ResolvedComponentResult other) { - if (componentResult.equals(other)) { - return true; - } - if (parent == null) { - return false; - } - return parent.hasParent(other); - } - - public String pathFromRoot() { - List dependencyPathElementsReversed = new ArrayList<>(); - for (ResolvedComponentResultNode iter = this; iter != null; iter = iter.parent) { - dependencyPathElementsReversed.add(formatComponentResult(iter.componentResult)); - } - Collections.reverse(dependencyPathElementsReversed); - String dependencyPath = Joiner.on(" / ").join(dependencyPathElementsReversed); - return dependencyPath; - } - - @Override - public String toString() { - return MoreObjects.toStringHelper(this) - .add("componentResult", componentResult) - .add("parent", parent) - .toString(); - } - - @Override - public boolean equals(Object other) { - if (this == other) { - return true; - } - if (other == null || getClass() != other.getClass()) { - return false; - } - ResolvedComponentResultNode that = (ResolvedComponentResultNode) other; - return Objects.equals(componentResult, that.componentResult) - && Objects.equals(parent, that.parent); - } - - @Override - public int hashCode() { - return Objects.hash(componentResult, parent); - } - } - /** * Returns mapping from Maven coordinates to their dependency paths appearing in the dependency - * graph + * graph. The output do not have duplicate dependency paths. * - * @param componentResult The root project in the dependency graph + * @param rootProject The root project in the dependency graph * @param targetCoordinatesSet The Maven coordinates to check their dependency paths */ private ListMultimap groupCoordinatesToDependencyPaths( - ResolvedComponentResult componentResult, Set targetCoordinatesSet) { + ResolvedComponentResult rootProject, Set targetCoordinatesSet) { ListMultimap coordinatesToDependencyPaths = MultimapBuilder.hashKeys().arrayListValues().build(); - // No need to print the same circular dependency multiple times - Set checkedCircularDependency = new HashSet<>(); for (String targetCoordinates : targetCoordinatesSet) { - // Queue of dependnecy nodes. Each node knows its parent. - ArrayDeque queue = new ArrayDeque<>(); - ResolvedComponentResultNode firstItem = - new ResolvedComponentResultNode(componentResult, null); + // Queue of dependency nodes. Each node knows its parent. + ArrayDeque queue = new ArrayDeque<>(); + DependencyNode firstItem = + new DependencyNode(rootProject, null); queue.add(firstItem); - // Mapping to omit duplicate dependency paths in the output. This is a mapping from Maven - // coordinates to dependency nodes that has the dependency of the coordinate in their direct - // or transitive dependencies. + // A set to omit duplicate dependency paths in the output. When a node is found to be in this + // set while traversing the graph, we do not need to check the children, because we know that + // the dependency paths from that node to the targetCoordinates are already added to + // coordinatesToDependencyPaths. Set nodesDependOnTarget = new HashSet<>(); while (!queue.isEmpty()) { - ResolvedComponentResultNode node = queue.poll(); + DependencyNode node = queue.poll(); ResolvedComponentResult item = node.componentResult; ModuleVersionIdentifier identifier = item.getModuleVersion(); @@ -300,59 +239,63 @@ private ListMultimap groupCoordinatesToDependencyPaths( String dependencyPath = node.pathFromRoot(); coordinatesToDependencyPaths.put(targetCoordinates, dependencyPath); - for (ResolvedComponentResultNode iter = node.parent; iter != null; iter = iter.parent) { - nodesDependOnTarget.add(iter.componentResult); + if (node.parent != null) { + nodesDependOnTarget.addAll(node.parent.rootToNode()); } } if (nodesDependOnTarget.contains(item)) { - // Do not show duplicate dependency paths. If we know that this node contains dependency - // having targetCoordinates in its direct/transitive dependencies, there is no need to - // print the dependency paths again. + // Omitting duplicate dependency paths by checking nodesDependOnTarget. String dependencyPath = node.pathFromRoot() + " (omitted for duplicate)"; coordinatesToDependencyPaths.put(targetCoordinates, dependencyPath); - for (ResolvedComponentResultNode iter = node; iter != null; iter = iter.parent) { - nodesDependOnTarget.add(iter.componentResult); - } + nodesDependOnTarget.addAll(node.rootToNode()); continue; } - for (DependencyResult dependencyResult : item.getDependencies()) { - if (dependencyResult instanceof ResolvedDependencyResult) { - ResolvedDependencyResult resolvedDependencyResult = - (ResolvedDependencyResult) dependencyResult; - ResolvedComponentResult child = resolvedDependencyResult.getSelected(); - - if (node.hasParent(child)) { - // Circular dependency check - if (checkedCircularDependency.add(child)) { - getLogger() - .error( - "Circular dependency for: " - + resolvedDependencyResult - + "\n The stack is: " - + node.pathFromRoot()); - } - } else { - ResolvedComponentResultNode childNode = new ResolvedComponentResultNode(child, node); - queue.add(childNode); - } - } else if (dependencyResult instanceof UnresolvedDependencyResult) { - UnresolvedDependencyResult unresolvedResult = - (UnresolvedDependencyResult) dependencyResult; + queue.addAll(getDependencies(node)); + } + } + + return coordinatesToDependencyPaths; + } + + private List getDependencies(DependencyNode node) { + ResolvedComponentResult item = node.componentResult; + + List childNodes = new ArrayList<>(); + for (DependencyResult dependencyResult : item.getDependencies()) { + if (dependencyResult instanceof ResolvedDependencyResult) { + ResolvedDependencyResult resolvedDependencyResult = + (ResolvedDependencyResult) dependencyResult; + ResolvedComponentResult child = resolvedDependencyResult.getSelected(); + + if (node.isDescendantOf(child)) { + // The child appears in the descendants of the node. It's a circular dependency. + if (checkedCircularDependency.add(child)) { + // No need to print the circular dependency information multiple times. getLogger() .error( - "Could not resolve dependency: " - + unresolvedResult.getAttempted().getDisplayName()); - } else { - getLogger().error("Unexpected dependency result type: " + dependencyResult); + "Circular dependency for: " + + resolvedDependencyResult + + "\n The stack is: " + + node.pathFromRoot()); } + } else { + childNodes.add(new DependencyNode(child, node)); } + } else if (dependencyResult instanceof UnresolvedDependencyResult) { + UnresolvedDependencyResult unresolvedResult = + (UnresolvedDependencyResult) dependencyResult; + getLogger() + .error( + "Could not resolve dependency: " + + unresolvedResult.getAttempted().getDisplayName()); + } else { + getLogger().error("Unexpected dependency result type: " + dependencyResult); } } - - return coordinatesToDependencyPaths; + return childNodes; } private String dependencyPathToArtifacts( From 701bfff1f389aaf4976bc029fa5582e25ea9a518 Mon Sep 17 00:00:00 2001 From: Tomo Suzuki Date: Mon, 16 Aug 2021 17:17:48 -0400 Subject: [PATCH 5/6] Clarifying if-statements --- .../tools/dependencies/gradle/LinkageCheckTask.java | 9 +++------ 1 file changed, 3 insertions(+), 6 deletions(-) diff --git a/gradle-plugin/src/main/java/com/google/cloud/tools/dependencies/gradle/LinkageCheckTask.java b/gradle-plugin/src/main/java/com/google/cloud/tools/dependencies/gradle/LinkageCheckTask.java index 0b912979a0..561f442025 100644 --- a/gradle-plugin/src/main/java/com/google/cloud/tools/dependencies/gradle/LinkageCheckTask.java +++ b/gradle-plugin/src/main/java/com/google/cloud/tools/dependencies/gradle/LinkageCheckTask.java @@ -242,18 +242,15 @@ private ListMultimap groupCoordinatesToDependencyPaths( if (node.parent != null) { nodesDependOnTarget.addAll(node.parent.rootToNode()); } - } - - if (nodesDependOnTarget.contains(item)) { + } else if (nodesDependOnTarget.contains(item)) { // Omitting duplicate dependency paths by checking nodesDependOnTarget. String dependencyPath = node.pathFromRoot() + " (omitted for duplicate)"; coordinatesToDependencyPaths.put(targetCoordinates, dependencyPath); nodesDependOnTarget.addAll(node.rootToNode()); - continue; + } else { + queue.addAll(getDependencies(node)); } - - queue.addAll(getDependencies(node)); } } From fb2b05387a83c6f48dd31db53bc24f24a56585c5 Mon Sep 17 00:00:00 2001 From: Tomo Suzuki Date: Mon, 16 Aug 2021 23:30:58 -0400 Subject: [PATCH 6/6] getDependencies -> findDependencies --- .../dependencies/gradle/DependencyNode.java | 18 +++++++-------- .../dependencies/gradle/LinkageCheckTask.java | 22 ++++++++----------- 2 files changed, 17 insertions(+), 23 deletions(-) diff --git a/gradle-plugin/src/main/java/com/google/cloud/tools/dependencies/gradle/DependencyNode.java b/gradle-plugin/src/main/java/com/google/cloud/tools/dependencies/gradle/DependencyNode.java index a8db93ae82..e7351d56dc 100644 --- a/gradle-plugin/src/main/java/com/google/cloud/tools/dependencies/gradle/DependencyNode.java +++ b/gradle-plugin/src/main/java/com/google/cloud/tools/dependencies/gradle/DependencyNode.java @@ -16,28 +16,25 @@ package com.google.cloud.tools.dependencies.gradle; -import com.google.common.base.Joiner; import com.google.common.base.MoreObjects; import com.google.common.collect.ImmutableList; import java.util.ArrayDeque; -import java.util.List; import java.util.Objects; import java.util.stream.Collectors; import org.gradle.api.artifacts.result.ResolvedComponentResult; /** Dependency nodes to record dependency paths while traversing the dependency tree */ -class DependencyNode { +final class DependencyNode { ResolvedComponentResult componentResult; DependencyNode parent; - DependencyNode( - ResolvedComponentResult componentResult, DependencyNode parent) { + DependencyNode(ResolvedComponentResult componentResult, DependencyNode parent) { this.componentResult = componentResult; this.parent = parent; } - boolean isDescendantOf(ResolvedComponentResult other) { + boolean isDescendantOf(ResolvedComponentResult other) { if (componentResult.equals(other)) { return true; } @@ -47,12 +44,13 @@ boolean isDescendantOf(ResolvedComponentResult other) { return parent.isDescendantOf(other); } - String pathFromRoot() { - return rootToNode().stream().map(LinkageCheckTask::formatComponentResult) - .collect(Collectors.joining(" / ")); + String pathFromRoot() { + return fromRootToNode().stream() + .map(LinkageCheckTask::formatComponentResult) + .collect(Collectors.joining(" / ")); } - ImmutableList rootToNode() { + ImmutableList fromRootToNode() { ArrayDeque nodes = new ArrayDeque<>(); for (DependencyNode iter = this; iter != null; iter = iter.parent) { nodes.addFirst(iter.componentResult); diff --git a/gradle-plugin/src/main/java/com/google/cloud/tools/dependencies/gradle/LinkageCheckTask.java b/gradle-plugin/src/main/java/com/google/cloud/tools/dependencies/gradle/LinkageCheckTask.java index 561f442025..b4dc7d96c0 100644 --- a/gradle-plugin/src/main/java/com/google/cloud/tools/dependencies/gradle/LinkageCheckTask.java +++ b/gradle-plugin/src/main/java/com/google/cloud/tools/dependencies/gradle/LinkageCheckTask.java @@ -217,14 +217,10 @@ private ListMultimap groupCoordinatesToDependencyPaths( for (String targetCoordinates : targetCoordinatesSet) { // Queue of dependency nodes. Each node knows its parent. ArrayDeque queue = new ArrayDeque<>(); - DependencyNode firstItem = - new DependencyNode(rootProject, null); + DependencyNode firstItem = new DependencyNode(rootProject, null); queue.add(firstItem); - // A set to omit duplicate dependency paths in the output. When a node is found to be in this - // set while traversing the graph, we do not need to check the children, because we know that - // the dependency paths from that node to the targetCoordinates are already added to - // coordinatesToDependencyPaths. + // A set of dependencies to omit duplicate dependency paths in the output. Set nodesDependOnTarget = new HashSet<>(); while (!queue.isEmpty()) { @@ -240,16 +236,17 @@ private ListMultimap groupCoordinatesToDependencyPaths( coordinatesToDependencyPaths.put(targetCoordinates, dependencyPath); if (node.parent != null) { - nodesDependOnTarget.addAll(node.parent.rootToNode()); + nodesDependOnTarget.addAll(node.parent.fromRootToNode()); } } else if (nodesDependOnTarget.contains(item)) { - // Omitting duplicate dependency paths by checking nodesDependOnTarget. + // Omitting duplicate dependency paths when we already know this item depends on + // targetCoordinates, directly or transitively. String dependencyPath = node.pathFromRoot() + " (omitted for duplicate)"; coordinatesToDependencyPaths.put(targetCoordinates, dependencyPath); - nodesDependOnTarget.addAll(node.rootToNode()); + nodesDependOnTarget.addAll(node.fromRootToNode()); } else { - queue.addAll(getDependencies(node)); + queue.addAll(findDependencies(node)); } } } @@ -257,7 +254,7 @@ private ListMultimap groupCoordinatesToDependencyPaths( return coordinatesToDependencyPaths; } - private List getDependencies(DependencyNode node) { + private List findDependencies(DependencyNode node) { ResolvedComponentResult item = node.componentResult; List childNodes = new ArrayList<>(); @@ -282,8 +279,7 @@ private List getDependencies(DependencyNode node) { childNodes.add(new DependencyNode(child, node)); } } else if (dependencyResult instanceof UnresolvedDependencyResult) { - UnresolvedDependencyResult unresolvedResult = - (UnresolvedDependencyResult) dependencyResult; + UnresolvedDependencyResult unresolvedResult = (UnresolvedDependencyResult) dependencyResult; getLogger() .error( "Could not resolve dependency: "