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

display-dependency-updates version comparison algorithm bug for -pfd #744

Closed
garretwilson opened this issue Oct 7, 2022 · 43 comments
Closed
Labels

Comments

@garretwilson
Copy link

garretwilson commented Oct 7, 2022

I have the following managed dependency in my Maven POM:

<dependency>
  <groupId>javax.faces</groupId>
  <artifactId>javax.faces-api</artifactId>
  <version>2.3</version>
  <scope>provided</scope>
</dependency>

However when I invoke mvn versions:display-dependency-updates, Versions Maven Plugin shows me:

javax.faces:javax.faces-api ........................... 2.3 -> 2.3-pfd

Offhand I don't know what -pfd means (pre-final-deliverable?), but that's irrelevant. You can see on Maven Central that this version was released before v2.3.

The semantic versioning specification says that any -* suffix (which would include -pfd) is considered a "pre-release" version. Thus 2.3-pfd is considered a pre-release version of 2.3 and should not be listed as a new available version. (The specification then goes on to explain how to calculate version precedence for pre-release suffixes, but that's even not the case here, as any pre-release version should not appear greater than the release version.)

(On a related note, the specification also says that version metadata, that is suffixes beginning with +*, must be completely ignored when determining version precedence.)

@andrzejj0
Copy link
Contributor

Confirmed

@garretwilson
Copy link
Author

Thank you, @ajarmoniuk . It's really so nice to see this project is getting quick and useful responses. I sincerely appreciate it. Have a good weekend.

@andrzejj0
Copy link
Contributor

No worries! Looking into it. Have a nice weekend too!

@andrzejj0
Copy link
Contributor

andrzejj0 commented Oct 7, 2022

Apparently, the problem also exists in Maven itself -- looks like the class ComparableVersion got carried over to org.apache.maven.artifact.versioning.ComparableVersion there.

If you use a range specification like

<dependency>
  <groupId>javax.faces</groupId>
  <artifactId>javax.faces-api</artifactId>
  <version>[2.3,)</version>
</dependency>

your project will actually depend on version 2.3-pfd (you can check with e.g. dependency:tree). But this is indeed incorrect.

@andrzejj0
Copy link
Contributor

@slachiewicz

@andrzejj0
Copy link
Contributor

https://issues.apache.org/jira/browse/MNG-7559

@mprins
Copy link

mprins commented Oct 8, 2022

both javadoc and version policy proposal describe exactly this behaviour;

Unknown qualifiers are considered after known qualifiers, with lexical order (always case insensitive),

I have failed to find any Maven documentation that says Maven follows what semver describes.

https://octopus.com/blog/maven-versioning-explained provides a very readable summary

@andrzejj0
Copy link
Contributor

andrzejj0 commented Oct 8, 2022

On the other hand, https://www.mojohaus.org/versions-maven-plugin/version-rules.html and e.g. https://docs.oracle.com/middleware/1212/core/MAVEN/maven_version.htm#MAVEN401 seem to promote a more generic approach, conflicting with the one presented in the blog you mention, @mprins

The thing about this blog post is that it actually treats the class as a source of truth. But we kinda can read code anyway, can't we?

All versions with a qualifier are older than the same version without a qualifier (release version).

Unless we only consider "standard" qualifiers as qualifiers.

@mprins
Copy link

mprins commented Oct 8, 2022

I don't think linking to an oracle document from 2015 is helpful at all; it is clear from the maven versioning wiki that I pointed to (and that the mojohaus document you gave us links to in the first line) that unknown qualifiers added to a version are considered to be a higher version than no qualifier (eg. commonly used Final, GA by hibernate work fine as do things like jre8, jre17, atlassian-1, atlassian-2, redhat-2... https://mvnrepository.com/artifact/jaxen/jaxen?repo=redhat-ga )

@garretwilson garretwilson changed the title display-dependency-updates version comparison algorithm bug for -pdf display-dependency-updates version comparison algorithm bug for -pfd Oct 8, 2022
@andrzejj0
Copy link
Contributor

andrzejj0 commented Oct 8, 2022

I'm questioning the implementation, but the blog you're citing provides that implementation in question as the authority...

Any reason to disqualify the Oracle document based on its age? Has something changed in the meanwhile?

Anyway, I'll wait for someone else to chime in.

@hboutemy
Copy link
Member

hboutemy commented Oct 8, 2022

https://cwiki.apache.org/confluence/display/MAVENOLD/Versioning is our Maven reference, written and implemented before Semver 1.0.0 even existed
Changing version comparison won't bring much value: whatever convention is written by someone, other people use other convention, then there are edge cases...

@andrzejj0
Copy link
Contributor

Then this should be stated clearly in the documentation. I'll go ahead and add a link to this to the docs.

@garretwilson
Copy link
Author

garretwilson commented Oct 8, 2022

It's not surprising that at one time Maven created some reasonable, internally consistent versioning schema, and it is notable and commendable that Maven actually created a specification for this at a time when so many projects were making versioning format decisions based upon arbitrary whims of the current developer. I would guess that Maven probably even had some influence on the codification of the Semantic Versioning specification.

But it is unquestionable that the software industry is now converging around Semantic Version, and the last thing that we need is a central component of the build pipeline to be marching to some other tune. Just because something was specified at one time doesn't mean it can't be improved in a later version. HTML 4 wasn't bad. We should not be using HTML 4 in 2022—we should be using HTML 5, because that good specification has been improved.

It is certainly illustrative that after I filed this bug, initially everyone expected it to be a bug until we found some little-known Maven versioning specification. This expectation illustrates that in 2022 developers expect to be using Semantic Versioning. Developer do not expect that certain suffixes are special-cased and treated differently based upon whether they are "well known" or not. Marking certain limited set of identifiers as "well-known" leads exactly to the "other people user other convention" problem @hboutemy mentioned. It is better to treat them all the same.

The difference in version number interpretation between Maven versions and Semantic Versions is small. It is time for Maven to be using Semantic Versioning, in the least regarding the comparison of pre-release version suffixes.

@garretwilson
Copy link
Author

Let me ask you this question: looking at Maven Central, does it appear that the authors of javax.faces:javax.faces-api intended the -pfd suffix to be interpreted as Maven interprets it? Or did they intend for it to be interpreted according to how Semantic Version interprets it? The answer is clear: they intended it to be interpreted as Semantic Versioning interprets it. They likely didn't even know this Maven versioning documentation existed.

Changing version comparison won't bring much value …

What is the value of furthering some noble but outdated versioning logic that the average developer doesn't even know about, when the average developer is instead publishing artifacts that follow Semantic Versioning?

Are there very many artifacts published using a non-well-known extension for which the developer intended the Maven rules to apply? In this case at least, they expected Semantic Version rules to apply.

@andrzejj0
Copy link
Contributor

andrzejj0 commented Oct 8, 2022

Agreed with Garrett. I understand that changing the established behaviour is not fortunate. Luckily, the impact of this is relatively small since custom qualifiers are not common.

Also, rules should be clearly stated by which I don't mean an internal wiki or the source code of a class imported from versions-maven-plugin. And a source code of a class is not the source of truth.

Lastly, it's worth noting that other similar systems (e.g. Gradle) consider custom qualifiers less major than release versions.

@pzygielo
Copy link
Contributor

https://maven.apache.org/pom.html#version-order-specification

@andrzejj0
Copy link
Contributor

andrzejj0 commented Oct 10, 2022

So Maven is not exactly in line with its own specification:

else ".qualifier" < "-qualifier" < "-number" < ".number"

@pzygielo
Copy link
Contributor

So Maven doesn't even respect its own specification:

else ".qualifier" < "-qualifier" < "-number" < ".number"

Not sure what you mean by that, as

$ java -jar maven-artifact-3.8.6.jar 2.3-1 2.3-pfd
Display parameters as parsed by Maven (in canonical form and as a list of tokens) and comparison result:
1. 2.3-1 -> 2.3-1; tokens: [2, 3, [1]]
   2.3-1 > 2.3-pfd
2. 2.3-pfd -> 2.3-pfd; tokens: [2, 3, [pfd]]
$ java -jar maven-artifact-3.8.6.jar 2.3-0 2.3-pfd
Display parameters as parsed by Maven (in canonical form and as a list of tokens) and comparison result:
1. 2.3-0 -> 2.3; tokens: [2, 3]
   2.3-0 < 2.3-pfd
2. 2.3-pfd -> 2.3-pfd; tokens: [2, 3, [pfd]]

@andrzejj0
Copy link
Contributor

andrzejj0 commented Oct 10, 2022

So Maven doesn't even respect its own specification:

else ".qualifier" < "-qualifier" < "-number" < ".number"

Not sure what you mean by that, as

$ java -jar maven-artifact-3.8.6.jar 2.3-1 2.3-pfd
Display parameters as parsed by Maven (in canonical form and as a list of tokens) and comparison result:
1. 2.3-1 -> 2.3-1; tokens: [2, 3, [1]]
   2.3-1 > 2.3-pfd
2. 2.3-pfd -> 2.3-pfd; tokens: [2, 3, [pfd]]
$ java -jar maven-artifact-3.8.6.jar 2.3-0 2.3-pfd
Display parameters as parsed by Maven (in canonical form and as a list of tokens) and comparison result:
1. 2.3-0 -> 2.3; tokens: [2, 3]
   2.3-0 < 2.3-pfd
2. 2.3-pfd -> 2.3-pfd; tokens: [2, 3, [pfd]]

2.3-pdf is considered > 2.3

java -jar maven-artifact-3.8.6.jar 2.3 2.3-pfd

1. 2.3 -> 2.3; tokens: [2, 3]
   2.3 < 2.3-pfd
2. 2.3-pfd -> 2.3-pfd; tokens: [2, 3, [pfd]]

which is not in line with the specificication you posted. That's the whole problem here. I pointed out that it should probably be otherwise, i.e. release versions should always be considered more major than versions with unknown qualifiers.

@pzygielo
Copy link
Contributor

pzygielo commented Oct 10, 2022

This is exactly in line,

Empty tokens are replaced with "0"

and then 2.3-0 -> 2.3, which I shown as

2.3-0 < 2.3-pfd

I think the problem is in expectation not being matched.

@pzygielo
Copy link
Contributor

I think the specification is clear...

Me too.

As 2.3 has no qualifier, it's considered as 2.3-final for comparison, and pfd comes after all special tokens.

You can see on Maven Central that this version was released before v2.3.

Time of the release does not participate in the process. Only version string.

@andrzejj0
Copy link
Contributor

Thanks. I missed the part about empty tokens becoming zeros. Thus, the current behaviour is indeed reflected in the specification.

@sultan
Copy link
Contributor

sultan commented Oct 16, 2022

i also encountered versioning discrepancies with .RC2 < -RC1 and MR/RM/CR qualifiers and things like that.
eager to help when i can.

@sultan
Copy link
Contributor

sultan commented Oct 16, 2022

image

the order could be alpha beta rc
severity level : low

@sultan
Copy link
Contributor

sultan commented Oct 16, 2022

image

RC < MR < FINAL
severity level : medium, label is wrongly placed and use latest mojos might not take the good one

@garretwilson
Copy link
Author

garretwilson commented Oct 16, 2022

Time of the release does not participate in the process. Only version string.

I did not assert that the time of release participates in the comparison process or that it should. Rather I only mentioned this as one piece of evidence that the authors of javax.faces:javax.faces-api expected their version numbers to be interpreted according to rules similar to semantic versioning, and not according to some outdated versioning rules in an obscure Maven document that give special meaning to magic strings.

@garretwilson
Copy link
Author

garretwilson commented Oct 16, 2022

FYI I did some checking and found out what "-pfd" means in this case: apparently it is a stage in the Java Community Process that means "Proposed Final Draft" (e.g. Enterprise Java Beans 3.0 Proposed Final Draft). In other words, they are using it in much the same way as other teams use the term "release candidate". This means all the Java Specification Requests will likely wind up with a -pfd artifact in Maven (which will be sorted incorrectly) before each final release.

@sultan
Copy link
Contributor

sultan commented Oct 16, 2022

we are unlucky that someone thought it would be a good idea to use SP1 to qualify a patch release. we sure cant take every magic string into account especially if two originators used incompatible rules. but at least we can make our best to have a correct answer when asked for the latest version. currently it looks as if qualifiers after a "-" are treated as pre release and qualifier after a "." is treated as post release.

@andrzejj0
Copy link
Contributor

andrzejj0 commented Oct 17, 2022

@sultan any irregular qualifier is considered later/greater than the release version, not only by this plugin, but also by Maven. That's the whole point of this issue.

@sultan
Copy link
Contributor

sultan commented Oct 17, 2022

OK! do you happen to know an artifact having a later qualifier (other than SP)?
maybe this can be improved/discussed on the Maven git?

@andrzejj0
Copy link
Contributor

andrzejj0 commented Oct 17, 2022

Please read this whole discussion. The current maven versioning algorithm is described here. Also, the versioning implementation is basically the ComparableVersion class.

The rules (as implemented by Maven) are also outlined in https://maven.apache.org/pom.html#version-order-specification

@sultan
Copy link
Contributor

sultan commented Oct 17, 2022

i missed the issue link, thanks !

@andrzejj0
Copy link
Contributor

Oh and re your question, any "non-standard", so, e.g. "-pfd" qualifier is treated as later than the release by Maven, that's the premise of this whole thread.

This is specific to Maven btw.

@sultan
Copy link
Contributor

sultan commented Oct 17, 2022

yes, i think i understand the actual/current behaviour.

what i am not sure about, is what you expect for the future. ie not changing anything and use versions exclusion/rules.xml?

this is indeed not a bug if the actual spec and impl are in line.

i wish there would be room for improvement on maven part though

@sultan
Copy link
Contributor

sultan commented Oct 17, 2022

the spec could be improved to take more milestones qualifiers like the PREVIEW qualifiers used on jdbc drivers.

as to make .RC1 < -RC2

@sultan
Copy link
Contributor

sultan commented Nov 6, 2022

@garretwilson you can emoji upvote this if you want:

sultan added a commit to sultan/versions-maven-plugin that referenced this issue Nov 30, 2022
sultan added a commit to sultan/versions-maven-plugin that referenced this issue Nov 30, 2022
sultan added a commit to sultan/versions-maven-plugin that referenced this issue Dec 16, 2022
sultan added a commit to sultan/versions-maven-plugin that referenced this issue Dec 16, 2022
@michael-o
Copy link
Member

michael-o commented Dec 18, 2022

For those who care I am copying my latest message from https://issues.apache.org/jira/browse/MNG-7559:

The handling of this issue is a mess. @elharo, please clean up which PRs have been merged, close this and link the commit message in our canonical repos. I am not able to follow/understand what happened here.

--

I am willing to merge into 3.8.7 if the mess is cleaned up.

@sultan
Copy link
Contributor

sultan commented Dec 18, 2022

For those who care I am copying my latest message from https://issues.apache.org/jira/browse/MNG-7559:

@michael-o here are the merged PR:

I am willing to merge into 3.8.7

here is the backport PR:

@michael-o
Copy link
Member

For those who care I am copying my latest message from https://issues.apache.org/jira/browse/MNG-7559:

@michael-o here are the merged PR:

* PR [[MNG-7559] Fix versions comparison apache/maven#845](https://github.com/apache/maven/pull/845)

I am willing to merge into 3.8.7

here is the backport PR:

* PR [[MNG-7559] Fix versions comparison, backport to 3.8.x apache/maven#925](https://github.com/apache/maven/pull/925)

Totally useless merge because it cannnot be associated with the JIRA issue.

@sultan
Copy link
Contributor

sultan commented Dec 18, 2022

because it cannnot be associated with the JIRA issue.

i get your point.

maybe there is room to add this requirement in the checklist for new PRs?
"Each commit in the pull request should have a meaningful subject line and body."

Each commit in the pull request should have a meaningful subject line (including JIRA#) and body.

@michael-o
Copy link
Member

michael-o commented Dec 18, 2022

because it cannnot be associated with the JIRA issue.

i get your point.

maybe there is room to add this requirement in the checklist for new PRs? "Each commit in the pull request should have a meaningful subject line and body."

Each commit in the pull request should have a meaningful subject line (including JIRA#) and body.

It does: https://github.com/apache/maven/blob/master/.github/pull_request_template.md

I also would expect every committer to check a PR for this before merging it.

@elharo
Copy link

elharo commented Jun 17, 2023

"Each commit in the pull request should have a meaningful subject line (including JIRA#) and body." does not reflect how pull requests are actually used and merged in github maven repos in 2023. Most maven PRs are "Squash and Merge". Some repos accept squash and merge only, though that's inconsistent. When a PR is squashed, there's only one commit and the single commit message is only finalized when the PR is merged. Everything before that is at most a work in progress that does not affect the git log.

Copy link

This issue is stale because it has been open 365 days with no activity. Remove stale label or comment or this will be closed in 30 days.

@github-actions github-actions bot added the Stale label Jun 17, 2024
@github-actions github-actions bot closed this as not planned Won't fix, can't repro, duplicate, stale Jul 17, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

8 participants