[flutter_tools] Skip version freshness check for non-standard remotes #97202

royarg02 · 2022-01-25T11:50:19Z

This PR makes the tool skip checking for version freshness if the remote being tracked by the current channel/branch is "not-standard".

A standard remote is either https://github.com/flutter/flutter.git/[email protected]:flutter/flutter.git or the one set in the environment variable FLUTTER_GIT_URL.

This makes the tool check for freshness from the same repo it will fetch the updates when running the upgrade sub-command. Another added bonus, it prevents the unnecessary check for freshness from a user's forked repo, however the user can still configure FLUTTER_GIT_URL for freshness check anyway in that case.

Fixes #14120.

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-02-17T21:54:44Z

packages/flutter_tools/lib/src/version.dart

rather than hard-coding here, could you also store this in globals, next to flutterGit?

Getting back to this, why does this need to be in globals? This and flutterGit would be only used in version.dart. We could probably remove them from globals and instead pass globals.platform around.

packages/flutter_tools/lib/src/version.dart

christopherfujino · 2022-02-18T17:43:33Z

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

this context platform never gets called in the test, right?

Unless the test doesn't call globals.flutterGit, which it definitely does :)

christopherfujino · 2022-02-18T17:47:17Z

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

I can see that before we were injecting the platform, we were accidentally leaking the real system platform into the test suite, which is a huge cause of flakiness in our tool test suite. For the tests that only depend on a created instance of VersionUpstreamValidator, can you use testWithoutContext() instead of testUsingContext()? With testWithoutContext(), any test that tries to call context.get() via the globals. or any other method will throw. This way we know exactly what our the code depends on, and we don't get tests accidentally passing because the system they were running on had the correct platform setup. I imagine you will have to also inject flutterGitSshUrl via the constructor.

We would also need to provide globals.flutterGit too if that's the case.

Yeah. I know it's a pain, but it's the best way we have right now for ensuring that we're not leaking global state.

...which adds two parameters which similar semantics?

VersionUpstreamValidator({ required this.version, required this.platform, required this.flutterGitUrl, required this.flutterGitSshUrl, });

Can't think of a better name to give to these parameters.

Yeah, that feels bad. However, the only other way I can think of would be to define a class for GitHubRemote, that takes in an org and project name, and has two getters for httpsUrl and sshUrl, but that seems like overkill?

packages/flutter_tools/lib/src/version.dart

royarg02 · 2022-02-23T10:13:07Z

Adding to my comment here and my inference of the usage of globals.dart:

globals.flutterGit is only used twice in version.dart(it is also used in upgrade.dart, which this PR removes):

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

