Consider Require-Capability when validating BREE #1000

alshamams · 2023-12-11T11:09:05Z

Presence of osgi.ee capability ‘Require-Capability’ should mitigate absence of BREE headers in the manifest. The current parser has no awareness of Require-Capability header. In future the parser should consider this header too while evaluating BREE header.

Fixes: #140

github-actions · 2023-12-11T11:30:34Z

Test Results

290 files + 6 290 suites +6 51m 27s ⏱️ + 7m 57s
3 526 tests - 1 3 467 ✅ ± 0 58 💤 ± 0 1 ❌ - 1
10 823 runs +2 723 10 658 ✅ +2 700 164 💤 +24 1 ❌ - 1

For more details on these failures, see this check.

Results for commit f53f808. ± Comparison against base commit 1d5bd08.

This pull request removes 1 test.

AllPDETests org.eclipse.pde.ui.tests.launcher.AllLauncherTests org.eclipse.pde.ui.tests.launcher.FeatureBasedLaunchTest ‑ Unknown test

♻️ This comment has been updated with latest results.

laeubi · 2023-12-11T19:56:21Z

@alshamams congratulation for open the 1.000 PR/Issue on PDE 🥇

Presence of osgi.ee capability ‘Require-Capability’ should mitigate absence of BREE headers in the manifest.

So I understand it correctly that it just not complains if any Require-Capability is found?

In future the parser should consider this header too while evaluating BREE header

Do you maybe want to try add support for this? e.g on Tych you can take a look at:

where Tycho tries to decide the EE value from the capability and parse the headers so maybe that can be used as some kind of inspiration here?

As a bonus, it would be great if such EE would be shown at the Manifest-Editor overview page (maybe with the edit buttons disabled)!

HannesWell

Thank you @alshamams for working on this, but I think the current implementation is not sufficient.
Unfortunately the osgi.ee capability is not as simple as the BREE header since it shares the header with possible other required capabilities.

As a bonus, it would be great if such EE would be shown at the Manifest-Editor overview page (maybe with the edit buttons disabled)!

Yes. As mentioned in the linked issue, we could even consider to fully replace the BREE header by a ee-capability´.

HannesWell · 2023-12-11T20:21:17Z

ui/org.eclipse.pde.core/src/org/eclipse/pde/internal/core/builders/BundleErrorReporter.java

