-
Notifications
You must be signed in to change notification settings - Fork 215
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.
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
pin flutter SDK version via git submodule #579
base: main
Are you sure you want to change the base?
Conversation
b97d551
to
320a24d
Compare
IMO this ends up being cleaner and still meets the goals of #15 . |
Thanks! I agree — I think this plan looks good. The main thing it needs is some more instructions, in particular on that Android Studio / IntelliJ setup we discussed around #577 (comment) . The instructions for upgrading Flutter will also need some playtesting — I suspect the One peculiarity I notice, from playing with this, is that
That contrasts with, for my existing main Flutter tree at a slightly older version:
Not sure what-all downstream effects that might have. If it sufficiently confuses things, we might need to find a way to coax Hmm, in fact, here's one downstream effect:
So that use of a detached HEAD by I think taking |
23d9a74
to
c985ec9
Compare
By adding a manual setup step to do I'm now following up on the Android Studio investigation but it does look like manual instructions are needed. |
8d57c92
to
988f1c4
Compare
54e8eed
to
75535d3
Compare
Updated the PR, so it's ready for review. |
75535d3
to
ee8a1ae
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks! Generally this looks great. Various comments below.
c83b396
to
ce97735
Compare
Updated the PR and addressed the comments. |
Thanks again @chrisirhc for all your work on this! Sorry I haven't managed to return for another review for a bit — we've just this week gotten past the peak PR volume from GSoC application season. I'll be going on vacation shortly for the next couple of weeks. I'll aim to pick this back up the week I'm back, as I catch up with things. Based on where it was at my last review, I imagine we'll be pretty close to merge. This will be a very nice improvement to have! |
ce97735
to
37a5bf7
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @chrisirhc! Various small comments below. There's also one substantive question (about the resulting developer workflow) which I'll copy here for visibility:
What happens when as a contributor you rebase (or reset to a new origin/main
), and the version that's pinned has changed? You'll need to run some git submodule
command to update your local clone, right?
In fact, when I look at git help submodule
, I'm not sure what an appropriate command would be. One obvious candidate is git submodule update
… but I think that will take the submodule onto a detached HEAD, same as it does on first setup (as discussed above starting at #579 (comment)).
In that case we probably end up recommending the developer run a script like tools/setup-vendor-flutter
any time git status
tells them vendor/flutter
is out of sync, or some sort of instruction like that.
Once we have all the phases of the main contributor workflow (so upgrade/rebase as well as setup) shaken out, I'll make one more editing pass over all the docs; and then I think it'll be time to play-test the instructions.
I'll handle that by setting up a branch with this PR plus an ad-hoc Flutter upgrade, in order to simulate that; then asking people to switch to the PR branch, follow the README instructions, then to the upgrade branch, and to test things work and give feedback on anything confusing.
README.md
Outdated
1.1. Make sure you have initialized submodule by doing: | ||
|
||
```sh | ||
# Update and initialize submodule | ||
git submodule update --init | ||
# Run setup-vendor-flutter | ||
tools/setup-vendor-flutter | ||
``` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Similar to #579 (comment):
Let's try to write this message with the audience in mind of someone who's just cloned this repo and is starting to go through the README, and has never heard of Git submodules (and even if they have, doesn't know our particular layout or how we're using submodules).
So here:
1.1. Make sure you have initialized submodule by doing: | |
```sh | |
# Update and initialize submodule | |
git submodule update --init | |
# Run setup-vendor-flutter | |
tools/setup-vendor-flutter | |
``` | |
1.1. Initialize the pinned Flutter tree: | |
```sh | |
git submodule update --init | |
tools/setup-vendor-flutter | |
``` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Relatedly: can we move the git submodule update --init
inside the script, so that this step is just a single command?
Hmm, though on the other hand, and zooming out a bit: what happens when as a contributor you rebase (or reset to a new origin/main
), and the version that's pinned has changed? You'll need to run some git submodule
command to update your local clone, right?
If so, that seems like something we should mention in this doc too. But it also points to keeping an explicit git submodule
step here in the setup after all, to avoid having something that's initially magic but where the developer later needs to look behind the magic anyway.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, yes, in my latest changes I've explored moving git submodule update --init
into the setup-vendor-flutter
script and making this script runnable any/all the time. If it's already set up, it'll be a noop.
Will take a stab at adding a note in the readme to run setup-vendor-flutter
whenever user sees something different on vendor/flutter via git status.
* Introduce a git submodule to pin the version of flutter SDK under `vendor/flutter`. * Use direnv to add `vendor/flutter/bin` to the current PATH so that subsequent calls to `flutter` on the shell uses the our pinned version of flutter. * Update instructions to use pinned version of flutter. * Pin to 18340ea16c of flutter which matches the current lowerbound version in pubsec.yaml (3.21.0-12.0.pre.26). NOTE: Users can still choose to opt-out and use their own version of flutter. This just provides a recommended and tested version on our CI. Closes zulip#15
The `check-flutter-main` job serves 2 purposes: 1. It checks that the code works with the flutter's latest main channel. 2. It checks that our code environment setup works with a typical Flutter SDK installation setup (via ~/flutter).
37a5bf7
to
b990e53
Compare
I've also rebased the PR branch, resolving a small merge conflict (foreshadowed at #579 (comment) 🙂 ) and updating the pinned version. |
Co-authored-by: Greg Price <[email protected]>
Co-authored-by: Greg Price <[email protected]>
Co-authored-by: Greg Price <[email protected]>
Co-authored-by: Greg Price <[email protected]>
b0e5a7a
to
6004882
Compare
Updated the PR and addressed the comments though I haven't squashed the commits yet. One other thing to consider is to add a A preview of the message if the setup-vendor-flutter fails, for review:
|
This version leaves it to the user/developer to handle actually upgrading their Flutter install to latest; but it takes care of updating pubspec.yaml and pubspec.lock. The upgrade of the actual Flutter install will presumably look different anyway after zulip#15 / zulip#579, so we leave that to the future.
This version leaves it to the user/developer to handle actually upgrading their Flutter install to latest; but it takes care of updating pubspec.yaml and pubspec.lock. The upgrade of the actual Flutter install will presumably look different anyway after zulip#15 / zulip#579, so we leave that to the future.
vendor/flutter
.vendor/flutter/bin
to the current PATH so thatsubsequent calls to
flutter
on the shell uses the our pinnedversion of flutter.
version in pubsec.yaml (3.21.0-12.0.pre.26).
In addition, this PR also includes in a separate commit:
NOTE: Users can still choose to opt-out and use their own version of
flutter. This just provides a recommended and tested version on our
CI.
Remaining TODOs:
Closes #15
Trade-offs when using git submodule (this PR) compared to using fvm (#577)
Pros
flutter upgrade
just works™️ , just need to commit the change of what the submodule commit is pointing to, which is simple.Cons
git worktree
.