ci: Pin Flutter to the version we use in release#2140
ci: Pin Flutter to the version we use in release#2140rajveermalviya merged 6 commits intozulip:mainfrom
Conversation
gnprice
left a comment
gnprice
left a comment
There was a problem hiding this comment.
Thanks! I ended up sending the immediate fix as #2138, which I'm about to merge. (Ironically it was delayed a bit as I debugged why the script wasn't working… which turned out to be another instance of using --git-dir when -C is needed, for git reset.)
So please rebase these changes atop that, and we can do this as a quick follow-up to clean up some of this logic. Generally it all looks good — just one comment below.
tools/ci/clone-flutter-sdk
Outdated
| # shellcheck disable=SC2207 # output has controlled whitespace | ||
| parsed=( $( | ||
| perl <pubspec.yaml -0ne ' | ||
| print "$1 $2" if ( | ||
| /^ sdk: .*\n flutter: '\''>=\S+'\''\s*# ([0-9a-f]{40})$/m)' | ||
| ) ) || return |
There was a problem hiding this comment.
This is fairly gnarly-looking logic, and it duplicates some code that's in tools/check . So let's have a prep commit factor that logic out, and then we can re-use it.
The factored-out logic can go as a shell function in tools/lib/git.sh . (Or we could make a separate file like maybe tools/lib/deps.sh , move the existing flutter_tree function there, and put this new function there too.)
rajveermalviya
force-pushed
the
pr-git
branch
2 times, most recently
from
2f22cfd to
bbb7351
Compare
|
This ready for another review @gnprice. PTAL. |
| # shellcheck source=tools/lib/deps.sh | ||
| . "${this_dir}"/lib/deps.sh | ||
|
|
||
| ## CLI PARSING |
There was a problem hiding this comment.
nit: maintain the double blank line before heading
tools/lib/deps.sh
Outdated
| pubspec_flutter_version() { | ||
| # Parse our Flutter version spec and its commit-ID comment. |
There was a problem hiding this comment.
nit: this information is now in the doc comment
| pubspec_flutter_version() { | |
| # Parse our Flutter version spec and its commit-ID comment. | |
| pubspec_flutter_version() { |
| # Output Parameters: | ||
| # - flutter_version | ||
| # - flutter_commit | ||
| pubspec_flutter_version() { |
There was a problem hiding this comment.
Subtle!
Seems like a fine use of this pattern, though.
tools/ci/clone-flutter-sdk
Outdated
|
|
||
| flutter_tree=~/flutter | ||
|
|
||
| clone_flutter_sdk() { |
There was a problem hiding this comment.
What's the purpose of moving this script's contents into a function?
I think the fact that the function has the same name as the script (modulo punctuation conventions) is evidence that it's not a useful encapsulation in this case.
There was a problem hiding this comment.
Hmm, I did that to be able to define local output variable for pubspec_flutter_version. Is there another way to do it?
There was a problem hiding this comment.
Mmm, I see.
Can you just skip the local definition, though? That statement isn't allowed outside a function, but I think the reason for that is basically that it wouldn't have any real effect anyway.
There was a problem hiding this comment.
Okay removed the local definition.
Similar reasoning to 17853b9.
rajveermalviya
force-pushed
the
pr-git
branch
from
bbb7351 to
7f6895a
Compare
|
Thanks for the review @gnprice! Pushed an update, PTAL. |
|
Thanks! All looks good except the one subthread at #2140 (comment) . |
rajveermalviya
force-pushed
the
pr-git
branch
from
7f6895a to
5538d2a
Compare
rajveermalviya
force-pushed
the
pr-git
branch
from
5538d2a to
1b2496c
Compare
|
Thanks! Changes pushed. |
|
Thanks! All looks good. Please go ahead and merge once CI passes. |
2 participants
No description provided.