Sitelet https://github.com/flutter/flutter/pull/158020
Skip to content

update mostRecentSemanticVersion to handle strings like "8.6-rc-2" - #158020

Merged
auto-submit[bot] merged 8 commits into
flutter:masterfrom
AbdeMohlbi:patch-12
Feb 12, 2025
Merged

auto-submit[bot] merged 8 commits into
flutter:masterfrom
AbdeMohlbi:patch-12

Conversation

@AbdeMohlbi

@AbdeMohlbi AbdeMohlbi commented Nov 1, 2024 •

Copy link
Copy Markdown
Member

updated mostRecentSemanticVersion func to handle strings like "8.6-rc-2" .
see :

/**
* Compares semantic versions ignoring labels.
*
* If the versions are equal (ignoring labels), returns one of the two strings arbitrarily.
*
* If minor or patch are omitted (non-conformant to semantic versioning), they are considered zero.
* If the provided versions in both are equal, the longest version string is returned.
* For example, "2.8.0" vs "2.8" will always consider "2.8.0" to be the most recent version.
* TODO: Remove this or compareVersionStrings. This does not handle strings like "8.6-rc-2".
*/

Pre-launch Checklist

If you need help, consider asking for advice on the #hackers-new channel on Discord.

@github-actions github-actions Bot added the tool Affects the "flutter" command-line tool. See also t: labels. label Nov 1, 2024
@AbdeMohlbi

Copy link
Copy Markdown
Member Author

i have no idea how to write tests for this one

@reidbaker

Copy link
Copy Markdown
Contributor

@gmackall I think this is not unit testable given our current structure. Can you confirm if that is true?

@AbdeMohlbi

Copy link
Copy Markdown
Member Author

Something i noticed is that this function does compare the ndk versions of two plugins and AFAIK there is no ndk version that have letters in it , i'm i right ?

@AbdeMohlbi

Copy link
Copy Markdown
Member Author

this seems related :

// Helper class to parse the versions that are provided as plain strings (Gradle, Kotlin) and
// perform easy comparisons. All versions will have a major, minor, and patch value. These values
// default to 0 when they are not provided or are otherwise unparseable.
// For example the version strings "8.2", "8.2.2hfd", and "8.2.0" would parse to the same version.
class Version(val major: Int, val minor: Int, val patch: Int) : Comparable<Version> {
companion object {
fun fromString(version: String): Version {
val asList: List<String> = version.split(".")
val convertedToNumbers: List<Int> = asList.map { it.toIntOrNull() ?: 0 }
return Version(
major = convertedToNumbers.getOrElse(0, { 0 }),
minor = convertedToNumbers.getOrElse(1, { 0 }),
patch = convertedToNumbers.getOrElse(2, { 0 })
)
}
}
override fun compareTo(other: Version): Int {
if (major != other.major) {
return major - other.major
}
if (minor != other.minor) {
return minor - other.minor
}
if (patch != other.patch) {
return patch - other.patch
}
return 0
}
override fun toString(): String {
return major.toString() + "." + minor.toString() + "." + patch.toString()
}
}

@AbdeMohlbi

AbdeMohlbi commented Nov 17, 2024 •

Copy link
Copy Markdown
Member Author

using groovy online compiler, u can test the code :

 static String mostRecentSemanticVersion(String version1, String version2) {
        def v1Parts = version1.tokenize('.-')
        def v2Parts = version2.tokenize('.-')

        for (int i = 0; i < Math.max(v1Parts.size(), v2Parts.size()); i++) {
            def v1Part = i < v1Parts.size() ? v1Parts[i] : '0'
            def v2Part = i < v2Parts.size() ? v2Parts[i] : '0'

            def v1Num = v1Part.isNumber() ? v1Part.toInteger() : 0
            def v2Num = v2Part.isNumber() ? v2Part.toInteger() : 0

            if (v1Num != v2Num) {
                return v1Num > v2Num ? version1 : version2
            }

            if (v1Part.isNumber() && !v2Part.isNumber()) {
                return version1
            } else if (!v1Part.isNumber() && v2Part.isNumber()) {
                return version2
            } else if (v1Part != v2Part) {
                return comparePreReleaseIdentifiers(v1Part, v2Part) ? version1 : version2
            }
        }

        // If versions are equal, return the longest version string
        return version1.length() >= version2.length() ? version1 : version2
    }

    static boolean comparePreReleaseIdentifiers(String v1Part, String v2Part) {
        def v1PreRelease = v1Part.replaceAll("\\D", "")
        def v2PreRelease = v2Part.replaceAll("\\D", "")

        return v1PreRelease < v2PreRelease
    }
    
        println mostRecentSemanticVersion("1.2", "1.2.0")           // Output: 1.2.0
        println mostRecentSemanticVersion("1.0", "1")               // Output: 1.0
        println mostRecentSemanticVersion("1.2.0-alpha", "1.2")     // Output: 1.2
        println mostRecentSemanticVersion("1.2.3", "1.2.3")         // Output: 1.2.3
        println mostRecentSemanticVersion("1.2.3-beta", "1.2.3")    // Output: 1.2.3
        println mostRecentSemanticVersion("1.2.3", "1.2.3.4")       // Output: 1.2.3.4
        println mostRecentSemanticVersion("rc-2", "rc-1")           // Output: rc-2
        println mostRecentSemanticVersion("8.7-rc-1", "8.7")        // Output: 8.7
        println mostRecentSemanticVersion("8.7-rc-1", "8.7.2")      // Output: 8.7.2
        println mostRecentSemanticVersion("8.7.2", "8.7.1")         // Output: 8.7.2  

