Improve handling of unresolved bundles for feature-launches #2367

[ptziegler]

The logic for calculating the included plug-ins for a feature-based launch configuration assigns resolved bundles a higher priority than bundles, that perfectly match the version in a feature.xml.

This means that even though the user configured their application to run with a specific version of a plug-in, PDE might decide to silently update it to its latest version, simply because it conflicts with a different plug-in in the IDE (which might not even be included in the app).

With this change, the logic has been adapted to first check for a version match, before checking the resolution state. So if a version matches perfectly, it will always be chosen first, even if it's not resolved.

Closes #2367

[ptziegler]

Without this change, the test case would fail with:

org.opentest4j.AssertionFailedError: [featureResolution workspace] 
expected: {
plugin.a-1.0.0(e)="default:default",
plugin.b-1.0.0(e)="default:default"
}
 but was: {
plugin.a-1.0.0(e)="default:default",
plugin.a-2.0.0(e)="default:default",
plugin.b-2.0.0(e)="default:default"
}

Which is completely different than what's expected when looking at the feature.xml.

[laeubi]

I would say that if a specific version is given in the feature this exact version must be used and no higher version is actually valid at all, so for me it looks not only like an ordering problem but that other plugins are actually selected at all.

[github-actions]

Test Results

  126 files  ±0    126 suites  ±0   38m 49s ⏱️ + 1m 12s
3 517 tests +1  3 463 ✅ +1   54 💤 ±0  0 ❌ ±0 
9 354 runs  +3  9 224 ✅ +3  130 💤 ±0  0 ❌ ±0 

Results for commit 1be2e2e. ± Comparison against base commit 109a4f5.

[ptziegler]

I would say that if a specific version is given in the feature this exact version must be used and no higher version is actually valid at all

That's what I would expect as well. But there is this test case, where a 1.0.0 is set as additional plugin, even though the target definition only contains a 2.0.0 and where the expectation is that this newer version is then selected.

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

Lines 1263 to 1284 in 109a4f5

	wc.setAttribute(IPDELauncherConstants.ADDITIONAL_PLUGINS, Set.of( //
	// id:version:location:enabled:startLeval:autoStart
	"plugin.a:1.0.0:default:true:1:true", //
	"plugin.b:1.0.0:workspace:true:2:true", //
	"plugin.c:1.0.0:workspace:true:3:true", //
	"plugin.d:1.0.0:external:true:4:true", // overwrite from feature
	"plugin.e:1.0.0:external:true:5:true", // attempted overwrite
	"plugin.f:1.0.0:default:true:6:true", // not matching version
	"plugin.z:1.0.0:external:false:7:true") // disabled
	);
	// overwriting the plug-in also included by a feature only works
	// if the same primary location is used and both pull in the
	// same version. Otherwise two different bundles are added
	assertGetMergedBundleMap(wc, Map.of( //
	targetBundle("plugin.a", "1.0.0"), "1:true", //
	workspaceBundle("plugin.b", "1.0.0"), "2:true", //
	targetBundle("plugin.c", "1.0.0"), "3:true", //
	targetBundle("plugin.d", "1.0.0"), "4:true", //
	targetBundle("plugin.e", "1.0.0"), "5:true", //
	workspaceBundle("plugin.e", "1.0.0"), "default:default", //
	targetBundle("plugin.f", "2.0.0"), "6:true"));

If I were to filter out all plugins that don't match the specified version, this test case would break. This is not the case when changing the order, as this only comes into play when the specified version really exists.

[ptziegler]

I'm also not quite happy with doing the change here. I'll try to have a look later today to move it down into the getIncluded(...) method, so that it's in the same place as the already existing version logic.

[laeubi]

That's what I would expect as well. But there is this test case, where a 1.0.0 is set as additional plugin, even though the target definition only contains a 2.0.0 and where the expectation is that this newer version is then selected.

Where of course the question is if it is actual wanted behavior that is tested or the test just crafted in a way that it passes... At least for P2 an exact version is generated for a feature and Tycho resolves all 0.0.0 to the exact highest version for this reason as well.

[ptziegler]

At least for P2 an exact version is generated for a feature and Tycho resolves all 0.0.0 to the exact highest version for this reason as well.

I also painfully remember that Tycho fails if a plugin has e.g. the version 1.0.0.20260616, but the feature.xml expects a 1.0.0.20260617. Meaning the version has to match down to the qualifier, otherwise it's not a valid configuration.

By that logic, the following assertion is also wrong, because it expects the bundle with version 1.0.0.someQualifier to be matched against the bundle with version 1.0.0.

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

Lines 367 to 374 in 109a4f5

	// Unqualified version-match in primary location
	{
	ILaunchConfigurationWorkingCopy lc = createFeatureLaunchConfig();
	lc.setAttribute(IPDELauncherConstants.SELECTED_FEATURES, Set.of("feature.e:workspace"));
	assertGetMergedBundleMap("pluginResolution explicit workspace", lc, Set.of( //
	workspaceBundle("plugin.a", "1.0.0")));
	}

@HannesWell I think you initially worked on this as part of Bug 578005 - Extend FeatureBasedLaunchTest to fully cover Feature-based launches. Do you remember the use-cases you had in mind when you originally wrote those tests?