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

Consider EE requirements in EE-Section of ManifestEditor #1318

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

HannesWell
Copy link
Member

The Execution Environments section in the Manifest Editor is currently empty if a bundle only specifies an EE requirement instead of a dedicated Bundle-RequiredExecutionEnvironment header.
For example for third-party OSGi bundles published to Maven-Central this is quite common.

With this change EE requirements are now considered as well:
grafik

The main building block for that is the addition of new method ManifestUtils.getRequiredExecutionEnvironments() in the first commit, that derives the required EEs of an OSGi resource based on its EE requirements. For that it only uses the OSGi resources API and is thus one small step to migrate PDE off the Equinox resolver API.

Copy link

github-actions bot commented Jun 30, 2024

Test Results

   291 files  ±0     291 suites  ±0   1h 1m 16s ⏱️ + 6m 38s
 3 580 tests ±0   3 504 ✅ ±0   76 💤 ±0  0 ❌ ±0 
11 037 runs  ±0  10 806 ✅ ±0  231 💤 ±0  0 ❌ ±0 

Results for commit 7151716. ± Comparison against base commit 029c7d7.

♻️ This comment has been updated with latest results.

@HannesWell HannesWell force-pushed the consider-ee-requirements branch from 56dfac0 to c815c5b Compare June 30, 2024 22:37
@HannesWell HannesWell marked this pull request as ready for review June 30, 2024 22:37
@HannesWell HannesWell force-pushed the consider-ee-requirements branch from c815c5b to a54f79f Compare June 30, 2024 22:39
@HannesWell
Copy link
Member Author

Previously the ExecutionEnvironmentSection just operated on the text in the Manifest. With the current state it requires a resolved target-platform that contains a BundleDescription/osgi.Resource for the Manifest to open. While the latter is present anyways in probably most of the cases I'm not confident it is always the case.
Therefore I think we should continue to operate just on the textual model of the Manifest opened in the editor. A lot of the new logic introduced in ManifestUtils can still be used for that and the changes in PluginInputContextManager would be avoided.

@HannesWell HannesWell marked this pull request as draft July 1, 2024 21:24
@HannesWell HannesWell force-pushed the consider-ee-requirements branch 2 times, most recently from c1392be to b7ee184 Compare July 2, 2024 23:17
@HannesWell HannesWell marked this pull request as ready for review July 2, 2024 23:18
@HannesWell HannesWell force-pushed the consider-ee-requirements branch 3 times, most recently from 7ddc4e9 to ccc7dcd Compare July 9, 2024 21:40
@HannesWell HannesWell force-pushed the consider-ee-requirements branch from ccc7dcd to 7151716 Compare July 17, 2024 19:01
@laeubi
Copy link
Contributor

laeubi commented Sep 10, 2024

Previously the ExecutionEnvironmentSection just operated on the text in the Manifest. With the current state it requires a resolved target-platform that contains a BundleDescription/osgi.Resource for the Manifest to open

I just wanted to note one do not need a resolved state for this.

Assume you have the String and parse it into a Manifest you can then use (full namespaces to prevent confusion with possible existing classes):

java.util.jar.Manifest manifest = ...
aQute.bnd.osgi.ResourceBuilder resourceBuilder = new aQute.bnd.osgi.ResourceBuilder();
resourceBuilder.addManifest(manifest);
org.osgi.resource.Resource resource = resourceBuilder.build();

to get an OSGi Resource from it, something similar can be archived with P2 in BundlesAction.createBundleDescription(Dictionary<String, String>, File) but requires the legacy fragment.

One can possibly hide this of course in a Util method as well, e.g. ManifestUtils#asResource(String)

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 this pull request may close these issues.

2 participants