Detect dependency cycles formed through Import-Package

ui/org.eclipse.pde.core/src/org/eclipse/pde/internal/core/plugin/PluginBase.java

@@ -21,11 +21,13 @@
 import org.eclipse.osgi.service.resolver.BundleDescription;
 import org.eclipse.osgi.service.resolver.BundleSpecification;
 import org.eclipse.osgi.service.resolver.ExportPackageDescription;
+import org.eclipse.osgi.service.resolver.State;
 import org.eclipse.pde.core.IModelChangedEvent;
 import org.eclipse.pde.core.plugin.IMatchRules;
 import org.eclipse.pde.core.plugin.IPluginBase;
 import org.eclipse.pde.core.plugin.IPluginImport;
 import org.eclipse.pde.core.plugin.IPluginLibrary;
+import org.eclipse.pde.core.plugin.IPluginModelBase;
 import org.eclipse.pde.internal.core.ICoreConstants;
 import org.eclipse.pde.internal.core.PDECoreMessages;
 import org.eclipse.pde.internal.core.PDEState;
@@ -48,6 +50,9 @@ public abstract class PluginBase extends AbstractExtensions implements IPluginBa
 	private boolean fHasBundleStructure;
 	private String fBundleSourceEntry;
 	private boolean fExportsExternalAnnotations;
+	private boolean fImportsFromState;
+	private boolean fImportsLoaded;
+	private boolean fImportsResolved;
 
 	public PluginBase(boolean readOnly) {
 		super(readOnly);
@@ -90,9 +95,38 @@ public IPluginLibrary[] getLibraries() {
 
 	@Override
 	public IPluginImport[] getImports() {
+		ensureImportsLoaded();
 		return fImports.toArray(new IPluginImport[fImports.size()]);
 	}
 
+	/**
+	 * Computes the imports of a plug-in read from a {@link BundleDescription}
+	 * on demand. The bundles behind the Import-Package header are only known
+	 * once the state is resolved, which happens after the models have been
+	 * created, so reading them while loading the model would silently drop
+	 * them. A result computed from an unresolved state is provisional and
+	 * recomputed once the state has been resolved; after that it is kept.
+	 */
+	private synchronized void ensureImportsLoaded() {
+		if (!fImportsFromState || fImportsResolved) {
+			return;
+		}
+		IPluginModelBase model = getPluginModel();
+		BundleDescription description = model != null ? model.getBundleDescription() : null;
+		if (description == null) {
+			return;
+		}
+		State state = description.getContainingState();
+		boolean resolved = state == null || state.isResolved();
+		if (fImportsLoaded && !resolved) {
+			return;
+		}
+		fImports = new ArrayList<>();
+		loadImports(description);
+		fImportsLoaded = true;
+		fImportsResolved = resolved;
+	}
+
 	@Override
 	public IPluginBase getPluginBase() {
 		return this;
@@ -122,7 +156,7 @@ void load(BundleDescription bundleDesc, PDEState state) {
 		fBundleSourceEntry = state.getBundleSourceEntry(bundleDesc.getBundleId());
 		fExportsExternalAnnotations = state.exportsExternalAnnotations(bundleDesc.getBundleId());
 		loadRuntime(bundleDesc, state);
-		loadImports(bundleDesc);
+		fImportsFromState = true;
 	}
 
 	@Override
@@ -296,6 +330,9 @@ public void remove(IPluginImport[] iimports) throws CoreException {
 	public void reset() {
 		fLibraries = new ArrayList<>();
 		fImports = new ArrayList<>();
+		fImportsFromState = false;
+		fImportsLoaded = false;
+		fImportsResolved = false;
 		fProviderName = null;
 		fSchemaVersion = null;
 		fVersion = ""; //$NON-NLS-1$

ui/org.eclipse.pde.ui.tests/src/org/eclipse/pde/core/tests/internal/core/builders/DependencyLoopFinderTest.java

@@ -15,8 +15,11 @@
 
 import static java.util.Map.entry;
 import static org.eclipse.pde.ui.tests.util.TargetPlatformUtil.bundle;
+import static org.eclipse.pde.ui.tests.util.TargetPlatformUtil.version;
 import static org.junit.Assert.assertEquals;
 import static org.junit.Assert.assertNotNull;
+import static org.osgi.framework.Constants.EXPORT_PACKAGE;
+import static org.osgi.framework.Constants.IMPORT_PACKAGE;
 import static org.osgi.framework.Constants.REQUIRE_BUNDLE;
 
 import java.io.IOException;
@@ -115,6 +118,58 @@ public void testTwoSeparateCyclesThroughRoot() throws Exception {
 		assertEquals(List.of("loop.r -> loop.a", "loop.r -> loop.b"), loopSignatures("loop.r"));
 	}
 
+	/**
+	 * Bundles wired to each other through Export-Package/Import-Package form a
+	 * cycle just like bundles wired through Require-Bundle.
+	 */
+	@Test
+	public void testImportPackageCycle() throws Exception {
+		// a and b depend on each other only via package wiring (no Require-Bundle)
+		setTargetPlatform( //
+				bundle("loop.a", "1.0.0", //
+						entry(EXPORT_PACKAGE, "loop.a.pack"), //
+						entry(IMPORT_PACKAGE, "loop.b.pack")), //
+				bundle("loop.b", "1.0.0", //
+						entry(EXPORT_PACKAGE, "loop.b.pack"), //
+						entry(IMPORT_PACKAGE, "loop.a.pack")));
+
+		assertEquals(List.of("loop.a -> loop.b"), loopSignatures("loop.a"));
+	}
+
+	/**
+	 * A cycle that closes through a mix of both dependency kinds is reported
+	 * too: {@code a} requires {@code b}, and {@code b} imports a package that
+	 * {@code a} exports.
+	 */
+	@Test
+	public void testCycleMixingRequireBundleAndImportPackage() throws Exception {
+		setTargetPlatform( //
+				bundle("loop.a", "1.0.0", //
+						entry(REQUIRE_BUNDLE, "loop.b"), //
+						entry(EXPORT_PACKAGE, "loop.a.pack")), //
+				bundle("loop.b", "1.0.0", entry(IMPORT_PACKAGE, "loop.a.pack")));
+
+		assertEquals(List.of("loop.a -> loop.b"), loopSignatures("loop.a"));
+	}
+
+	/**
+	 * A package import creates a dependency only to the exporter it is wired
+	 * to, not to every bundle exporting that package.
+	 */
+	@Test
+	public void testImportPackageWiredElsewhereIsNoCycle() throws Exception {
+		// a is wired to b's 2.0.0 export; c exports the same package at 1.0.0
+		// and requires a, so an edge a -> c would close a cycle
+		setTargetPlatform( //
+				bundle("loop.a", "1.0.0", entry(IMPORT_PACKAGE, "loop.shared.pack" + version("2.0.0"))), //
+				bundle("loop.b", "1.0.0", entry(EXPORT_PACKAGE, "loop.shared.pack" + version("2.0.0"))), //
+				bundle("loop.c", "1.0.0", //
+						entry(EXPORT_PACKAGE, "loop.shared.pack" + version("1.0.0")), //
+						entry(REQUIRE_BUNDLE, "loop.a")));
+
+		assertEquals(List.of(), loopSignatures("loop.a"));
+	}
+
 	/**
 	 * A cycle that is only reachable through a second dependency of the root
 	 * must be reported too.