Uh oh!
There was an error while loading. Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork 80
LevelOrder not to call resolveCompileDependencies#1239
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Uh oh!
There was an error while loading. Please reload this page.
Changes from all commits
ac1d35151b56f1a0a508d48366c2844032bc0ad980b2bca2d39ed664098b05bb4f71cf643618de1577c31c341ce1d68530File filter
Filter by extension
Conversations
Uh oh!
There was an error while loading. Please reload this page.
Jump to
Uh oh!
There was an error while loading. Please reload this page.
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,69 @@ | ||
| /* | ||
| * Copyright 2020 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.opensource.dependencies; | ||
| import com.google.common.collect.ImmutableList; | ||
| import java.util.HashSet; | ||
| import java.util.Set; | ||
| import org.eclipse.aether.RepositoryException; | ||
| import org.eclipse.aether.artifact.Artifact; | ||
| import org.eclipse.aether.collection.DependencyGraphTransformationContext; | ||
| import org.eclipse.aether.collection.DependencyGraphTransformer; | ||
| import org.eclipse.aether.graph.DependencyNode; | ||
| /** | ||
| * Transforms a dependency graph so that it will not contain cycles. | ||
| * | ||
| * <p>A cycle in a dependency graph is a situation where a path to a node from the root contains the | ||
| * same node. For example, jaxen 1.1-beta-6 is known to have cycle with dom4j 1.6.1. | ||
| */ | ||
| final class CycleBreakerGraphTransformer implements DependencyGraphTransformer { | ||
| @Override | ||
| public DependencyNode transformGraph( | ||
| DependencyNode dependencyNode, DependencyGraphTransformationContext context) | ||
| throws RepositoryException { | ||
| removeCycle(null, dependencyNode, new HashSet<>()); | ||
| return dependencyNode; | ||
| } | ||
| private static void removeCycle( | ||
| DependencyNode parent, DependencyNode node, Set<Artifact> ancestors) { | ||
| Artifact artifact = node.getArtifact(); | ||
| if (ancestors.contains(artifact)) { // Set (rather than List) gives O(1) lookup here | ||
| // parent is not null when ancestors is not empty | ||
Contributor There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. How many elements are we talking about though? 10? Maybe 20 at the outside? Even if it were 1000 the difference is unlikely to be measurable. ContributorAuthor There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Sometimes around 1,000,000 nodes in a full dependency tree.
| ||
| removeChildFromParent(parent, node); | ||
| return; | ||
| } | ||
| ancestors.add(artifact); | ||
| for (DependencyNode child : node.getChildren()) { | ||
| removeCycle(node, child, ancestors); | ||
| } | ||
| ancestors.remove(artifact); | ||
| } | ||
| private static void removeChildFromParent(DependencyNode child, DependencyNode parent) { | ||
| ImmutableList<DependencyNode> children = | ||
| parent.getChildren().stream() | ||
| .filter(node -> node != child) | ||
| .collect(ImmutableList.toImmutableList()); | ||
| parent.setChildren(children); | ||
| } | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -253,11 +253,8 @@ private DependencyGraphResult buildDependencyGraph( | ||
| } | ||
| } | ||
| DependencyGraphResult result = levelOrder(node, traversalOption); | ||
| // Duplicate problems found in resolveDependencyGraph and levelOrder are removed by ImmutableSet | ||
| artifactProblems.addAll(result.getArtifactProblems()); | ||
| return new DependencyGraphResult(result.getDependencyGraph(), artifactProblems.build()); | ||
| DependencyGraph graph = levelOrder(node); | ||
| return new DependencyGraphResult(graph, artifactProblems.build()); | ||
| } | ||
| /** | ||
| @@ -316,15 +313,11 @@ private enum GraphTraversalOption { | ||
| * just follows the given dependency tree starting with firstNode. | ||
| * | ||
| * @param firstNode node to start traversal | ||
| * @param graphTraversalOption option to recursively resolve the dependency to build complete | ||
| * dependency tree, with or without dependencies of provided scope | ||
| */ | ||
| private DependencyGraphResult levelOrder( | ||
| DependencyNode firstNode, GraphTraversalOption graphTraversalOption) { | ||
| private DependencyGraph levelOrder(DependencyNode firstNode) { | ||
ContributorAuthor There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. No need to wrap DependencyGraph with DependencyGraphResult. Because this function does not encounter | ||
| DependencyGraph graph = new DependencyGraph(); | ||
| boolean resolveFullDependency = graphTraversalOption == GraphTraversalOption.FULL; | ||
| Queue<LevelOrderQueueItem> queue = new ArrayDeque<>(); | ||
| queue.add(new LevelOrderQueueItem(firstNode, new Stack<>())); | ||
| @@ -358,29 +351,6 @@ private DependencyGraphResult levelOrder( | ||
| path.add(dependencyNode.getDependency()); | ||
| parentNodes.push(dependencyNode); | ||
| graph.addPath(path); | ||
| if (resolveFullDependency && !"system".equals(dependencyNode.getDependency().getScope())) { | ||
| try { | ||
| dependencyNode = | ||
| resolveCompileTimeDependencies( | ||
| ImmutableList.of(dependencyNode), resolveFullDependency); | ||
| } catch (DependencyResolutionException resolutionException) { | ||
| // A dependency may be unavailable. For example, com.google.guava:guava-gwt:jar:20.0 | ||
| // has a transitive dependency to org.eclipse.jdt.core.compiler:ecj:jar:4.4RC4 (not | ||
| // found in Maven central) | ||
| for (ArtifactResult artifactResult : | ||
| resolutionException.getResult().getArtifactResults()) { | ||
| if (artifactResult.getArtifact() == null) { | ||
| DependencyNode failedDependencyNode = | ||
| artifactResult.getRequest().getDependencyNode(); | ||
| List<DependencyNode> fullPath = makeFullPath(parentNodes, failedDependencyNode); | ||
| artifactProblems.add(new UnresolvableArtifactProblem(fullPath)); | ||
| } | ||
| } | ||
| } | ||
| } | ||
Comment on lines
-362
to
-383
| ||
| } | ||
| for (DependencyNode child : dependencyNode.getChildren()) { | ||
| @@ -390,7 +360,7 @@ private DependencyGraphResult levelOrder( | ||
| } | ||
| } | ||
| return new DependencyGraphResult(graph, artifactProblems.build()); | ||
| return graph; | ||
| } | ||
| private static List<DependencyNode> makeFullPath( | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,58 @@ | ||
| /* | ||
| * Copyright 2020 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.opensource.dependencies; | ||
| import com.google.common.collect.ImmutableList; | ||
| import org.eclipse.aether.DefaultRepositorySystemSession; | ||
| import org.eclipse.aether.RepositorySystem; | ||
| import org.eclipse.aether.artifact.DefaultArtifact; | ||
| import org.eclipse.aether.collection.CollectRequest; | ||
| import org.eclipse.aether.graph.Dependency; | ||
| import org.eclipse.aether.resolution.DependencyRequest; | ||
| import org.eclipse.aether.resolution.DependencyResolutionException; | ||
| import org.eclipse.aether.util.graph.selector.ScopeDependencySelector; | ||
| import org.eclipse.aether.util.graph.transformer.ChainedDependencyGraphTransformer; | ||
| import org.eclipse.aether.util.graph.transformer.JavaDependencyContextRefiner; | ||
| import org.junit.Test; | ||
| public class CycleBreakerGraphTransformerTest { | ||
| @Test | ||
| public void testCycleBreaking() throws DependencyResolutionException { | ||
| RepositorySystem system = RepositoryUtility.newRepositorySystem(); | ||
| DefaultRepositorySystemSession session = | ||
| RepositoryUtility.createDefaultRepositorySystemSession(system); | ||
| // This dependencySelector selects everything except test scope. This creates a dependency tree | ||
| // with a cycle of dom4j:dom4j:jar:1.6.1 (optional) and jaxen:jaxen:jar:1.1-beta-6 (optional). | ||
| session.setDependencySelector(new ScopeDependencySelector("test")); | ||
| session.setDependencyGraphTransformer( | ||
| new ChainedDependencyGraphTransformer( | ||
| new CycleBreakerGraphTransformer(), // This prevents StackOverflowError | ||
ContributorAuthor There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Without this class, this test would throw StackOverflowError. | ||
| new JavaDependencyContextRefiner())); | ||
| // dom4j:1.6.1 is known to have a cycle | ||
| CollectRequest collectRequest = new CollectRequest(); | ||
| collectRequest.setRepositories(ImmutableList.of(RepositoryUtility.CENTRAL)); | ||
| collectRequest.setRoot(new Dependency(new DefaultArtifact("dom4j:dom4j:1.6.1"), "compile")); | ||
| DependencyRequest request = new DependencyRequest(collectRequest, null); | ||
| // This should not raise StackOverflowError | ||
| system.resolveDependencies(session, request); | ||
| } | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -74,9 +74,9 @@ public void testGetCompleteDependencies() throws RepositoryException { | ||
| HashSet<DependencyPath> noDups = new HashSet<>(paths); | ||
| Assert.assertEquals(paths.size(), noDups.size()); | ||
| // This method should find Guava multiple times. | ||
| // This method should find Guava multiple times, respecting exclusion elements | ||
| int guavaCount = countGuava(graph); | ||
| Assert.assertEquals(30, guavaCount); | ||
| Assert.assertEquals(29, guavaCount); | ||
| } | ||
| private static int countGuava(DependencyGraph graph) { | ||
| @@ -250,4 +250,34 @@ public void testBuildLinkageCheckDependencyGraph_catchRootException() throws Rep | ||
| "xml-apis:xml-apis:jar:2.6.2 was not resolved. Dependency path: ant:ant:jar:1.6.2" | ||
| + " (compile) > xml-apis:xml-apis:jar:2.6.2 (compile?)"); | ||
| } | ||
| @Test | ||
| public void testAlts_exclusionElements() throws RepositoryException { | ||
ContributorAuthor There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Before this PR, this test would fail because levelOrder was not respecting exclusion elements. | ||
| Correspondence<DependencyPath, String> dependencyPathToString = | ||
| Correspondence.transforming(DependencyPath::toString, "has string representation"); | ||
| DefaultArtifact artifact = new DefaultArtifact("io.grpc:grpc-alts:jar:1.27.0"); | ||
| DependencyGraph graph = | ||
| dependencyGraphBuilder | ||
| .buildFullDependencyGraph(ImmutableList.of(artifact)) | ||
| .getDependencyGraph(); | ||
| List<DependencyPath> dependencyPaths = graph.list(); | ||
| String expectedDependencyPathForOpencensusContribHttpUtil = | ||
| "io.grpc:grpc-alts:1.27.0 (compile) " // this has exclusion of Guava | ||
| + "/ com.google.auth:google-auth-library-oauth2-http:0.19.0 (compile) " | ||
| + "/ com.google.http-client:google-http-client:1.33.0 (compile) " | ||
| + "/ io.opencensus:opencensus-contrib-http-util:0.24.0 (compile)"; | ||
| Truth.assertThat(dependencyPaths) | ||
| .comparingElementsUsing(dependencyPathToString) | ||
| .contains(expectedDependencyPathForOpencensusContribHttpUtil); | ||
| String unexpectedDependencyPathForGuava = | ||
| expectedDependencyPathForOpencensusContribHttpUtil | ||
| + " / com.google.guava:guava:jar:26.0-android (compile)"; | ||
| Truth.assertThat(dependencyPaths) | ||
| .comparingElementsUsing(dependencyPathToString) | ||
| .doesNotContain(unexpectedDependencyPathForGuava); | ||
| } | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -74,8 +74,6 @@ public void testFindUpdates() throws RepositoryException { | ||
| + "upgrade com.google.protobuf:protobuf-java:3.5.1 to 3.6.0", | ||
| "com.google.api.grpc:proto-google-iam-v1:0.12.0 needs to " | ||
| + "upgrade com.google.protobuf:protobuf-java:3.5.1 to 3.6.0", | ||
| "com.google.guava:guava-jdk5:17.0 needs to " | ||
| + "upgrade com.google.code.findbugs:jsr305:1.3.9 to 3.0.2", | ||
ContributorAuthor There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The nodes in the tree changed because now exclusion elements take effect entire subtree. | ||
| "org.apache.httpcomponents:httpclient:4.0.1 needs to " | ||
| + "upgrade commons-codec:commons-codec:1.3 to 1.6"); | ||
| } | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Before this fix the artifact report for grpc-google-common-protos was showing Upper Bound Fixes for jsr305 version 3.0.1. But that version should not appear in the tree because grpc-core declares exclusions.
Tree: https://screenshot.googleplex.com/g8cQ3BXq36O.png
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The comment above was correct but
grpc-google-common-protos:1.14.0's dependency tree still has old protobuf-java dependency. So the section should be red.