Use inferred flutter version#5207
Use inferred flutter version#5207mctofu merged 19 commits intodependabot:mainfrom sigurdm:use_preferred_flutter_version
Conversation
Installs the flutter version dynamically instead of fixed in the docker-file. Also sets up pub helpers. Specifically the executable for dependency_services now lives in dependabot.
|
The actual dependency-services is still implemented in the But that's fine, one day when dependency-services is more formalized maybe we can expose it properly and document it as part of a Dart SDK. For now, the dependency-services logic we have in off-topic: or maybe dependency-services should be an LSP extension of sorts) |
mctofu
left a comment
There was a problem hiding this comment.
@mctofu does this approach make sense to you?
This does make sense to me. I'm going to find some time to give this a test run this week to get a better idea of how it's working.
There are some CI tasks failing on this branch. You can fix the npm CI errors by updating with the latest changes on main. The pub errors look to be from the linter which you can fix by running rubocop -A
pub/lib/dependabot/pub/helpers.rb
Outdated
| "git", | ||
| "clone", | ||
| "--no-checkout", | ||
| # "--reference", |
There was a problem hiding this comment.
minor: it'd be best to remove this if we won't use it
Co-authored-by: David McIntosh <804610+mctofu@users.noreply.github.com>
|
PTAL |
| .toList(); | ||
| } | ||
|
|
||
| /// The "best" Flutter release for a given set of constraints is the first one |
There was a problem hiding this comment.
does "first" mean: oldest or newest Flutter version?
There was a problem hiding this comment.
It is the first in the list passed to this function.
I tried to rephrase a bit.
| final pubspec = loadYaml(File(pubspecPath).readAsStringSync(), | ||
| sourceUrl: Uri.file(pubspecPath)); |
There was a problem hiding this comment.
Inferring is a good start, but dependabot should allow pointing to an exact git-ref (tag, sha1).
There was a problem hiding this comment.
Hmm - do you have an idea for a good place to put this configuration?
What are scenarios where you would need this?
There was a problem hiding this comment.
Scenario: We use flutterw to pin the flutter version. Others use fvm or custom solutions.
Tree options pop into my mind:
- Lookup: dependabot checks the version of those pinning solutions (i.e. check git submodule sha1 or a configuration file)
- Add a flutter version to .github/dependabot.yaml
- Define a new
dependabot.flutter_versionproperty topubspec.yamlof each package and read it
There was a problem hiding this comment.
I think that it sufficient that we allow pinning the Flutter SDK constraint in environment.flutter.
Generally, the dart pub client will ignore the upper-bound in environment.flutter, as a result of a decision made in Flutter 2 that Flutter wouldn't break backwards compatibility.
But in dependabot, I think we made it respect the upper-bound, thus, the environment.flutter constraint can be used to pin a Flutter version. You can't pin to arbitrary sha or tag, but you can pin to stable and beta releases of Flutter.
My two cents is that this is a reasonable compromise.
There was a problem hiding this comment.
Using the lower bound of environment.flutter would work for us. But it could prevent dependabot from adding the latest versions.
Using the upper bound wouldn't be practical. We'd have to change it every time we upgrade in all packages of our mono repo.
mctofu
left a comment
There was a problem hiding this comment.
@sigurdm: I'm trying to run this locally and running into an error. Do you get the same thing or have I done something wrong?
[dependabot-core-dev] ~/dependabot-core $ bin/dry-run.rb pub passsy/flutter_dependabot_example
warning: parser/current is loading parser/ruby27, which recognizes2.7.6-compliant syntax, but you are running 2.7.5.
Please see https://github.com/whitequark/parser#compatibility-with-ruby-mri.
=> cloning into /home/dependabot/dependabot-core/tmp/passsy/flutter_dependabot_example
=> parsing dependency files
=> updating 5 dependencies: cupertino_icons, flutter, flutter_lints, flutter_test, go_router
=== cupertino_icons (1.0.4)
=> checking for updates 1/5
Traceback (most recent call last):
15: from bin/dry-run.rb:635:in `<main>'
14: from bin/dry-run.rb:635:in `each'
13: from bin/dry-run.rb:643:in `block in <main>'
12: from /home/dependabot/dependabot-core/pub/lib/dependabot/pub/update_checker.rb:13:in `latest_version'
11: from /home/dependabot/dependabot-core/pub/lib/dependabot/pub/update_checker.rb:101:in `current_report'
10: from /home/dependabot/dependabot-core/pub/lib/dependabot/pub/update_checker.rb:97:in `report'
9: from /home/dependabot/dependabot-core/pub/lib/dependabot/pub/helpers.rb:44:in `dependency_services_report'
8: from /home/dependabot/dependabot-core/pub/lib/dependabot/pub/helpers.rb:150:in `run_dependency_services'
7: from /home/dependabot/dependabot-core/common/lib/dependabot/shared_helpers.rb:49:in `in_a_temporary_directory'
6: from /home/dependabot/dependabot-core/common/lib/dependabot/shared_helpers.rb:49:in `chdir'
5: from /home/dependabot/dependabot-core/common/lib/dependabot/shared_helpers.rb:49:in `block in in_a_temporary_directory'
4: from /home/dependabot/dependabot-core/pub/lib/dependabot/pub/helpers.rb:157:in `block in run_dependency_services'
3: from /home/dependabot/dependabot-core/common/lib/dependabot/shared_helpers.rb:168:in `with_git_configured'
2: from /home/dependabot/dependabot-core/pub/lib/dependabot/pub/helpers.rb:167:in `block (2 levels) in run_dependency_services'
1: from /home/dependabot/dependabot-core/pub/lib/dependabot/pub/helpers.rb:167:in `chdir'
/home/dependabot/dependabot-core/pub/lib/dependabot/pub/helpers.rb:174:in `block (3 levels) in run_dependency_services': dependency_services failed: Package not available (the Flutter SDK is not available). (Dependabot::DependabotError)
|
@mctofu sorry - I had not tested this thoroughly. I was downloading the flutter sdk, but never using it from the resolution. Now I've added a test of update-checking a package actually using flutter. PTAL |
|
Now we get: |
|
@sigurdm: Thanks, it's working locally for me now! There are a few tests failing now in |
mctofu
left a comment
There was a problem hiding this comment.
Thanks! I think this looks good to merge but let me know if you'd want to make any other changes first.
Co-authored-by: Jonas Finnemann Jensen <jopsen@gmail.com>
|
@mctofu I think I'm happy now. |
This reorganizes the pub integration somewhat:
Also the dart sdk is updated (this is less important, as the sdk version used for analysis is will be the one associated with the flutter version).
fixes: #5084