[flutter_tools] Make flutter git upstream configurable

[TooYoungTooSimp]

Description

Make flutter git upstream configurable from environment variable.

Used FLUTTER_GIT_URL to control _flutterGit value.

Related Issues

#61286

Checklist

Before you create this PR confirm that it meets all requirements listed below by checking the relevant checkboxes ([x]). This will ensure a smooth and quick review process.

• I read the Contributor Guide and followed the process outlined there for submitting PRs.
• I signed the CLA.
• I read and followed the Flutter Style Guide, including Features we expect every widget to implement.
• I read the Tree Hygiene wiki page, which explains my responsibilities.
• I updated/added relevant documentation (doc comments with ///).
• All existing and new tests are passing.
• The analyzer (flutter analyze --flutter-repo) does not report any problems on my PR.
• I am willing to follow-up on review comments in a timely manner.

Breaking Change

Did any tests fail when you ran them? Please read Handling breaking changes.

• No, no existing tests failed, so this is not a breaking change.
• Yes, this is a breaking change. If not, delete the remainder of this section.
  • I wrote a design doc: https://flutter.dev/go/template Replace this with a link to your design doc's short link
  • I got input from the developer relations team, specifically from: Replace with the names of who gave advice
  • I wrote a migration guide: https://flutter.dev/go/breaking-changes-template Replace this with a link to a pull request that adds the migration guide to https://flutter.dev/docs/release/breaking-changes

[fluttergithubbot]

It looks like this pull request may not have tests. Please make sure to add tests before merging. If you need an exemption to this rule, contact Hixie on the #hackers channel in Chat.

Reviewers: Read the Tree Hygiene page and make sure this patch meets those guidelines before LGTMing.

[googlebot]

Thanks for your pull request. It looks like this may be your first contribution to a Google open source project (if not, look below for help). Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

📝 Please visit https://cla.developers.google.com/ to sign.

Once you've signed (or fixed any issues), please reply here with @googlebot I signed it! and we'll verify it.

---

What to do if you already signed the CLA

Individual signers

• It's possible we don't have your GitHub username or you're using a different email address on your commit. Check your existing CLA data and verify that your email is set on your git commits.

Corporate signers

• Your company has a Point of Contact who decides which employees are authorized to participate. Ask your POC to be added to the group of authorized contributors. If you don't know who your Point of Contact is, direct the Google project maintainer to go/cla#troubleshoot (Public version).
• The email used to register you as an authorized contributor must be the email used for the Git commit. Check your existing CLA data and verify that your email is set on your git commits.
• The email used to register you as an authorized contributor must also be attached to your GitHub account.

ℹ️ Googlers: Go here for more info.

[TooYoungTooSimp]

@googlebot I signed it!

[googlebot]

CLAs look good, thanks!

ℹ️ Googlers: Go here for more info.

[TooYoungTooSimp]

Sorry for making many wrong commits at beginning. Please use rebase merge to hide them. 😢

[christopherfujino]

Sorry for making many wrong commits at beginning. Please use rebase merge to hide them.

No prob, we default to squash and merge, so each PR becomes one commit on master :)

[christopherfujino]

