Support version-ranges and no-version for units in IU target locations

[HannesWell]

Relates to eclipse-pde/eclipse.pde#1245

[HannesWell]

From looking at all other callers of TargetDefinition.Unit.getVersion() I think no more changes should be required.

[github-actions]

Test Results

  603 files  ±0    603 suites  ±0   4h 15m 8s ⏱️ - 7m 6s
  430 tests ±0    422 ✅  - 1   7 💤 ±0  1 ❌ +1 
1 290 runs  ±0  1 267 ✅  - 1  22 💤 ±0  1 ❌ +1 

For more details on these failures, see this check.

Results for commit 3f04ec1. ± Comparison against base commit d2f0299.

♻️ This comment has been updated with latest results.

[laeubi]

A "real" version range should start wit either ( or [ is maybe a better check. I'm wondering if VersionRange not already having such a helper method to parse a string to version range?!

[merks]

The next line does the "parse a string to version range". That method will check that the range is well formed.

Note though that VersionRange.parse("1.0.0") is a valid version range too and that these two are equivalent:

new VersionRange(Version.create("1.0.0"), true, Version.MAX_VERSION, true)
VersionRange.create("1.0.0");

And note too that that's not the version range that PDE will create for it, which is okay/good for backward compatibility but begs the question "how do I represent the version range where only a lower bound is specified?" The following is very ugly but I think is correct and will be happily consumed by the PDE logic:

VersionRange.create("raw:[1.0.0,MpM]/format(r(.r)*p?)")

Do we actually want PDE to be able to consume these raw formatted ranges?

https://wiki.eclipse.org/Equinox/p2/Omni_Version

This using MAX_INT would be another possibility:

VersionRange.create("[3.0.0,2147483647]")

though technically "2147483647.2147483647" doesn't fix in that range.

How do we expect the users to best express a range that is just a lower bound?

[laeubi]

How do we expect the users to best express a range that is just a lower bound?

[3.0.0,)

should represent a version with only a lower bound.... maybe if we need a special syntax one can use 3.0.0+

[merks]

It's unfortunate when reality and expectations are divergent:

We might use Pattern to match that idiom, and create the appropriate VersionRange from the parts.

[HannesWell]

A "real" version range should start wit either ( or [ is maybe a better check.

I think both is true for a real version range, it starts with ( or [ and has a comma. AFAICT neither of these chars is valid in a single version. So we can chose any criteria or both. I just used the comma-check because it was just one check.

Note though that VersionRange.parse("1.0.0") is a valid version range too and that these two are equivalent:

new VersionRange(Version.create("1.0.0"), true, Version.MAX_VERSION, true)
VersionRange.create("1.0.0");

And note too that that's not the version range that PDE will create for it,

Exactly. In PDE the version is therefore also processed with these three distinctions in:

https://github.com/eclipse-pde/eclipse.pde/blob/80dc588c0df3b8b062760eedd04f7842034d0c0a/ui/org.eclipse.pde.core/src/org/eclipse/pde/internal/core/target/IUBundleContainer.java#L87-L94

Ideally the implementation would be shared, but for that we would need something like eclipse-pde/eclipse.pde#34.

How do we expect the users to best express a range that is just a lower bound?

[3.0.0,)

should represent a version with only a lower bound.... maybe if we need a special syntax one can use 3.0.0+

It's unfortunate when reality and expectations are divergent:

Yes and yes. I tried the same while implementing this for PDE and noticed the same.
And yes again using a pattern and extracting the lower and upper bound 'manually' would certainly work. We could then also support the inverse and allow an empty lower bound.

But I didn't implement the suggested pattern yet (here and in PDE) because I'm not sure that's really a relevant use-case, given that one can simply use just the latest (either no-version or 0.0.0).
Assuming that versions usually just grow, I think the most relevant use-case for version ranges in general is just to limit the upper bound because one needs the lower version than the latest. But then the lower version is already known.
One example are the jakarta.annotation/inject bundles that we need in version one and two or three.

But if you think it's still necessary do you know a reason that speaks against implementing it in P2's version range directly? Then we would save adjustments in PDE and Tycho?

[merks]

Given all the omni-craziness in p2, implementing something directly in p2's VersionRange that is shared by PDE and Tycho sounds like a good option to me. Allowing the upper bound to be empty, where empty implies Version.MAX_VERSION seems reasonable. In any case, something concise and relatively intuitive will be good. (Perhaps it is just a corner case, but expressing only a lower bound is really common in a MANIFEST.MF, so it seems better we have a way now than have to add a way later.)

[HannesWell]

I have now created eclipse-equinox/p2#565 to implement the desired behavior in P2 directly.
PDE and Tycho will automatically be capable to use such ranges then as soon as the change is available to them. I think the short delay for Tycho is acceptable.

So I think there is nothing else that has to be done here.

[merks]

Yes, that looks good.

[eclipse-tycho-bot]

💔 All backports failed

Status	Branch	Result
❌	tycho-4.0.x	Backport failed because of merge conflicts You might need to backport the following PRs to tycho-4.0.x: - Add support for bumping maven-target locations - Add p2-aware model converter for CycloneDX SBOM generation - Skip CHECK_TRUST phase and allow parallel execution of product assembly - Add support to specify target locations in the pom configuration - Support javac as a compiler for Tycho - Add support for API Tools Annotations to Tycho - Support repository references

Manual backport

To create the backport manually run:

backport --pr 4361

Questions ?

Please refer to the Backport tool documentation and see the Github Action logs for details