@AbdeMohlbi
AbdeMohlbi marked this pull request as ready for review November 17, 2024 17:39
@reidbaker
reidbaker marked this pull request as draft November 19, 2024 19:33
@reidbaker

Copy link
Copy Markdown
Contributor

Marking as draft since pr description says WIP

@flutter-dashboard

Copy link
Copy Markdown

This pull request has been changed to a draft. The currently pending flutter-gold status will not be able to resolve until a new commit is pushed or the change is marked ready for review again.

For more guidance, visit Writing a golden file test for package:flutter.

Reviewers: Read the Tree Hygiene page and make sure this patch meets those guidelines before LGTMing.

@AbdeMohlbi

AbdeMohlbi commented Nov 19, 2024 •

Copy link
Copy Markdown
Member Author

Marking as draft since pr description says WIP

fair, i forget to change it .
although I still don't know if is it working right ?

@reidbaker
reidbaker requested a review from jesswrd December 17, 2024 16:40
@AbdeMohlbi
AbdeMohlbi marked this pull request as ready for review January 7, 2025 09:54
@reidbaker

Copy link
Copy Markdown
Contributor

@AbdeMohlbi getting this on our review queue. @jesswrd will take a first pass.

Comment thread packages/flutter_tools/gradle/src/main/groovy/flutter.groovy Outdated
Comment thread packages/flutter_tools/gradle/src/main/groovy/flutter.groovy Outdated
int num2 = version2Tokenized[i].toInteger()
if (num1 > num2) {
return version1
static String mostRecentSemanticVersion(String version1, String version2) {

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

We should either define the input space to be only Major.Minor.Patch in the above doc, just to be clear that we don't care about cases like 8.7-rc-2 vs 8.7.0.1, or otherwise handle them in way that compares initially based on a tokenization of . (i.e., here 7-rc-2 in the former gets compares to 7 in the latter)

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

sorry for the late response i have some issues with github , so i'm still a bit confused now , shoud it do something like that now ?

version1 version2 Result
1.2.3 1.2.4 1.2.4
1.2.3-rc-1 1.2.3 1.2.3
1.2.3-beta 1.2.3-alpha 1.2.3
1.2.3.1 1.2.3 1.2.3
8.7-rc-2 8.7.0.1 8.7.0

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Sorry for the late reply - I made that comment during a triage meeting, and on second reading the comparison method seems good to me

Co-authored-by: Gray Mackall <34871572+gmackall@users.noreply.github.com>

@gmackall gmackall left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Sorry for the delay! LGTM

@gmackall

Copy link
Copy Markdown
Member

You may need to update the branch, as it looks like the ci.yaml check failed here for reasons I presume are unrelated (and that check in particular can't be re-run from the github ui without updating the branch).

engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Feb 13, 2025
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Feb 13, 2025
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Feb 13, 2025
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Feb 14, 2025
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Feb 14, 2025
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Feb 14, 2025
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Feb 14, 2025
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Feb 14, 2025
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Feb 15, 2025
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Feb 15, 2025
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Feb 16, 2025
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Feb 16, 2025
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Feb 17, 2025
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Feb 17, 2025
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Feb 17, 2025
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Feb 18, 2025
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Feb 18, 2025
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Feb 18, 2025
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Feb 19, 2025
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Feb 19, 2025
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Feb 19, 2025
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Feb 20, 2025
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Feb 20, 2025
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Feb 20, 2025
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Feb 20, 2025
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request May 20, 2025
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request May 20, 2025
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request May 21, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

platform-android Android applications specifically tool Affects the "flutter" command-line tool. See also t: labels.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants