check: Update the flutter_version check to follow upstream #2002

rajveermalviya · 2025-11-20T16:30:01Z

Stacked on top of #2009

Since flutter/flutter@e45fd36aa the flutter tool now uses a combination of git commands, instead of using the result of git describe to generate the version string.

So, we update the check to use the same commands here, and then do some transformations to match the string from the flutter tool:
https://github.com/flutter/flutter/blob/9f383e099/packages/flutter_tools/lib/src/version.dart#L1162

Fixes: #1851

chrisbobbe · 2025-11-21T01:12:52Z

Hmm CI is failing, could you take a look?

chrisbobbe · 2025-12-10T21:40:36Z

Hmm looks like CI is failing again.

See: https://github.com/actions/checkout/blob/8e8c483db/action.yml#L74-L76 Without this change, the main branch is not fetched when the current branch is non-main. We need this because soon we will run the `flutter_version` suite in CI only when there are changes in `pubspec.yaml` or `tools/check` files. For that, we need the `main` branch to be able to calculate the file diff. Otherwise the suite will fail when calling `git_base_commit` saying: fatal: Not a valid object name refs/remotes/origin/main

Flutter's new versioning depends on the latest tag pushed upstream. And we are going to add the check for Flutter version string back soon. To avoid frequent CI failures that may be caused by a new tag being pushed upstream, run the flutter_version check in CI only when there are changes in the `pubspec.yaml` or `tools/check` files.

Since flutter/flutter@e45fd36aa the flutter tool now uses a combination of git commands, instead of using the result of `git describe` to generate the version string. So, we update the check to use the same commands here, and then do some transformations to match the string from the flutter tool: https://github.com/flutter/flutter/blob/9f383e099/packages/flutter_tools/lib/src/version.dart#L1162 Fixes: zulip#1851

rajveermalviya · 2025-12-15T17:00:49Z

@chrisbobbe This PR is ready for review now, PTAL.

chrisbobbe · 2025-12-15T21:22:51Z

Thanks! I think it'll be more efficient if I just pass this to Greg for review, I hope that's OK. 🙂

gnprice

Thanks @rajveermalviya for taking care of this! Comments below.

gnprice · 2025-12-15T23:48:28Z

tools/check

 default_suites=(
    analyze test
-    flutter_version


This should get added to extra_suites below, so that the --help output remains accurate.

Or we could make other changes to the API, too. Whatever we do, we should keep the documentation in --help accurate, though.

gnprice · 2025-12-15T23:50:03Z

.github/workflows/ci.yml

+    - name: Run tools/check (all default suites)
      run: TERM=dumb tools/check --all --verbose


… Also if you want "all default suites", rather than "all suites", that is spelled --all-files.

gnprice · 2025-12-15T23:54:46Z

tools/check

-    # Omitted from this files check:
-    #   tools/check
-    files_check pubspec.yaml \
+    files_check pubspec.yaml tools/check \


nit: Keep this "Omitted from …" comment, but just update it — that way it's explicit that we've thought this through and believe there are no files being omitted. E.g. like this:

# Omitted from this files check: # (none known!)

See the doc comment on files_check:

# Do, though, write down in a comment the known types of changes that could affect # the outcome and are being left out. That's helpful when either revising which # changes we choose to omit, or debugging inadvertent omissions, by separating # the two from each other.

But also, why does this start making this suite run when tools/check changes? See the files_check doc comment:

# In particular, omit pubspec.yaml, pubspec.lock, and tools/check itself, # except where the suite is specifically aimed at those files.

The flutter_version suite is all about what appears in pubspec.yaml, which is why it's an exception in that respect. But I don't see how it's about tools/check, any more specifically than all the other suites are.

gnprice · 2025-12-16T00:00:54Z

tools/check

+    if [ -z "${predicted_version}" ]; then
+        return 1
+    fi


What's the case where this condition would happen?

If predict_flutter_version returns prematurely, that should be propagated by the || return on the previous line, right? So I think we should only get here if predicted_version really is the string that predict_flutter_version predicted.

Quick demo that confirms my expectation of the behavior of that || return:

$ ( f(){ local a; a=$( echo 3; false ) || return; echo continued; declare -p a; }; f ) $

The function didn't proceed past the || return, because the command inside the $( … ) returned failure.

Compare if I replace "false" by "true":

$ ( f(){ local a; a=$( echo 3; true ) || return; echo continued; declare -p a; }; f ) continued declare -- a="3"

where the output shows the function proceeded past || return.

gnprice · 2025-12-16T00:04:39Z

tools/check

+    ) || return
+    if [ -z "${latest_tag}" ]; then
+        echo >&2 "error: Failed to determine the latest tag"
+        return 1


