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

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

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

HannesWell
Copy link
Member

@HannesWell HannesWell added the backport-to-tycho-4.0.x Can be added to a PR to trigger an automatic backport of the change label Oct 16, 2024
@HannesWell
Copy link
Member Author

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

Copy link

github-actions bot commented Oct 16, 2024

Test Results

  603 files    603 suites   4h 55m 4s ⏱️
  430 tests   422 ✅  7 💤 1 ❌
1 290 runs  1 267 ✅ 22 💤 1 ❌

For more details on these failures, see this check.

Results for commit d63cd1d.

♻️ This comment has been updated with latest results.

return Version.parseVersion(unitReference.getVersion());
if ("0.0.0".equals(version)) {
return VersionRange.emptyRange;
} else if (version.contains(",")) { // a real version range
Copy link
Member

@laeubi laeubi Oct 17, 2024

Choose a reason for hiding this comment

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

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?!

Copy link
Contributor

Choose a reason for hiding this comment

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

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?

Copy link
Member

@laeubi laeubi Oct 17, 2024

Choose a reason for hiding this comment

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

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+

Copy link
Contributor

Choose a reason for hiding this comment

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

It's unfortunate when reality and expectations are divergent:

image

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

Copy link
Member Author

Choose a reason for hiding this comment

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

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?

Copy link
Contributor

Choose a reason for hiding this comment

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

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport-to-tycho-4.0.x Can be added to a PR to trigger an automatic backport of the change
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants