Skip to content

Commit 7a71134

Browse files
HannesWellHannesWell
committed
Prefer explicitly included plugins for Eclipse/OSGi launches
When launching an application create a new Equinox resolver state, where explicitly included plugins/bundles are preferred and compute the requirements closure from that state. This avoids adding other providers of a capability if another provider is already (explicitly) included in a launch, just because the former is wired to the requirement in the target-platform state. Potentially this also makes the set of launched plugins more compact. Fixes #1604
1 parent 98a5134 commit 7a71134

File tree

7 files changed

+278
-27
lines changed

7 files changed

+278
-27
lines changed

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

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -262,7 +262,7 @@ private static boolean isTestWorkspaceProject(Resource f) {
262262
*
263263
* @return a set of bundle ids
264264
*/
265-
private static Collection<NameVersionDescriptor> getImplicitDependencies() {
265+
public static Collection<NameVersionDescriptor> getImplicitDependencies() {
266266
try {
267267
ITargetPlatformService service = PDECore.getDefault().acquireService(ITargetPlatformService.class);
268268
if (service != null) {

‎ui/org.eclipse.pde.launching/src/org/eclipse/pde/internal/launching/launcher/BundleLauncherHelper.java‎

Lines changed: 80 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -50,7 +50,9 @@
5050
import org.eclipse.core.runtime.Status;
5151
import org.eclipse.debug.core.ILaunchConfiguration;
5252
import org.eclipse.debug.core.ILaunchConfigurationWorkingCopy;
53+
import org.eclipse.osgi.service.resolver.BaseDescription;
5354
import org.eclipse.osgi.service.resolver.BundleDescription;
55+
import org.eclipse.osgi.service.resolver.State;
5456
import org.eclipse.osgi.util.NLS;
5557
import org.eclipse.pde.core.plugin.IMatchRules;
5658
import org.eclipse.pde.core.plugin.IPluginBase;
@@ -59,10 +61,12 @@
5961
import org.eclipse.pde.core.plugin.PluginRegistry;
6062
import org.eclipse.pde.core.target.ITargetDefinition;
6163
import org.eclipse.pde.core.target.ITargetPlatformService;
64+
import org.eclipse.pde.internal.build.BundleHelper;
6265
import org.eclipse.pde.internal.build.IPDEBuildConstants;
6366
import org.eclipse.pde.internal.core.DependencyManager;
6467
import org.eclipse.pde.internal.core.FeatureModelManager;
6568
import org.eclipse.pde.internal.core.PDECore;
69+
import org.eclipse.pde.internal.core.PluginModelManager;
6670
import org.eclipse.pde.internal.core.TargetPlatformHelper;
6771
import org.eclipse.pde.internal.core.ifeature.IFeature;
6872
import org.eclipse.pde.internal.core.ifeature.IFeatureChild;
@@ -75,7 +79,6 @@
7579
import org.eclipse.pde.internal.launching.PDEMessages;
7680
import org.eclipse.pde.launching.IPDELauncherConstants;
7781
import org.osgi.framework.Version;
78-
import org.osgi.resource.Resource;
7982

8083
public class BundleLauncherHelper {
8184

@@ -166,12 +169,7 @@ private static void addRequiredBundles(Map<IPluginModelBase, String> bundle2star
166169
RequirementHelper.addApplicationLaunchRequirements(appRequirements, configuration, bundle2startLevel);
167170

168171
boolean includeOptional = configuration.getAttribute(IPDELauncherConstants.INCLUDE_OPTIONAL, true);
169-
Set<BundleDescription> requiredDependencies = includeOptional //
170-
? DependencyManager.getDependencies(bundle2startLevel.keySet(), DependencyManager.Options.INCLUDE_OPTIONAL_DEPENDENCIES)
171-
: DependencyManager.getDependencies(bundle2startLevel.keySet());
172-
173-
requiredDependencies.stream().map(Resource.class::cast) //
174-
.map(PluginRegistry::findModel).filter(Objects::nonNull) //
172+
computeDependencies(bundle2startLevel.keySet(), includeOptional, true) //
175173
.forEach(p -> addDefaultStartingBundle(bundle2startLevel, p));
176174
}
177175

@@ -223,16 +221,12 @@ private static Map<IPluginModelBase, String> getMergedBundleMapFeatureBased(ILau
223221
launchPlugins.addAll(additionalPlugins.keySet());
224222

225223
if (addRequirements) {
226-
// Add all missing plug-ins required by the application/product set in the config
224+
// Add all missing plug-ins required by the application/product set in the config
227225
List<String> appRequirements = RequirementHelper.getApplicationLaunchRequirements(configuration);
228226
RequirementHelper.addApplicationLaunchRequirements(appRequirements, configuration, launchPlugins, launchPlugins::add);
229227

230228
// Get all required plugins
231-
Set<BundleDescription> additionalBundles = DependencyManager.getDependencies(launchPlugins);
232-
for (BundleDescription bundle : additionalBundles) {
233-
IPluginModelBase plugin = getRequiredPlugin(bundle.getSymbolicName(), bundle.getVersion().toString(), IMatchRules.PERFECT, defaultPluginResolution);
234-
launchPlugins.add(Objects.requireNonNull(plugin));// should never be null
235-
}
229+
computeDependencies(launchPlugins, false, isWorkspace(defaultPluginResolution)).forEach(launchPlugins::add);
236230
}
237231

238232
// Create the start levels for the selected plugins and add them to the map
@@ -546,6 +540,79 @@ private static void addBundleToMap(Map<IPluginModelBase, String> map, IPluginMod
546540
map.put(bundle, startData);
547541
}
548542

543+
// --- dependency resolution ---
544+
545+
private static Stream<IPluginModelBase> computeDependencies(Set<IPluginModelBase> includedPlugins, boolean includeOptional, boolean preferWorkspaceBundles) {
546+
if (includedPlugins.isEmpty()) {
547+
return Stream.empty();
548+
}
549+
// Create and resolve the new 'launch'-state where bundles explicitly included in the launch are preferred. Then compute the requirement closure on that 'launch'-state.
550+
Map<BundleDescription, IPluginModelBase> launchBundlePlugins = new HashMap<>(includedPlugins.size() * 4 / 3 + 1);
551+
Set<BundleDescription> launchBundles = reresolveBundlesPreferringIncludedBundles(includedPlugins, launchBundlePlugins, preferWorkspaceBundles);
552+
State launchState = launchBundles.iterator().next().getContainingState();
553+
554+
DependencyManager.getImplicitDependencies().stream().map(descriptor -> {
555+
String versionStr = descriptor.getVersion();
556+
Version version = versionStr != null ? Version.parseVersion(versionStr) : null;
557+
return launchState.getBundle(descriptor.getId(), version);
558+
}).forEach(launchBundles::add);
559+
560+
DependencyManager.Options[] options = includeOptional //
561+
? new DependencyManager.Options[] {DependencyManager.Options.INCLUDE_OPTIONAL_DEPENDENCIES}
562+
: new DependencyManager.Options[] {};
563+
Set<BundleDescription> closure = DependencyManager.findRequirementsClosure(launchBundles, options);
564+
return closure.stream().map(launchBundlePlugins::get).map(Objects::requireNonNull) //
565+
.filter(p -> !includedPlugins.contains(p));
566+
}
567+
568+
private static Set<BundleDescription> reresolveBundlesPreferringIncludedBundles(Set<IPluginModelBase> includedPlugins, Map<BundleDescription, IPluginModelBase> launchBundlePlugins, boolean preferWorkspaceBundles) {
569+
PluginModelManager modelManager = PDECore.getDefault().getModelManager();
570+
State tpState = modelManager.getState().getState();
571+
572+
State launchState = BundleHelper.getPlatformAdmin().getFactory().createState(true);
573+
launchState.setPlatformProperties(tpState.getPlatformProperties());
574+
575+
// Collect all bundles explicitly included in the launch
576+
for (IPluginModelBase plugin : includedPlugins) {
577+
addPluginBundle(plugin, launchState, launchBundlePlugins, tpState);
578+
}
579+
Set<BundleDescription> launchBundles = new HashSet<>(launchBundlePlugins.keySet());
580+
581+
// Iterate workspace- and TP-models separately to avoid shadowing of TP models by workspace models
582+
Stream.of(modelManager.getWorkspaceModels(), modelManager.getExternalModels()).flatMap(Arrays::stream) //
583+
.filter(IPluginModelBase::isEnabled).filter(p -> !includedPlugins.contains(p)) //
584+
.forEach(plugin -> addPluginBundle(plugin, launchState, launchBundlePlugins, tpState));
585+
586+
launchState.getResolver().setSelectionPolicy(Comparator
587+
// prefer bundles explicitly included in the launch
588+
.comparing((BaseDescription d) -> !launchBundles.contains(d.getSupplier())) //false<true
589+
.thenComparing(d -> { // choose bundles originating from the preferred location (workspace or TP)
590+
boolean isWorkspaceBundle = launchBundlePlugins.get(d.getSupplier()).getUnderlyingResource() != null;
591+
return isWorkspaceBundle != preferWorkspaceBundles; //false<true
592+
}).thenComparing(tpState.getResolver().getSelectionPolicy()));
593+
594+
launchState.resolve(false);
595+
return launchBundles;
596+
}
597+
598+
private static void addPluginBundle(IPluginModelBase plugin, State launchState, Map<BundleDescription, IPluginModelBase> launchBundlePlugin, State tpState) {
599+
BundleDescription bundle = plugin.getBundleDescription();
600+
if (bundle != null) {
601+
if (bundle.getContainingState() != tpState) { // consistency check
602+
throw new IllegalStateException("Plugins have different TP state"); //$NON-NLS-1$
603+
}
604+
BundleDescription launchBundle = launchState.getFactory().createBundleDescription(bundle);
605+
if (!launchState.addBundle(launchBundle)) {
606+
throw new IllegalStateException("Failed to add bundle to launch state: " + launchBundle); //$NON-NLS-1$
607+
}
608+
if (launchBundlePlugin.put(launchBundle, plugin) != null) {
609+
throw new IllegalStateException("Duplicated launch bundle for plugin: " + plugin); //$NON-NLS-1$
610+
}
611+
}
612+
}
613+
614+
// -- start data ---
615+
549616
public static String getStartData(BundleDescription desc, String defaultStartData) {
550617
String runLevel = resolveSystemRunLevelText(desc);
551618
String auto = resolveSystemAutoText(desc);

‎ui/org.eclipse.pde.ui.tests/src/org/eclipse/pde/ui/tests/classpathresolver/ClasspathResolverTest.java‎

Lines changed: 1 addition & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -357,8 +357,7 @@ private static <V> V setStaticField(Class<?> cl, String fieldName, V newValue) t
357357
@SafeVarargs
358358
private static void createWorkspacePluginProjects(
359359
Entry<NameVersionDescriptor, Map<String, String>>... workspacePlugins) throws CoreException {
360-
Set<NameVersionDescriptor> descriptions = Map.ofEntries(workspacePlugins).keySet();
361-
List<IProject> pluginProjects = ProjectUtils.createWorkspacePluginProjects(descriptions);
360+
List<IProject> pluginProjects = ProjectUtils.createWorkspacePluginProjects(Map.ofEntries(workspacePlugins));
362361
while (pluginProjects.stream().anyMatch(ClasspathResolverTest::isUpdatePending)) {
363362
Thread.yield(); // await async classpath update of projects
364363
}

‎ui/org.eclipse.pde.ui.tests/src/org/eclipse/pde/ui/tests/launcher/FeatureBasedLaunchTest.java‎

Lines changed: 67 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -29,6 +29,8 @@
2929
import static org.eclipse.pde.ui.tests.util.ProjectUtils.createPluginProject;
3030
import static org.eclipse.pde.ui.tests.util.TargetPlatformUtil.bundle;
3131
import static org.eclipse.pde.ui.tests.util.TargetPlatformUtil.resolution;
32+
import static org.osgi.framework.Constants.EXPORT_PACKAGE;
33+
import static org.osgi.framework.Constants.IMPORT_PACKAGE;
3234
import static org.osgi.framework.Constants.REQUIRE_BUNDLE;
3335
import static org.osgi.framework.Constants.RESOLUTION_OPTIONAL;
3436

@@ -1377,6 +1379,71 @@ public void testGetMergedBundleMap_automaticallyAddRequirements() throws Throwab
13771379
targetBundle("plugin.z", "1.0.0")));
13781380
}
13791381

1382+
@Test
1383+
public void testGetMergedBundleMap_automaticallyAddRequirements_multipleProviders() throws Throwable {
1384+
// Test that, in case multiple-providers of a capability exists, the one
1385+
// explicitly included into the launch is preferred, when adding
1386+
// required dependencies
1387+
1388+
var targetBundles = Map.ofEntries( //
1389+
bundle("plugin.a", "1.0.0", //
1390+
entry(IMPORT_PACKAGE, "pack.a")), //
1391+
1392+
bundle("plugin.x", "1.0.0", //
1393+
entry(EXPORT_PACKAGE, "pack.a")), //
1394+
bundle("plugin.y", "1.0.0", //
1395+
entry(EXPORT_PACKAGE, "pack.a")));
1396+
1397+
createPluginProject("plugin.z", "1.0.0", Map.ofEntries(//
1398+
entry(EXPORT_PACKAGE, "pack.a")));
1399+
1400+
List<NameVersionDescriptor> targetFeatures = List.of( //
1401+
targetFeature("feature.a", "1.0.0", f -> {
1402+
addIncludedPlugin(f, "plugin.a", "1.0.0");
1403+
addIncludedPlugin(f, "plugin.x", "1.0.0");
1404+
}), //
1405+
targetFeature("feature.b", "1.0.0", f -> {
1406+
addIncludedPlugin(f, "plugin.a", "1.0.0");
1407+
addIncludedPlugin(f, "plugin.y", "1.0.0");
1408+
}), //
1409+
targetFeature("feature.c", "1.0.0", f -> {
1410+
addIncludedPlugin(f, "plugin.a", "1.0.0");
1411+
}));
1412+
1413+
setTargetPlatform(targetBundles, targetFeatures);
1414+
1415+
ILaunchConfigurationWorkingCopy wc = createFeatureLaunchConfig();
1416+
wc.setAttribute(IPDELauncherConstants.AUTOMATIC_INCLUDE_REQUIREMENTS, true);
1417+
wc.setAttribute(IPDELauncherConstants.FEATURE_PLUGIN_RESOLUTION, IPDELauncherConstants.LOCATION_WORKSPACE); // default
1418+
1419+
wc.setAttribute(IPDELauncherConstants.SELECTED_FEATURES, Set.of("feature.a:external"));
1420+
assertGetMergedBundleMap(wc, Set.of(//
1421+
targetBundle("plugin.a", "1.0.0"), //
1422+
targetBundle("plugin.x", "1.0.0")));
1423+
1424+
// The second feature included the other provider, so it's expected that
1425+
// only that one is included
1426+
wc.setAttribute(IPDELauncherConstants.SELECTED_FEATURES, Set.of("feature.b:external"));
1427+
assertGetMergedBundleMap(wc, Set.of(//
1428+
targetBundle("plugin.a", "1.0.0"), //
1429+
targetBundle("plugin.y", "1.0.0")));
1430+
1431+
// If FEATURE_PLUGIN_RESOLUTION=LOCATION_WORKSPACE, bundles originating
1432+
// from the workspace are to prefer:
1433+
wc.setAttribute(IPDELauncherConstants.SELECTED_FEATURES, Set.of("feature.c:external"));
1434+
assertGetMergedBundleMap(wc, Set.of(//
1435+
targetBundle("plugin.a", "1.0.0"), //
1436+
workspaceBundle("plugin.z", "1.0.0")));
1437+
1438+
// If FEATURE_PLUGIN_RESOLUTION=LOCATION_EXTERNAL, bundles originating
1439+
// from the TP are to prefer. And without other distinction the bundle
1440+
// with the higher version and then 'lower' id is preferred.
1441+
wc.setAttribute(IPDELauncherConstants.FEATURE_PLUGIN_RESOLUTION, IPDELauncherConstants.LOCATION_EXTERNAL);
1442+
assertGetMergedBundleMap(wc, Set.of(//
1443+
targetBundle("plugin.a", "1.0.0"), //
1444+
targetBundle("plugin.x", "1.0.0")));
1445+
}
1446+
13801447
static Map<BundleLocationDescriptor, String> getEclipseAppRequirementClosureForRunningPlatform(
13811448
DependencyManager.Options... closureOptions) throws Exception {
13821449
// ensure app requirements are registered (done at class initialization)

‎ui/org.eclipse.pde.ui.tests/src/org/eclipse/pde/ui/tests/launcher/LaunchConfigurationMigrationTest.java‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -34,8 +34,8 @@ public class LaunchConfigurationMigrationTest extends AbstractLaunchTest {
3434

3535
@BeforeClass
3636
public static void setupPluginProjects() throws Exception {
37-
ProjectUtils.createPluginProject("org.eclipse.pde.plugin1", "org.eclipse.pde.plugin1", null);
38-
ProjectUtils.createPluginProject("org.eclipse.pde.plugin2", "org.eclipse.pde.plugin2", null);
37+
ProjectUtils.createPluginProject("org.eclipse.pde.plugin1", "org.eclipse.pde.plugin1", "0.0.0");
38+
ProjectUtils.createPluginProject("org.eclipse.pde.plugin2", "org.eclipse.pde.plugin2", "0.0.0");
3939
}
4040

4141
@Test

‎ui/org.eclipse.pde.ui.tests/src/org/eclipse/pde/ui/tests/launcher/PluginBasedLaunchTest.java‎

Lines changed: 64 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -22,6 +22,8 @@
2222
import static org.hamcrest.CoreMatchers.containsString;
2323
import static org.hamcrest.MatcherAssert.assertThat;
2424
import static org.junit.Assert.assertEquals;
25+
import static org.osgi.framework.Constants.EXPORT_PACKAGE;
26+
import static org.osgi.framework.Constants.IMPORT_PACKAGE;
2527
import static org.osgi.framework.Constants.REQUIRE_BUNDLE;
2628
import static org.osgi.framework.Constants.RESOLUTION_OPTIONAL;
2729

@@ -869,6 +871,66 @@ public void testGetMergedBundleMap_automaticallyAddRequirements() throws Excepti
869871
targetBundle("plugin.b", "1.0.0")))));
870872
}
871873

874+
@Test
875+
public void testGetMergedBundleMap_automaticallyAddRequirements_multipleProviders() throws Exception {
876+
877+
var targetPlatformBundles = Map.ofEntries( //
878+
bundle("plugin.a", "1.0.0", //
879+
entry(IMPORT_PACKAGE, "pack.a")), //
880+
881+
bundle("plugin.x", "1.0.0", //
882+
entry(EXPORT_PACKAGE, "pack.a")), //
883+
bundle("plugin.y", "1.0.0", //
884+
entry(EXPORT_PACKAGE, "pack.a")));
885+
886+
var workspacePlugins = ofEntries( //
887+
bundle("plugin.z", "1.0.0", //
888+
entry(EXPORT_PACKAGE, "pack.a")) //
889+
);
890+
891+
Consumer<ILaunchConfigurationWorkingCopy> basicSetup = wc -> {
892+
wc.setAttribute(IPDELauncherConstants.AUTOMATIC_INCLUDE_REQUIREMENTS, true);
893+
};
894+
{ // three providers are available and none is yet included
895+
// -> the one originating from the workspace is preferred
896+
Consumer<ILaunchConfigurationWorkingCopy> launchConfigSetup = wc -> {
897+
basicSetup.accept(wc);
898+
wc.setAttribute(IPDELauncherConstants.SELECTED_TARGET_BUNDLES, Set.of("plugin.a*1.0.0"));
899+
};
900+
Set<BundleLocationDescriptor> expectedBundles = Set.of( //
901+
targetBundle("plugin.a", "1.0.0"), //
902+
workspaceBundle("plugin.z", "1.0.0"));
903+
904+
assertGetMergedBundleMap(workspacePlugins, targetPlatformBundles, launchConfigSetup, expectedBundles);
905+
}
906+
{ // three providers are available and one is included
907+
// -> select included one
908+
Consumer<ILaunchConfigurationWorkingCopy> launchConfigSetup = wc -> {
909+
basicSetup.accept(wc);
910+
wc.setAttribute(IPDELauncherConstants.SELECTED_TARGET_BUNDLES,
911+
Set.of("plugin.a*1.0.0", "plugin.x*1.0.0"));
912+
};
913+
Set<BundleLocationDescriptor> expectedBundles = Set.of( //
914+
targetBundle("plugin.a", "1.0.0"), //
915+
targetBundle("plugin.x", "1.0.0"));
916+
917+
assertGetMergedBundleMap(workspacePlugins, targetPlatformBundles, launchConfigSetup, expectedBundles);
918+
}
919+
{// three providers are available and one is included
920+
// -> select included one
921+
Consumer<ILaunchConfigurationWorkingCopy> launchConfigSetup = wc -> {
922+
basicSetup.accept(wc);
923+
wc.setAttribute(IPDELauncherConstants.SELECTED_TARGET_BUNDLES,
924+
Set.of("plugin.a*1.0.0", "plugin.y*1.0.0"));
925+
};
926+
Set<BundleLocationDescriptor> expectedBundles = Set.of( //
927+
targetBundle("plugin.a", "1.0.0"), //
928+
targetBundle("plugin.y", "1.0.0"));
929+
930+
assertGetMergedBundleMap(workspacePlugins, targetPlatformBundles, launchConfigSetup, expectedBundles);
931+
}
932+
}
933+
872934
@Test
873935
public void testTwoVersionsOfSameBundleConfigIni() throws Exception {
874936
var workspacePlugins = ofEntries( //
@@ -1033,7 +1095,7 @@ private static void assertGetMergedBundleMap(Consumer<ILaunchConfigurationWorkin
10331095

10341096
private void setUpWorkspace(Map<NameVersionDescriptor, Map<String, String>> workspacePlugins,
10351097
Map<NameVersionDescriptor, Map<String, String>> targetPlugins) throws Exception {
1036-
ProjectUtils.createWorkspacePluginProjects(workspacePlugins.keySet());
1098+
ProjectUtils.createWorkspacePluginProjects(workspacePlugins);
10371099
TargetPlatformUtil.setDummyBundlesAsTarget(targetPlugins, List.of(), tpJarDirectory);
10381100
}
10391101

@@ -1049,8 +1111,7 @@ private static ILaunchConfigurationWorkingCopy createPluginLaunchConfig(String n
10491111

10501112
private static final Pattern WHITESPACE = Pattern.compile("\\s+");
10511113

1052-
private Path getConfigurationFolder(ILaunchConfigurationWorkingCopy launchConfig)
1053-
throws CoreException {
1114+
private Path getConfigurationFolder(ILaunchConfigurationWorkingCopy launchConfig) throws CoreException {
10541115
ILaunch launch = new Launch(launchConfig, ILaunchManager.RUN_MODE, null);
10551116
var config = new EclipseApplicationLaunchConfiguration();
10561117
String commandLine = config.showCommandLine(launchConfig, ILaunchManager.RUN_MODE, launch, null);

0 commit comments

Comments
 (0)