Fix creation of generic-capabilities with multiple versions

[HannesWell]

This PR aims to fix the flawed read/processing of provided Generic-capbailities that have a list of versions, which was discovered in eclipse-equinox/equinox.framework#44 (comment).

I think I should add some tests for this.
Furthermore I would like to test this fix first locally with Tycho as described here:
https://github.com/eclipse/tycho/blob/master/CONTRIBUTING.md#building-tycho-against-a-locally-built-version-of-p2

[mickaelistria]

Indeed, some tests would be welcome. I believe this part of the code is relatively well tested already, so it shouldn't be too hard to add 1 test.

[HannesWell]

This change is ready for submission (tests have been added).

It would be great if this would make it into RC1, but it requires Project-lead/PMC approval.
@tjwatson, @akurtakov please approve/submit this, if you also think this is ready now. Let me know if you think more test-cases should be added.

I also managed to build/install a version of Tycho locally that uses this change and can confirm this change fixes the build problem from eclipse-equinox/equinox.framework#44 (comment). It only requires to use the updated target-platform from eclipse-platform/eclipse.platform.releng.aggregator#239 which can be achieved by adding the containing module locally.

[akurtakov]

I would leave that decision to @tjwatson

[laeubi]

@HannesWell the freeze period check still success here should it not give a red x here?

[HannesWell]

@HannesWell the freeze period check still success here should it not give a red x here?

There is no entry that contains the word "stabilization" in the calendar for today. Therefore the build succeeds:
https://calendar.google.com/calendar/u/0/htmlembed?src=cHJmazI2ZmRtcHJ1MW1wdGxiMDZwMGpoNHNAZ3JvdXAuY2FsZW5kYXIuZ29vZ2xlLmNvbQ

In general I think it would be good if the freeze period would get their dedicated entries in the calendar. That would make it simpler to understand.

[laeubi]

There is no entry that contains the word "stabilization" in the calendar for today.

Maybe "Endgame" is an alias here?

[HannesWell]

There is no entry that contains the word "stabilization" in the calendar for today.

Maybe "Endgame" is an alias here?

I created eclipse-platform/eclipse.platform.releng.aggregator#272 to discuss further improvements of the freezePeriod check so we can keep this one on topic :)

[HannesWell]

@tjwatson could please review this PR.

If we submit this until tomorrow (Wednesday) evening it will be considered for RC1.
If we not come to a conclusion until then, I don't know if we could submit this for RC2 or would have to wait until the next release.
@akurtakov could jump in in case Tom does not respond until tomorrow?

[merks]

