Unexpected behavior of -DallowMajorReleases=false

[daniel-beck]

Recently seen output of display-dependency-updates:

$ mvn org.codehaus.mojo:versions-maven-plugin:2.8.1:display-dependency-updates
[INFO] Scanning for projects...
[INFO] 
[INFO] -------------------< org.jenkins-ci:update-center2 >--------------------
[INFO] Building Jenkins Update Center Generator 3.6-SNAPSHOT
[INFO] --------------------------------[ jar ]---------------------------------
[INFO] 
[INFO] --- versions-maven-plugin:2.8.1:display-dependency-updates (default-cli) @ update-center2 ---
[INFO] The following dependencies in Dependency Management have newer versions:
[INFO]   com.github.spotbugs:spotbugs-annotations ............. 3.1.12 -> 4.2.2
[INFO]   junit:junit ........................................... 4.13 -> 4.13.2
[INFO]   org.codehaus.mojo:animal-sniffer-annotations ............ 1.18 -> 1.20
[INFO] 
[INFO] The following dependencies in Dependencies have newer versions:
[INFO]   com.squareup.okhttp3:mockwebserver ............ 4.8.0 -> 5.0.0-alpha.2
[INFO]   com.squareup.okhttp3:okhttp-urlconnection ..... 4.8.0 -> 5.0.0-alpha.2
[INFO]   commons-codec:commons-codec .................. 1.14 -> 20041127.091804
[INFO]   commons-io:commons-io ......................... 2.7 -> 20030203.000550
[INFO]   jaxen:jaxen ............................... 1.2.0 -> 1.2.0-atlassian-2
[INFO]   org.jetbrains.kotlin:kotlin-stdlib-common ......... 1.3.72 -> 1.5.0-M1
[INFO] 
[INFO] The following dependencies in Plugin Dependencies have newer versions:
[INFO]   org.codehaus.mojo:extra-enforcer-rules .................... 1.2 -> 1.3
[INFO] 
[INFO] ------------------------------------------------------------------------
[INFO] BUILD SUCCESS
[INFO] ------------------------------------------------------------------------
[INFO] Total time:  1.058 s
[INFO] Finished at: 2021-03-20T10:59:07+01:00
[INFO] ------------------------------------------------------------------------

A lot of these suggested updates are to prereleases (alpha, RC, M). A quick look at the docs suggests -DallowMajorUpdates=false -DallowAnyUpdates=false. Applying that:

$ mvn -DallowMajorUpdates=false -DallowAnyUpdates=false -DallowSnapshots=false org.codehaus.mojo:versions-maven-plugin:2.8.1:display-dependency-updates
[INFO] Scanning for projects...
[INFO] 
[INFO] -------------------< org.jenkins-ci:update-center2 >--------------------
[INFO] Building Jenkins Update Center Generator 3.6-SNAPSHOT
[INFO] --------------------------------[ jar ]---------------------------------
[INFO] 
[INFO] --- versions-maven-plugin:2.8.1:display-dependency-updates (default-cli) @ update-center2 ---
[INFO] The following dependencies in Dependency Management have newer versions:
[INFO]   com.github.spotbugs:spotbugs-annotations ......... 3.1.12 -> 4.0.0-RC3
[INFO]   org.codehaus.mojo:animal-sniffer-annotations ............ 1.18 -> 1.20
[INFO] 
[INFO] The following dependencies in Dependencies have newer versions:
[INFO]   com.squareup.okhttp3:mockwebserver ............ 4.8.0 -> 5.0.0-alpha.2
[INFO]   com.squareup.okhttp3:okhttp-urlconnection ..... 4.8.0 -> 5.0.0-alpha.2
[INFO]   commons-codec:commons-codec ............................. 1.14 -> 1.15
[INFO]   commons-io:commons-io ................................... 2.7 -> 2.8.0
[INFO]   org.jetbrains.kotlin:kotlin-stdlib-common ......... 1.3.72 -> 1.5.0-M1
[INFO] 
[INFO] The following dependencies in Plugin Dependencies have newer versions:
[INFO]   org.codehaus.mojo:extra-enforcer-rules .................... 1.2 -> 1.3
[INFO] 
[INFO] ------------------------------------------------------------------------
[INFO] BUILD SUCCESS
[INFO] ------------------------------------------------------------------------
[INFO] Total time:  1.072 s
[INFO] Finished at: 2021-03-20T10:59:23+01:00
[INFO] ------------------------------------------------------------------------

Well, that's not helpful. spotbugs-annotations is even worse now, instead of 4.2.2 I get 4.0.0-RC3.

It looks like -DallowMajorUpdates=false allows the latest release older than (X+1).0, and based on Maven version math AFAIUI, that includes pre-release versions like (X+1).0-RC3 -- which is pretty unhelpful for my use case of trying to apply simple updates to actual releases.

Either of the following would be an improvement:

• With -DallowMajorUpdates=false, look only for releases within the same major version (i.e. "version number starts with X").
• Add an option to exclude prerelease versions. This would also provide better results for some of the other dependencies shown, and combined with -DallowMajorUpdates=false I could get the latest available release of the current major line.

