search

eclipse-equinox / equinox

equinox
Eclipse Public License 2.0
31 stars 65 forks source link

Extend StateBuilder to handle EE from Require-Capability header #645

Closed ptziegler closed 2 months ago

ptziegler commented 3 months ago

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 https://github.com/eclipse-equinox/equinox/issues/643

HannesWell commented 3 months ago

It would be good if the code to extract the BREEs would be made available from a place independent from org.eclipse.eclipse.osgi.compatibility.state. Then it could be re-used in PDE for example to display the BREE in the Manifest Editor of bundles in the target-platform that only require an EE capability or to validate EE requirements like in https://github.com/eclipse-pde/eclipse.pde/pull/1000. I didn't have the time yet to look into your issue in detail, but because such code could be usable in multiple places in PDE but I think nowhere else in the platform I still think it maybe would be better to handle this only in PDE.

tjwatson commented 3 months ago

I didn't have the time yet to look into your issue in detail, but because such code could be usable in multiple places in PDE but I think nowhere else in the platform I still think it maybe would be better to handle this only in PDE.

PDE could do the conversion to BREE and add the header (if missing) to the Map used to create the BundleDescription. That would result in duplicate requires on the osgi.ee namespace also, unless you also removed the element for osgi.ee from the Require-Capability header.

ptziegler commented 3 months ago

I didn't have the time yet to look into your issue in detail, but because such code could be usable in multiple places in PDE but I think nowhere else in the platform I still think it maybe would be better to handle this only in PDE.

So how should we proceed with this item? Fixing this in either Equinox or PDE should take roughly the same amount of effort. However, I believe that this should be decided before investing more time into this issue.

I don't have a deep understanding of either projects, so I would leave it up to you (or any other committer).

tjwatson commented 3 months ago

I don't have a deep understanding of either projects, so I would leave it up to you (or any other committer).

Seems more simple to fix in Equinox (and correct?). But Hannes has a better understanding of PDE usage of the resolver. I'm fine either way.

HannesWell commented 3 months ago

I didn't have the time yet to look into your issue in detail, but because such code could be usable in multiple places in PDE but I think nowhere else in the platform I still think it maybe would be better to handle this only in PDE.

So how should we proceed with this item? Fixing this in either Equinox or PDE should take roughly the same amount of effort. However, I believe that this should be decided before investing more time into this issue.

For now I think it is better added to PDE since it allows us to reuse it in multiple places in PDE while keeping it internal and consequently kind of incubate, fix, etc. it there. If more users show up we can still move this at a more general place. In general the resolver is intended to be faded out (although the time to fully clean up the usage is often missing).

Sorry for the delayed reply but this is a busy week. I intend to look into your issue in more detail to give a more detailed advice at the weekend.

HannesWell commented 2 months ago

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