Lines 274 to 283 in cddd552

    
             static Future<String> fetchRemoteFrameworkCommitDate(String branch) async { 
        
               await _removeVersionCheckRemoteIfExists(); 
        
               try { 
        
                 await _run(<String>[ 
        
                   'git', 
        
                   'remote', 
        
                   'add', 
        
                   _versionCheckRemote, 
        
                   globals.flutterGit, 
        
                 ]);

This will be redundant once this PR lands(and can be removed).

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

Lines 634 to 641 in cddd552

    
           static GitTagVersion determine(ProcessUtils processUtils, {String? workingDirectory, bool fetchTags = false, String gitRef = 'HEAD'}) { 
        
             if (fetchTags) { 
        
               final String channel = _runGit('git rev-parse --abbrev-ref HEAD', processUtils, workingDirectory); 
        
               if (channel == 'dev' || channel == 'beta' || channel == 'stable') { 
        
                 globals.printTrace('Skipping request to fetchTags - on well known channel $channel.'); 
        
               } else { 
        
                 _runGit('git fetch ${globals.flutterGit} --tags -f', processUtils, workingDirectory); 
        
               }

From what I can see, this is used for version resolution, before which the tool fetches in the newer tags. An interesting consequence of using FLUTTER_GIT_URL here that if the custom remote does not have the newer tags (which is the case for forks updated through git pull upstream master && git push origin master), a wrong version is calculated(#15529).

I confirmed this by running FLUTTER_GIT_URL='https://github.com/RoyARG02/flutter/flutter.git' ./bin/flutter --version --verbose on a fresh clone of my fork which does not have the newer tags, and the tool gives me a version of 1.24.0-0.0.pre.7043 😱

~~Not sure if allowing FLUTTER_GIT_URL here would be considered a bug or feature, but we should remove globals.flutterGit here as well if it's the former.~~ This would be a bad idea for users facing problems accessing GitHub.

christopherfujino · 2022-02-24T19:48:42Z

Adding to my comment here and my inference of the usage of globals.dart:

globals.flutterGit is only used twice in version.dart(it is also used in upgrade.dart, which this PR removes):

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

Lines 274 to 283 in cddd552

static Future<String> fetchRemoteFrameworkCommitDate(String branch) async {

await _removeVersionCheckRemoteIfExists();

try {

await _run(<String>[

'git',

'remote',

'add',

_versionCheckRemote,

globals.flutterGit,

]);

This will be redundant once this PR lands(and can be removed).

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

Lines 634 to 641 in cddd552

static GitTagVersion determine(ProcessUtils processUtils, {String? workingDirectory, bool fetchTags = false, String gitRef = 'HEAD'}) {

if (fetchTags) {

final String channel = _runGit('git rev-parse --abbrev-ref HEAD', processUtils, workingDirectory);

if (channel == 'dev' || channel == 'beta' || channel == 'stable') {

globals.printTrace('Skipping request to fetchTags - on well known channel $channel.');

} else {

_runGit('git fetch ${globals.flutterGit} --tags -f', processUtils, workingDirectory);

}

From what I can see, this is used for version resolution, before which the tool fetches in the newer tags. An interesting consequence of using FLUTTER_GIT_URL here that if the custom remote does not have the newer tags (which is the case for forks updated through git pull upstream master && git push origin master), a wrong version is calculated(#15529).
I confirmed this by running FLUTTER_GIT_URL='https://github.com/RoyARG02/flutter/flutter.git' ./bin/flutter --version --verbose on a fresh clone of my fork which does not have the newer tags, and the tool gives me a version of 1.24.0-0.0.pre.7043 😱

Yeah, I don't think there's much we can do about this, other than deprecating the use of git tags for version resolution, which I would like to do but don't really have time for.

royarg02 · 2022-02-25T10:23:07Z

@christopherfujino to solve the issue you mentioned at #97202 (comment), can we instead do #97202 (comment) and use a different _flutterGit in VersionUpstreamValidator? So that would look like the following:

   VersionUpstreamValidator({
     required this.version,
     required this.platform,
-    required this.flutterGitSshUrl,
-    required this.flutterGitUrl,
-  });
+  }) : _flutterGit = platform.environment['FLUTTER_GIT_URL'] ?? 'https://github.com/flutter/flutter.git';

-  final String flutterGitSshUrl;
-  final String flutterGitUrl;
+  final String _flutterGit;
   final FlutterVersion version;
   final Platform platform;
...
     // Strip `.git` suffix before comparing the remotes
-    final String sanitizedRepositoryUrl = stripDotGit(repositoryUrl);
-    final String sanitizedFlutterGitUrl = stripDotGit(flutterGitUrl);
-    final String sanitizedFlutterGitSshUrl = stripDotGit(flutterGitSshUrl);
+    final List<String> sanitizedStandardRemotes = <String>[
+      _flutterGit,
+      '[email protected]:flutter/flutter.git',
+    ].map((String remote) => stripDotGit(remote)).toList();

A similar change can be made(in a future PR) where FLUTTER_GIT_URL is used:

-  static GitTagVersion determine(ProcessUtils processUtils, {String? workingDirectory, bool fetchTags = false, String gitRef = 'HEAD'}) {
+  static GitTagVersion determine(
+    ProcessUtils processUtils,
+    Platform platform, {
+      String? workingDirectory,
+      bool fetchTags = false,
+      String gitRef = 'HEAD'
+    }) {
     if (fetchTags) {
       final String channel = _runGit('git rev-parse --abbrev-ref HEAD', processUtils, workingDirectory);
       if (channel == 'dev' || channel == 'beta' || channel == 'stable') {
         globals.printTrace('Skipping request to fetchTags - on well known channel $channel.');
       } else {
-        _runGit('git fetch ${globals.flutterGit} --tags -f', processUtils, workingDirectory);
+        final String flutterGit = platform.environment['FLUTTER_GIT_URL'] ?? 'https://github.com/flutter/flutter.git';
+        _runGit('git fetch $flutterGit --tags -f', processUtils, workingDirectory);

christopherfujino · 2022-03-03T22:23:28Z

@christopherfujino to solve the issue you mentioned at #97202 (comment), can we instead do #97202 (comment) and use a different _flutterGit in VersionUpstreamValidator? So that would look like the following:

   VersionUpstreamValidator({
     required this.version,
     required this.platform,
-    required this.flutterGitSshUrl,
-    required this.flutterGitUrl,
-  });
+  }) : _flutterGit = platform.environment['FLUTTER_GIT_URL'] ?? 'https://github.com/flutter/flutter.git';

-  final String flutterGitSshUrl;
-  final String flutterGitUrl;
+  final String _flutterGit;
   final FlutterVersion version;
   final Platform platform;
...
     // Strip `.git` suffix before comparing the remotes
-    final String sanitizedRepositoryUrl = stripDotGit(repositoryUrl);
-    final String sanitizedFlutterGitUrl = stripDotGit(flutterGitUrl);
-    final String sanitizedFlutterGitSshUrl = stripDotGit(flutterGitSshUrl);
+    final List<String> sanitizedStandardRemotes = <String>[
+      _flutterGit,
+      '[email protected]:flutter/flutter.git',
+    ].map((String remote) => stripDotGit(remote)).toList();

A similar change can be made(in a future PR) where FLUTTER_GIT_URL is used:

-  static GitTagVersion determine(ProcessUtils processUtils, {String? workingDirectory, bool fetchTags = false, String gitRef = 'HEAD'}) {
+  static GitTagVersion determine(
+    ProcessUtils processUtils,
+    Platform platform, {
+      String? workingDirectory,
+      bool fetchTags = false,
+      String gitRef = 'HEAD'
+    }) {
     if (fetchTags) {
       final String channel = _runGit('git rev-parse --abbrev-ref HEAD', processUtils, workingDirectory);
       if (channel == 'dev' || channel == 'beta' || channel == 'stable') {
         globals.printTrace('Skipping request to fetchTags - on well known channel $channel.');
       } else {
-        _runGit('git fetch ${globals.flutterGit} --tags -f', processUtils, workingDirectory);
+        final String flutterGit = platform.environment['FLUTTER_GIT_URL'] ?? 'https://github.com/flutter/flutter.git';
+        _runGit('git fetch $flutterGit --tags -f', processUtils, workingDirectory);

SGTM

christopherfujino · 2022-03-21T19:42:05Z

packages/flutter_tools/lib/src/commands/upgrade.dart

I think it's a little unusual for the API of this method to be void while it is the responsibility of the caller to catch a VersionCheckError. Could you instead have this method return VersionCheckError? and then in the method, instead of throwing, simply return the error.

christopherfujino

This overall LGTM, with the nit about the validator.run returning a nullable error rather than throwing.

christopherfujino

LGTM, thanks!

fluttergithubbot · 2022-03-21T21:30:20Z

This pull request is not suitable for automatic merging in its current state.

Please get at least one approved review if you are already a member or two member reviews if you are not a member before re-applying this label. Reviewers: If you left a comment approving, please use the "approve" review action instead.

christopherfujino · 2022-03-21T23:05:28Z

@Jasguerrero can you take a look too?

Jasguerrero

LGTM

… remotes (flutter/flutter#97202)

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

royarg02 changed the title ~~[WIP] [flutter_tools] Skip version freshness check for non-standard remotes~~ [flutter_tools] Skip version freshness check for non-standard remotes Jan 25, 2022

royarg02 marked this pull request as ready for review January 25, 2022 12:53

royarg02 force-pushed the nonstandard-freshness-skip branch from d5b20da to cf122c3 Compare January 30, 2022 16:22

christopherfujino reviewed Feb 17, 2022

View reviewed changes

packages/flutter_tools/lib/src/version.dart Outdated Show resolved Hide resolved

christopherfujino reviewed Feb 17, 2022

View reviewed changes

packages/flutter_tools/lib/src/version.dart Outdated Show resolved Hide resolved

royarg02 force-pushed the nonstandard-freshness-skip branch 2 times, most recently from 14afae7 to 7045442 Compare February 18, 2022 13:58

christopherfujino reviewed Feb 18, 2022

View reviewed changes

packages/flutter_tools/lib/src/version.dart Outdated Show resolved Hide resolved

christopherfujino reviewed Feb 18, 2022

View reviewed changes

royarg02 commented Feb 23, 2022

View reviewed changes

packages/flutter_tools/lib/src/version.dart Outdated Show resolved Hide resolved

royarg02 force-pushed the nonstandard-freshness-skip branch 2 times, most recently from fa51406 to 720a22a Compare February 26, 2022 05:56

royarg02 force-pushed the nonstandard-freshness-skip branch from 720a22a to e145881 Compare March 6, 2022 07:13

royarg02 force-pushed the nonstandard-freshness-skip branch from 7e00826 to c24c877 Compare March 13, 2022 07:28

royarg02 force-pushed the nonstandard-freshness-skip branch from c24c877 to 89e9739 Compare March 21, 2022 06:56

christopherfujino reviewed Mar 21, 2022

View reviewed changes

royarg02 added 6 commits March 22, 2022 01:21

Skip version freshness check for non-standard remotes

bb5d51b

Tests

fda2c23

Review fixes

864521d

Review fixes #2

28c9b16

Remove usage of globals.flutterGit from VersionUpstreamValidator

11e71ba

Move flutterGit out of contructor initializer

15a75cd

Make VersionUpstreamValidator.run return instead of throw error

efb5d35

royarg02 force-pushed the nonstandard-freshness-skip branch from 89e9739 to efb5d35 Compare March 21, 2022 20:29

christopherfujino approved these changes Mar 21, 2022

View reviewed changes

christopherfujino added the waiting for tree to go green label Mar 21, 2022

fluttergithubbot removed the waiting for tree to go green label Mar 21, 2022

christopherfujino requested a review from Jasguerrero March 21, 2022 23:05

Jasguerrero approved these changes Mar 22, 2022

View reviewed changes

christopherfujino added the waiting for tree to go green label Mar 22, 2022

fluttergithubbot merged commit 74f08c7 into flutter:master Mar 22, 2022

engine-flutter-autoroll mentioned this pull request Mar 22, 2022

Roll Flutter from 2e5ad37b0639 to 74f08c735262 (2 revisions) flutter/packages#1312

Closed

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

74f08c7 [flutter_tools] Skip version freshness check for non-standard…

b34b3ad

… remotes (flutter/flutter#97202)

engine-flutter-autoroll mentioned this pull request Mar 22, 2022

Roll Flutter from 2e5ad37b0639 to dc331d393be5 (3 revisions) flutter/packages#1313

Closed

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

74f08c7 [flutter_tools] Skip version freshness check for non-standard…

0b6b6ef

… remotes (flutter/flutter#97202)

engine-flutter-autoroll mentioned this pull request Mar 22, 2022

Roll Flutter from 2e5ad37b0639 to 22a9ff74eba9 (4 revisions) flutter/packages#1314

Merged

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

74f08c7 [flutter_tools] Skip version freshness check for non-standard…

0a1583d

… remotes (flutter/flutter#97202)

royarg02 mentioned this pull request Mar 23, 2022

[tool] VersionUpstreamValidator ignores FLUTTER_GIT_URL if upstream repository is standard #100604

Closed

christopherfujino mentioned this pull request Mar 23, 2022

[flutter_tools] Fix VersionUpstreamValidator to respect FLUTTER_GIT_URL #100605

Merged

8 tasks

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

74f08c7 [flutter_tools] Skip version freshness check for non-standard…

bd6f100

… remotes (flutter/flutter#97202)

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

74f08c7 [flutter_tools] Skip version freshness check for non-standard…

a4df05d

… remotes (flutter/flutter#97202)

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

74f08c7 [flutter_tools] Skip version freshness check for non-standard…

c3b1acb

… remotes (flutter/flutter#97202)

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

74f08c7 [flutter_tools] Skip version freshness check for non-standard…

19f57cf

… remotes (flutter/flutter#97202)

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

74f08c7 [flutter_tools] Skip version freshness check for non-standard…

6edf839

… remotes (flutter/flutter#97202)

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

74f08c7 [flutter_tools] Skip version freshness check for non-standard…

9d06895

… remotes (flutter/flutter#97202)

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

74f08c7 [flutter_tools] Skip version freshness check for non-standard…

ceb2a21

… remotes (flutter/flutter#97202)

royarg02 mentioned this pull request May 2, 2022

[flutter_tools] Remove usage of globals.flutterGit from version #100744

Merged

8 tasks

royarg02 deleted the nonstandard-freshness-skip branch May 7, 2022 07:38

[flutter_tools] Skip version freshness check for non-standard remotes #97202

[flutter_tools] Skip version freshness check for non-standard remotes #97202

Uh oh!

Conversation

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

Uh oh!

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!

Uh oh!

Uh oh!

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!

Uh oh!

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

Uh oh!

Uh oh!

christopherfujino commented Feb 24, 2022

Uh oh!

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

Uh oh!

Uh oh!

christopherfujino commented Mar 3, 2022

Uh oh!

christopherfujino Mar 21, 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!

christopherfujino left a comment

Choose a reason for hiding this comment

Uh oh!

christopherfujino left a comment

Choose a reason for hiding this comment

Uh oh!

fluttergithubbot commented Mar 21, 2022

Uh oh!

christopherfujino commented Mar 21, 2022

Uh oh!

Jasguerrero left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

royarg02 commented Jan 25, 2022 •

edited

Loading

royarg02 commented Feb 23, 2022 •

edited

Loading

royarg02 commented Feb 25, 2022 •

edited

Loading

christopherfujino Mar 21, 2022 •

edited

Loading