Skip to content

Commit 1506bb4

Browse files
iloveeclipseiloveeclipse
committed
Don't include transitive optional dependencies to PDE container
Since ~20 years PDE never allowed not imported/required bundles be on the classpath of a bundle. From PDE point of view, bundle code should only see references to the code from other bundles on project classpath if: - the bundle requires another bundle - or the bundle imports a package from other bundle Transitive dependencies are only added to the classpath if the required bundle reexports one of its dependencies. This has a simple rationale: 1) Developers can't compile code in the IDE which would be not working at runtime, saving time for development by showing the errors before starting application. 2) The IDE can quickly recompile one line code change in a workspace with hundreds of bundles, because the classpath only has the dependencies mentioned above. With 89b00be above expectations are broken. All transitively used projects are now on the classpath. With `org.eclipse.jdt.core.compiler.problem.forbiddenReferenc` set to `ignore` or `warning` code that can compile (but uses illegal references) will not work at runtime. With this option set to `error` the code will not compile, but compilation will suffer from all transitive dependencies added to the bundle classpath, incremental build is significantly slower now. With 89b00be also optional transitive dependencies are added transitively, increasing the problem impact. Optional dependencies can be re-exported, but they are unlikely can be part of the bundle own API. As long as proper solution for the problem introduced by 89b00be is identified, don't add optional transitive dependencies to the classpath. This partly fixes the problem by slightly reducing set of projects added to the classpath. Also added regression test for RequiredPluginsClasspathContainer Shows the regression caused by eclipse-pde#2218 / 89b00be The test will fail on master after 89b00be and pass before this commit. See eclipse-pde#2244
1 parent 12eae51 commit 1506bb4

66 files changed

Lines changed: 770 additions & 2 deletions

File tree

Some content is hidden

Large Commits have some content hidden by default. Use the searchbox below for content that may be hidden.

‎ui/org.eclipse.pde.core/src/org/eclipse/pde/internal/core/RequiredPluginsClasspathContainer.java‎

