Support old and new git release tag formats by christopherfujino · Pull Request #53715 · flutter/flutter

christopherfujino · 2020-04-01T00:28:04Z

Description

Update the flutter version determination logic to support both old and new tag formats. NOTE: #53775 was landed before this as part of the fix.

Old Tags

v1.2.3
v1.2.3+hotfix.1

New Tags

1.2.3
1.2.3-dev.4.5

Related Issues

Fixes #53688

Tests

Updated existing unit test to verify both old and new style tag format.

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

…st to support both

tvolkert · 2020-04-01T01:47:30Z

It might be simpler to try to parse it twice - once using the old regexp and one using the new. And then to add new fields to the GitTagVersion class for the dev.([0-9]+).([0-9]+) values (e.g. devRevision, patchNumber?)

christopherfujino · 2020-04-02T17:48:27Z

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

    }
  }

-  /// Attempts to reset to the last non-hotfix tag.


Removing this as from this point forward, upgrades will be possible with mere fast forward.

jonahwilliams · 2020-04-02T18:47:59Z

packages/flutter_tools/lib/src/version.dart

-  static GitTagVersion parse(String version) {
-    final RegExp versionPattern = RegExp(r'^v([0-9]+)\.([0-9]+)\.([0-9]+)(?:\+hotfix\.([0-9]+))?-([0-9]+)-g([a-f0-9]+)$');
+  /// Check for the release tag format pre-v1.17.0
+  static GitTagVersion parseFirstVersion(String version) {


Is there a point in time where it would be safe to remove the old tag parsing logic? If so, lets file an issue

Good idea, filed #53850 to track.

jonahwilliams · 2020-04-02T19:06:02Z

packages/flutter_tools/lib/src/version.dart

    }
-    final List<int> parsedParts = parts.take(5).map<int>((String source) => source == null ? null : int.tryParse(source)).toList();
-    return GitTagVersion(parsedParts[0], parsedParts[1], parsedParts[2], parsedParts[3], parsedParts[4], parts[5]);
+    final List<int> parsedParts = parts.take(6).map<int>((String source) => source == null ? null : int.tryParse(source)).toList();


why did this change to 6?

Oops, bad copy and paste, fixed.

jonahwilliams

LGTM

zanderso · 2020-04-02T21:09:03Z

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

+        <String>['git', 'tag', '-l', '*.*.*', '--sort=-creatordate'],
        throwOnError: true,
        workingDirectory: Cache.flutterRoot,
      );


Looking at the toolExit below. If git is not installed, I think the exception would be an ArgumentError and not a ProcessException.

should the action point be to remove the reference to git not being installed? I was gonna check if git canRun, but then i realize if they don't have git installed they'll never get to this point, as it would fail earlier.

in terms of how to handle the case of the user not having git installed, I'm planning to address this for #52262

Yeah, removing the reference to git not being installed sgtm.

zanderso · 2020-04-02T21:26:04Z

packages/flutter_tools/lib/src/version.dart

