Fix: API tool reports fictitious changes to execution environment #1302
Conversation
|
Argh... this test setup is way too confusing. The new bundle doesn't seem to be registered properly, though the issue still shows up when changing the manifest file of "bundle.a". |
|
There we go... the expected error is now: |
|
@ptziegler great work in reproducing the issue! |
|
Let's just hope that this time, less than all the tests crash... I forgot to push the .project and .classpath files, which is why it worked locally. |
ptziegler
force-pushed
the
issue1301
branch
2 times, most recently
from
5be7308 to
1a30591
Compare
|
@HannesWell This is effectively the same fix I applied in the Equinox PR. The important part is to not use |
Thanks for that. I'll try my best to have a look at this tomorrow (Sorry was a busy weekend). |
There was a problem hiding this comment.
@ptziegler thanks for this. The general idea is correct and the tests are good.
But the algorithm to extract the BREE id from the EE filter only works for the simple, but very common case of filters like (&(osgi.ee=JavaSE)(version=17). Still other cases should be supported too.
Therefore I have now completed the work I started a while ago for the mentioned other use-cases and added it as a separate commit to this PR.
That commits add a re-usable method ManifestUtils.getRequiredExecutionEnvironments() to returns all required EE ids, regardless if they are defined via BREE header or EE requirement.
Internally this method provides a fast path for the common case, but also a complete path, where all available EEs in the runtime are matched against the filter and the matching ones are collected. Basically what has been suggested in #1301 (comment).
I also changed your commit to use the new method and force pushed the result to this PR's branch, I hope that's fine for you. If you want to keep the old code, you should make sure to make a local copy of the branch.
In general this PR would then be ready for me. I just would like to resolver the comment below.
@laeubi or @merks do you want to have a look at this as well?
| Method getOSGiEENameVersion = OSGiManifestBuilderFactory.class | ||
| .getDeclaredMethod("getOSGiEENameVersion", String.class); //$NON-NLS-1$ |
There was a problem hiding this comment.
@tjwatson can this be made available to PDE in some way since I would like to avoid to duplicate the logic in PDE.
It does not have to be an API, some internals that are accessible should be fine.
There was a problem hiding this comment.
In its current form it would have to be internal since returning a String[] is rather strange. Would something like this help as possible API:
public static Map<String, String> getOSGiEERequirementDirectives(String bree)Where the directives just have the osgi.ee filter directive?
Or maybe just a method to create the filter:
public static String getOSGiEERequirementFilter(String bree)There was a problem hiding this comment.
Yes the former would be helpful. But I have now solved that by simply 'hard-coding' the filter attributes of the old EEs CDC-1.0/Foundation-1.0 and CDC-1.1/Foundation-1.1. For all other resent EE ids the simple method to split the version and name at the last dash is sufficient and hopefully for all future ones as well.
So I think it is not necessary to introduce a new API method for that.
HannesWell
changed the title
As I have written elsewhere this is a tough case and probably can only be solved by matching the filter against a list of available EEs. I would just postpone that to the time where it is really needed. |
That's more than fine by me. I'm not overly familiar with all possible combinations with which one could describe the EE, so I appreciate you taking the time and cleaning up those edge cases. |
ui/org.eclipse.pde.core/src/org/eclipse/pde/internal/core/util/ManifestUtils.java
Outdated
Show resolved
Hide resolved
HannesWell
force-pushed
the
issue1301
branch
4 times, most recently
from
99c29a2 to
5e8ac63
Compare
|
This change itself is ready, but I'm currently investigating other use-cases for the new method, e.g. in #1318. |
|
@HannesWell That is correct. The problem only shows up at the start of a release cycle, so it'll only come up again at the start of 2024-09. |
The API tool only checks the Bundle-RequiredExecutionEnvironment header when comparing the baseline with the workspace bundle. If the EE of the baseline bundle is described via the Require-Capability header, an empty string is compared to the workspace EE, which is treated as an incompatibility. This also adds a simple test case where the execution environment of two bundles is checked. One bundle uses the Require-Capability, the other one the Bundle-RequiredExecutionEnvironment header. Resolves eclipse-pde#1301
There was a problem hiding this comment.
I have now added the new utility method via #1325 (the other PR that will use it takes some more time) and rebased this PR on that.
So this is now ready.
Thanks again.
The API tool only checks the Bundle-RequiredExecutionEnvironment header when comparing the baseline with the workspace bundle. If the EE of the baseline bundle is described via the Require-Capability header, an empty string is compared to the workspace EE, which is treated as an incompatibility.
This also adds a simple test case where the execution environment of two bundles is checked. One bundle uses the Require-Capability, the other one the Bundle-RequiredExecutionEnvironment header.
Resolves #1301