This looks like a good solution. One thing though, is that if the user has a typo in this env variable (or if it the remote is down), we should explicitly show the override value in any error message. If you check line 654 of this file, we throw an error if a command we run does not return a 0 exit code. Even though this error includes the complete command that failed, unfortunately, if FLUTTER_GIT_URL env variable is wrong, we won't hit an error until line 257, which tries to run the command git fetch $_versionCheckRemote $branch, which doesn't include the upstream URL in it. Probably the better version would be to add a catch around 262, and IF we have an explicit FLUTTER_GIT_URL override set in the environment, globals.printError('Warning: the Flutter git upstream was overriden by the environment variable FLUTTER_GIT_URL = ${globals.platform.environment['FLUTTER_GIT_URL']}'); (something like that, I didn't test it).

Also, this change should have unit tests in the file: https://github.com/flutter/flutter/blob/master/packages/flutter_tools/test/general.shard/version_test.dart. Using the testUsingContext() function, you can provide a FakePlatform and ProcessManager overrides (if you search the codebase for FakePlatform, you'll find a lot of examples) where you set the environment variable and then expect that you are getting the correct git commands to the mockProcessManager, specifically the one setting the upstream.

Let me know if you you run into any obstacles!

[TooYoungTooSimp]

I have finished print warning and unit test. But test running failed unexpected. Sorry for troubling but I have a problem now.

[jmagman]

Thanks for your contribution! This needs a test in order to be merged. How about something like this in version_test.dart?

  testUsingContext('determine uses overridden git url', () {
    final MockProcessUtils processUtils = MockProcessUtils();
    when(processUtils.runSync(
      <String>['git', 'rev-parse', '--abbrev-ref', 'HEAD'],
      workingDirectory: anyNamed('workingDirectory'),
      environment: anyNamed('environment'),
    )).thenReturn(RunResult(ProcessResult(108, 0, 'master', ''), <String>['git', 'fetch']));
    when(processUtils.runSync(
      <String>['git', 'fetch', 'https://githubmirror.com/flutter.git', '--tags'],
      workingDirectory: anyNamed('workingDirectory'),
      environment: anyNamed('environment'),
    )).thenReturn(RunResult(ProcessResult(109, 0, '', ''), <String>['git', 'fetch']));
    when(processUtils.runSync(
      <String>['git', 'tag', '--contains', 'HEAD'],
      workingDirectory: anyNamed('workingDirectory'),
      environment: anyNamed('environment'),
    )).thenReturn(
        RunResult(ProcessResult(110, 0, '', ''),
          <String>['git', 'tag', '--contains', 'HEAD'],
        ));
    when(processUtils.runSync(
      <String>['git', 'describe', '--match', '*.*.*-*.*.pre', '--first-parent', '--long', '--tags'],
      workingDirectory: anyNamed('workingDirectory'),
      environment: anyNamed('environment'),
    )).thenReturn(RunResult(ProcessResult(111, 0, 'v0.1.2-3-1234abcd', ''), <String>['git', 'describe']));

    GitTagVersion.determine(processUtils, workingDirectory: '.', fetchTags: true);

    verify(processUtils.runSync(
      <String>['git', 'fetch', 'https://githubmirror.com/flutter.git', '--tags'],
      workingDirectory: anyNamed('workingDirectory'),
      environment: anyNamed('environment'),
    ));
  }, overrides: <Type, Generator>{
    Platform: () => FakePlatform(environment: <String, String>{
      'FLUTTER_GIT_URL': 'https://githubmirror.com/flutter.git',
    }),
  });

Those tests are kind of hard to write, sorry about that. I'd be happy to push the test to your fork, if you'd like!

#61286 (comment)

Except flutter-build, all other test passed.
To be honest, I don't know what is it and other prs have this problem too.

flutter-build is our post-submit CI system, and at the moment it's failing, but this has nothing to do with your PR. Once this is in good shape, we'll handle merging your PR once it goes green!

[jmagman]

Unfortunately this isn't really testable since it can't be changed after the first time it's set. I wrote a unit test for this that passed on its own, but failed when run with the entire test suite because it always returned https://github.com/flutter/flutter.git because a previous test had initialized it.

How about:

String get _flutterGit => globals.platform.environment['FLUTTER_GIT_URL'] ??
    'https://github.com/flutter/flutter.git';

[christopherfujino]

Good catch

[TooYoungTooSimp]

Really great. Thanks for all of your kindly helps. I'll work on them right now.
// Learned a lot this time 😃

[TooYoungTooSimp]

I got trouble writing tests...sorry.
After I copied the test from @jmagman and pushed to my branch.
Some CI tests failed.
Which result is

[test/general.shard/version_test.dart:main]:  NoSuchMethodError: The getter 'stdout' was called on null.
Receiver: null
Tried calling: stdout[test/general.shard/version_test.dart:main]:  #0      Object.noSuchMethod (dart:core-patch/object_patch.dart:51:5)
#1      _runGit (package:flutter_tools/src/version.dart:676:5)
#2      GitTagVersion.determine (package:flutter_tools/src/version.dart:760:9)
#3      main.<anonymous closure> (org-dartlang-app:///test/general.shard/version_test.dart:640:19)
#4      testUsingContext.<anonymous closure>.<anonymous closure>.<anonymous closure>.<anonymous closure>.<anonymous closure> (org-dartlang-app:///test/src/context.dart:151:42)
#5      AppContext.run.<anonymous closure> (package:flutter_tools/src/base/context.dart:150:29)

And so on, but I believe these should be enough.
detailed CI status

So basically, I need your help. 😢

[TooYoungTooSimp]

Finally I managed to pass CI with tests.
So I would like to request you to review my pr again now.
@christopherfujino @jmagman

[jmagman]

Other than the one spacing nit, this LGTM. I'll approve and merge once that gets fixed, assuming @christopherfujino doesn't have any more suggestions.

[christopherfujino]

LGTM once you make @jmagman 's spacing change

[TooYoungTooSimp]

The space problem fixed. Thanks.

[jmagman]

LGTM