This is what the 'a.jre.javase' version 18.0.0 IU looks like in p2:

    <provided namespace='osgi.ee' name='OSGi/Minimum' version='1.0.0'/>
    <provided namespace='osgi.ee' name='OSGi/Minimum' version='1.1.0'/>
    <provided namespace='osgi.ee' name='OSGi/Minimum' version='1.2.0'/>
    <provided namespace='osgi.ee' name='JRE' version='1.0.0'/>
    <provided namespace='osgi.ee' name='JRE' version='1.1.0'/>
    <provided namespace='osgi.ee' name='JavaSE' version='1.0.0'/>
    <provided namespace='osgi.ee' name='JavaSE' version='1.1.0'/>
    <provided namespace='osgi.ee' name='JavaSE' version='1.2.0'/>
    <provided namespace='osgi.ee' name='JavaSE' version='1.3.0'/>
    <provided namespace='osgi.ee' name='JavaSE' version='1.4.0'/>
    <provided namespace='osgi.ee' name='JavaSE' version='1.5.0'/>
    <provided namespace='osgi.ee' name='JavaSE' version='1.6.0'/>
    <provided namespace='osgi.ee' name='JavaSE' version='1.7.0'/>
    <provided namespace='osgi.ee' name='JavaSE' version='1.8.0'/>
    <provided namespace='osgi.ee' name='JavaSE' version='9.0.0'/>
    <provided namespace='osgi.ee' name='JavaSE' version='10.0.0'/>
    <provided namespace='osgi.ee' name='JavaSE' version='11.0.0'/>
    <provided namespace='osgi.ee' name='JavaSE' version='12.0.0'/>
    <provided namespace='osgi.ee' name='JavaSE' version='13.0.0'/>
    <provided namespace='osgi.ee' name='JavaSE' version='14.0.0'/>
    <provided namespace='osgi.ee' name='JavaSE' version='15.0.0'/>
    <provided namespace='osgi.ee' name='JavaSE' version='16.0.0'/>
    <provided namespace='osgi.ee' name='JavaSE' version='17.0.0'/>
    <provided namespace='osgi.ee' name='JavaSE' version='18.0.0'/>
    <provided namespace='osgi.ee' name='JavaSE/compact1' version='1.8.0'/>
    <provided namespace='osgi.ee' name='JavaSE/compact1' version='9.0.0'/>
    <provided namespace='osgi.ee' name='JavaSE/compact1' version='10.0.0'/>
    <provided namespace='osgi.ee' name='JavaSE/compact1' version='11.0.0'/>
    <provided namespace='osgi.ee' name='JavaSE/compact2' version='1.8.0'/>
    <provided namespace='osgi.ee' name='JavaSE/compact2' version='9.0.0'/>
    <provided namespace='osgi.ee' name='JavaSE/compact2' version='10.0.0'/>
    <provided namespace='osgi.ee' name='JavaSE/compact2' version='11.0.0'/>
    <provided namespace='osgi.ee' name='JavaSE/compact3' version='1.8.0'/>
    <provided namespace='osgi.ee' name='JavaSE/compact3' version='9.0.0'/>
    <provided namespace='osgi.ee' name='JavaSE/compact3' version='10.0.0'/>
    <provided namespace='osgi.ee' name='JavaSE/compact3' version='11.0.0'/>

Changing the API of IProvidedCapability seems like a bit of a non-starter... Furthermore, the p2 repositories are so full of a.jre.javase IUs in the above form that they will never be stamped out of existence. And even if that wasn't a problem, it seems to me that because the a.jre.javase IUs aren't singletons, more than one can together satisfy a requirement that seeks to exclude Java 18. So I can't foresee that we could fix this problem for osgi.ee...

It seems odd that org.eclipse.equinox.internal.p2.metadata.RequiredPropertiesMatch was introduced, primarily to support osgi.ee requirements, but this was not recognized as a problem...

The following (the selected negative requirement) is the only way I could figure out how to express in p2 to exclude an IU with Java 17 osgi.ee capability:

[HannesWell]

Thanks Ed.
So P2 is already handling multi-versioned osgi.ee capabilities in the way proposed here for generic provided capabilities?
I looked quickly into org.eclipse.equinox.internal.p2.metadata.RequiredPropertiesMatch but have to admit I did not fully understand it yet.
But in general I think if the behaviour suggested by Tom is wanted, the 'Matcher' could implement it by collecting all capabilities from the same provider with equal namespace and attributes (expect for the version) and then treating them as one multi-version capability. So it is not only a question how multi version capabilities are modelled but also how they are processed (maybe the RequiredPropertiesMatch already does that).

And in general multi-versioned provided-capabilities are not working at all the moment, so I think we cannot break it.

[merks]

I don't know how these a.jre.javase IUs are manufactured; I think Tycho does that.

In the JustJ generator, it creates a p2.inf for this purpose using this to produce the strings

https://git.eclipse.org/c/justj/justj.tools.git/tree/plugins/org.eclipse.justj.codegen/src/org/eclipse/justj/codegen/model/util/Generator.java#n569

And this to generate capability advice

https://git.eclipse.org/c/justj/justj.tools.git/tree/plugins/org.eclipse.justj.codegen/templates/fragment/p2.inf.jet#n33

I've never looked in detail at how the relatively new RequiredPropertiesMatch works either...

Perhaps as you suggest it's better that it be overly permissive with multiple capabilities rather than not working at all...

[laeubi]

I don't know how these a.jre.javase IUs are manufactured;

I think it is generated here:
https://github.com/eclipse-equinox/p2/blob/master/bundles/org.eclipse.equinox.p2.publisher/src/org/eclipse/equinox/p2/publisher/actions/JREAction.java

[merks]

