[flutter_tools] Have FlutterValidator fail on non-ideal git config by royarg02 · Pull Request #103259 · flutter/flutter

royarg02 · 2022-05-07T07:35:18Z

Fixes #102035.

Makes the version and the upstream repository message display an issue in cases where flutter upgrade is not supported.

Pre-launch Checklist

I read the Contributor Guide and followed the process outlined there for submitting PRs.
I read the Tree Hygiene wiki page, which explains my responsibilities.
I read and followed the Flutter Style Guide, including Features we expect every widget to implement.
I signed the CLA.
I listed at least one issue that this PR fixes in the description above.
I updated/added relevant documentation (doc comments with ///).
I added new tests to check the change I am making, or this PR is test-exempt.
All existing and new tests are passing.

If you need help, consider asking for advice on the #hackers-new channel on Discord.

christopherfujino · 2022-05-19T21:30:16Z

packages/flutter_tools/lib/src/doctor.dart

can you pull some of this logic into helper methods? This cascade of if/else's is very difficult for my poor brain to parse.

I made each statement have its separate function returning ValidationMessage. Hope that suffices.

christopherfujino · 2022-05-26T21:11:26Z

packages/flutter_tools/lib/src/doctor.dart

Out of curiosity, when is this value 0.0.0-unknown? And can you add a comment with the answer so I don't ask you next time :)

christopherfujino · 2022-05-26T21:13:23Z

packages/flutter_tools/lib/src/doctor.dart

I think this can be before run the validator on line 544?

christopherfujino · 2022-05-26T21:19:38Z

packages/flutter_tools/lib/src/doctor.dart

Rather than doing this check for gitUrl != null, can we not re-use the error message of upstreamValidationError? If that doesn't make sense, I think we should at least sub-class VersionCheckError for the different types of errors, rather than doing a check for the value of gitUrl here. It would be better for that implementation detail to be encapsulated inside of the VersionUpstreamValidator.

That actually sounds like a good idea.

christopherfujino · 2022-06-02T21:27:34Z

packages/flutter_tools/test/general.shard/flutter_validator_test.dart

This is great!

christopherfujino · 2022-06-02T21:28:32Z

packages/flutter_tools/test/general.shard/flutter_validator_test.dart

should this be a hint? is it not a legitimate state to be in to be on a non-release branch?

This is because I want to nudge users towards hints that might cause flutter upgrade to fail. The tool sets channel unknown if the branch doesn't have an upstream.

It doesn't seem like this message gives the user actionable information, though.

Should we recommend the user to reinstall here(and other errors where applicable)?

what if the if i'm tracking branch delta from my own mirror? Won't that also resolve to "Unknown"? This seems like a reasonable workflow.

The tool parses the information from this command for the channel:

flutter/packages/flutter_tools/lib/src/version.dart

Lines 113 to 114 in c181e45

final String gitChannel = _runGit(

'git rev-parse --abbrev-ref --symbolic $kGitTrackingUpstream',

and sets 'unknown' if the above returns nothing(or fails), which shouldn't be possible if the current branch is tracking something, be it a remote or a local branch.

Ahh, ok, I assumed we were validating that this was in a known set. Thanks for educating me!

christopherfujino · 2022-06-02T21:28:55Z

packages/flutter_tools/test/general.shard/flutter_validator_test.dart

Is the assumption here that git describe ... outputted something we can't parse?

christopherfujino

LGTM

royarg02 · 2022-06-17T13:05:11Z

It doesn't seem like this message gives the user actionable information, though.

This is what I have in mind:

If upstream repository is a non-standard remote, we show a message to set FLUTTER_GIT_URL to the non-standard remote.
If upstream repository is unknown, we display a message to reinstall Flutter.
If channel is unknown, we display a message to switch to a release channel using flutter channel(to fix detached HEAD), and if that doesn't work, reinstall Flutter.
If frameworkVersion is unknown, we display a message that the tool was unable to resolve version possibly due to local changes, and if is okay to lose changes, reinstall Flutter.

Jasguerrero

LGTM

…onfig (flutter/flutter#103259)

…lutter#103259)

…onfig (flutter/flutter#103259)

flutter-dashboard bot added the tool Affects the "flutter" command-line tool. See also t: labels. label May 7, 2022

zanderso requested a review from christopherfujino May 7, 2022 13:56

christopherfujino reviewed May 20, 2022

View reviewed changes

royarg02 force-pushed the fluttervalidator branch 2 times, most recently from 4d5d1e0 to e6f6019 Compare May 21, 2022 12:18

christopherfujino reviewed May 26, 2022

View reviewed changes

packages/flutter_tools/lib/src/doctor.dart Outdated

Copy link

Contributor

christopherfujino May 26, 2022

Choose a reason for hiding this comment

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

I think this can be before run the validator on line 544?

christopherfujino reviewed May 26, 2022

View reviewed changes

royarg02 force-pushed the fluttervalidator branch 2 times, most recently from 0ef7ca3 to 38bb6eb Compare May 27, 2022 12:08

christopherfujino reviewed Jun 2, 2022

View reviewed changes

packages/flutter_tools/test/general.shard/flutter_validator_test.dart Outdated

Copy link

Contributor

christopherfujino Jun 2, 2022

Choose a reason for hiding this comment

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

This is great!

christopherfujino reviewed Jun 2, 2022

View reviewed changes

royarg02 force-pushed the fluttervalidator branch from 8377c7b to edb657d Compare June 3, 2022 05:43

christopherfujino approved these changes Jun 16, 2022

View reviewed changes

christopherfujino requested a review from Jasguerrero June 16, 2022 21:33

Jasguerrero approved these changes Jun 17, 2022

View reviewed changes

christopherfujino added the autosubmit Merge PR when tree becomes green via auto submit App label Jun 19, 2022

royarg02 added 9 commits June 20, 2022 10:24

[flutter_tools] Have FlutterValidator fail on non-ideal git config

3133b78

Add/fix tests

2c56609

Make CI happy

e921652

Extract version message and upstream message into separate functions

0e9df12

Add comments

ec2e69a

Use error messages to determine upstream remote message

81abb70

Moar detailed comments

c4420f8

Add a space

edfdd4d

Version unknown iff no upstream and detached head

0057438

royarg02 force-pushed the fluttervalidator branch from edb657d to 0057438 Compare June 20, 2022 06:56

auto-submit bot merged commit 47b4060 into flutter:master Jun 20, 2022

royarg02 mentioned this pull request Jun 21, 2022

Suggest actions to resolve failing FlutterValidator #106334

Closed

This was referenced Jun 21, 2022

Roll Flutter from 02558d69d923 to d4929bc828ff (23 revisions) flutter/plugins#6008

Closed

Roll Flutter from 4f034a40454b to d4929bc828ff (5 revisions) flutter/packages#2267

Merged

engine-flutter-autoroll added a commit to engine-flutter-autoroll/plugins that referenced this pull request Jun 21, 2022

47b4060 [flutter_tools] Have FlutterValidator fail on non-ideal git c…

0065d98

…onfig (flutter/flutter#103259)

engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Jun 21, 2022

47b4060 [flutter_tools] Have FlutterValidator fail on non-ideal git c…

dcb4522

…onfig (flutter/flutter#103259)

engine-flutter-autoroll mentioned this pull request Jun 21, 2022

Roll Flutter from 02558d69d923 to b5c0f8ab9a96 (24 revisions) flutter/plugins#6010

Closed

engine-flutter-autoroll added a commit to engine-flutter-autoroll/plugins that referenced this pull request Jun 21, 2022

47b4060 [flutter_tools] Have FlutterValidator fail on non-ideal git c…

d409d5e

…onfig (flutter/flutter#103259)

engine-flutter-autoroll mentioned this pull request Jun 21, 2022

Roll Flutter from 02558d69d923 to dfaec11ba519 (29 revisions) flutter/plugins#6011

Merged

engine-flutter-autoroll added a commit to engine-flutter-autoroll/plugins that referenced this pull request Jun 21, 2022

47b4060 [flutter_tools] Have FlutterValidator fail on non-ideal git c…

fa11a54

…onfig (flutter/flutter#103259)

royarg02 deleted the fluttervalidator branch August 4, 2022 17:48

camsim99 pushed a commit to camsim99/flutter that referenced this pull request Aug 10, 2022

[flutter_tools] Have FlutterValidator fail on non-ideal git config (f…

e603d92

…lutter#103259)

engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Aug 30, 2022

47b4060 [flutter_tools] Have FlutterValidator fail on non-ideal git c…

d02586f

…onfig (flutter/flutter#103259)

engine-flutter-autoroll added a commit to engine-flutter-autoroll/plugins that referenced this pull request Aug 30, 2022

47b4060 [flutter_tools] Have FlutterValidator fail on non-ideal git c…

051adfe

…onfig (flutter/flutter#103259)

	final String gitChannel = _runGit(
	'git rev-parse --abbrev-ref --symbolic $kGitTrackingUpstream',

Conversation

royarg02 commented May 7, 2022

Pre-launch Checklist

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

royarg02 Jun 6, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

christopherfujino left a comment

Choose a reason for hiding this comment

Uh oh!

royarg02 commented Jun 17, 2022

Uh oh!

Jasguerrero left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

royarg02 Jun 6, 2022 •

edited

Loading