TargetPlatformFactoryImpl.gatherP2InfUnits should use artifact version #3722
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
The expected failure is present: |
Test Results 594 files 594 suites 3h 34m 35s ⏱️ Results for commit 7425d30. ♻️ This comment has been updated with latest results. |
merks
force-pushed
the
pr-tp-factory-use-artifact-version
branch
2 times, most recently
from
1c41dda
to
f14cee8
Compare
laeubi
added
the
backport-to-tycho-4.0.x
laeubi
reviewed
Mar 31, 2024
| } | ||
| try { | ||
| return Version.parseVersion(version); | ||
| } catch (IllegalAccessError ex) { |
There was a problem hiding this comment.
IllegalAccessError --> IllegalArgumentException?
laeubi
reviewed
Mar 31, 2024
| @@ -325,6 +334,31 @@ private void gatherP2InfUnits(ReactorProject reactorProject, Set<IInstallableUni | |||
| } | |||
| } | |||
|
|
|||
| private static Version getVersion(ReactorProject reactorProject) { | |||
| String version = reactorProject.getVersion(); | |||
There was a problem hiding this comment.
Have you tried projectManager.getArtifactKey(...) ?
There was a problem hiding this comment.
It supports (maven) Artifact, Maven Projects or Reactor Projects as a parameter.
There was a problem hiding this comment.
I’ll look when I get home later. Is projectManager already available? (I’m on my iPhone now. )
There was a problem hiding this comment.
It seemed to work okay for the particular tests directly affected, but apparently there isn't always a project manager:
java.lang.AssertionError: unexpected exception type thrown; expected:<org.eclipse.tycho.core.resolver.target.DuplicateReactorIUsException> but was:<java.lang.NullPointerException>
at org.junit.Assert.assertThrows(Assert.java:1020)
at org.junit.Assert.assertThrows(Assert.java:981)
at org.eclipse.tycho.p2resolver.P2ResolverTest.testDuplicateInstallableUnit(P2ResolverTest.java:113)
at java.base/jdk.internal.reflect.NativeMethodAccessorImpl.invoke0(Native Method)
at java.base/jdk.internal.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:77)
at java.base/jdk.internal.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43)
at java.base/java.lang.reflect.Method.invoke(Method.java:568)
at org.junit.runners.model.FrameworkMethod$1.runReflectiveCall(FrameworkMethod.java:59)
at org.junit.internal.runners.model.ReflectiveCallable.run(ReflectiveCallable.java:12)
at org.junit.runners.model.FrameworkMethod.invokeExplosively(FrameworkMethod.java:56)
at org.junit.internal.runners.statements.InvokeMethod.evaluate(InvokeMethod.java:17)
at org.junit.internal.runners.statements.RunBefores.evaluate(RunBefores.java:26)
at org.junit.internal.runners.statements.RunAfters.evaluate(RunAfters.java:27)
at org.junit.rules.Verifier$1.evaluate(Verifier.java:35)
at org.junit.rules.ExternalResource$1.evaluate(ExternalResource.java:54)
at org.junit.runners.ParentRunner$3.evaluate(ParentRunner.java:306)
at org.junit.runners.BlockJUnit4ClassRunner$1.evaluate(BlockJUnit4ClassRunner.java:100)
at org.junit.runners.ParentRunner.runLeaf(ParentRunner.java:366)
at org.junit.runners.BlockJUnit4ClassRunner.runChild(BlockJUnit4ClassRunner.java:103)
at org.junit.runners.BlockJUnit4ClassRunner.runChild(BlockJUnit4ClassRunner.java:63)
at org.junit.runners.ParentRunner$4.run(ParentRunner.java:331)
at org.junit.runners.ParentRunner$1.schedule(ParentRunner.java:79)
at org.junit.runners.ParentRunner.runChildren(ParentRunner.java:329)
at org.junit.runners.ParentRunner.access$100(ParentRunner.java:66)
at org.junit.runners.ParentRunner$2.evaluate(ParentRunner.java:293)
at org.junit.runners.ParentRunner$3.evaluate(ParentRunner.java:306)
at org.junit.runners.ParentRunner.run(ParentRunner.java:413)
at org.apache.maven.surefire.junit4.JUnit4Provider.execute(JUnit4Provider.java:316)
at org.apache.maven.surefire.junit4.JUnit4Provider.executeWithRerun(JUnit4Provider.java:240)
at org.apache.maven.surefire.junit4.JUnit4Provider.executeTestSet(JUnit4Provider.java:214)
at org.apache.maven.surefire.junit4.JUnit4Provider.invoke(JUnit4Provider.java:155)
at org.apache.maven.surefire.booter.ForkedBooter.runSuitesInProcess(ForkedBooter.java:385)
at org.apache.maven.surefire.booter.ForkedBooter.execute(ForkedBooter.java:162)
at org.apache.maven.surefire.booter.ForkedBooter.run(ForkedBooter.java:507)
at org.apache.maven.surefire.booter.ForkedBooter.main(ForkedBooter.java:495)
Caused by: java.lang.NullPointerException: Cannot invoke "org.eclipse.tycho.core.TychoProjectManager.getArtifactKey(org.eclipse.tycho.ReactorProject)" because "this.projectManager" is null
at org.eclipse.tycho.p2resolver.TargetPlatformFactoryImpl.getVersion(TargetPlatformFactoryImpl.java:333)
at org.eclipse.tycho.p2resolver.TargetPlatformFactoryImpl.gatherP2InfUnits(TargetPlatformFactoryImpl.java:315)
at org.eclipse.tycho.p2resolver.TargetPlatformFactoryImpl.createTargetPlatform(TargetPlatformFactoryImpl.java:273)
at org.eclipse.tycho.p2resolver.TargetPlatformFactoryImpl.createTargetPlatform(TargetPlatformFactoryImpl.java:214)
at org.eclipse.tycho.p2resolver.P2ResolverTest.getTargetPlatform(P2ResolverTest.java:491)
at org.eclipse.tycho.p2resolver.P2ResolverTest.getTargetPlatform(P2ResolverTest.java:486)
at org.eclipse.tycho.p2resolver.P2ResolverTest.getTargetPlatform(P2ResolverTest.java:478)
at org.eclipse.tycho.p2resolver.P2ResolverTest.lambda$testDuplicateInstallableUnit$0(P2ResolverTest.java:114)
at org.junit.Assert.assertThrows(Assert.java:1001)
... 34 more
That should have been obvious because of this code:
private List<MavenArtifactKey> getMissingJunitBundles(ReactorProject project, Set<IInstallableUnit> externalUIs) {
List<MavenArtifactKey> missing = new ArrayList<>();
if (projectManager != null) {
There was a problem hiding this comment.
I'm a little doubtful now whether this
private Version getVersion(ReactorProject reactorProject) {
if (projectManager == null) {
return DEFAULT_P2_ADVICE_VERSION;
}
try {
return projectManager.getArtifactKey(reactorProject).map(key -> Version.parseVersion(key.getVersion()))
.orElseGet(() -> DEFAULT_P2_ADVICE_VERSION);
} catch (IllegalArgumentException ex) {
return DEFAULT_P2_ADVICE_VERSION;
}
}
is better than this:
private static final Pattern REACTOR_PROJECT_VERSION_PATTERN = Pattern
.compile("((?:[0-9]+)(?:\\.(?:[0-9]+)(?:\\.(?:[0-9]+))?)?)?([.-][A-Za-z0-9_-]+)?");
private static Version getVersion(ReactorProject reactorProject) {
String version = reactorProject.getVersion();
if (version == null) {
return DEFAULT_P2_ADVICE_VERSION;
}
Matcher matcher = REACTOR_PROJECT_VERSION_PATTERN.matcher(version);
if (!matcher.matches()) {
return DEFAULT_P2_ADVICE_VERSION;
}
String qualifier = matcher.group(2);
if (qualifier != null && qualifier.startsWith("-")) {
if (TychoConstants.SUFFIX_SNAPSHOT.equals(qualifier)) {
version = version.substring(0, version.length() - TychoConstants.SUFFIX_SNAPSHOT.length())
+ TychoConstants.SUFFIX_QUALIFIER;
} else {
version = matcher.group(1) + '.' + qualifier.substring(1);
}
}
try {
return Version.parseVersion(version);
} catch (IllegalAccessError ex) {
return DEFAULT_P2_ADVICE_VERSION;
}
}
f14cee8
to
884534a
Compare
Currently the implementation hard codes the version to 1.0.0 when parsing advice from a p2.inf. This can result in an IAE if the version is used in a range such as [1.0.0,$version$) where because [1.0.0,1.0.0) is not a valid range. Instead use the reactor project's actual version.
merks
force-pushed
the
pr-tp-factory-use-artifact-version
branch
from
884534a
to
7425d30
Compare
There are some unit test sadly that do not have a complete setup but in any real world usecase there is always a project manager. |
Ah, I see. So the guard is good for the test, and the other logic is a simpler expression that reuses existing infrastructure. So that's all goodness. Let's hope all the tests pass now. |
💚 All backports created successfully
Note: Successful backport PRs will be merged automatically after passing CI. Questions ?Please refer to the Backport tool documentation and see the Github Action logs for details |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Currently the implementation hard codes the version to 1.0.0 when parsing advice from a p2.inf. This can result in an IAE if the version is used in a range such as [1.0.0,$version$) where because [1.0.0,1.0.0) is not a valid range.