Lines changed: 1 addition & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -678,8 +678,7 @@ private void addImplicitDependencies(BundleDescription desc, Set<BundleDescripti
678678
*/
679679
private void addTransitiveDependenciesWithForbiddenAccess(Set<BundleDescription> added,
680680
List<IClasspathEntry> entries) throws CoreException {
681-
Set<BundleDescription> closure = DependencyManager.findRequirementsClosure(added,
682-
INCLUDE_OPTIONAL_DEPENDENCIES);
681+
Set<BundleDescription> closure = DependencyManager.findRequirementsClosure(added);
683682
String systemBundleBSN = TargetPlatformHelper.getPDEState().getSystemBundle();
684683
Iterator<BundleDescription> transitiveDeps = closure.stream()
685684
.filter(desc -> !desc.getSymbolicName().equals(systemBundleBSN))

‎ui/org.eclipse.pde.ui.tests/src/org/eclipse/pde/core/tests/internal/classpath/ClasspathResolutionTest2.java‎

Lines changed: 201 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,201 @@
1+
/*******************************************************************************
2+
* Copyright (c) 2026 Andrey Loskutov <loskutov@gmx.de> and others.
3+
*
4+
* This program and the accompanying materials
5+
* are made available under the terms of the Eclipse Public License 2.0
6+
* which accompanies this distribution, and is available at
7+
* https://www.eclipse.org/legal/epl-2.0/
8+
*
9+
* SPDX-License-Identifier: EPL-2.0
10+
*
11+
* Contributors:
12+
* Andrey Loskutov <loskutov@gmx.de> - initial API and implementation
13+
*******************************************************************************/
14+
package org.eclipse.pde.core.tests.internal.classpath;
15+
16+
import static org.assertj.core.api.Assertions.assertThat;
17+
import static org.junit.Assert.assertFalse;
18+
import static org.junit.Assert.assertTrue;
19+
import static org.junit.Assert.fail;
20+
21+
import java.util.ArrayList;
22+
import java.util.Arrays;
23+
import java.util.List;
24+
import java.util.stream.Stream;
25+
26+
import org.eclipse.core.resources.IMarker;
27+
import org.eclipse.core.resources.IProject;
28+
import org.eclipse.core.resources.IResource;
29+
import org.eclipse.core.resources.IncrementalProjectBuilder;
30+
import org.eclipse.core.resources.ResourcesPlugin;
31+
import org.eclipse.core.runtime.CoreException;
32+
import org.eclipse.core.runtime.IPath;
33+
import org.eclipse.core.runtime.NullProgressMonitor;
34+
import org.eclipse.jdt.core.IClasspathEntry;
35+
import org.eclipse.pde.core.plugin.IPluginModelBase;
36+
import org.eclipse.pde.internal.core.ClasspathComputer;
37+
import org.eclipse.pde.internal.core.PDECore;
38+
import org.eclipse.pde.ui.tests.runtime.TestUtils;
39+
import org.eclipse.pde.ui.tests.util.ProjectUtils;
40+
import org.junit.BeforeClass;
41+
import org.junit.ClassRule;
42+
import org.junit.Rule;
43+
import org.junit.Test;
44+
import org.junit.rules.TestRule;
45+
46+
/**
47+
* Regression test for classpath resolution of plugin projects. Tests that only
48+
* the expected (no transitive optional) bundles are on the classpath, and that
49+
* errors are reported for missing packages from not accessible bundles, but not
50+
* for missing packages.
51+
*
52+
* See https://github.com/eclipse-pde/eclipse.pde/issues/2244.
53+
*/
54+
public class ClasspathResolutionTest2 {
55+
56+
@ClassRule
57+
public static final TestRule CLEAR_WORKSPACE = ProjectUtils.DELETE_ALL_WORKSPACE_PROJECTS_BEFORE_AND_AFTER;
58+
59+
@Rule
60+
public final TestRule deleteCreatedTestProjectsAfter = ProjectUtils.DELETE_CREATED_WORKSPACE_PROJECTS_AFTER;
61+
62+
private static IProject projectA;
63+
64+
private static IClasspathEntry[] classpathEntriesA;
65+
66+
static final List<String> expectedAccessibleBundles = List.of("B", "G");
67+
static final List<String> expectedApiPackages = apiPackagesFor(expectedAccessibleBundles);
68+
static final List<String> expectedInternalPackages = internalPackagesFor(expectedAccessibleBundles);
69+
70+
static final List<String> notAccessibleBundles = List.of("C", "D", "E", "F", "H");
71+
static final List<String> notAccessibleApiPackages = apiPackagesFor(notAccessibleBundles);
72+
static final List<String> notAccessibleInternalPackages = internalPackagesFor(notAccessibleBundles);
73+
74+
static final List<String> expectedIndirectlyRequiredBundles = List.of("C", "D");
75+
76+
static final List<String> allOtherProjects = Stream
77+
.concat(expectedAccessibleBundles.stream(), notAccessibleBundles.stream()).sorted().toList();
78+
79+
static final List<String> expectedProjectsOnClasspath = Stream
80+
.concat(expectedAccessibleBundles.stream(), expectedIndirectlyRequiredBundles.stream()).sorted().toList();
81+
82+
@BeforeClass
83+
public static void setupBeforeClass() throws Exception {
84+
List<IProject> importedProjects = new ArrayList<>();
85+
86+
for (String name : allOtherProjects) {
87+
IProject project = ProjectUtils.importTestProject("tests/projects/" + name);
88+
importedProjects.add(project);
89+
}
90+
91+
// Build all projects in reversed order to ensure that dependencies are
92+
// built before dependents
93+
for (IProject project : importedProjects.reversed()) {
94+
project.open(new NullProgressMonitor());
95+
project.build(IncrementalProjectBuilder.FULL_BUILD, new NullProgressMonitor());
96+
}
97+
TestUtils.processUIEvents(100);
98+
99+
// Now import and build project A, which depends on all other projects.
100+
projectA = ProjectUtils.importTestProject("tests/projects/A");
101+
projectA.open(new NullProgressMonitor());
102+
projectA.build(IncrementalProjectBuilder.FULL_BUILD, new NullProgressMonitor());
103+
104+
IPluginModelBase modelA = PDECore.getDefault().getModelManager().findModel(projectA);
105+
classpathEntriesA = ClasspathComputer.computeClasspathEntries(modelA, projectA);
106+
107+
TestUtils.processUIEvents(100);
108+
}
109+
110+
/**
111+
* Check that the classpath of plugin A contains exactly the expected
112+
* bundles. Checks that "missing type" compilation errors are reported for
113+
* all references form not accessible bundles. Checks that no dependencies
114+
* to transitive optional bundles are added to the classpath. This bundle
115+
* classpath is computed by RequiredPluginsClasspathContainer.
116+
*/
117+
@Test
118+
public void testRequiredPluginsClasspathContainerContract() throws Exception {
119+
// Check every project except A - they should build without errors
120+
List<IProject> otherProjects = allOtherProjects.stream().map(ClasspathResolutionTest2::getProject).toList();
121+
for (IProject project : otherProjects) {
122+
IMarker[] markers = project.findMarkers(IMarker.PROBLEM, true, IResource.DEPTH_INFINITE);
123+
for (IMarker marker : markers) {
124+
if (marker.getAttribute(IMarker.SEVERITY, -1) == IMarker.SEVERITY_ERROR) {
125+
fail("Unexpected error in project " + project.getName() + ": "
126+
+ marker.getAttribute(IMarker.MESSAGE, ""));
127+
}
128+
}
129+
}
130+
131+
// Check that project A has errors, and that all errors are related to
132+
// missing packages from not accessible bundles, and that no error is
133+
// related to missing packages from expected accessible bundles
134+
IMarker[] markers = projectA.findMarkers(IMarker.PROBLEM, true, IResource.DEPTH_INFINITE);
135+
136+
List<String> errorMessages = Arrays.asList(markers).stream()
137+
.filter(marker -> marker.getAttribute(IMarker.SEVERITY, -1) == IMarker.SEVERITY_ERROR)
138+
.map(marker -> marker.getAttribute(IMarker.MESSAGE, "")).toList();
139+
140+
for (String message : errorMessages) {
141+
// Check that no error is related to missing packages from expected
142+
// accessible bundles
143+
boolean isRelatedToExpectedAccessibleBundle = false;
144+
for (String bundle : expectedAccessibleBundles) {
145+
String pack = bundle.toLowerCase();
146+
if (message.contains(pack + " cannot be resolved to a type")
147+
|| message.contains("project '" + bundle + "'")) {
148+
isRelatedToExpectedAccessibleBundle = true;
149+
break;
150+
}
151+
}
152+
assertFalse("Unexpected error in project A: " + message, isRelatedToExpectedAccessibleBundle);
153+
154+
// and that all errors are related to missing packages from not
155+
// accessible bundles
156+
boolean isRelatedToNotAccessibleBundle = false;
157+
for (String bundle : notAccessibleBundles) {
158+
String pack = bundle.toLowerCase();
159+
if (message.contains(pack + " cannot be resolved to a type")
160+
|| message.contains("project '" + bundle + "'")) {
161+
isRelatedToNotAccessibleBundle = true;
162+
break;
163+
}
164+
}
165+
assertTrue("Unexpected error in project A: " + message, isRelatedToNotAccessibleBundle);
166+
}
167+
168+
// There must be at least one error, otherwise the test would not be
169+
// meaningful
170+
assertFalse("Expected errors in project A, but found none!", errorMessages.isEmpty());
171+
172+
// Check that all expected accessible bundles are on the classpath, and
173+
// only those
174+
List<String> projectNames = Arrays.asList(classpathEntriesA).stream()
175+
.map(entry -> entry.getPath().lastSegment()).toList();
176+
177+
assertThat(projectNames).containsExactlyInAnyOrderElementsOf(expectedProjectsOnClasspath);
178+
179+
// Same check using the API of PDECore and ClasspathComputer instead
180+
List<String> classpathEntries = getRequiredPluginContainerEntries(projectA);
181+
assertThat(classpathEntries).containsExactlyInAnyOrderElementsOf(expectedProjectsOnClasspath);
182+
}
183+
184+
static IProject getProject(String name) {
185+
return ResourcesPlugin.getWorkspace().getRoot().getProject(name);
186+
}
187+
188+
private static List<String> apiPackagesFor(List<String> expectedAccessibleBundles) {
189+
return expectedAccessibleBundles.stream().flatMap(bundle -> List.of(bundle + ".api").stream()).toList();
190+
}
191+
192+
private static List<String> internalPackagesFor(List<String> expectedAccessibleBundles) {
193+
return expectedAccessibleBundles.stream().flatMap(bundle -> List.of(bundle + ".internal").stream()).toList();
194+
}
195+
196+
private List<String> getRequiredPluginContainerEntries(IProject project) throws CoreException {
197+
IPluginModelBase model = PDECore.getDefault().getModelManager().findModel(project);
198+
IClasspathEntry[] computeClasspathEntries = ClasspathComputer.computeClasspathEntries(model, project);
199+
return Arrays.stream(computeClasspathEntries).map(IClasspathEntry::getPath).map(IPath::lastSegment).toList();
200+
}
201+
}

‎ui/org.eclipse.pde.ui.tests/src/org/eclipse/pde/ui/tests/AllPDETests.java‎

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -15,6 +15,7 @@
1515

1616
import org.eclipse.pde.core.tests.internal.AllPDECoreTests;
1717
import org.eclipse.pde.core.tests.internal.classpath.ClasspathResolutionTest;
18+
import org.eclipse.pde.core.tests.internal.classpath.ClasspathResolutionTest2;
1819
import org.eclipse.pde.core.tests.internal.core.builders.BundleErrorReporterTest;
1920
import org.eclipse.pde.core.tests.internal.util.PDESchemaHelperTest;
2021
import org.eclipse.pde.ui.tests.build.properties.AllValidatorTests;
@@ -63,6 +64,7 @@
6364
ClasspathContributorTest.class, //
6465
DynamicPluginProjectReferencesTest.class, //
6566
ClasspathResolutionTest.class, //
67+
ClasspathResolutionTest2.class, //
6668
BundleErrorReporterTest.class, //
6769
AllPDECoreTests.class, //
6870
ProjectSmartImportTest.class, //

‎ui/org.eclipse.pde.ui.tests/tests/projects/A/.classpath‎

Lines changed: 7 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,7 @@
1+
<?xml version="1.0" encoding="UTF-8"?>
2+
<classpath>
3+
<classpathentry kind="con" path="org.eclipse.jdt.launching.JRE_CONTAINER/org.eclipse.jdt.internal.debug.ui.launcher.StandardVMType/JavaSE-21"/>
4+
<classpathentry kind="con" path="org.eclipse.pde.core.requiredPlugins"/>
5+
<classpathentry kind="src" path="src"/>
6+
<classpathentry kind="output" path="bin"/>
7+
</classpath>

‎ui/org.eclipse.pde.ui.tests/tests/projects/A/.project‎

Lines changed: 28 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,28 @@
1+
<?xml version="1.0" encoding="UTF-8"?>
2+
<projectDescription>
3+
<name>A</name>
4+
<comment></comment>
5+
<projects>
6+
</projects>
7+
<buildSpec>
8+
<buildCommand>
9+
<name>org.eclipse.jdt.core.javabuilder</name>
10+
<arguments>
11+
</arguments>
12+
</buildCommand>
13+
<buildCommand>
14+
<name>org.eclipse.pde.ManifestBuilder</name>
15+
<arguments>
16+
</arguments>
17+
</buildCommand>
18+
<buildCommand>
19+
<name>org.eclipse.pde.SchemaBuilder</name>
20+
<arguments>
21+
</arguments>
22+
</buildCommand>
23+
</buildSpec>
24+
<natures>
25+
<nature>org.eclipse.pde.PluginNature</nature>
26+
<nature>org.eclipse.jdt.core.javanature</nature>
27+
</natures>
28+
</projectDescription>

‎ui/org.eclipse.pde.ui.tests/tests/projects/A/.settings/org.eclipse.core.resources.prefs‎

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,2 @@
1+
eclipse.preferences.version=1
2+
encoding/<project>=UTF-8

‎ui/org.eclipse.pde.ui.tests/tests/projects/A/.settings/org.eclipse.jdt.core.prefs‎

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,6 @@
1+
eclipse.preferences.version=1
2+
org.eclipse.jdt.core.compiler.codegen.targetPlatform=21
3+
org.eclipse.jdt.core.compiler.compliance=21
4+
org.eclipse.jdt.core.compiler.release=enabled
5+
org.eclipse.jdt.core.compiler.source=21
6+
org.eclipse.jdt.core.compiler.problem.forbiddenReference=error

‎ui/org.eclipse.pde.ui.tests/tests/projects/A/META-INF/MANIFEST.MF‎

Lines changed: 10 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,10 @@
1+
Manifest-Version: 1.0
2+
Bundle-ManifestVersion: 2
3+
Bundle-Name: A
4+
Bundle-SymbolicName: A
5+
Bundle-Version: 1.0.0.qualifier
6+
Export-Package: a.api
7+
Require-Bundle: B;bundle-version="1.0.0",
8+
G;bundle-version="1.0.0"
9+
Automatic-Module-Name: A
10+
Bundle-RequiredExecutionEnvironment: JavaSE-21

‎ui/org.eclipse.pde.ui.tests/tests/projects/A/build.properties‎

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,4 @@
1+
source.. = src/
2+
output.. = bin/
3+
bin.includes = META-INF/,\
4+
.

‎ui/org.eclipse.pde.ui.tests/tests/projects/A/src/a/api/AClass.java‎

Lines changed: 39 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,39 @@
1+
package a.api;
2+
public class AClass {
3+
// bundle B b.api package imported by A
4+
public Object objectFromB_allowed = new b.api.MyObject();
5+
public Object objectFromB_restricted = new b.internal.MyObject();
6+
7+
// bundle G reexported via B, g.api package is imported by A
8+
public Object objectFromG_allowed = new g.api.MyObject();
9+
public Object objectFromG_restricted = new g.internal.MyObject();
10+
11+
/*
12+
* All references below never compilable before https://github.com/eclipse-pde/eclipse.pde/pull/2218
13+
*
14+
* bundles C, D, E, F, H not required by A, neither package is imported by A
15+
* bundles C, D, E, F, H only referenced in different ways from bundle B or G and not reexported
16+
*/
17+
18+
// Bundle C directly required by B, but not reexported by B
19+
public Object objectFromC_not_accessible1 = new c.api.MyObject();
20+
public Object objectFromC_not_accessible2 = new c.internal.MyObject();
21+
22+
// Bundle D package imported by B, but not reexported by B
23+
public Object objectFromD_not_accessible1 = new d.api.MyObject();
24+
public Object objectFromD_not_accessible2 = new d.internal.MyObject();
25+
26+
/*
27+
* Regression introduced: all (transitive optional) dependencies from B or G are now accessible from A,
28+
* even if not reexported by B or G and not imported by A.
29+
*/
30+
// Optionally required or imported by B, but not reexported by B
31+
public Object objectFromE_not_accessible1 = new e.api.MyObject();
32+
public Object objectFromE_not_accessible2 = new e.internal.MyObject();
33+
public Object objectFromF_not_accessible1 = new f.api.MyObject();
34+
public Object objectFromF_not_accessible2 = new f.internal.MyObject();
35+
36+
// Bundle H package optionally imported by G, not reexported by anyone
37+
public Object objectFromH_not_accessible1 = new h.api.MyObject();
38+
public Object objectFromH_not_accessible2 = new h.internal.MyObject();
39+
}

0 commit comments

Comments
 (0)