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

Make native-mt and agp version types for Maven and Gradle #4077

Merged
merged 3 commits into from Jul 26, 2021

Conversation

greysteil
Copy link
Contributor

Fixes #2547.

Version comparison in Dependabot uses the Maven specification when looking at Maven / Gradle versions. Whilst technically correct, this can cause problems where a popular dependency has chosen to append their own suffix onto their versions to distinguish between types. The problems are not unique to Dependabot.

The pragmatic solution is to add some special cases to our version fetching logic. We already treat jre, android and java as "types" when they appear in a version string - this PR extends that logic to do the same for native-mt and agp (short for "Android Gradle Plugin" and referenced here).

Note: the native-mt type is a particular pain, as it includes a token separator character (the -). I've added extra special casing to deal with that, and a test for it.

@greysteil greysteil requested a review from a team as a code owner July 23, 2021 15:31
@greysteil
Copy link
Contributor Author

@hiqua FYI

@hiqua
Copy link

hiqua commented Jul 25, 2021

@greysteil nice, thanks a lot!

I think this doesn't cover this case though:

or does it?

It's unfortunate, since, from my reading of https://semver.org/spec/v2.0.0.html#spec-item-9, this should be considered a pre-release. From my limited experience, it would be better to default to considering these x.y.z-$SUFFIX as prereleases, and whitelist a few of these $SUFFIX that could mean something else (like a specific flavor of the library).

@greysteil
Copy link
Contributor Author

greysteil commented Jul 25, 2021

It won't fix that, no, but it's not clear to me that it should - as far as I'm aware the Maven ecosystem uses this specification for version comparison, not SemVer 2.0, and I presume Gradle does the same for consistency. Dependabot would be doing the wrong thing in even more cases if it followed SemVer 2.0 for Maven/Gradle.

(FWIW, here's the version comparison logic Dependabot uses for Gradle. We have different version comparison logic for each language because they're all different - here's python's and here's npm's (the easy one, as it's just SemVer - they wrote the SemVer spec).)

Are you using other tooling that sort 1.0 ahead of 1.0-20050927.133100? I don't use Maven or Gradle myself, so am happy to be corrected, but my hunch here is that in this case it's the package author who has made a mistake, releasing alphas / betas with versions that in fact treat them as full releases.

@hiqua
Copy link

hiqua commented Jul 26, 2021

It won't fix that, no, but it's not clear to me that it should - as far as I'm aware the Maven ecosystem uses this specification for version comparison, not SemVer 2.0, and I presume Gradle does the same for consistency. Dependabot would be doing the wrong thing in even more cases if it followed SemVer 2.0 for Maven/Gradle.

Unless I'm misreading it, your link indicates that Maven should mostly follow Semver 1.0.0 (which seems similar for these cases: https://semver.org/spec/v1.0.0.html#spec-item-4):

If version strings are syntactically correct Semantic Versioning 1.0.0 version numbers, then in almost all cases version comparison follows the precedence rules outlined in that specification. These versions are the commonly encountered alphanumeric ASCII strings such as 2.15.2-alpha.

This example (2.15.2-alpha) seems to indicate that they indeed treat a -alpha version as a prerelease.

Are you using other tooling that sort 1.0 ahead of 1.0-20050927.133100?

Android Studio (and I guess vanilla Jetbrains' IDEs) correctly suggest to upgrade from the prerelease to the release for this example (and not the other way around):

2021-07-26-074631_1400x144_scrot

Of course they could have a specific exception for this library but that's unlikely.

@jurre jurre merged commit 98d9536 into main Jul 26, 2021
@jurre jurre deleted the make-native-mt-a-type branch July 26, 2021 13:50
@jurre jurre mentioned this pull request Jul 26, 2021
@Nishnha Nishnha mentioned this pull request Jul 26, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Bumps kotlinx-coroutines-core from 1.3.1 to 1.3.9-native-mt-2.
4 participants