Fix `frameworkVersionFor` for flutter doctor and usage by christopherfujino · Pull Request #54217 · flutter/flutter

christopherfujino · 2020-04-07T19:17:20Z

Description

With the new release tag format, the Flutter tool failed to detect the new tags #53688. Tag parsing was fixed in #53715, however, version reporting was not updated, so flutter doctor and usage reporting would only include x.y.z, and not the additional m.n in pre-stable releases.

This PR fixes frameworkVersionFor() AND accounts for another modification to the release tag format (1.2.3-dev.4.5 -> 1.2.3-4.5.pre). Parsing of the x.y.z-dev.m.n format is being moved to the parseLegacyVersion() method, and we are dropping support for vx.y.z+hotfix.m. This parsing logic is only for the nearest tagged parent, thus we can drop parsing support for old formats once the new format has been published on that channel.

Related Issues

fixes #54160, #53688

Tests

I extended the existing test to verify more permutations are correctly being parsed.

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

tvolkert · 2020-04-07T21:07:48Z

-  final RegExp pattern = RegExp(r'^\d+\.\d+\.\d+(\+hotfix\.\d+)?(-pre\.\d+)?$');
+  // 1.18.0-5.0-pre.8
+  final RegExp pattern = RegExp(
+    r'^(\d+)\.(\d+)\.(\d+)(-\d+\.\d+)?(-pre\.\d+)?$');


.pre (dot, not hyphen).

And elsewhere.

I can make that change, but to clarify, this is just for reporting commits past last tagged version in the tool, and not the git tag format itself, and is a break from how we used to do it.

// old way, 4 commits past 1.2.3 1.2.4-pre.4 // this PR, 4 commits past 1.2.3-0.0.pre 1.2.3-1.0-pre.4 // proposed change, 4 commits past 1.2.3-0.0.pre 1.2.3-1.0.pre.4

and also, if you are 4 commits past version 1.2.3? Should that be 1.2.3-pre.4 or 1.2.3.pre.4? (though I suppose this would be an edge case, where the user is adding their own commits on the stable branch).

tvolkert · 2020-04-07T21:08:48Z


  // TODO(fujino): Deprecate this https://github.com/flutter/flutter/issues/53850
-  /// Check for the release tag format pre-v1.17.0
+  /// Check for the release tag format of the form x.y.z-dev.m.n


No longer -dev

I had to support the -dev tag or else when I land this (before there are the new tags) the tool will be in a bad state (and pre-submit was failing because it couldn't parse the current version)

It was either support the -dev tag, or change the git describe --match glob, and I figured if I support the -dev tag here, I can safely remove it after the next dev release with the new tag format.

If possible, can you split out the -dev parsing code to more clearly delineate what logic can be removed after the next dev?

This entire method can be removed in the next dev.

tvolkert · 2020-04-07T21:09:03Z

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


No longer -dev

tvolkert · 2020-04-07T21:09:24Z

      return '$x.$y.$z+hotfix.${hotfix + 1}-pre.$commits';
    }
+    if (devPatch != null && devVersion != null) {
+      return '$x.$y.$z-${devVersion + 1}.0-pre.$commits';


tvolkert · 2020-04-07T21:09:35Z

    }
    if (hotfix != null) {
+      // This is an unexpected state where untagged commits exist past a hotfix
      return '$x.$y.$z+hotfix.${hotfix + 1}-pre.$commits';


christopherfujino · 2020-04-07T21:15:02Z

I'm looking at the failure to test/commands.shard/hermetic/version_test.dart

zanderso · 2020-04-07T22:28:37Z


  // TODO(fujino): Deprecate this https://github.com/flutter/flutter/issues/53850
-  /// Check for the release tag format pre-v1.17.0
+  /// Check for the release tag format of the form x.y.z-dev.m.n


If possible, can you split out the -dev parsing code to more clearly delineate what logic can be removed after the next dev?

zanderso · 2020-04-07T22:31:22Z

-    // legacy tag release format
-    gitTagVersion = GitTagVersion.parse('v1.2.3-4-g$hash');
-    expect(gitTagVersion.frameworkVersionFor(hash), '1.2.4-pre.4');
+    // legacy tag format (x.y.z-dev.m.n), master channel


Would a regression test of the doctor also be useful?

The diff in this PR for dev/bots/test.dart was because changing the output of frameworkVersionFor caused the verifyVersion integration test to fail. I think between that integration test and this unit test, we're covering any territory a doctor test would assert. What do you think?

* Update engine, includes Dart 2.8.0-20.11.beta * git pull --ff-only (#54987) * [flutter_tools] Reland: fix multiple dart defines (#54973) * Fix `frameworkVersionFor` for flutter doctor and usage (#54217) * update engine hash, use dart 2.8.0-20.10-beta (with old naming schema) Co-authored-by: Jenn Magder <[email protected]> Co-authored-by: Jonah Williams <[email protected]> Co-authored-by: Christopher Fujino <[email protected]>

christopherfujino added 2 commits April 7, 2020 11:22

track original tag

2973189

fix frameworkVersionFor and add a lot of tests

f395a6d

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

googlebot added the cla: yes label Apr 7, 2020

christopherfujino added 2 commits April 7, 2020 12:29

migrate to new 1.2.3-4.5.pre tag format

71c85c7

set parseLegacyVersion to parse x.y.z-dev.m.n

714a3d5

christopherfujino force-pushed the fix-flutter-version-take-two branch 2 times, most recently from 2d03c9f to f23114d Compare April 7, 2020 20:35

fix dev/bots/test.dart

ee33ead

christopherfujino force-pushed the fix-flutter-version-take-two branch from f23114d to ee33ead Compare April 7, 2020 20:46

fix test/test_test.dart

8c4fe93

christopherfujino marked this pull request as ready for review April 7, 2020 21:06

christopherfujino requested review from tvolkert and zanderso April 7, 2020 21:06

tvolkert reviewed Apr 7, 2020

View reviewed changes

fix commands.shard/hermetic/version_test.dart

6aefb27

zanderso reviewed Apr 7, 2020

View reviewed changes

christopherfujino added 3 commits April 7, 2020 16:26

change -pre -> .pre

a8b2f09

fix tst

4dca1fa

remove comment

e289246

zanderso approved these changes Apr 8, 2020

View reviewed changes

improve test/test_test regex

ed42f5a

christopherfujino added waiting for tree to go green and removed work in progress; do not review labels Apr 8, 2020

fluttergithubbot merged commit 19c8948 into flutter:master Apr 8, 2020

christopherfujino deleted the fix-flutter-version-take-two branch April 8, 2020 20:02

christopherfujino mentioned this pull request Apr 15, 2020

[flutter_tools] Fix roll dev script, add tests #54783

Merged

9 of 13 tasks

pcsosinski mentioned this pull request Apr 16, 2020

New release tag format (e.g. "1.17.0-dev.0.0") not detected by flutter tool #53688

Closed

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

Fix frameworkVersionFor for flutter doctor and usage (flutter#54217)

5c17582

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

Conversation

christopherfujino commented Apr 7, 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!

christopherfujino Apr 7, 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!

christopherfujino Apr 7, 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!

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!

christopherfujino commented Apr 7, 2020

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!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

christopherfujino commented Apr 7, 2020 •

edited

Loading

christopherfujino Apr 7, 2020 •

edited

Loading

christopherfujino Apr 7, 2020 •

edited

Loading