+  /// Check for the release tag format from v1.17.0 on
+  static GitTagVersion parseSecondVersion(String version) {
+    final RegExp versionPattern = RegExp(
+      r'^([0-9]+)\.([0-9]+)\.([0-9]+)(?:\+hotfix\.([0-9]+))?(-dev\.[0-9]+\.[0-9]+)?-([0-9]+)-g([a-f0-9]+)$');


Is it supposed to be possible to have both a +hotfix and a -dev?

you're right, i took out the hotfix from this regex pattern

zanderso · 2020-04-02T21:28:23Z

packages/flutter_tools/test/integration.shard/downgrade_upgrade_integration_test.dart

  });

  tearDown(() {
+    //print('Investigate ${parentDirectory.path}');


leftover debug print?

thanks, removed.

tvolkert · 2020-04-02T21:32:35Z

packages/flutter_tools/lib/src/version.dart

+  }
+
+  /// Check for the release tag format from v1.17.0 on
+  static GitTagVersion parseSecondVersion(String version) {


Naming bikeshed: maybe call this parseVersion() and the other one parseLegacyVersion()? Or feel free to ignore so you don't have to start tests over again just for a naming commit.

I had to kick the tests off again anyway, so I applied this change.

christopherfujino · 2020-04-02T23:40:28Z

Firebase test lab failure looks to be infra related, following up in infra chat.

* Support old and new git release tag formats (#53715) * Improve downgrade-upgrade integration test (#53775) * Fix diagnostics crash in profile mode (#53878) * Prevent diagnostics crash in profile mode * Prevent diagnostics crash in profile mode Co-authored-by: Christopher Fujino <[email protected]> Co-authored-by: Ferhat <[email protected]>

update git describe glob and regex to parse new tag format, update te…

84898ca

…st to support both

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

googlebot added the cla: yes label Apr 1, 2020

christopherfujino added 11 commits April 1, 2020 11:11

wip detect tag

240cc24

remove checking for hotfix == null; git ff fails, but test passes?

f29a3c2

add debugging

96208b4

more debugging

4b214af

check exit code

969e373

switch to dev

9162085

Merge branch 'minimal-upgrade-fix' into fix-new-version-tag

c0ec631

add support and tests for parsing both legacy and new tag versions

6840740

fix analyzer

724ca61

fix test

b059723

fix another test

82df27a

christopherfujino requested review from jmagman, jonahwilliams and zanderso April 2, 2020 17:38

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

deprecate git reset --hard functionality

b8fce16

christopherfujino commented Apr 2, 2020

View reviewed changes

remove override of deleted method

ca78368

fluttergithubbot added the work in progress; do not review label Apr 2, 2020

fix test

7859145

jonahwilliams reviewed Apr 2, 2020

View reviewed changes

have flutter version support old and new tag styles

c919fc6

christopherfujino mentioned this pull request Apr 2, 2020

Deprecate legacy release tag parsing logic #53850

Closed

add todo to remove parseFirstVersion()

4043723

jonahwilliams reviewed Apr 2, 2020

View reviewed changes

christopherfujino added 2 commits April 2, 2020 12:08

code review fix

6ca73fb

fix test by adding trim

b9b40ec

christopherfujino marked this pull request as ready for review April 2, 2020 19:40

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

add an instance of new version tag

3cdde17

jonahwilliams approved these changes Apr 2, 2020

View reviewed changes

Merge branch 'master' into fix-new-version-tag

6a4e3d0

zanderso reviewed Apr 2, 2020

View reviewed changes

tvolkert reviewed Apr 2, 2020

View reviewed changes

christopherfujino added 3 commits April 2, 2020 15:32

code review

367f552

code review

400b6d6

remove toolexit warning about git not being installed

79ea913

christopherfujino merged commit 2396616 into flutter:master Apr 3, 2020

christopherfujino deleted the fix-new-version-tag branch April 3, 2020 16:39

This was referenced Apr 3, 2020

Pull beta branch instead of master google/flutter.widgets#83

Merged

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

Closed

zanderso added the cp: 1.17 label Apr 6, 2020

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

Support old and new git release tag formats (flutter#53715)

9adf3be

pcsosinski mentioned this pull request Apr 6, 2020

flutter/flutter 1.17.0-dev.3.1 cherrypicks #54131

Merged

This was referenced Apr 7, 2020

Beta channel should report version including dev version and dev patch #54160

Closed

Fix frameworkVersionFor for flutter doctor and usage #54217

Merged

chunhtai mentioned this pull request Apr 8, 2020

[Web] broken gestures for mobile Safari #54269

Closed

pcsosinski removed the cp: 1.17 label Apr 9, 2020

christopherfujino mentioned this pull request Apr 24, 2020

[flutter_tools] enable flutter upgrade to support force pushed branches #55594

Merged

13 tasks

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

Conversation

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

Uh oh!

Description

Old Tags

New Tags

Related Issues

Tests

Checklist

Breaking Change

Uh oh!

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

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!

jonahwilliams left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

christopherfujino Apr 2, 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 2, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

christopherfujino commented Apr 1, 2020 •

edited

Loading

christopherfujino Apr 2, 2020 •

edited

Loading