@laeubi Indeed! That explains why searching for "a.jre.javase" didn't turn it up. :-P

[laeubi]

@laeubi Indeed! That explains why searching for "a.jre.javase" didn't turn it up. :-P

If I search for "how do P2 do it" I always open IPublisherAction and let me show type hierarchy, then guessing by name :-D

[HannesWell]

Some parts of the discussion accidentally shifted to eclipse-equinox/equinox.framework#44 (comment) and following:

If we live with representing each version as a separate capability in the short-term, is there any path forward that would allow us to fix that in the future? I worry about pushing such things so late in the release.

@merks replied:

It seems quite challenging to evolve this. With requirements we had IRequirement and IRequiredCapability so we could introduce something new (which had a negative impact on Oomph and the Aggregator). But with IProvidedCapability it seems harder to evolve. One possibility though is that the IProvidedCapability's getProperties could encode additional information and that's already recorded by MetadataWriter.writeProvidedCapability(IProvidedCapability). Perhaps a boolean flag/property that indicates "I am a singleton capability so if there are other capabilities with the same name and namespace, only one of us may be used to resolve a RequiredPropertiesMatch". I have no idea how to implement the latter though. Perhaps such an approach could be used to fix the broken osgi.ee stuff by making the "singleton property" default to true for certain known namespaces...

I replied:

We could consider to change the model to have a List/Set of versions in ProvidedCapability, as suggested there. But this would require more work I will definitely not complete today. And, I have to check it, but if the multi-version capabilities are written to p2-repo metadata, old versions of p2 are likely not able to consume new p2-repos any more.

Nevertheless if we continue with 'unrolled' single-version capabilities the 'matcher' could be enhanced to assemble the single version capabilities back into a multi-versioned one by merging all capabilities from the same provider with same namespace and properties. I think that should be distinct, shouldn't it? Or is there a difference in:

Provide-Capability: 
 cap1;cap1=name3;version:List<Version>="1,2.1";uses:="some.other"

and

Provide-Capability: 
 cap1;cap1=name1;version:Version="1";uses:="some.other",
 cap1;cap1=name1;version:Version="2.1";uses:="some.other"

Which would then be treated equivalent.

But I think we should discuss this in eclipse-equinox/p2#64

How should we proceed here? Submit this change now and later 'fix' the part of the code that is intended to handle multi-versioned capabilities to merge multiple equivalent single-version capabilities to one?
Or should we postpone this to the next release and think of a better general approach?

[tjwatson]

IIUC, we already represent the osgi.ee namespace capabilities (which can be multi-versioned) as multiple capabilities. If that is the case then this change is applying that more generally to all other namespaces. Is that a fair assessment? I am confused how the osgi.ee capabilities came to be represented in p2. I guess because the framework generates the osgi.ee capabilities at runtime according to the running JVM then p2 sort of had to do the same thing with JREAction. But it seems that the representation there decided to live with the limitation that each IProvidedCapability has a single version. That would seem to imply that the more complicated filters from OSGi Require-Capability cannot work. Perhaps we can live with that for this release and look to solve that later? I still worry that we are fanning out the capabilities in the meta-data that will live past this release and that we may be overlooking something that will complicate things in the future.

I also do not really understand how p2 represents the generic filter expression to produce VersionRange objects for the IRequiredCapability.getRange() method. There is a way to go from a VersionRange to a filter string with org.osgi.framework.VersionRange.toFilterString(String) But there is no standard way to go from a filter string to a VersionRange. The framework does this conversion for printing resolution errors so they can be printed as Import-Package syntax instead of the unexpected osgi.wiring.package generic capability syntax with the code in org.eclipse.osgi.container.ModuleResolutionReport.createOSGiRequirement(Requirement, String...). But here the framework gets away with that because for specific namespaces like osgi.wiring.package we know that the filter representation came from the call to VersionRange.toFilterString so I just reversed the logic to get back to a VersionRange. But that is not going to be true for all other random namespaces used in OSGi that can be used with Provide-Capability and Require-Capability. I also don't get how the IRequiredCapability represents any of the other matching attributes that can be specified in a generic filter or even on things like Import-Package and Require-Bundle. Perhaps that logic is elsewhere?

