Consider Require-Capability when validating BREE #1000
Conversation
Test Results 290 files + 6 290 suites +6 51m 27s ⏱️ + 7m 57s For more details on these failures, see this check. Results for commit f53f808. ± Comparison against base commit 1d5bd08. This pull request removes 1 test.♻️ This comment has been updated with latest results. |
There was a problem hiding this comment.
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); | |||
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
A follow up PR is on its way.👍 |
|
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
but it is even allowed and there can't be a conflict. |
laeubi
force-pushed
the
req_capability
branch
from
3dd6b15
to
682f7f0
Compare
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
laeubi
force-pushed
the
req_capability
branch
from
682f7f0
to
f53f808
Compare
There was a problem hiding this comment.
@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.
| String[] parts = ee.split(";"); //$NON-NLS-1$ | ||
| if (parts[0] == null) { | ||
| return false; | ||
| } | ||
| if (!parts[0].equalsIgnoreCase(osgiEE)) { | ||
| return false; | ||
| } | ||
| return true; |
There was a problem hiding this comment.
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.
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