@@ -587,7 +587,8 @@ private void validateRequiredExecutionEnvironment() {
 							IPath currentPath = entry.getPath();
 							if (JavaRuntime.newDefaultJREContainerPath().matchingFirstSegments(currentPath) > 0) {
 								String eeId = JavaRuntime.getExecutionEnvironmentId(currentPath);
-								if (eeId != null) {
+								IHeader header = getHeader(Constants.REQUIRE_CAPABILITY);


This is here and below is probably not sufficient because a Plugin/Bundle can have various difference required capabilities, e.g. custom ones or for osgi service loder mediator.
So the raw value has to be parsed for the osgi.ee capability. Please see the doc I linked in the issue.

My assumption was that osgi.ee is the only namespace defined for required capabilities. I have now changed the code to parse the header value and check for the presence of osgi.ee namespace. Kindly review and let me know.

alshamams · 2023-12-12T07:37:34Z

Thank you @alshamams for working on this, but I think the current implementation is not sufficient. Unfortunately the osgi.ee capability is not as simple as the BREE header since it shares the header with possible other required capabilities.

As a bonus, it would be great if such EE would be shown at the Manifest-Editor overview page (maybe with the edit buttons disabled)!

Yes. As mentioned in the linked issue, we could even consider to fully replace the BREE header by a ee-capability´.

A follow up PR is on its way.👍

alshamams · 2023-12-12T07:43:32Z

I can think of corner cases such as presence of both the headers with conflicting EE versions. If you think this is a valid concern, I plan to investigate this in a follow up PR.

laeubi · 2023-12-12T08:07:21Z

I can think of corner cases such as presence of both the headers with conflicting EE versions. If you think this is a valid concern, I plan to investigate this in a follow up PR.

It would of course be good to have a warning as one is only should to specify one of them but not both see https://docs.osgi.org/specification/osgi.core/7.0.0/framework.module.html#framework.module.bree

Bundles should not mix these headers but use either an osgi.ee requirement or this header. If both are used, both constraints must be met to resolve.

but it is even allowed and there can't be a conflict.

Presence of osgi.ee capability ‘Require-Capability’ should mitigate absence of BREE headers in the manifest. The current parser has no awareness of Require-Capability header. In future the parser should consider this header too while evaluating BREE header. Fixes: eclipse-pde#140

HannesWell

@alshamams I think in the current from this is not sufficient, because if osgi.ee headers are used, not only their existence should be checked, but they should be merged with any potentially existing BREE-headers and then treated similarly. Further below in the code, their version should be checked as well, just like for the BREE header.

Therefore I suggest to treat all ee-requirements (regardless if they originate from a BREE header or a osgi.ee requirement) the same and read them at a more abstract level.
I'm thinking about reading
List<Requirement> requirements = bundleDescription.getRequirements(ExecutionEnvironmentNamespace.EXECUTION_ENVIRONMENT_NAMESPACE);.
Then the EE Ids have to be extracted again, but that is probably not that easy. I have started to look into this a bit locally, but didn't found a satisfying solution yet.

HannesWell · 2024-02-12T00:02:12Z

ui/org.eclipse.pde.core/src/org/eclipse/pde/internal/core/builders/BundleErrorReporter.java

+		String[] parts = ee.split(";"); //$NON-NLS-1$
+		if (parts[0] == null) {
+			return false;
+		}
+		if (!parts[0].equalsIgnoreCase(osgiEE)) {
+			return false;
+		}
+		return true;


This is not very robust, in general you probably would want to use ManifestElement.parseHeader() instead.
But as written in the general comment, this should be done at a more abstract level.

HannesWell · 2024-07-07T11:41:13Z

I'm thinking about
List<Requirement> requirements = bundleDescription.getRequirements(ExecutionEnvironmentNamespace.EXECUTION_ENVIRONMENT_NAMESPACE);.
Then the EE Ids have to be extracted again, but that is probably not that easy. I have started to look into this a bit locally, but didn't found a satisfying solution yet.

@alshamams I have just added a new method that does exactly that via #1325.
It is named ManifestUtilss.getRequiredExecutionEnvironments(Resource).
Can you please update this PR to use that method to read the required EEs of a bundleDescription?

HannesWell · 2024-07-21T06:35:58Z

I'm thinking about
List<Requirement> requirements = bundleDescription.getRequirements(ExecutionEnvironmentNamespace.EXECUTION_ENVIRONMENT_NAMESPACE);.
Then the EE Ids have to be extracted again, but that is probably not that easy. I have started to look into this a bit locally, but didn't found a satisfying solution yet.

@alshamams I have just added a new method that does exactly that via #1325. It is named ManifestUtilss.getRequiredExecutionEnvironments(Resource). Can you please update this PR to use that method to read the required EEs of a bundleDescription?

@gireeshpunathil could you ping @alshamams if she wants to continue on this one?

laeubi requested a review from HannesWell December 11, 2023 19:56

HannesWell requested changes Dec 11, 2023

View reviewed changes

laeubi force-pushed the req_capability branch from 3dd6b15 to 682f7f0 Compare January 27, 2024 15:58

laeubi assigned HannesWell Jan 27, 2024

alshamams added 2 commits February 11, 2024 07:38

Fixup: Address review comments

f53f808

laeubi force-pushed the req_capability branch from 682f7f0 to f53f808 Compare February 11, 2024 06:38

laeubi requested a review from HannesWell February 11, 2024 06:38

HannesWell reviewed Feb 12, 2024

View reviewed changes

HannesWell mentioned this pull request Jun 17, 2024

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

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Consider Require-Capability when validating BREE #1000

Consider Require-Capability when validating BREE #1000

alshamams commented Dec 11, 2023

github-actions bot commented Dec 11, 2023 •

edited

Loading

laeubi commented Dec 11, 2023 •

edited

Loading

HannesWell left a comment

HannesWell Dec 11, 2023

alshamams Dec 12, 2023

alshamams commented Dec 12, 2023

alshamams commented Dec 12, 2023

laeubi commented Dec 12, 2023

HannesWell left a comment

HannesWell Feb 12, 2024

HannesWell commented Jul 7, 2024

HannesWell commented Jul 21, 2024

Consider Require-Capability when validating BREE #1000

Are you sure you want to change the base?

Consider Require-Capability when validating BREE #1000

Conversation

alshamams commented Dec 11, 2023

github-actions bot commented Dec 11, 2023 • edited Loading

Test Results

laeubi commented Dec 11, 2023 • edited Loading

HannesWell left a comment

Choose a reason for hiding this comment

HannesWell Dec 11, 2023

Choose a reason for hiding this comment

alshamams Dec 12, 2023

Choose a reason for hiding this comment

alshamams commented Dec 12, 2023

alshamams commented Dec 12, 2023

laeubi commented Dec 12, 2023

HannesWell left a comment

Choose a reason for hiding this comment

HannesWell Feb 12, 2024

Choose a reason for hiding this comment

HannesWell commented Jul 7, 2024

HannesWell commented Jul 21, 2024

github-actions bot commented Dec 11, 2023 •

edited

Loading

laeubi commented Dec 11, 2023 •

edited

Loading