Perhaps I'm diving into where all the dragons live and we don't really need to go there?

[HannesWell]

IIUC, we already represent the osgi.ee namespace capabilities (which can be multi-versioned) as multiple capabilities. If that is the case then this change is applying that more generally to all other namespaces. Is that a fair assessment?

Yes, I understand it the same.

I still worry that we are fanning out the capabilities in the meta-data that will live past this release and that we may be overlooking something that will complicate things in the future.

If we go the way suggested here, we definitely would have to support 'flattened' single version capabilities for a long time, even if we manage to implement multi-version capabilities in the future since the release-repos (ideally) life forever.
So yes it could complicated things in the future. But maybe we will continue with the single-version capabilities approach in the future. I cannot tell.

Perhaps I'm diving into where all the dragons live and we don't really need to go there?

Maybe, but I really cannot tell.

Although I would be happy to complete eclipse-equinox/equinox.framework#44 it is actually not urgent.
So for me it would be OK to postpone this so we have more time to find a better solution or to become more confident with this solution.

Besides that actually I could revert the removal of org.osgi.service.http.whiteboard so that the non-problematic parts can be replaced by the original OSGi jars from Maven-Central (but I think this is too late for 22-06 but can be submitted at the beginning of 22-09). And org.osgi.service.http.whiteboard is then replaced later as soon as this works?

[tjwatson]

Besides that actually I could revert the removal of org.osgi.service.http.whiteboard so that the non-problematic parts can be replaced by the original OSGi jars from Maven-Central (but I think this is too late for 22-06 but can be submitted at the beginning of 22-09). And org.osgi.service.http.whiteboard is then replaced later as soon as this works?

I'm fine keeping org.osgi.service.http.whiteboard for the short/near term and work out how to replace it with the standard OSGi one later. But even now I think that is to late to consider for RCs

The problem with this p2 issue is that once it is fixed we have to wait another release until we get a version of p2 in the field that can then be used to update to the version where we need the support for the multi-version capability/requirement matching.

[HannesWell]

Besides that actually I could revert the removal of org.osgi.service.http.whiteboard so that the non-problematic parts can be replaced by the original OSGi jars from Maven-Central (but I think this is too late for 22-06 but can be submitted at the beginning of 22-09). And org.osgi.service.http.whiteboard is then replaced later as soon as this works?

I'm fine keeping org.osgi.service.http.whiteboard for the short/near term and work out how to replace it with the standard OSGi one later. But even now I think that is to late to consider for RCs

Alright. Yes lets not make the SimRel team busy. As the replacement of the other packages showed it requires some adjustments of test-setups and products that use specific bundles.
So yes, lets do that at the beginning of the next cycle.

The problem with this p2 issue is that once it is fixed we have to wait another release until we get a version of p2 in the field that can then be used to update to the version where we need the support for the multi-version capability/requirement matching.

Unfortunately yes. But the only way to speed that up would be #23.
If we would publish snapshots, Tycho snapshots should use them as well and since Tycho 3.0.0-snapshot is currently used to build all Eclipse-Platform projects the fix would be available very quickly. Nevertheless we should likely wait for another milestone then because the produced update-sites can then likely only be consumed by Eclipse installations that contain that P2 version too.

[laeubi]

This is likely a rare and strange corner case, but I want to be sure you're aware that this corner actually exists.

I can't even tell if it makes any sense, that's like you would require that your Framework runs on Java 7 and Java 8 at the same time...

[merks]

This is likely a rare and strange corner case, but I want to be sure you're aware that this corner actually exists.

I can't even tell if it makes any sense, that's like you would require that your Framework runs on Java 7 and Java 8 at the same time...

You seem to be missing the point. The point isn't for me to make an example that is semantically meaningful (in your opinion) but rather to draw attention to the fact that there are things that one can express (syntactically) that resolve in OSGi but will not resolve when mapped to p2. If no one will ever sensibly/semantically do that because it's nonsense, great, it doesn't matter. But nevertheless, this corner exists and where there is room for existence, something may dwell in the future...

Again, I'm fine with ignoring this corner, but I don't think we should misunderstand it.

