Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

StateBuilder should consider Constant.REQUIRE_CAPABILITY when calculating BREE for BundleDescription #643

Closed
ptziegler opened this issue Jun 12, 2024 · 11 comments

Comments

@ptziegler
Copy link

This is quite a convoluted problem, so let's start from the very beginning...

At the start of every release, we see the following error popping up in the GEF workspace, which is produced by the PDE API tool:

The minor version should be incremented in version 3.16.0, since execution environments have been changed since version 3.16.0

The cause of this error is, as the description suggests, that the execution environment has changed; Except that this is not the case. In the previous release, we built against Java 17 and this is still the case now.

What's happening internally is that the API tool loads the BREE of the baseline plugin and compares it to the workspace plugin. The BREE of the workspace plugin is "JavaSE-17", but the BREE of the baseline plugin is an empty string => Error.
Or in other words, the baseline BREE is not calculated correctly.

The bundle description through which the BREE is loaded is created via the following method in the BundleComponent class, which delegates to the StateObjectFactory and from there to the StateBuilder:

protected BundleDescription getBundleDescription(Map<String, String> manifest, String location, long id) throws BundleException {
	State state = getState();
	BundleDescription bundle = lookupBundle(state, manifest);
	if (bundle != null) {
		return bundle;
	}
	StateObjectFactory factory = StateObjectFactory.defaultFactory;
	bundle = factory.createBundleDescription(state, FrameworkUtil.asDictionary(manifest), fLocation, id);
	state.addBundle(bundle);
	return bundle;
}

The reason no BREE is found for the baseline plugin is because the manifest is using the "Require-Capability" header, instead of the deprecated "Bundle-RequiredExecutionEnvironment" header, with the former not (yet) being supported by the StateBuilder:

String[] brees = ManifestElement.getArrayFromList(manifest.get(Constants.BUNDLE_REQUIREDEXECUTIONENVIRONMENT));
result.setExecutionEnvironments(brees);

This is obviously something that could be fixed in PDE, but I think it makes sense to fix it directly at the source...

For reference, this is what our manifest looks like:

Manifest-Version: 1.0
Created-By: Maven Archiver 3.6.1
Build-Jdk-Spec: 17
Bundle-ManifestVersion: 2
Bundle-Name: %Plugin.name
Bundle-SymbolicName: org.eclipse.draw2d;singleton:=true
Bundle-Version: 3.16.0.202405290843
Bundle-Vendor: %Plugin.providerName
Bundle-Localization: plugin
Export-Package: org.eclipse.draw2d,org.eclipse.draw2d.geometry,org.eclip
 se.draw2d.graph,org.eclipse.draw2d.images,org.eclipse.draw2d.internal;x
 -friends:="org.eclipse.gef",org.eclipse.draw2d.parts,org.eclipse.draw2d
 .text,org.eclipse.draw2d.widgets,org.eclipse.draw2d.zoom
Require-Bundle: org.eclipse.swt;bundle-version="[3.4.0,4.0.0)";visibilit
 y:=reexport
Bundle-ActivationPolicy: lazy
Automatic-Module-Name: org.eclipse.draw2d
Eclipse-SourceReferences: scm:git:https://github.com/eclipse/gef-classic
 .git;path="org.eclipse.draw2d";commitId=d606e6287d69a4183c888bf4147b167
 c94ebfc83
Require-Capability: osgi.ee;filter:="(&(osgi.ee=JavaSE)(version=17))"

As a fix, one can probably use a similar approach to what is used in PDE, to detect whether a execution environment is set, where the BREE is calculated as <Execution-Environment>-<Version>:

String capability = manifest.get(Constants.REQUIRE_CAPABILITY);
ManifestElement[] header = ManifestElement.parseHeader(Constants.REQUIRE_CAPABILITY, capability);
return header != null && Arrays.stream(header).map(ManifestElement::getValue).anyMatch(ExecutionEnvironmentNamespace.EXECUTION_ENVIRONMENT_NAMESPACE::equals);
@ptziegler
Copy link
Author

Unless there are any objections, I'll try to create a prototype over the next few days...

@merks
Copy link
Contributor

merks commented Jun 12, 2024

This sounds very similar to this issue

eclipse-pde/eclipse.pde#1283

Though in the case there is nothing at all except what PDE adds implicitly.

@tjwatson
Copy link
Contributor

Isn't the problem here that the system bundle created for the state doesn't have the system capabilities set for it to provide the necessary osgi.ee capabilities? For example, the ones specified by the JavaSE-1.8.profile:

org.osgi.framework.system.capabilities = \
 osgi.ee; osgi.ee="OSGi/Minimum"; version:List<Version>="1.0, 1.1, 1.2",\
 osgi.ee; osgi.ee="JRE"; version:List<Version>="1.0, 1.1",\
 osgi.ee; osgi.ee="JavaSE"; version:List<Version>="1.0, 1.1, 1.2, 1.3, 1.4, 1.5, 1.6, 1.7, 1.8",\
 osgi.ee; osgi.ee="JavaSE/compact1"; version:List<Version>="1.8",\
 osgi.ee; osgi.ee="JavaSE/compact2"; version:List<Version>="1.8",\
 osgi.ee; osgi.ee="JavaSE/compact3"; version:List<Version>="1.8"

@tjwatson
Copy link
Contributor

See

