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 Require-Capability when validating BREE #1000

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

alshamams
Copy link
Contributor

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

Copy link

github-actions bot commented Dec 11, 2023

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
Copy link
Contributor

laeubi commented Dec 11, 2023

@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)!

Copy link
Member

@HannesWell HannesWell left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@@ -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);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
Copy link
Contributor Author

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
Copy link
Contributor Author

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
Copy link
Contributor

laeubi commented Dec 12, 2023

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
Copy link
Member

@HannesWell HannesWell left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Comment on lines +563 to +570
String[] parts = ee.split(";"); //$NON-NLS-1$
if (parts[0] == null) {
return false;
}
if (!parts[0].equalsIgnoreCase(osgiEE)) {
return false;
}
return true;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
Copy link
Member

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
Copy link
Member

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?

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.

Treat 'Require-Capability: osgi.ee' as equivalent to 'Bundle-RequiredExecutionEnvironment'
3 participants