[laeubi]

The question for me is just, do we want to work on the larger scoped problem (that might be resolved on different levels), or solve just this case (that might not solve others we need to address in the future).

Just from a simple minded standpoint, probably the problem is more when evaluating the expression then, because obviously the Unit does provides versions 2 and 2.1, and writing it in one or two lines should not make a difference at all regardless on how we express this in the model... but I have no clue whats easier here, change the Expression evaluator or change the model.

I would be fine with any of those as long as we make progress here until the 2022-12 release so it can finally be fixed and we are not leaving behind just we are freeze because of the complexity and never move on in any direction.

[merks]

The question for me is just, do we want to work on the larger scoped problem (that might be resolved on different levels), or solve just this case (that might not solve others we need to address in the future).

I'm not sure that "this" case is well defined here. The solution being proposed is to expand a multi-version capability into multiple single-version capabilities. That doesn't seem unreasonable and it's likely that in the great majority of the cases the multi-version syntax is "just a shortcut" for that.

Just from a simple minded standpoint, probably the problem is more when evaluating the expression then, because obviously the Unit does provides versions 2 and 2.1, and writing it in one or two lines should not make a difference at all regardless on how we express this in the model... but I have no clue whats easier here, change the Expression evaluator or change the model.

Here you effectively argue that multi-version capabilities have no semantic meaning. But the resolution process is about binding requirements with capabilities; the hosting IUs aren't relevant during that processing. But let's not argue the point because that will get us nowhere, and certainly not toward the conclusion that you wish to reach.

I would be fine with any of those as long as we make progress here until the 2022-12 release so it can finally be fixed and we are not leaving behind just we are freeze because of the complexity and never move on in any direction.

So I think we mostly all agree that expanding a multi-version requirement is the path of least resistance and will cover the common sensible cases in the real world.

Let's give @tjwatson as chance to comment with any concerns he might have.

[laeubi]

But the resolution process is about binding requirements with capabilities; the hosting IUs aren't relevant during that processing.

