From 916ecb970ace163640c727e20c1b8628bd132650 Mon Sep 17 00:00:00 2001 From: Lars Vogel Date: Tue, 28 Jul 2026 19:31:58 +0200 Subject: [PATCH] Report all dependency cycles through the edited plug-in DependencyLoopFinder kept a shared list of plug-ins that had been visited without yielding a loop and skipped them on every later path. Whether a plug-in yields a loop depends on the path taken to reach it: a branch that ends in a cycle not passing through the root adds the plug-ins it visited to that list, so a cycle reachable only through another dependency of the root is never reported. Which cycles get lost depends on the order of the Require-Bundle entries. Replace the list with a prune that does not depend on the path: only plug-ins that are reachable from the root and lead back to it can sit on a cycle through the root, and that property is a plain graph reachability question. Plug-ins outside that set are skipped, which also keeps the common case of a plug-in without any cycle cheap, and the remaining search enumerates the cycles without dropping any. The number of reported loops is capped, as the search runs on the UI thread. Resolved dependencies are cached for the duration of one search, since the search visits a plug-in once per path leading to it. --- .../core/builders/DependencyLoopFinder.java | 201 +++++++++++------- .../builders/DependencyLoopFinderTest.java | 28 +++ 2 files changed, 151 insertions(+), 78 deletions(-) diff --git a/ui/org.eclipse.pde.core/src/org/eclipse/pde/internal/core/builders/DependencyLoopFinder.java b/ui/org.eclipse.pde.core/src/org/eclipse/pde/internal/core/builders/DependencyLoopFinder.java index 1178ea1744a..da6828e9cf4 100644 --- a/ui/org.eclipse.pde.core/src/org/eclipse/pde/internal/core/builders/DependencyLoopFinder.java +++ b/ui/org.eclipse.pde.core/src/org/eclipse/pde/internal/core/builders/DependencyLoopFinder.java @@ -1,5 +1,5 @@ /******************************************************************************* - * Copyright (c) 2000, 2012 IBM Corporation and others. + * Copyright (c) 2000, 2026 IBM Corporation and others. * * This program and the accompanying materials * are made available under the terms of the Eclipse Public License 2.0 @@ -13,7 +13,15 @@ *******************************************************************************/ package org.eclipse.pde.internal.core.builders; -import java.util.Vector; +import java.util.ArrayDeque; +import java.util.ArrayList; +import java.util.Arrays; +import java.util.Deque; +import java.util.HashMap; +import java.util.HashSet; +import java.util.List; +import java.util.Map; +import java.util.Set; import org.eclipse.osgi.util.NLS; import org.eclipse.pde.core.plugin.IPlugin; @@ -25,6 +33,13 @@ public class DependencyLoopFinder { + /** + * Upper bound on the number of reported loops. The search runs + * synchronously on the UI thread and the number of distinct cycles through + * a plug-in can grow exponentially with the size of the tangle it sits in. + */ + private static final int MAX_LOOPS = 100; + public static DependencyLoop[] findLoops(IPlugin root) { return findLoops(root, null); } @@ -34,94 +49,128 @@ public static DependencyLoop[] findLoops(IPlugin root, IPlugin[] candidates) { } public static DependencyLoop[] findLoops(IPlugin root, IPlugin[] candidates, boolean onlyCandidates) { - Vector loops = new Vector<>(); - - Vector path = new Vector<>(); - findLoops(loops, path, root, candidates, onlyCandidates, new Vector<>()); - return loops.toArray(new DependencyLoop[loops.size()]); + LoopSearch search = new LoopSearch(root.getId()); + List rootDependencies = new ArrayList<>(); + if (!onlyCandidates) { + rootDependencies.addAll(search.dependenciesOf(root)); + } + if (candidates != null) { + rootDependencies.addAll(Arrays.asList(candidates)); + } + search.restrictToLoopMembers(rootDependencies); + search.collectLoops(root, rootDependencies); + return search.loops(); } - private static void findLoops(Vector loops, Vector path, IPlugin subroot, IPlugin[] candidates, boolean onlyCandidates, Vector exploredPlugins) { - if (!path.isEmpty()) { - // test the path so far - // is the subroot the same as root - if yes, that's it - - IPlugin root = path.elementAt(0); - if (isEquivalent(root, subroot)) { - // our loop!! - DependencyLoop loop = new DependencyLoop(); - loop.setMembers(path.toArray(new IPlugin[path.size()])); - int no = loops.size() + 1; - loop.setName(NLS.bind(PDECoreMessages.Builders_DependencyLoopFinder_loopName, ("" + no))); //$NON-NLS-1$ - loops.add(loop); - return; + /** + * Enumerates the cycles that pass through one root plug-in. + */ + private static final class LoopSearch { + + private final String rootId; + private final List loops = new ArrayList<>(); + private final List path = new ArrayList<>(); + private final Map> dependencies = new HashMap<>(); + private Set loopMembers = Set.of(); + + LoopSearch(String rootId) { + this.rootId = rootId; + } + + /** + * Narrows the search to the plug-ins that can actually sit on a cycle + * through the root, that is those reachable from the root that also + * lead back to it. Unlike a "this plug-in yielded no loop" blacklist, + * this property does not depend on the path taken to reach a plug-in, + * so pruning by it cannot hide a cycle. + */ + void restrictToLoopMembers(List rootDependencies) { + Map> dependents = new HashMap<>(); + Set reached = new HashSet<>(); + Deque pending = new ArrayDeque<>(); + for (IPlugin dependency : rootDependencies) { + dependents.computeIfAbsent(dependency.getId(), id -> new ArrayList<>()).add(rootId); + if (reached.add(dependency.getId())) { + pending.add(dependency); + } } - // is the subroot the same as any other node? - // if yes, abort - local loop that is not ours - for (int i = 1; i < path.size(); i++) { - IPlugin node = path.elementAt(i); - if (isEquivalent(subroot, node)) { - // local loop - return; + while (!pending.isEmpty()) { + IPlugin plugin = pending.remove(); + for (IPlugin dependency : dependenciesOf(plugin)) { + dependents.computeIfAbsent(dependency.getId(), id -> new ArrayList<>()).add(plugin.getId()); + if (reached.add(dependency.getId())) { + pending.add(dependency); + } + } + } + // walking the collected edges backwards from the root yields the + // plug-ins that lead back to it + loopMembers = new HashSet<>(); + Deque backwards = new ArrayDeque<>(); + backwards.add(rootId); + while (!backwards.isEmpty()) { + for (String dependent : dependents.getOrDefault(backwards.remove(), List.of())) { + if (!dependent.equals(rootId) && loopMembers.add(dependent)) { + backwards.add(dependent); + } } } } - @SuppressWarnings("unchecked") - Vector newPath = !path.isEmpty() ? ((Vector) path.clone()) : path; - newPath.add(subroot); - if (!onlyCandidates) { - IPluginImport[] iimports = subroot.getImports(); - for (IPluginImport iimport : iimports) { - String id = iimport.getId(); - //Be paranoid - if (id == null) { - continue; + void collectLoops(IPlugin plugin, List pluginDependencies) { + path.add(plugin); + for (IPlugin dependency : pluginDependencies) { + if (loops.size() >= MAX_LOOPS) { + break; } - if (!exploredPlugins.contains(id)) { - // is plugin in list of non loop yielding plugins - //Commenting linear lookup - was very slow - //when called from here. We will use - //model manager instead because it - //has a hash table lookup that is much faster. - //IPlugin child = PDECore.getDefault().findPlugin(id); - IPlugin child = findPlugin(id); - if (child != null) { - // number of loops before traversing plugin - int oldLoopSize = loops.size(); - - findLoops(loops, newPath, child, null, false, exploredPlugins); - - // number of loops after traversing plugin - int newLoopsSize = loops.size(); - - if (oldLoopSize == newLoopsSize) {// no change in number of loops - // no loops from going to this node, skip next time - exploredPlugins.add(id); - } - } + String id = dependency.getId(); + if (rootId.equals(id)) { + addLoop(); + } else if (loopMembers.contains(id) && !isOnPath(id)) { + collectLoops(dependency, dependenciesOf(dependency)); } - } - + path.remove(path.size() - 1); } - if (candidates != null) { - for (IPlugin candidate : candidates) { - // number of loops before traversing plugin - int oldLoopSize = loops.size(); - findLoops(loops, newPath, candidate, null, false, exploredPlugins); + /** + * Returns the plug-ins required by the given one, resolved through the + * registry. Cached, as the search visits a plug-in once per path + * leading to it. + */ + List dependenciesOf(IPlugin plugin) { + return dependencies.computeIfAbsent(plugin.getId(), id -> { + List resolved = new ArrayList<>(); + for (IPluginImport iimport : plugin.getImports()) { + String importedId = iimport.getId(); + //Be paranoid + if (importedId == null) { + continue; + } + IPlugin imported = findPlugin(importedId); + if (imported != null) { + resolved.add(imported); + } + } + return resolved; + }); + } - // number of loops after traversing plugin - int newLoopsSize = loops.size(); + private boolean isOnPath(String id) { + return path.stream().anyMatch(plugin -> id.equals(plugin.getId())); + } - if (oldLoopSize == newLoopsSize) { // no change in number of loops - // no loops from going to this node, skip next time - exploredPlugins.add(candidate.getId()); - } - } + private void addLoop() { + DependencyLoop loop = new DependencyLoop(); + loop.setMembers(path.toArray(new IPlugin[path.size()])); + int no = loops.size() + 1; + loop.setName(NLS.bind(PDECoreMessages.Builders_DependencyLoopFinder_loopName, ("" + no))); //$NON-NLS-1$ + loops.add(loop); } + DependencyLoop[] loops() { + return loops.toArray(new DependencyLoop[loops.size()]); + } } private static IPlugin findPlugin(String id) { @@ -131,8 +180,4 @@ private static IPlugin findPlugin(String id) { } return (IPlugin) childModel.getPluginBase(); } - - private static boolean isEquivalent(IPlugin left, IPlugin right) { - return left.getId().equals(right.getId()); - } } diff --git a/ui/org.eclipse.pde.ui.tests/src/org/eclipse/pde/core/tests/internal/core/builders/DependencyLoopFinderTest.java b/ui/org.eclipse.pde.ui.tests/src/org/eclipse/pde/core/tests/internal/core/builders/DependencyLoopFinderTest.java index 496d54a3935..810878cf8df 100644 --- a/ui/org.eclipse.pde.ui.tests/src/org/eclipse/pde/core/tests/internal/core/builders/DependencyLoopFinderTest.java +++ b/ui/org.eclipse.pde.ui.tests/src/org/eclipse/pde/core/tests/internal/core/builders/DependencyLoopFinderTest.java @@ -115,6 +115,34 @@ public void testTwoSeparateCyclesThroughRoot() throws Exception { assertEquals(List.of("loop.r -> loop.a", "loop.r -> loop.b"), loopSignatures("loop.r")); } + /** + * A cycle that is only reachable through a second dependency of the root + * must be reported too. + * + * 
+	 *   r -> a, r -> d
+	 *   a -> b, a -> r
+	 *   b -> a
+	 *   d -> b
+	 *
+ * + * Two cycles pass through {@code r}: {@code r -> a -> r} and + * {@code r -> d -> b -> a -> r}. Reaching {@code b} from {@code a} ends in + * a cycle that does not touch {@code r}, which must not stop the search + * from reaching {@code b} again through {@code d}. + */ + @Test + public void testCycleReachableOnlyViaSecondImportPathIsFound() throws Exception { + setTargetPlatform( // + bundle("loop.r", "1.0.0", entry(REQUIRE_BUNDLE, "loop.a,loop.d")), // + bundle("loop.a", "1.0.0", entry(REQUIRE_BUNDLE, "loop.b,loop.r")), // + bundle("loop.b", "1.0.0", entry(REQUIRE_BUNDLE, "loop.a")), // + bundle("loop.d", "1.0.0", entry(REQUIRE_BUNDLE, "loop.b"))); + + assertEquals(List.of("loop.r -> loop.a", "loop.r -> loop.d -> loop.b -> loop.a"), + loopSignatures("loop.r")); + } + /** * The reported cycles must not depend on the order in which the root * declares its dependencies.