check: pin flutter version #577
Conversation
|
Thanks for picking this up! Since you're working on this, here's a couple of thoughts about our needs here which I think aren't yet explicit on the issue thread #15:
|
|
@gnprice I've outlined a prototype that addresses your concerns, and updated the description of this PR. This does introduce 2 dependencies in the setup for most users. Power users can skip this setup and just not use the pinned version at their own risk of using a mismatched flutter version. |
chrisirhc
force-pushed
the
chua/fvm
branch
3 times, most recently
from
a4b5060
to
91cae50
Compare
|
Thinking on this PR more, I think there's some value with testing the commit with the latest commit on flutter. Something like I don't like that I'm introducing two tools (direnv, fvm) at the same time, so it's not as clean as I'd like. But this seems like it would be most dummy-proof (hopefully) for most newcomers to the repository. Also, it seems like direnv unfortunately doesn't work with IntelliJ/Android Studio. So.. :/ Maybe I'll need to rethink this approach. |
Use direnv and flutter to enforce and validate a pinned version of flutter.
This approach uses `direnv` and `fvm` to setup the environment with the pinned
version of the repo. When setup correctly, `direnv` will turn on `fvm`'s
pinned flutter version whenever entering the zulip-flutter repository
(directory).
The `.envrc` does the following:
- Use fvm's flutter version if it's available.
- Check that whether the flutter binary version matches what was pinned for the
repo.
- Let users know if there's a mismatch of version so that the user can take
action.
Other notes:
- Power users can:
- use their checked out flutter sdk (either their own custom path, configured
in .envrc, or the system default)
- disable the check if they know what they're doing and want to use a custom
flutter installation
|
Very interesting, thanks! I installed direnv and played around with it a bit, and it seems great — a very solid design. I'd be happy to start relying on it. I also installed fvm, and… am less impressed. It scatters files across my homedir (
It's likely we could find ways to hack around that to some degree in order to get reasonable behavior. Effectively that's part of what the So I'd be interested in seeing a version of this that uses direnv, and uses |
Specifically, this could look like:
|
It should be a matter of telling the IDE what path to find Flutter at, right? Like this draft does for VS Code in I expect there's a corresponding setting we can set for IntelliJ / Android Studio. (It won't use direnv, but VS Code doesn't appear to be doing so either.) |
| - name: Install FVM | ||
| run: | | ||
| brew tap leoafarias/fvm | ||
| brew install fvm |
There was a problem hiding this comment.
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.
| 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) |
There was a problem hiding this comment.
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:
| 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
There was a problem hiding this comment.
(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.)
| PATH_add .fvm/flutter_sdk/bin | ||
|
|
||
| # Check flutter version matches what's in .fvmrc | ||
| check_flutter_version() { |
There was a problem hiding this comment.
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.)
| # 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 flutter_version_string=$(flutter --version) |
There was a problem hiding this comment.
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).
| 1. Install [direnv][direnv] and [fvm][fvm], for most | ||
| users, you can use Homebrew: |
There was a problem hiding this comment.
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.
| - name: Load direnv (.envrc) | ||
| uses: HatsuneMiku3939/direnv-action@v1 |
There was a problem hiding this comment.
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.
| @@ -0,0 +1,27 @@ | |||
| # This file is used by direnv to setup the environment when entering to use fvm's flutter. | |||
There was a problem hiding this comment.
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:
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.
|
Agree with the sentiment around fvm. I found myself working around unexpected behaviour — what felt like buggy behaviour. |
|
Cool, sounds good. I'm a little wary of Git submodules, because my experience has been that Git's UX with them can be annoying. But fundamentally they do implement pretty much exactly the design I sketched in #577 (comment) , without requiring us to write much or any code for it. So that sounds like a good experiment to try. And if we like the design but decide the Git submodules UX is annoying, we can always try replacing it later with our own implementation. |
Yes, same, I had somewhat bad memories from prior usage though it could've been minor annoyances in the grander scheme of things.
Yes it does look pretty much like that. #579 is working. I'll take a stab at what README instructions could look like.
+1 |
Yep, it does have a corresponding setting ✅ 
I didn't find way to easily set this up programmatically via the |
|
#579 is shaping up to look promising. |
Cool. Yeah, that screen looks familiar — I think I've had to set something there whenever first setting up Flutter and Android Studio on a given machine. It corresponds to a step in Flutter's own setup instructions: So we'll definitely need to write some clear instructions for it, but it shouldn't be too much of a burden. I also spent some time fiddling to try to figure out what file in IntelliJ's config controls that setting. IntelliJ's config setup is kind of a mess, but what I expected from past experience was:
For more background, see commit 62a0d7c. Instead, I couldn't find the setting anywhere! Even The closest match I found was this, over in the config that lives in a global place in my homedir: But that's a list of several possible values; it looks like it might be the list of values it'll suggest in that very dropdown. Anyway 🤷 I think we'll be OK with the manual setup step. |
This approach uses
direnvandfvmto setup the environment with the pinned version of the repo.When setup correctly,
direnvwill turn onfvm's pinned flutter version whenever entering the zulip-flutter repository (directory).The
.envrcdoes the following:Other notes:
TODOs:
Closes #15