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

Prefer -Dchangelist.format=%d.v%s #19

Merged
merged 1 commit into from
Mar 1, 2021

Conversation

jglick
Copy link
Member

@jglick jglick commented Mar 1, 2021

Judging by jenkinsci/lib-version-number@0071313 this should work better to support backport branches.

@daniel-beck
Copy link
Member

daniel-beck commented Mar 1, 2021

The test is a bit short. I like where this is going based on the description, but it doesn't express enough what the result it accomplishes is.

One minor gripe is that for me, v followed by a letter is not quickly identifiable as a prefix, but I guess there are few alternative options that don't look even weirder.

@jglick jglick requested a review from timja March 1, 2021 22:01
@jglick
Copy link
Member Author

jglick commented Mar 1, 2021

Basically I need to ensure that there is some prefix you can add to version before ${changelist} for a backport branch which

  • is recognizably derived from at least the main (numeric) portion of the version of the branch point
  • ensures that backport versions sort after the branch point but before the next trunk version
  • ensures that backport versions sort numerically

With the %d.%s format I could not find a good option that worked reliably: the truncated commit hash was too liable to be misinterpreted as an integer with some trailing garbage. With this amendment you can just use 123. where the branch point was 123.v************: 123.v************ < 123.1.v************ < 123.2.v************ < 124.v************

Copy link
Member

@daniel-beck daniel-beck left a comment

Choose a reason for hiding this comment

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

Thanks for taking care of this!

@jglick
Copy link
Member Author

jglick commented Mar 1, 2021

To test, I am deliberately being a temporary jerk and merging jenkinsci/file-parameters-plugin#27 before jenkinsci/file-parameters-plugin#31, but then relenting and cutting a 99.x branch with a backport of the bug fix.

@jglick
Copy link
Member Author

jglick commented Mar 1, 2021

jenkins-infra/github-reusable-workflows#14 may be an issue, unclear.

@jglick jglick changed the title Prefer -Dchangelist.format=%d.v%s Prefer -Dchangelist.format=%d.v%s Dec 28, 2021
@jtnord
Copy link
Member

jtnord commented Jan 13, 2022

this seems incorrect or at least incorrectly applied in plugins where they are just using this and not prefixed version (e.g. mailer) - Maven treats 123.vxxxx as a String. and the entire major version will be empty.

This leads to people using versions like 391.ve4a_38c1b_cf4b_ which is older (in Maven) than 1.1

Whilst Jenkins (hudson.util.VersionNumber) is fine with this - Maven and all its related tooling is not.
This will break for example things that use maven version comparison, for example dependabot, maven-versions-plugin etc..

def cdVerJ = new hudson.util.VersionNumber("391.ve4a_38c1b_cf4b_")
def oldVerJ = new hudson.util.VersionNumber("1.1")

println("is Jenkins CD older " + cdVerJ.isOlderThan(oldVerJ))


def cdVerM = new org.apache.maven.artifact.versioning.DefaultArtifactVersion("391.ve4a_38c1b_cf4b_")
def oldVerM = new org.apache.maven.artifact.versioning.DefaultArtifactVersion("1.1")


println("cd version major maven" + cdVerM.getMajorVersion())
println("cd version major maven " + oldVerM.getMajorVersion())

prints (in the Jenkins console)

is Jenkins CD older false
cd version major maven 0
old version major maven 1

it would seem to me like the changelist format should be %d-v%s to work in all cases where a revision is specified or not.

@jglick
Copy link
Member Author

jglick commented Jan 13, 2022

More likely just seeing #24?

@daniel-beck
Copy link
Member

More likely just seeing #24?

@jglick The sample code in the previous comment demonstrates the problem with a version number with a_ and b_ though.

@jtnord
Copy link
Member

jtnord commented Jan 13, 2022

nope
sorry for the noise - I ran the check inside Jenkins in the script console. When I use Maven libraries directly from maven 3 (3.8.4) maven does sort the version as newer, older versions of maven (2.x) sort as originally described.

Jenkins is exposing the mave2 version of the library.

There is no reason to be using maven 2 so I guess this should be closed.
The Major version is parsed as 0 in both cases, but a version comparison (compareTo) with maven3 libs works correctly.

@jglick
Copy link
Member Author

jglick commented Jan 13, 2022

Not noise! Maven handling of version numbers is complex and prone to odd behaviors (as we saw in #24) so we need to remain on the lookout for subtle problems affecting JEP-229.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants