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

DefaultArtifactVersion.majorVersion 0 for JEP-229 versions #29

Open
jglick opened this issue Jan 13, 2022 · 8 comments
Open

DefaultArtifactVersion.majorVersion 0 for JEP-229 versions #29

jglick opened this issue Jan 13, 2022 · 8 comments
Labels
bug Something isn't working

Comments

@jglick
Copy link
Member

jglick commented Jan 13, 2022

@jtnord reports #19 (comment).

  • Debug behavior of Maven’s version number parser here.
  • Determine which tools are affected: apparently Dependabot is not, but maven-versions-plugin claimed to be?
  • Check whether switching . to - indeed placates Maven, without breaking comparisons for the Jenkins parser or Dependabot, and if so:
  • Edit recommendations on jenkins.io.
  • Roll out changes to all components currently using ..
@jglick jglick added the bug Something isn't working label Jan 13, 2022
@jglick
Copy link
Member Author

jglick commented Jan 13, 2022

jenkinsci/mailer-plugin#149 (comment) says this applies only to Maven 2.x and not 3.x?

@jglick
Copy link
Member Author

jglick commented Jan 13, 2022

At a minimum https://github.com/jenkinsci/lib-version-number/blob/f1ed0e92de5cf083e710b4d5a4d2c214635d4674/src/test/java/hudson/util/VersionNumberTest.java#L138 is inadequate and should be extended to cover comparisons between traditional and JEP-229 formats.

@jtnord
Copy link
Member

jtnord commented Jan 13, 2022

https://github.com/jtnord/maven-version-compare-tool shows a different behaviour when using the DefaultArtifactVersion from Maven 2 vs Maven3
Jenkins in the script console is then presumably getting an old version of the library.

the library from the Maven 3 series agrees with hudson.util.VersionNumber that the 391.vxxx version was newer than 1.x

@jglick
Copy link
Member Author

jglick commented Jan 13, 2022

presumably getting an old version of the library

org.apache.maven.artifact.versioning.ComparableVersion.class.protectionDomain.codeSource.location

giving me 3.8.2 in a reasonable current installation?

I have not checked behavior of DefaultArtifactVersion, only ComparableVersion. Would have to dig into which is used where.

@jglick
Copy link
Member Author

jglick commented Jan 13, 2022

@jglick
Copy link
Member Author

jglick commented Jan 13, 2022

Not called at all in Maven core AFAICT. https://github.com/apache/maven/blob/18725ec1e05e01b06557830ae10e83bdb933ab1a/maven-artifact/src/main/java/org/apache/maven/artifact/versioning/DefaultArtifactVersion.java#L77 means that majorVersion is actually ignored for purposes of implementing Comparable. versions-maven-plugin seems to have some calls; checking.

@jglick
Copy link
Member Author

jglick commented Jan 13, 2022

/**
 * @author Stephen Connolly
 */

LOL

@jglick
Copy link
Member Author

jglick commented Jan 13, 2022

Having a hard time following the logic in versions-maven-plugin. It does a lot of different things, evidently making assumptions about version formats that go well beyond what ComparableVersion cares about, and there are very few comments. I doubt we care much about MajorMinorIncrementalFilter since allowMajorUpdates seems to default to true anyway. MavenVersionComparator again implements Comparator by delegating to ComparableVersion, but then has additional “segment” functionality which seems to be used for calculations of bounds for update detection in certain cases. PropertyVersions appears to pay attention to majorVersion for purposes of a lower bound on something or other.

Not sure how much any of this matters; I do not think we routinely use versions-maven-plugin any more, since Dependabot is normally tasked with keeping versions new, though on occasion it is helpful to run it locally to search for outstanding updates.

Switching from . to - would appear to improve behavior in this mojo in some scenarios TBD; unknown if it would cause regressions somewhere else. I am reluctant to make such a change without a particular driver. My understanding is that this issue came up in the context of a proprietary tool which is known to be flawed in other ways and which would likely be corrected just by switching to lib-version-number.

@jglick jglick changed the title DefaultArtifactVersion.majorVersion evaluating to 0 for JEP-229 versions? DefaultArtifactVersion.majorVersion evaluating to 0 for JEP-229 versions Jan 13, 2022
@jglick jglick changed the title DefaultArtifactVersion.majorVersion evaluating to 0 for JEP-229 versions DefaultArtifactVersion.majorVersion 0 for JEP-229 versions Jan 13, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

No branches or pull requests

2 participants