[michael-o]

Same issue here. I would not expect to see prerelease versions as well.

[jarmoniuk]

Please use the newly added -Dmaven.version.ignore switch to filter out pre-release qualifiers. E.g.

"-Dmaven.version.ignore=.+-(M\d+|SNAPSHOT|RC.*)"

(On POSIX-compatible shells, the option must be surrounded in single or double quotes so that it's not expanded by the shell)

See also #684

[michael-o]

I personally dislike the fact, that the property begins with maven., since this is occupied by official Maven plugins. We obviously need to document that properly.

@slawekjaranowski @cstamas WDYT?

[cstamas]

Use mojohaus. or something, not maven. please.

[jarmoniuk]

How about mojohaus.versions. for all such switches then (and then aliases for the existing ones for the time being)?

[michael-o]

How about mojohaus.versions. for all such switches then (and then aliases for the existing ones for the time being)?

Sounds good

[fgabolde]

I stumbled on this issue after looking for a fix specifically for update-properties (https://github.com/mojohaus/versions-maven-plugin/issues/552).

While -Dmaven.version.ignore (or mojohaus.version.ignore, or whatever the parameter name is at release) would certainly be a workaround, it's a painful one. It only filters out some pre-releases, e.g. SLF4J has alpha/beta releases like 2.0.0-beta1. Sure we can pile on some more patterns in the ignore property, but really what is needed is for allowMajorUpdates and others to work out of the box according to what the user meant. 2.0.0-beta1 is a major update from 1.x, so it should be disallowed.

[jarmoniuk]

Sorry. This is indeed a bug. I must have overlooked this paragraph:

It looks like -DallowMajorUpdates=false allows the latest release older than (X+1).0, and based on Maven version math AFAIUI, that includes pre-release versions like (X+1).0-RC3 -- which is pretty unhelpful for my use case of trying to apply simple updates to actual releases.

Yes, those qualified releases are indeed considered older than the .0 version, so the range which is being considered by the plugin is not at all correct.

@sultan are you working on this by any chance? I saw that you were still busy with a PR for correcting range limits. Otherwise I'll gladly tackle this one.

[sultan]

not working on anything rn, feel free to start, i'll jump on the boat later to help

[sultan]

@ajarmoniuk tests on my personal poms looks better today than the 2y old screen cap.

looks fixed to me

here from

[jarmoniuk]

You tried that with -DallowMajorUpdates=false? Then it indeed looks like the issue is fixed, since it's not proposing a qualified version of release 4. I think your changes might have fixed that if I remember correctly. I'll recheck to verify.

[sultan]

am also trying more cases before final word

[sultan]

-DallowMajorUpdates=false -DallowSnapshots=false org.codehaus.mojo:versions-maven-plugin:display-dependency-updates

not good ....

[jarmoniuk]

Still not working on the master branch:

mvn org.codehaus.mojo:versions-maven-plugin:2.12.1-SNAPSHOT:display-dependency-updates -DallowMajorUpdates=false

...

org.apache.maven.reporting:maven-reporting-impl .... 3.2.0 -> 4.0.0-M2

[sultan]

@ajarmoniuk you and i both let allowAnyUpdates=true by default return empty

[sultan]

what is the intended behaviour, let allowAny true by default ? should allowMajor=false imply allowAny=false ?

[jarmoniuk]

This ? : operator looks correct to me. Only if allowAny is false can we descend further and look for the unchanged segment. If it's true then we're returning empty(). By the way, the whole system of allow...updates is misconceived because those options are not independent. An enum would make much more sense. I was actually planning to overhaul it.

Any allow...updates switch equals false implies that the more major switches are also false. And allowAllUpdates is more major than allowMajorUpdates.

Anyway, I'll look into this issue tomorrow morning 😀

[sultan]

ternary operator looks correct to me too.

in order to get correct result i was required to execute : -DallowAnyUpdates=false -DallowMajorUpdates=false

about improvements: allowMinor allowMinor etc could be replaced by an enum with NULL/EMPTY equivalent to MAJOR, examples -DupdateScope=major -DupdateUpTo=major

Subinc	Inc	Minor	Major
YES	YES	YES	YES	Major/Any/All/Empty/Null/
YES	YES	YES	no	Minor/MinorOrLess
YES	YES	no	no	Inc/IncOrLess
YES	no	no	no	SubInc/SubincOnly

others are a bit more complicated, i would like to introduce the possibility to disallow previews :

Snapshots	Previews : Milestones RCs	Releases
YES	YES	YES	Any/All/Empty/Null
YES	no	no	Snapshots Only
no	YES	no	Previews Only
no	no	YES	Releases Only
no	YES	YES	All but Snapshots
YES	no	YES	All but Previews
YES	YES	no	All but Releases

[sultan]

default previews pattern would be

public static final Pattern PREVIEW_PATTERN =
        Pattern.compile( "(?i)(?:.*[-.](a|alpha|b|beta|cr|m|mr|preview|rc|rm)"
                + "(\\d{0,2}[a-z]?|\\d{6}\\.\\d{4})|\\d{8}(?:\\.?\\d{6})?)$" );

overridable by -DpreviewsPattern

default snapshot pattern would be

public static final Pattern SNAPSHOT_PATTERN =
        Pattern.compile( "(?i)(?:.*[-.](snapshot)"
                + "(\\d{0,2}[a-z]?|\\d{6}\\.\\d{4})|\\d{8}(?:\\.?\\d{6})?)$" );

overridable by -DsnapshotsPattern

[jarmoniuk]

Hmm the point the OP is making here is that the ranges for version discovery are incorrect. In particular, upper bound is constructed so that its (segment-1) segment is incremented, and the rest is left at zero.

So, if we say that we want to see the possible upgrades filtered to minor versions for, say, 1.0.4, the range would be:

[1.1.0, 2.0.0)

The problem is that 2.0.0 is newer than 2.0.0-SNAPSHOT or 2.0.0-rc1, or 2.0.0-M1, or 2.0.0-ajarmoniuk. So 2.0.0-ajarmoniuk would be inside the range... Whilst we actually don't want anything with 2 in front!

By the way, so the code responsible for computing the next range version (with the given segment incremented) is this:

static ArtifactVersion copySnapshot( ArtifactVersion source, ArtifactVersion destination )
{
    if ( isSnapshot( destination ) )
    {
        destination = stripSnapshot( destination );
    }
    final Matcher matcher = SNAPSHOT_PATTERN.matcher( source.toString() );
    if ( matcher.find() )
    {
        return new DefaultArtifactVersion( destination.toString() + "-" + matcher.group( 0 ) );
    }
    else
    {
        return new DefaultArtifactVersion( destination.toString() + "-SNAPSHOT" );
    }
}

[jarmoniuk]

Hmm I saw that these are your changes.

Actually, qualifiers are not restricted to

private static final String[] QUALIFIERS = {
                "snapshot", "alpha", "beta", "milestone", "preview", "rc", "", "sp" };

They can be anything after the hyphen. So, if we happen to have a version with the qualifier -ajarmoniuk, it will be counted as inferior to the actual release version.

[jarmoniuk]

So, instead of incrementing the next segment to create a bound, which makes you want to exclude those "qualifiers", I'm actually thinking of introducing a notion of infinity on a given segment location. So, the filtered range for the lookup for a next minor version for 1.0.4 would be:

[1.1.0, 1.∞.∞) ...-ish.

[sultan]

well yes the system is hard on us. composer for php as a nice syntax with "1^". infinity or ^ will need change maven itself, for the better haha ^^ (or not because build non reproducible)

alas, would it be ok to read [1.1.0, 2) as [1.1.0, 2-snapshot) when the right bound is exclusive ? (non qualified ?) this would be on par with "non range" versions being incremented to snapshots bounds already.

do we ask the user to limit "releases only" themself with -D ?

looks like there is 2 problems to solve.

[jarmoniuk]

infinity or ^ will need change maven itself

Noo, it's not that hard. It's the version comparator which needs to be aware of it, and it's a part of this plugin. :)

[sultan]

ok i'll try to work on a PR when i can

[jarmoniuk]

Ah, actually I've been looking at it. 😄

[jarmoniuk]

Tbh, the function computing the update scope should probably look like this (but, like I said, the scope representation should probably be (internally at least) changed to an enum):

    private Optional<Segment> calculateUpdateScope()
    {
        if ( !allowIncrementalUpdates && !allowMinorUpdates && !allowMajorUpdates && !allowAnyUpdates )
        {
            throw new IllegalArgumentException( "One of: allowAnyUpdates, allowMajorUpdates, allowMinorUpdates, "
                    + "allowIncrementalUpdates must be true" );
        }

        return allowAnyUpdates && allowMajorUpdates && allowMinorUpdates
                ? empty()
                : allowMajorUpdates && allowMinorUpdates
                    ? of( MAJOR )
                    : allowMinorUpdates
                        ? of( MINOR )
                        : of( INCREMENTAL );
    }

and this infinity calculus + probably an additionally something less than zero for representing the snapshots. As it is now, the lowest bound gets restricted at version 0, which is, as we know, greater than all versions with the qualifier.

[jarmoniuk]

Almost done....

[fgabolde]

This looks great. When can we expect 2.13.0 to be released? :sweat_smile:

Edit: actually, I just tried it out from a local clone, and it doesn't seem to work for me? I tested with

mvn org.codehaus.mojo:versions-maven-plugin:2.12.1-SNAPSHOT:update-properties -DallowMajorUpdates=false

and had e.g.

-    <spring-boot.version>2.7.4</spring-boot.version>
+    <spring-boot.version>3.0.0-M5</spring-boot.version>

[jarmoniuk]

I see. The update (using the new BoundArtifactVersion) was not applied to all possible Mojos and all possible methods retrieving versions. The scope of the PR was limited to the issue at hand. So there's room for improvement.

Working on it then.

@slawekjaranowski to minimize the PR size, this won't be a big-bang PR fixing everything across the board, but multiple smallish PRs fixing particular mojos.