[flutter_tools] check first for stable tag, then dev tag #55342

christopherfujino · 2020-04-21T23:58:20Z

Description

In the situation where beta release 1.2.3-4.5.pre is promoted to the stable channel as 1.2.3, the tool will have to disambiguate between multiple tags on the same commit. The desired outcome is to always favor the stable tag. Prior to this PR, the tool relied on git describe ...to pick the right tag. Testing locally, it seemed to do the right thing, but as this may not be stable (e.g. https://stackoverflow.com/questions/8089002/git-describe-with-two-tags-on-the-same-commit), this PR makes the following changes:

Rather than always running git describe ... to determine the version, first check if the current commit is already tagged (git tag --contains HEAD), and if it is, check first for a stable tag, then for a dev tag.
If no stable or dev tag is found, then run git describe ... as before and parse.
Removes support for the previous tag format x.y.z-dev.m.n, closing Deprecate legacy release tag parsing logic #53850.

Related Issues

Fixes #55332 and #53850.

Tests

Updated test mocks to support an additional process call to git, and added additional test expectations.

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: Replace with a link to your migration guide

jonahwilliams · 2020-04-23T01:20:42Z

packages/flutter_tools/lib/src/version.dart

+    // Check first for a stable tag
+    for (final String tag in tags) {
+      final String trimmedTag = tag.trim();
+      if (RegExp(r'^\d+\.\d+\.\d+$').hasMatch(trimmedTag)) {


I would hoist this and the other RegExp out of the loop. Then, document the format they are expected to match.

Is it possible to reuse some of the existing version parsing logic here?

Good call on hoisting. I thought about reusing, but (at least in this file), they're all slightly different.

Yeah, that is understandable - consider centralizing and standardizing them, so that we can easily verify that each version has the same level of documentation and testing.

Also would hopefully prevent anyone from accidentally re-implementing them

well, I take that back, I can do a little reuse, lemme play with this...

hoisted regexes. PTAL

jmagman · 2020-04-23T01:17:05Z

packages/flutter_tools/lib/src/version.dart

+      }
+    }
+
+    // If we're not currently on a tag, use git describe


If we're not currently on a tag, use git describe to find the most recent version tag.

jmagman · 2020-04-23T01:20:24Z

packages/flutter_tools/lib/src/version.dart

+    final List<int> parsedParts = parts.map<int>(
      (String source) => source == null ? null : int.tryParse(source)).toList();
    List<int> devParts = <int>[null, null];
    if (parts[3] != null) {


This method is confusing, any way we can name the groups since parts[3] is group 4? Or create locals like:

final String majorVersion = match?.group(1); final String minorVersion = match?.group(2); final String patchVersion = match?.group(3); final String preSuffix = match?.group(4);

jmagman · 2020-04-23T01:21:15Z

packages/flutter_tools/lib/src/version.dart

-    final List<String> parts = versionPattern.matchAsPrefix(version)?.groups(<int>[1, 2, 3, 4, 5, 6]);
+      r'^(\d+)\.(\d+)\.(\d+)(-\d+\.\d+\.pre)?$');
+    final Match match = versionPattern.matchAsPrefix(version);
+    final List<String> parts = match?.groups(<int>[1, 2, 3, 4]);


For 1.2.3 will this return ['1', '2', 3'] or ['1', '2', 3', null]?

The latter.

jmagman · 2020-04-23T01:26:48Z

packages/flutter_tools/lib/src/version.dart

    List<int> devParts = <int>[null, null];
    if (parts[3] != null) {
-      devParts = RegExp(r'^-dev\.(\d+)\.(\d+)')
+      devParts = RegExp(r'^-(\d+)\.(\d+)\.pre')


Same here, the chaining is nice, but it's very confusing. I'd rather this be spelled out with some locals.

jmagman · 2020-04-23T01:27:34Z

packages/flutter_tools/lib/src/version.dart

+      commits: 0,
+      hash: '',


Are these used anymore?

they will only be present if we're parsing the output of git describe. this method is checking if we are exactly on a tag.

jmagman · 2020-04-23T19:13:48Z

packages/flutter_tools/lib/src/version.dart

+    final int y = matchGroups[1] == null ? null : int.tryParse(matchGroups[1]);
+    final int z = matchGroups[2] == null ? null : int.tryParse(matchGroups[2]);
+    final String devString = matchGroups[3];
+    int m, n;


Can you rename these devVersion and devPatch?

jmagman · 2020-04-23T19:14:07Z

packages/flutter_tools/lib/src/version.dart

+    final int y = matchGroups[1] == null ? null : int.tryParse(matchGroups[1]);
+    final int z = matchGroups[2] == null ? null : int.tryParse(matchGroups[2]);
+    final String devString = matchGroups[3];
+    int m, n;


Same, can you rename to devVersion and devPatch?

jmagman · 2020-04-23T19:19:11Z

packages/flutter_tools/lib/src/version.dart

-  // TODO(fujino): Deprecate this https://github.com/flutter/flutter/issues/53850
-  /// Check for the release tag format of the form x.y.z-dev.m.n
-  static GitTagVersion parseLegacyVersion(String version) {
+  /// Check for the release tag format of the forms x.y.z or x.y.z-m.n.pre


Can you give a real example with digits, too?

jmagman · 2020-04-23T19:19:42Z

packages/flutter_tools/lib/src/version.dart


-  /// Check for the release tag format of the form x.y.z-m.n.pre
-  static GitTagVersion parseVersion(String version) {
+  /// Detect output of git describe, of the form x.y.z-m.n.pre-<commits>-g<hash>


Same, can you give a real example here?

jmagman · 2020-04-23T19:30:13Z

packages/flutter_tools/lib/src/version.dart

-  /// Check for the release tag format of the form x.y.z-dev.m.n
-  static GitTagVersion parseLegacyVersion(String version) {
+  /// Check for the release tag format of the forms x.y.z or x.y.z-m.n.pre
+  static GitTagVersion parseTaggedVersion(String version) {


parseTaggedVersion and parseGitVersion look almost the same. Can they be combined with a regex that handles either case? ?> is a non-capturing group.
I didn't run the unit tests, so this might not be quite right:

^(\d+)\.(\d+)\.(\d+)(-\d+\.\d+\.pre)?(?>-(\d+)-g([a-f0-9]+))?$

We could combine the regexes, but i originally split these into two different methods as a code review suggestion because the the regex was getting difficult to read (which I agree). In particular, I feel like more explicit the regex, the less likely it is to be wrong.

At the same time, it feels bad having two methods that are almost the same. So not sure what to do.

The methods seem sufficiently complex at this point to deduplicate, I'd rather trade an evil regex for fewer potential copy-paste issues like:

m = devGroups[0] == null ? null : int.tryParse(devGroups[0]); n = devGroups[1] == null ? null : int.tryParse(devGroups[1]);

Ok, I collapsed it into one. PTAL

jmagman

LGTM, thank you for sacrificing your mental wellbeing on this upgrade problem.

jonahwilliams

LGTM

I'm not going to pretend that I've fully understood the git and regexps used, but the testing seems good so lets ship it

* [flutter_tools] fix type error in symbolize (#55212) * [flutter_tools] check first for stable tag, then dev tag (#55342) * [flutter_tools] enable `flutter upgrade` to support force pushed branches (#55594) * [flutter_tools] catch ProcessException and throw ToolExit during upgrade (#55759) * Update engine version to 376ad6a64b08aa26005e3f82aed26de2e290b572 Co-authored-by: Jonah Williams <campfish91@gmail.com> Co-authored-by: Christopher Fujino <fujino@google.com> Co-authored-by: Christopher Fujino <christopherfujino@gmail.com>

check first for stable tag, fix existing tests

a98e5d9

fluttergithubbot added tool Affects the "flutter" command-line tool. See also t: labels. work in progress; do not review labels Apr 21, 2020

googlebot added the cla: yes label Apr 21, 2020

christopherfujino added 10 commits April 21, 2020 17:02

really fix all tests

2bc2bf4

support parsing stable version

19b49f1

improve expectations

fe7bb25

add a new test verifying behavior

b54d8d3

fix comment

f262ab0

fix flutter_command_runner_test

da3a39b

fix upgrade test

edfb16a

fix another test

f52bfec

add a failing test

146d815

get test passing

967671a

christopherfujino marked this pull request as ready for review April 23, 2020 01:03

christopherfujino requested a review from jmagman April 23, 2020 01:03

christopherfujino removed the work in progress; do not review label Apr 23, 2020

christopherfujino requested a review from jonahwilliams April 23, 2020 01:03

jonahwilliams reviewed Apr 23, 2020

View reviewed changes

jmagman reviewed Apr 23, 2020

View reviewed changes

christopherfujino added 2 commits April 22, 2020 18:33

hoist regex declaration out of for loop

919c724

code review

6af6609

jmagman reviewed Apr 23, 2020

View reviewed changes

christopherfujino added 2 commits April 23, 2020 12:56

code review

e50ea79

code review

9b5e203

jmagman approved these changes Apr 23, 2020

View reviewed changes

jonahwilliams approved these changes Apr 23, 2020

View reviewed changes

christopherfujino merged commit 37ac901 into flutter:master Apr 23, 2020

christopherfujino deleted the explicitly-check-for-stable-tag branch April 23, 2020 22:19

pcsosinski pushed a commit to pcsosinski/flutter that referenced this pull request Apr 28, 2020

[flutter_tools] check first for stable tag, then dev tag (flutter#55342)

668e0a7

github-actions bot locked as resolved and limited conversation to collaborators Jul 31, 2021

[flutter_tools] check first for stable tag, then dev tag #55342

[flutter_tools] check first for stable tag, then dev tag #55342

Uh oh!

Conversation

christopherfujino commented Apr 21, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Related Issues

Tests

Checklist

Breaking Change

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!

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!

jmagman Apr 23, 2020 • 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!

jmagman Apr 23, 2020 • 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!

jmagman left a comment

Choose a reason for hiding this comment

Uh oh!

jonahwilliams left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

christopherfujino commented Apr 21, 2020 •

edited

Loading

jmagman Apr 23, 2020 •

edited

Loading

jmagman Apr 23, 2020 •

edited

Loading