private void addSystemCapabilities(List<GenericDescription> capabilities) {
for (int i = 0; i < platformProperties.length; i++)
try {
addSystemCapabilities(capabilities, ManifestElement.parseHeader(Constants.PROVIDE_CAPABILITY, (String) platformProperties[i].get(Constants.FRAMEWORK_SYSTEMCAPABILITIES)), i);
addSystemCapabilities(capabilities, ManifestElement.parseHeader(Constants.PROVIDE_CAPABILITY, (String) platformProperties[i].get(Constants.FRAMEWORK_SYSTEMCAPABILITIES_EXTRA)), i);
checkOSGiEE(capabilities, (String) platformProperties[i].get(Constants.FRAMEWORK_EXECUTIONENVIRONMENT), i);
} catch (BundleException e) {
// TODO consider throwing this...
}
}

Where it is expecting to get these extra capabilities from the platform properties.

@ptziegler
Copy link
Author

ptziegler commented Jun 13, 2024

Isn't the problem here that the system bundle created for the state doesn't have the system capabilities set for it to provide the necessary osgi.ee capabilities? For example, the ones specified by the JavaSE-1.8.profile:

I'm afraid I can't follow your explanation. From the debugger, I can tell that the system bundle has loaded the JavaSE-17 profile, which is what I expected. So assuming that this is required by a bundle, it should be possible to satisfy this capability.

However, the issue is that, given the manifest mentioned above, I would expect that after I've created the BundleDescription object, a call to getExecutionEnvironments() returns { Java-SE17 }, as specified by the documentation:

/**
* Returns the list of execution environments that are required by this bundle.
* Any one of the listed execution environments will allow this bundle to be
* resolved.
*
* @since 3.2
* @return the list of execution environments that are required.
*/
public String[] getExecutionEnvironments();

But this is only the case when the manifest is using the Bundle-RequiredExecutionEnvironment header.

@merks
Copy link
Contributor

merks commented Jun 13, 2024

I do think PDE is processing this differently than in Equinox. I’ll help review proposed changes.

@HannesWell
Copy link
Member

The PDE state, which uses the equinox resolver, in general supports EE requirements and it works when resolving such bundles in the target platform.
But the UI is often not prepared for that and therefore shows it as absent.
It could be the case that API tools don't support that either.
Internally the BREE is converted into EE requirements, but IIRC the equinox state is doing that and not PDE.

So maybe this is a problem laying somewhere in-between.

Before drawing conclusions where to solve this best, could you create a minimal reproducer and create a PDE issue for API-tools with that?

@ptziegler
Copy link
Author

@HannesWell

I've created eclipse-pde/eclipse.pde#1301 and a test case via eclipse-pde/eclipse.pde#1302

@merks
Copy link
Contributor

merks commented Jun 14, 2024

@ptziegler

It’s really impressive how often you dive deeply to fix problems wherever they may lurk.

@ptziegler
Copy link
Author

At the end of the day, this is already something that bothered me the last time I saw this. Of course I could just increase the version numbers whenever it shows up, but that would probably frustrate me even more...

I just generally believe it's better to speak up rather than to keep quiet, whenever something doesn't run as smoothly as it should. 😅

ptziegler added a commit to ptziegler/equinox that referenced this issue Jun 17, 2024
With this change, the BundleDescription instance that is returned by the
StateBuilder now also contains the execution environments that are
required by the Require-Capability header, rather than only the
Bundle-RequireExecutionEnvironment header.

Previously, a call to getExecutionEnvironments() would always return an
empty array, if a bundle lacks the Bundle-RequireExecutionEnvironment
header, even though a minimum version is required as a capability.

Resolves eclipse-equinox#643
ptziegler added a commit to ptziegler/equinox that referenced this issue Jun 17, 2024
With this change, the BundleDescription instance that is returned by the
StateBuilder now also contains the execution environments that are
required by the Require-Capability header, rather than only the
Bundle-RequireExecutionEnvironment header.

Previously, a call to getExecutionEnvironments() would always return an
empty array, if a bundle lacks the Bundle-RequireExecutionEnvironment
header, even though a minimum version is required as a capability.

Resolves eclipse-equinox#643
ptziegler added a commit to ptziegler/equinox that referenced this issue Jun 20, 2024
With this change, the BundleDescription instance that is returned by the
StateBuilder now also contains the execution environments that are
required by the Require-Capability header, rather than only the
Bundle-RequireExecutionEnvironment header.

Previously, a call to getExecutionEnvironments() would always return an
empty array, if a bundle lacks the Bundle-RequireExecutionEnvironment
header, even though a minimum version is required as a capability.

Resolves eclipse-equinox#643
ptziegler added a commit to ptziegler/equinox that referenced this issue Jun 20, 2024
With this change, the BundleDescription instance that is returned by the
StateBuilder now also contains the execution environments that are
required by the Require-Capability header, rather than only the
Bundle-RequireExecutionEnvironment header.

Previously, a call to getExecutionEnvironments() would always return an
empty array, if a bundle lacks the Bundle-RequireExecutionEnvironment
header, even though a minimum version is required as a capability.

Resolves eclipse-equinox#643
ptziegler added a commit to ptziegler/equinox that referenced this issue Jun 20, 2024
With this change, the BundleDescription instance that is returned by the
StateBuilder now also contains the execution environments that are
required by the Require-Capability header, rather than only the
Bundle-RequireExecutionEnvironment header.

Previously, a call to getExecutionEnvironments() would always return an
empty array, if a bundle lacks the Bundle-RequireExecutionEnvironment
header, even though a minimum version is required as a capability.

Resolves eclipse-equinox#643
@HannesWell
Copy link
Member

Since eclipse-pde/eclipse.pde#1302 is submitted I think this is obsolete.
Closing.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants