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.

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

check: pin flutter version #577

Closed
wants to merge 1 commit into from
Closed

check: pin flutter version #577

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
27 changes: 27 additions & 0 deletions .envrc
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,27 @@
# This file is used by direnv to setup the environment when entering to use fvm's flutter.
Copy link
Member

Choose a reason for hiding this comment

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

Please do keep prose to around 68 columns wide, and at most 70. It makes a real difference in readability — for example here's what the commit message looks like in my terminal:
image

Even when the terminal is wide and the lines don't wrap, prose wider than around 70 columns gets less comfortable to read as your eyes have to scan back and forth across the lines.

This guideline is explicit in our commit style guide:
https://zulip.readthedocs.io/en/latest/contributing/commit-discipline.html#formatting-guidelines
but it applies to prose elsewhere, like Markdown and comments, too.


# Comment out the next line if you want to use your system flutter.
PATH_add .fvm/flutter_sdk/bin

# Check flutter version matches what's in .fvmrc
check_flutter_version() {
Copy link
Member

Choose a reason for hiding this comment

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

This file is a shell script, so let's include it in tools/check shellcheck. That means in the run_shellcheck function, adding this file to both the files_check line and the targets array.

(Then that will give a few warnings on this version, which you can fix.)

local fvm_flutter_version_string=$(cat .fvmrc)
# Fetch string from format `"flutter": "18340ea16c"``
local fvm_flutter_version=$(echo "$fvm_flutter_version_string" | grep -o '"flutter": "[a-z0-9]*"' | cut -d '"' -f 4)
Comment on lines +8 to +10
Copy link
Member

Choose a reason for hiding this comment

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

The behavior of cut is finicky to reason through. Since there's already a regexp describing what the line should look like, better to take that just slightly further to identify which part of the line we want:

Suggested change
local fvm_flutter_version_string=$(cat .fvmrc)
# Fetch string from format `"flutter": "18340ea16c"``
local fvm_flutter_version=$(echo "$fvm_flutter_version_string" | grep -o '"flutter": "[a-z0-9]*"' | cut -d '"' -f 4)
local fvm_flutter_version=$(
<.fvmrc perl -lne 'print $1 if (/"flutter": "([a-z0-9]*)"/)'
)

Then two other points:

  • should be a-f, not a-z :-) (and let's write it as [0-9a-f], so the digits are in order of their value)
  • a fine point of shell semantics which shellcheck will tell you about — more about that in my next comment

Copy link
Member

Choose a reason for hiding this comment

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

(Oh, and since the file being parsed here is actually JSON, the cleanest solution would be to use jq instead. But because this is a script that will run for all developers in this repo, best to do without that extra dependency. We do regularly use jq in scripts that only a smaller set of maintainers are expected to need to run.)


local flutter_version_string=$(flutter --version)
Copy link
Member

Choose a reason for hiding this comment

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

Sadly flutter --version is fairly slow — which makes this take around 1.5 seconds when I cd into this directory. That'd get pretty annoying.

To handle that, see the workaround in print_header in tools/check. For any code there which you'd like to reuse, please feel free to move functions to a file in tools/lib/ (and/or an existing file there like tools/lib/git.sh).

# Fetch string from format `Framework • revision 11c034f037`
local flutter_version=$(echo $flutter_version_string | grep -o 'Framework • revision [a-z0-9]*' | cut -d ' ' -f 4)

if [ "$fvm_flutter_version" != "$flutter_version" ]; then
echo "Flutter version mismatch: $fvm_flutter_version != $flutter_version"
echo "Expected Flutter version: $fvm_flutter_version"
echo "Actual Flutter version: $flutter_version"
echo "Run 'fvm install' to fix this or see README on Setup."
return 1
fi
}

# Check flutter version when entering directory
# Comment this out at your own risk if you want to use a custom flutter version.
check_flutter_version
4 changes: 4 additions & 0 deletions .fvmrc
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,4 @@
{
"flutter": "18340ea16c",
"updateVscodeSettings": false
}
23 changes: 15 additions & 8 deletions .github/workflows/ci.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,15 +8,22 @@ jobs:
steps:
- uses: actions/checkout@v3

- name: Clone Flutter SDK
# We can't do a depth-1 clone, because we need the most recent tag
# so that Flutter knows its version and sees the constraint in our
# pubspec is satisfied. It's uncommon for flutter/flutter to go
# more than 100 commits between tags. Fetch 1000 for good measure.
- name: Set up Homebrew
id: set-up-homebrew
uses: Homebrew/actions/setup-homebrew@master

- name: Install FVM
run: |
brew tap leoafarias/fvm
brew install fvm
Comment on lines +15 to +18
Copy link
Member

Choose a reason for hiding this comment

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

This is fairly slow — this step took 27s in the latest job recorded at https://github.com/zulip/zulip-flutter/pull/577/checks . That seems to be because it's building fvm from source:

==> Downloading https://github.com/leoafarias/fvm/archive/3.1.1.tar.gz
==> Downloading from https://codeload.github.com/leoafarias/fvm/tar.gz/refs/tags/3.1.1
==> Installing fvm from leoafarias/fvm
==> /home/linuxbrew/.linuxbrew/Cellar/fvm/3.1.1/libexec/bin/dart pub get
==> /home/linuxbrew/.linuxbrew/Cellar/fvm/3.1.1/libexec/bin/dart compile exe -Dversion=3.1.1 bin/main.dart -o fvm
🍺  /home/linuxbrew/.linuxbrew/Cellar/fvm/3.1.1: 1,022 files, 572.2MB, built in 20 seconds

If we instead do the curl|bash command suggested as an alternative in the docs:

$ curl -fsSL https://fvm.app/install.sh | bash

then on my machine that takes about 1.5s. The basic reason is that it's downloading a published release artifact rather than a source tree:

# Download FVM
URL="https://github.com/leoafarias/fvm/releases/download/$FVM_VERSION/fvm-$FVM_VERSION-$OS-$ARCH.tar.gz"
if ! curl -L "$URL" -o fvm.tar.gz; then

I don't love running curl|bash installs. For this one, I downloaded the script first, read it, then ran it. But ultimately it's not any different from what brew tap leofarias/fvm; brew install fvm is doing — both are taking a script published by the same person, and running it.


- name: Install Flutter SDK
run: |
git clone --depth=1000 https://github.com/flutter/flutter ~/flutter
TZ=UTC git --git-dir ~/flutter/.git log -1 --format='%h | %ci | %s' --date=iso8601-local
echo ~/flutter/bin >> "$GITHUB_PATH"
fvm install
fvm use

- name: Load direnv (.envrc)
uses: HatsuneMiku3939/direnv-action@v1
Comment on lines +25 to +26
Copy link
Member

Choose a reason for hiding this comment

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

What's this action do? Can we just say apt install direnv, and then a line or two from the direnv setup instructions?

I'd be more comfortable with that, because it makes it a lot more transparent what's happening.

Alternatively, we could just add an item to PATH directly, without direnv — the real value of direnv is in its automation for interactive shell use as people enter and leave the directory.


- name: Download Flutter SDK artifacts (flutter precache)
run: flutter precache --universal
Expand Down
3 changes: 3 additions & 0 deletions .gitignore
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -49,3 +49,6 @@ app.*.map.json

# Old scaffolding hack
lib/credential_fixture.dart

# FVM Version Cache
.fvm/
1 change: 1 addition & 0 deletions .vscode/settings.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,4 +17,5 @@

// This much more focused automatic fix is helpful, though.
"files.trimTrailingWhitespace": true,
"dart.flutterSdkPaths": [".fvm/flutter_sdk"],
}
53 changes: 38 additions & 15 deletions README.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -91,27 +91,50 @@ Two specific points to expand on:
## Getting started in developing this beta app

### Setting up

1. Follow the [Flutter installation guide](https://docs.flutter.dev/get-started/install)
for your platform of choice.
2. Switch to the latest version of Flutter by running `flutter channel main`
and `flutter upgrade` (see [Flutter version](#flutter-version) below).
> [!NOTE]
> (Advanced Users) If you want to manage own flutter SDK installation, you can skip step 1-2. See below section on [Flutter version](#flutter-version)

1. Install [direnv][direnv] and [fvm][fvm], for most
users, you can use Homebrew:
Comment on lines +97 to +98
Copy link
Member

Choose a reason for hiding this comment

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

Let's not assume "most users" are using Homebrew. :-) It's widely used on macOS, but I think not so much on Linux. I'm primarily on Linux (where I don't have Homebrew installed); and I think among Zulip contributors in general, Linux is most common, followed by Windows, with relatively few on macOS.

Fine to include Homebrew instructions as an example. For direnv, let's mention sudo apt install direnv as another example.


```sh
brew install direnv
# Follow instructions for to add the hook https://direnv.net/docs/hook.html
brew tap leoafarias/fvm
brew install fvm
# Close and open a new terminal to ensure that direnv is loaded.
# For any changes in the contents of direnv, and first use, you need to call
# this.
direnv allow
```
2. Run `fvm use` to setup the flutter version.
3. Ensure Flutter is correctly configured by running `flutter doctor`.
4. Start the app with `flutter run`, or from your IDE.


### Flutter version

While in the beta phase, we use the latest Flutter from Flutter's
main branch. Use `flutter channel main` and `flutter upgrade`.

We don't pin a specific version, because Flutter itself doesn't offer
a way to do so. So far that hasn't been a problem. When it becomes one,
we'll figure it out; there are several tools for this in the Flutter
community. See [issue #15][].

[issue #15]: https://github.com/zulip/zulip-flutter/issues/15

We use [direnv][direnv] and [fvm][fvm] to ensure that the version of flutter SDK
that you're using matches what has been tested on CI, and across developer
setups.
The Flutter version pinned for the current build is in the [.fvmrc](.fvmrc)
file under the `flutter` key. It can either be a hash like
[`18340ea16c`](https://github.com/flutter/flutter/commit/18340ea16c) or a
version number like `3.21.0-11.0.pre`.

However, if you want to manage your own flutter SDK version you can opt out of
this behavior.
Do note that if you do this, you need to manually make sure that the flutter SDK
version matches or is compatible with the pinned version in `.fvmrc`. Otherwise,
the build can fail as we have not tested the current code with that particular
flutter SDK version.

If you want manage your own flutter SDK version, follow the [Flutter
installation guide](https://docs.flutter.dev/get-started/install) for your
platform of choice.

[direnv]: https://direnv.net/
[fvm]: https://fvm.app/

### Tests

Expand Down
Loading