Similar comment here and a few more spots below: what's the scenario where these -z conditions are expected to happen?

gnprice · 2025-12-16T00:12:47Z

tools/check

+             # If we find cases where it fails, we can study
+             # how the `flutter` tool actually decides the version name.


Well, now you have in fact studied that, right? So this comment seems a bit out of place 🙂

gnprice · 2025-12-16T00:16:29Z

tools/check

+
+    local flutter_tree flutter_git
+    flutter_tree=$(flutter_tree)
+    flutter_git=( git --git-dir="${flutter_tree}"/.git )


I think you've mentioned to me how it'd be better to use -C instead of --git-dir here, and in other places it appears in our scripts.

(The difference doesn't matter for these particular Git commands; but if it did matter, then --git-dir would produce very confusing results, unless we had a very specific reason to use it instead of -C. So it's best to use the pattern that consistently has more natural results.)

That'd be good to do as a quick commit of its own either before or after, while we're still thinking of it.

gnprice · 2025-12-16T00:21:11Z

tools/check

+error: Flutter commit in pubspec.yaml seems to differ from version bound
+Commit ${flutter_commit}
+We therefore expect the Flutter version name to be: ${predicted_version}
+But the Flutter version bound in pubspec.yaml is: ${flutter_version}


This wording doesn't make a ton of sense to me. Why "therefore" — how does the commit ID explain why we expect that Flutter version name?

The old code this appears to be copied from said:

error: Flutter commit in pubspec.yaml seems to differ from version bound Commit ${flutter_commit} was described as: ${commit_described} We therefore expect the Flutter version name to be: ${predicted_version} But the Flutter version bound in pubspec.yaml is: ${flutter_version}

so the commit_described value explains how we came to the particular predicted_version value.

That detail was there basically for debugging — if the test fails, it gives you a start on working out why. But that isn't critical. It's fine not to have any more information than this version does, since it's probably annoying to wire up more information to print here; we can always add more detail if we find we need it. We should just adapt the wording so it continues to make sense.

rajveermalviya force-pushed the pr-flutter-version-check branch from 5f5471e to fd5eb30 Compare December 10, 2025 13:20

rajveermalviya marked this pull request as draft December 11, 2025 17:15

rajveermalviya added 3 commits December 15, 2025 21:54

rajveermalviya force-pushed the pr-flutter-version-check branch from fd5eb30 to 5389e66 Compare December 15, 2025 16:58

rajveermalviya marked this pull request as ready for review December 15, 2025 16:59

rajveermalviya added the maintainer review PR ready for review by Zulip maintainers label Dec 15, 2025

rajveermalviya assigned chrisbobbe Dec 15, 2025

rajveermalviya requested a review from chrisbobbe December 15, 2025 17:00

chrisbobbe requested a review from gnprice December 15, 2025 21:22

chrisbobbe assigned gnprice and unassigned chrisbobbe Dec 15, 2025

chrisbobbe added integration review Added by maintainers when PR may be ready for integration and removed maintainer review PR ready for review by Zulip maintainers labels Dec 15, 2025

gnprice reviewed Dec 16, 2025

View reviewed changes

		- name: Run tools/check (all default suites)
		run: TERM=dumb tools/check --all --verbose

		# If we find cases where it fails, we can study
		# how the `flutter` tool actually decides the version name.

check: Update the flutter_version check to follow upstream #2002

Are you sure you want to change the base?

check: Update the flutter_version check to follow upstream #2002

Uh oh!

Conversation

rajveermalviya commented Nov 20, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

chrisbobbe commented Nov 21, 2025

Uh oh!

chrisbobbe commented Dec 10, 2025

Uh oh!

rajveermalviya commented Dec 15, 2025

Uh oh!

chrisbobbe commented Dec 15, 2025

Uh oh!

gnprice left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

rajveermalviya commented Nov 20, 2025 •

edited

Loading