eclipse-equinox / equinox

equinox
Eclipse Public License 2.0
31 stars 65 forks source link

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

Closed ptziegler closed 2 months ago

ptziegler commented 3 months ago

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:

https://github.com/eclipse-equinox/equinox/blob/343e816ec9820a090c0c1bec56e5131513270bd3/bundles/org.eclipse.osgi.compatibility.state/src/org/eclipse/osgi/internal/resolver/StateBuilder.java#L131-L132

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 commented 3 months ago

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

merks commented 3 months ago

This sounds very similar to this issue

https://github.com/eclipse-pde/eclipse.pde/issues/1283

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

tjwatson commented 3 months ago

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 commented 3 months ago

See https://github.com/eclipse-equinox/equinox/blob/343e816ec9820a090c0c1bec56e5131513270bd3/bundles/org.eclipse.osgi.compatibility.state/src/org/eclipse/osgi/internal/resolver/StateImpl.java#L981-L990

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

ptziegler commented 3 months ago

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:

https://github.com/eclipse-equinox/equinox/blob/343e816ec9820a090c0c1bec56e5131513270bd3/bundles/org.eclipse.osgi/container/src/org/eclipse/osgi/service/resolver/BundleDescription.java#L275-L283

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

merks commented 3 months ago

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

HannesWell commented 3 months ago

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 commented 3 months ago

@HannesWell

I've created https://github.com/eclipse-pde/eclipse.pde/issues/1301 and a test case via https://github.com/eclipse-pde/eclipse.pde/pull/1302

merks commented 3 months ago

@ptziegler

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

ptziegler commented 3 months ago

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. 😅

HannesWell commented 2 months ago

Since https://github.com/eclipse-pde/eclipse.pde/pull/1302 is submitted I think this is obsolete. Closing.