At least there is IInstallableUnit.satisfies(IRequirement) that delegates to IRequirement.isMatch(IInstallableUnit) what (as far as I can see) evaluates the MatchExpression of the requirement against the IInstallableUnit (I haven't be able to follow the rabbit hole further down), so from an API users point of view one might assume that an IU satisfies a requirement (by something not directly visible to the user). So i naively assumed that actually such a MatchExpression has access to all provided capabilities of the IInstallableUnit in question and thus can evaluate an and expression in a way that it returns true if there is one or more capabilities that satisfy the requirement and thus the IInstallableUnit matches.

[merks]

To reiterate, let's not argue this point because that will get us nowhere, and certainly not toward the conclusion that you wish to reach...

[tjwatson]

I think what is being asked of me is if I think it is OK to represent a multi-version capability as separate distinct capabilities for the purposes of p2 resolution. As Ed points out, that will not solve some (likely corner case) filters that try to be excluded when matching some specific capability at a specific version for some reason. I don't think that is a common thing. I've only seen it on osgi.ee requirements say if you want to match on all versions >=7 and < Java 9 it would be something like "(&(osgi.ee=1.7)(!(osgi.ee=9.0)))". But I've not seen a common use case for that in other capabilities like the osgi.contract capabilities.

So for the short to mid-term solution it may be OK to live with this limitation. We have been living with it for osgi.ee and (AFAIK) we have no one really complaining. The main issue I have is if we ever do want to consolidate such fanned out capabilities back into a single muilt-value capability will we have made that more complicated to achieve in the future. My suspicion is that we cannot really know if that will be overly complicated by this decision or not. Furthermore, my suspicion is that we will never really revisit this topic unless it becomes a real pain point for some use case discovered in the field.

I don't see anyone wanting to spend large amounts of time solving some academic issue that nobody has really run into.

[HannesWell]

As Ed points out, that will not solve some (likely corner case) filters that try to be excluded when matching some specific capability at a specific version for some reason. I don't think that is a common thing.

If I understand it correctly, those corner cases don't work today either, do they? I think it is OK, if this solution only fixes some cases ,hopefully the most common ones, but not all.

The main issue I have is if we ever do want to consolidate such fanned out capabilities back into a single muilt-value capability will we have made that more complicated to achieve in the future. My suspicion is that we cannot really know if that will be overly complicated by this decision or not.

That is IMHO the more important point, we should not block a more complete solution in the future (if required) by this not perfect solution. To me it looks like this is boiled down to one question:
Is there a semantic difference in creating one capability with multiple versions compared to a corresponding set of multiple capabilities, where each capability has one of those versions, for the same IU?
If there is no difference the matcher could simply aggregate all capabilities of a IU that have equal properties (besides the version) into one multi-versioned capability, when this is required.

[merks]

I believe in OSGi there is a semantic difference. I thought Thomas has already mentioned that twice. I think if necessary, in the future we could stop expanding to multiple capabilities and create a new type of capability. I don't think that would break the existing uses of the proposed approach.

[tjwatson]

I believe in OSGi there is a semantic difference. I thought Thomas has already mentioned that twice. I think if necessary, in the future we could stop expanding to multiple capabilities and create a new type of capability. I don't think that would break the existing uses of the proposed approach.

Thanks Ed, yes to be clear a single capability with a list of values for a single attribute is semantically different than multiple capabilities with a single value for the attribute from the original list of values. For OSGi capabilities a version is treated no differently than any other capability attribute.

Perhaps another example, not using versions, will make it more clear

supported.colors; supported.colors:List<String>="red,green,blue"

vs.

supported.colors; supported.colors="red"
supported.colors; supported.colors="green"
supported.colors; supported.colors="blue"

In OSGi when you have a requirement like:

supported.colors; filter:="(&(supported.colors=red)(supported.colors=blue))"

That will try to resolve the requirement to a single capability that can satisfy supporting both red and blue. If you separate that single capability into 3 different capabilities there will be no single capability that can support both red and blue. So the resolution will fail. OSGi resolution does not consider a group of capabilities and their combined attributes to be able to satisfy a requirement. Each capability that the resolver considers to be a valid candidate must satisfy the requirement filter on its own.

With that being said, I am fine moving forward with a solution that does not do this for version attributes and fanning out the capabilities like that which is being suggested. The main issue is that p2 (and the old equinox resolver BTW) treat version as a special attribute when compared to other matching attributes. It did this because back then there were only a limited number of capability types (osgi.wiring.package, osgi.wiring.host, osgi.wiring.bundle) and all of them were considered to have a single defined version.

[HannesWell]

Sorry for asking the same question again.
The requirement part was clear to me, my point was the capability-part, especially in OSGi, but I formulated the question bad (and used P2 terms).

OSGi resolution does not consider a group of capabilities and their combined attributes to be able to satisfy a requirement. Each capability that the resolver considers to be a valid candidate must satisfy the requirement filter on its own.

That is the information I was searching for and now its crystal clear. Thank you Tom. :)

Since everybody seems to be fine with the proposed approach, do you have any more remarks regarding the implementation?
I update the PR on to of the current master, so it is ready for review.

[mickaelistria]

I think this change is an improvement over current state, does fix some bugs without really harming anything else, so it is good enough in practice.

[tjwatson]

Let move this forward. Thanks.

[HannesWell]

Great, thank you.
Lets merge this then.

Should we then publish a P2-Snapshot soon so that it can be used in a Tycho Snapshot?

[laeubi]

Should we then publish a P2-Snapshot soon so that it can be used in a Tycho Snapshot?

This workflow is currently not really supported ... but I'm working on it ;-)

[HannesWell]

Should we then publish a P2-Snapshot soon so that it can be used in a Tycho Snapshot?

This workflow is currently not really supported ... but I'm working on it ;-)

Great!

[mickaelistria]

Should we then publish a P2-Snapshot soon

Snapshots are supposed to be published on every I-Build to https://repo.eclipse.org/content/repositories/eclipse-snapshots already. But looking at https://repo.eclipse.org/content/repositories/eclipse-snapshots/org/eclipse/platform/org.eclipse.equinox.p2.publisher.eclipse/ , it looks like there is currently some glitch, for which I've opened eclipse-platform/eclipse.platform.releng#121.