Skip to content

check: Update the flutter_version check to follow upstream #2002

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.

Sign up for GitHub

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

Open
rajveermalviya wants to merge 3 commits into
base: main
Choose a base branch
from
Open

check: Update the flutter_version check to follow upstream #2002

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
356fc9e
ci: Fetch all branches during the checkout step
rajveermalviya Dec 15, 2025
6fed533
ci: Run flutter_version suite conditionally
rajveermalviya Dec 15, 2025
5389e66
check: Update the flutter_version check to follow upstream
rajveermalviya Oct 16, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 6 additions & 1 deletion .github/workflows/ci.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,8 @@ jobs:
runs-on: ubuntu-24.04
steps:
- uses: actions/checkout@v6
with:
fetch-depth: 0

- name: Install system dependencies
run: TERM=dumb sudo apt install libsqlite3-dev -y
Expand Down Expand Up @@ -40,5 +42,8 @@ jobs:
- name: Download our dependencies (flutter pub get)
run: flutter pub get

- name: Run tools/check
- name: Run tools/check (flutter_version)
run: TERM=dumb tools/check --verbose flutter_version

- name: Run tools/check (all default suites)
run: TERM=dumb tools/check --all --verbose
Comment on lines +48 to 49
Copy link
Member

@gnprice gnprice Dec 15, 2025

Choose a reason for hiding this comment

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

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

88 changes: 83 additions & 5 deletions tools/check
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,7 +29,6 @@ this_dir=${BASH_SOURCE[0]%/*}

default_suites=(
analyze test
flutter_version
Comment on lines 30 to -32
Copy link
Member

@gnprice gnprice Dec 15, 2025

Choose a reason for hiding this comment

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

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

Copy link
Member

@gnprice gnprice Dec 15, 2025

Choose a reason for hiding this comment

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

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

build_runner l10n drift pigeon icons
android # This takes multiple minutes in CI, so do it last.
)
Expand Down Expand Up @@ -222,12 +221,75 @@ run_test() {
flutter test
}

# Predict a Flutter version string from the given Flutter commit SHA.
predict_flutter_version() {
local flutter_commit
flutter_commit="$1"

local flutter_tree flutter_git
flutter_tree=$(flutter_tree)
flutter_git=( git --git-dir="${flutter_tree}"/.git )
Copy link
Member

@gnprice gnprice Dec 16, 2025

Choose a reason for hiding this comment

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

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.


# Check the version name matches the calculated version using the
# latest tag. We mimic the upstream Flutter tools's implementation here:
# https://github.com/flutter/flutter/blob/7258223ea/packages/flutter_tools/lib/src/version.dart#L1060-L1091
local latest_tag ancestor_ref commit_count
latest_tag=$(
"${flutter_git[@]}" for-each-ref \
--sort=-v:refname \
--count=1 \
--format='%(refname:short)' \
'refs/tags/[0-9]*.*.*'
) || return
if [ -z "${latest_tag}" ]; then
echo >&2 "error: Failed to determine the latest tag"
return 1
Comment on lines +243 to +246
Copy link
Member

@gnprice gnprice Dec 16, 2025

Choose a reason for hiding this comment

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

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

fi
ancestor_ref=$(
"${flutter_git[@]}" merge-base "${flutter_commit}" "${latest_tag}"
) || return
if [ -z "${ancestor_ref}" ]; then
echo >&2 "error: Failed to determine merge-base between ${flutter_commit} and ${latest_tag}"
return 1
fi
commit_count=$(
"${flutter_git[@]}" rev-list --count "${ancestor_ref}..${flutter_commit}"
) || return
if [ -z "${commit_count}" ]; then
echo >&2 "error: Failed to count commits from ${ancestor_ref} to ${flutter_commit}"
return 1
fi

latest_tag_version="${latest_tag}-${commit_count}"

# Generate a predicted Flutter version string by parsing the latest tag
# version and applying the same transformations as the upstream Flutter
# tool:
# https://github.com/flutter/flutter/blob/7258223ea/packages/flutter_tools/lib/src/version.dart#L1162
predicted_version=$(
echo "${latest_tag_version}" \
| perl -lne 'print if (s
# This transformation is ad hoc.
# If we find cases where it fails, we can study
# how the `flutter` tool actually decides the version name.
Comment on lines +273 to +274
Copy link
Member

@gnprice gnprice Dec 16, 2025

Choose a reason for hiding this comment

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

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

<^(\d+\.\d+\.\d+-) (\d+) \.\d+\.pre -(\d+)$>
<$1 . ($2 + 1) . ".0.pre-" . $3>xe)'
) || return
if [ -z "${predicted_version}" ]; then
cat >&2 <<EOF
error: unexpected latest tag version on Flutter commit in pubspec.yaml
Commit ${flutter_commit} was described as: ${latest_tag_version}
EOF
return 1
fi

echo "${predicted_version}"
}

# Check the Flutter version in pubspec.yaml is commented with a commit ID,
# which agrees with the version, and the commit is from upstream main.
run_flutter_version() {
# Omitted from this files check:
# tools/check
files_check pubspec.yaml \
files_check pubspec.yaml tools/check \
Comment on lines -228 to +292
Copy link
Member

@gnprice gnprice Dec 15, 2025

Choose a reason for hiding this comment

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

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.

Copy link
Member

@gnprice gnprice Dec 15, 2025

Choose a reason for hiding this comment

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

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.

|| return 0

local flutter_tree flutter_git
Expand All @@ -249,7 +311,23 @@ run_flutter_version() {
flutter_version="${parsed[0]}"
flutter_commit="${parsed[1]}"

# TODO(#1851) Add the updated Flutter version name check
local predicted_version
predicted_version=$(
predict_flutter_version "${flutter_commit}"
) || return
if [ -z "${predicted_version}" ]; then
return 1
fi
Comment on lines +318 to +320
Copy link
Member

@gnprice gnprice Dec 16, 2025

Choose a reason for hiding this comment

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

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.

Copy link
Member

@gnprice gnprice Dec 16, 2025

Choose a reason for hiding this comment

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

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.


if [ "${flutter_version}" != "${predicted_version}" ]; then
cat >&2 <<EOF
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}
Comment on lines +324 to +327
Copy link
Member

@gnprice gnprice Dec 16, 2025

Choose a reason for hiding this comment

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

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.

EOF
return 1
fi

# Check the commit is an acceptable commit.
local commit_count
Expand Down