Detect dependency cycles formed through Import-Package - #2408
Draft
vogella wants to merge 3 commits into
Draft
Conversation
Cover the cases the dependency-cycle finder behind the MANIFEST.MF editor's "Look for cycles in the dependency graph" action handles today: no cycle, a direct mutual dependency, a three-bundle cycle, two separate cycles through the root, independence from the order in which the root declares its dependencies, and a cycle beside the root that must not hide the cycle through it. The tests build dummy target bundles and resolve them through PluginRegistry, exercising the production code path. Registered in AllPDECoreTests.
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.
The imports of a plug-in read from the state were computed while the model was loaded, which happens before the state is resolved: PDEState creates the models in its constructor and PluginModelManager resolves the state afterwards, once the workspace bundles have been added. At that point BundleDescription.getResolvedImports() is still empty, so the bundles behind the Import-Package header were dropped and the empty result was kept for the lifetime of the model. getRequiredBundles() does not need a resolved state, which is why only Require-Bundle edges survived. Compute those imports on demand instead. Until the state is resolved they are recomputed on every access, which keeps the previous result for early callers, and once it is resolved they are computed once and kept. Only models read from a BundleDescription are affected, and those are the read-only target models, so a model being edited cannot lose its imports. As a result the MANIFEST.MF editor's "Look for cycles in the dependency graph" action reports cycles that close through package wiring instead of silently ignoring them.
vogella
force-pushed
the
import-package-dependencies
branch
from
August 2, 2026 20:27
1698798 to
e99b3a1
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
The imports of a plug-in read from the state were computed while the model was loaded, which happens before the state is resolved: PDEState creates the models in its constructor and PluginModelManager resolves the state afterwards, once the workspace bundles have been added. At that point
BundleDescription.getResolvedImports()is still empty, so the bundles behind the Import-Package header were dropped and that empty result was kept for the lifetime of the model, whilegetRequiredBundles()needs no resolved state and kept working. As a result the MANIFEST.MF editor's "Look for cycles in the dependency graph" action silently ignored every cycle that closes through package wiring.Those imports are now computed on demand: until the state is resolved they are recomputed on each access, which preserves today's result for early callers, and afterwards they are computed once and kept. Only models read from a BundleDescription are affected and those are the read-only target models, so a model being edited cannot lose its imports.
Draft because I could not verify it locally: the test harness here installs the released
org.eclipse.pde.coreinstead of the reactor build (visible intarget/work/configuration/config.ini), regardless of-pl,-amor thejavacprofile, so local results say nothing about a change in that bundle. The three added tests are expected to fail without this change and pass with it, and I would like CI to confirm that. Stacked on #2405 and #2406, so it shows their commits until those are merged.