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.

Sign up for GitHub

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

Jump to bottom

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
+40 −6
Open

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
18 changes: 13 additions & 5 deletions tycho-core/src/main/java/org/eclipse/tycho/p2resolver/InstallableUnitResolver.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -275,7 +275,7 @@ private static IInstallableUnit findUnits(Unit unitReference, IQueryable<IInstal

private static IQueryResult<IInstallableUnit> findUnit(Unit unitReference, IQueryable<IInstallableUnit> units)
throws TargetDefinitionSyntaxException {
Version version = parseVersion(unitReference);
VersionRange version = parseVersion(unitReference);

// the createIUQuery treats 0.0.0 version as "any version", and all other versions as exact versions
IQuery<IInstallableUnit> matchingIUQuery = QueryUtil.createIUQuery(unitReference.getId(), version);
Expand All @@ -285,12 +285,20 @@ private static IQueryResult<IInstallableUnit> findUnit(Unit unitReference, IQuer
return queryResult;
}

private static Version parseVersion(Unit unitReference) throws TargetDefinitionSyntaxException {
private static VersionRange parseVersion(Unit unitReference) throws TargetDefinitionSyntaxException {
String version = unitReference.getVersion();
try {
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 •
edited
Loading

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 •
edited
Loading

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

return VersionRange.create(version);
} else { // an explicit/exact version -> create strict version range
Version v = Version.parseVersion(version);
return new VersionRange(v, true, v, true);
}
} catch (IllegalArgumentException e) {
throw new TargetDefinitionSyntaxException(NLS.bind("Cannot parse version \"{0}\" of unit \"{1}\"",
unitReference.getVersion(), unitReference.getId()), e);
throw new TargetDefinitionSyntaxException(
NLS.bind("Cannot parse version \"{0}\" of unit \"{1}\"", version, unitReference.getId()), e);
}
}

Expand Down
14 changes: 13 additions & 1 deletion tycho-core/src/test/java/org/eclipse/tycho/p2resolver/TargetPlatformFactoryTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -42,6 +42,7 @@
import org.eclipse.equinox.p2.core.IProvisioningAgent;
import org.eclipse.equinox.p2.metadata.IInstallableUnit;
import org.eclipse.equinox.p2.metadata.IVersionedId;
import org.eclipse.equinox.p2.metadata.VersionedId;
import org.eclipse.tycho.ArtifactType;
import org.eclipse.tycho.DefaultArtifactKey;
import org.eclipse.tycho.IDependencyMetadata.DependencyMetadataType;
Expand Down Expand Up @@ -298,7 +299,7 @@ public void testDuplicateReactorUnits() throws Exception {
@Ignore("This test don't work because maven provides a 'not real' local repo to the test")
public void testMavenArtifactsInTargetDefinitionResolveToMavenPath() throws Exception {
File targetDefinition = resourceFile("targetresolver/mavenDep.target");
tpConfig.getTargetDefinitions().add(TargetDefinitionFile.read(targetDefinition));
tpConfig.addTargetDefinition(TargetDefinitionFile.read(targetDefinition));
P2TargetPlatform targetPlatform = subject.createTargetPlatform(tpConfig, NOOP_EE_RESOLUTION_HANDLER, List.of());
File artifactLocation = targetPlatform.getArtifactLocation(
new DefaultArtifactKey(ArtifactType.TYPE_ECLIPSE_PLUGIN, "org.apache.commons.logging", "1.2.0"));
Expand All @@ -307,6 +308,17 @@ public void testMavenArtifactsInTargetDefinitionResolveToMavenPath() throws Exce
assertTrue(p2ArtifactPath.startsWith(localM2Repo));
}

@Test
public void testUnitsWithVersionRangeAndNoVersionInTargetDefinition() throws Exception {
File targetDefinition = resourceFile("targetresolver/versionRanges.target");
tpConfig.addTargetDefinition(TargetDefinitionFile.read(targetDefinition));
P2TargetPlatform tp = subject.createTargetPlatform(tpConfig, NOOP_EE_RESOLUTION_HANDLER, List.of());
Set<IInstallableUnit> ius = tp.getInstallableUnits();
assertThat(ius, hasItem(unitWithIdAndVersion(new VersionedId("jakarta.activation-api", "2.1.3"))));
assertThat(tp.getInstallableUnits(),
hasItem(unitWithIdAndVersion(new VersionedId("jakarta.inject.jakarta.inject-api", "1.0.5"))));
}

private static TargetDefinition plannerTargetDefinition(TestRepositories repository, IVersionedId unit) {
TargetDefinition.Location location = new TargetDefinitionResolverIncludeModeTest.PlannerLocationStub(repository,
unit);
Expand Down
11 changes: 11 additions & 0 deletions tycho-core/src/test/resources/targetresolver/versionRanges.target
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,11 @@
<?xml version="1.0" encoding="UTF-8" standalone="no"?>
<?pde version="3.8"?>
<target name="version-ranges">
<locations>
<location includeAllPlatforms="false" includeConfigurePhase="true" includeMode="planner" includeSource="true" type="InstallableUnit">
<repository location="https://download.eclipse.org/eclipse/updates/4.33/R-4.33-202409030240/"/>
<unit id="jakarta.activation-api" />
<unit id="jakarta.inject.jakarta.inject-api" version="[1.0,2)"/>
</location>
</locations>
</target>
3 changes: 3 additions & 0 deletions ...o-targetplatform/src/main/java/org/eclipse/tycho/targetplatform/TargetDefinitionFile.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -755,6 +755,9 @@ private static IULocation parseIULocation(Element dom) {
for (Element unitDom : getChildren(dom, "unit")) {
String id = unitDom.getAttribute("id");
String version = unitDom.getAttribute("version");
if (version == null || version.isBlank()) {
version = "0.0.0";
}
units.add(new Unit(id, version));
}
final List<Repository> repositories = new ArrayList<>();
Expand Down
Loading