[flutter_tools] Add --verify-only flag to flutter upgrade by royarg02 · Pull Request #68866 · flutter/flutter

royarg02 · 2020-10-23T09:29:47Z

Description

This PR adds a new flag for flutter upgrade, --verify-only, which will only check for and inform available updates, if any.

Adding this flag to flutter upgrade will not fetch updates.

$ flutter upgrade --verify-only
A new version of Flutter is available on channel <user channel>

The latest revision: <update version>
Your current version: <user installed version>

To upgrade now, run "flutter upgrade".
$

Moreover, if the user is on stable channel, it displays an url to see the release notes/changelog:

See the announcement and release notes:
https://flutter.dev/docs/development/tools/sdk/release-notes

Related Issues

Fixes #67532.

Tests

I added the following tests:

Test to check whether the upgrade message is printed with correct user and update versions of flutter.

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.

royarg02 · 2020-10-23T09:59:55Z

Some tests are failing on my local fork, mostly in the commands.shard/permeable/analyze_once_test.dart and commands.shard/hermetic/run_test.dart files.

anurag@manjaro flutter_tools[flutterupgradeverifyonly]$ ../../bin/cache/dart-sdk/bin/pub run test
Precompiling executable... (36.2s)
Precompiled test:test.
03:46 +273: test/commands.shard/hermetic/run_test.dart: run run app fails when targeted device is not Android with --device-user
Expected: throws (<Instance of 'ToolExit'> and satisfies function)
  Actual: <Instance of 'Future<void>'>
   Which: threw StdoutException:<StdoutException: unspecified mock value>
...
03:46 +273 -1: test/commands.shard/hermetic/run_test.dart: run run app fails when targeted device is not Android with --device-user [E]
  Expected: throws (<Instance of 'ToolExit'> and satisfies function)
    Actual: <Instance of 'Future<void>'>
     Which: threw StdoutException:<StdoutException: unspecified mock value>
...
03:46 +275 -1: test/commands.shard/hermetic/run_test.dart: run run app passes device target platform to usage          
Expected: match 'screenView {cd3: false, cd4: ios, cd22: iOS 13, cd23: debug, cd18: false, cd15: swift, cd31: false, cd47: false, cd33: .*, viewName: run'
  Actual: 'analytics: screenView {cd3: false, cd4: ios, cd22: iOS 13, cd23: debug, cd18: false, cd15: swift, cd31: true, cd47: false, cd33: 2020-10-08 00:00:00.000 +0530, viewName: run}\n'
            'analytics: event {category: tool-command-result, action: run, label: fail, value: null, cd33: 2020-10-08 00:00:00.000 +0530}\n'
            'analytics: event {category: tool-command-max-rss, action: run, label: fail, value: 938954752, cd33: 2020-10-08 00:00:00.000 +0530}\n'
            'analytics: timing {variableName: run, time: 189, category: flutter, label: fail}\n'
            ''
...
03:46 +275 -2: test/commands.shard/hermetic/run_test.dart: run run app passes device target platform to usage [E]      
  Expected: match 'screenView {cd3: false, cd4: ios, cd22: iOS 13, cd23: debug, cd18: false, cd15: swift, cd31: false, cd47: false, cd33: .*, viewName: run'
    Actual: 'analytics: screenView {cd3: false, cd4: ios, cd22: iOS 13, cd23: debug, cd18: false, cd15: swift, cd31: true, cd47: false, cd33: 2020-10-08 00:00:00.000 +0530, viewName: run}\n'
              'analytics: event {category: tool-command-result, action: run, label: fail, value: null, cd33: 2020-10-08 00:00:00.000 +0530}\n'
              'analytics: event {category: tool-command-max-rss, action: run, label: fail, value: 938954752, cd33: 2020-10-08 00:00:00.000 +0530}\n'
              'analytics: timing {variableName: run, time: 189, category: flutter, label: fail}\n'
              ''
...
04:59 +302 -2: test/commands.shard/permeable/analyze_once_test.dart: working directory with errors                     
Expected: contains 'warning • The parameter \'onPressed\' is required'
  Actual: 'Analyzing flutter_project...\n'
            '\n'
            '   info • Avoid empty else statements • ../../../../../../tmp/flutter_tools.CRMWVI/flutter_analyze_once_test_1.FBTVJA/flutter_project/lib/main.dart:8:48 • avoid_empty_else\n'
            '   info • Avoid empty statements • ../../../../../../tmp/flutter_tools.CRMWVI/flutter_analyze_once_test_1.FBTVJA/flutter_project/lib/main.dart:8:48 • empty_statements\n'
            '   info • The declaration \'_incrementCounter\' isn\'t referenced • ../../../../../../tmp/flutter_tools.CRMWVI/flutter_analyze_once_test_1.FBTVJA/flutter_project/lib/main.dart:30:8 • unused_element\n'
            '\x1B[33mwarning\x1B[39m • The parameter \'onPressed\' is required • ../../../../../../tmp/flutter_tools.CRMWVI/flutter_analyze_once_test_1.FBTVJA/flutter_project/lib/main.dart:56:29 • missing_required_param\n'
            '\n'
            ''
...
04:59 +302 -3: test/commands.shard/permeable/analyze_once_test.dart: working directory with errors [E]                 
  Expected: contains 'warning • The parameter \'onPressed\' is required'
    Actual: 'Analyzing flutter_project...\n'
              '\n'
              '   info • Avoid empty else statements • ../../../../../../tmp/flutter_tools.CRMWVI/flutter_analyze_once_test_1.FBTVJA/flutter_project/lib/main.dart:8:48 • avoid_empty_else\n'
              '   info • Avoid empty statements • ../../../../../../tmp/flutter_tools.CRMWVI/flutter_analyze_once_test_1.FBTVJA/flutter_project/lib/main.dart:8:48 • empty_statements\n'
              '   info • The declaration \'_incrementCounter\' isn\'t referenced • ../../../../../../tmp/flutter_tools.CRMWVI/flutter_analyze_once_test_1.FBTVJA/flutter_project/lib/main.dart:30:8 • unused_element\n'
              '\x1B[33mwarning\x1B[39m • The parameter \'onPressed\' is required • ../../../../../../tmp/flutter_tools.CRMWVI/flutter_analyze_once_test_1.FBTVJA/flutter_project/lib/main.dart:56:29 • missing_required_param\n'
              '\n'
              ''
...
05:11 +305 -3: test/commands.shard/permeable/analyze_once_test.dart: working directory with local options              
Expected: contains 'warning • The parameter \'onPressed\' is required'
  Actual: 'Analyzing flutter_project...\n'
            '\n'
            '   info • Avoid empty else statements • ../../../../../../tmp/flutter_tools.CRMWVI/flutter_analyze_once_test_1.FBTVJA/flutter_project/lib/main.dart:8:48 • avoid_empty_else\n'
            '   info • Avoid empty statements • ../../../../../../tmp/flutter_tools.CRMWVI/flutter_analyze_once_test_1.FBTVJA/flutter_project/lib/main.dart:8:48 • empty_statements\n'
            '   info • The declaration \'_incrementCounter\' isn\'t referenced • ../../../../../../tmp/flutter_tools.CRMWVI/flutter_analyze_once_test_1.FBTVJA/flutter_project/lib/main.dart:30:8 • unused_element\n'
            '\x1B[33mwarning\x1B[39m • The parameter \'onPressed\' is required • ../../../../../../tmp/flutter_tools.CRMWVI/flutter_analyze_once_test_1.FBTVJA/flutter_project/lib/main.dart:56:29 • missing_required_param\n'
            '\n'
            'Analyzing flutter_project...\n'
            '\n'
            '   info • The declaration \'_incrementCounter\' isn\'t referenced • ../../../../../../tmp/flutter_tools.YFXQIU/flutter_analyze_once_test_1.WTOLQR/flutter_project/lib/main.dart:30:8 • unused_element\n'
            '   info • Only throw instances of classes extending either Exception or Error • ../../../../../../tmp/flutter_tools.YFXQIU/flutter_analyze_once_test_1.WTOLQR/flutter_project/lib/main.dart:32:25 • only_throw_errors\n'
            '\x1B[33mwarning\x1B[39m • The parameter \'onPressed\' is required • ../../../../../../tmp/flutter_tools.YFXQIU/flutter_analyze_once_test_1.WTOLQR/flutter_project/lib/main.dart:56:29 • missing_required_param\n'
            '\n'
            ''
05:11 +305 -4: test/commands.shard/permeable/analyze_once_test.dart: working directory with local options [E]          
  Expected: contains 'warning • The parameter \'onPressed\' is required'
    Actual: 'Analyzing flutter_project...\n'
              '\n'
              '   info • Avoid empty else statements • ../../../../../../tmp/flutter_tools.CRMWVI/flutter_analyze_once_test_1.FBTVJA/flutter_project/lib/main.dart:8:48 • avoid_empty_else\n'
              '   info • Avoid empty statements • ../../../../../../tmp/flutter_tools.CRMWVI/flutter_analyze_once_test_1.FBTVJA/flutter_project/lib/main.dart:8:48 • empty_statements\n'
              '   info • The declaration \'_incrementCounter\' isn\'t referenced • ../../../../../../tmp/flutter_tools.CRMWVI/flutter_analyze_once_test_1.FBTVJA/flutter_project/lib/main.dart:30:8 • unused_element\n'
              '\x1B[33mwarning\x1B[39m • The parameter \'onPressed\' is required • ../../../../../../tmp/flutter_tools.CRMWVI/flutter_analyze_once_test_1.FBTVJA/flutter_project/lib/main.dart:56:29 • missing_required_param\n'
              '\n'
              'Analyzing flutter_project...\n'
              '\n'
              '   info • The declaration \'_incrementCounter\' isn\'t referenced • ../../../../../../tmp/flutter_tools.YFXQIU/flutter_analyze_once_test_1.WTOLQR/flutter_project/lib/main.dart:30:8 • unused_element\n'
              '   info • Only throw instances of classes extending either Exception or Error • ../../../../../../tmp/flutter_tools.YFXQIU/flutter_analyze_once_test_1.WTOLQR/flutter_project/lib/main.dart:32:25 • only_throw_errors\n'
              '\x1B[33mwarning\x1B[39m • The parameter \'onPressed\' is required • ../../../../../../tmp/flutter_tools.YFXQIU/flutter_analyze_once_test_1.WTOLQR/flutter_project/lib/main.dart:56:29 • missing_required_param\n'
              '\n'
              ''
...

Switching to master and rerunning tests yields the same result, even in a cloned repo.

I'm happy to provide a complete log if you require.

christopherfujino · 2020-10-23T17:12:21Z

Switching to master and rerunning tests yields the same result, even in a cloned repo.

I'm happy to provide a complete log if you require.

I wouldn't worry too much about it, since tests are passing on CI. Some of these tests need careful environment setup to pass. I generally just run the tests for files I directly changed locally, and rely on CI to run the rest of them.

Thanks for putting this out! I'll review it today.

royarg02 · 2020-10-23T18:31:14Z

I wouldn't worry too much about it, since tests are passing on CI. Some of these tests need careful environment setup to pass. I generally just run the tests for files I directly changed locally, and rely on CI to run the rest of them.

That's great! Let me update the checklist accordingly.

royarg02 · 2020-10-24T04:50:23Z

I suppose we could include the changelog here, but it is only available for the stable version; and displaying git log output will not be something the users would want to see just to check updates.

christopherfujino · 2020-10-26T16:29:23Z

I'm sorry I didn't review this last week. I will DEFINITELY review today!

christopherfujino · 2020-10-26T16:30:26Z

I suppose we could include the changelog here, but it is only available for the stable version; and displaying git log output will not be something the users would want to see just to check updates.

I agree, having a changelog would be great, but I think it would have to involve changing the way the team tracks changes, in order to dynamically present them in a machine-parseable way.

christopherfujino · 2020-10-26T16:31:08Z

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

      globals.printStatus('Flutter is already up to date on channel ${flutterVersion.channel}');
      globals.printStatus('$flutterVersion');
      return;
+    }else if(verifyOnly){


nit: there should be a space before else.

Sure, fixing it now.

royarg02 · 2020-10-26T16:43:32Z

I suppose we could include the changelog here, but it is only available for the stable version; and displaying git log output will not be something the users would want to see just to check updates.

On second thought, we could use a check whether the user is on the stable channel, and provide the changelog url, either from flutter.dev or the wiki.

christopherfujino · 2020-10-26T16:48:53Z

On second thought, we could use a check whether the user is on the stable channel, and provide the changelog url, either from flutter.dev or the wiki.

That's a good idea. I think you should use this link: https://flutter.dev/docs/development/tools/sdk/release-notes. It doesn't look to me like the actual URLs to the releases is stable, and the github wiki is not consistently maintained.

christopherfujino

Thanks, this is a really good change! I made one request that modify the information that we provide.

christopherfujino · 2020-10-26T17:13:46Z

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

      globals.printStatus('$flutterVersion');
      return;
+    } else if(verifyOnly) {
+      globals.printStatus('A new version of Flutter is available!\n');


Can you update this to:

globals.printStatus('A new version of Flutter is available on channel ${flutterVersion.channel}\n'); // TODO(fujino): use a [FlutterVersion] once that class supports arbitrary revisions. globals.printStatus('The latest revision: $upstreamRevision\n', emphasis: true); globals.printStatus('Your current version: ${flutterVersion.frameworkRevision}');

Looks like you'll have to update test expectations if you make this change (sorry).

Can you update this to:

Sure, will do that!

Looks like you'll have to update test expectations if you make this change (sorry).

I'm not quite sure that I follow.

Oh I get that now, I would have to change the tests, my bad.

No problem! I just wanted to save you the time in case you missed it, pushed it up to GitHub, and found out 30 minutes later that CI is failing. Because that's basically the story of my life :)

christopherfujino · 2020-10-26T17:58:14Z

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

+      globals.printStatus('Installed: ${flutterVersion.frameworkRevision}');
+      globals.printStatus('Available: $upstreamRevision\n', emphasis: true);
+      globals.printStatus('To upgrade now, run "flutter upgrade".');
+      if(flutterVersion.channel == 'stable'){


nit: if (flutterVersion.channel == 'stable') {

christopherfujino

LGTM, thanks for this contribution!

royarg02 · 2020-10-26T19:05:50Z

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

+    } else if (verifyOnly) {
+      globals.printStatus('A new version of Flutter is available on channel ${flutterVersion.channel}\n');
+      // TODO(fujino): use a [FlutterVersion] once that class supports arbitrary revisions.
+      globals.printStatus('The latest revision: $upstreamRevision\n', emphasis: true);


I think the \n has incorrectly remained on line 118.

@christopherfujino

fluttergithubbot · 2020-10-26T20:47:04Z

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

The status or check suite Windows build_tests has failed. Please fix the issues identified (or deflake) before re-applying this label.

royarg02 added 2 commits October 23, 2020 12:36

Add --verify-only flag for flutter upgrade

86fc866

Add message mentioning flutter upgrade

3763f8f

flutter-dashboard bot added the tool Affects the "flutter" command-line tool. See also t: labels. label Oct 23, 2020

google-cla bot added the cla: yes label Oct 23, 2020

royarg02 changed the title ~~[flutter_tools] Add --verify-only flag for flutter upgrade~~ [flutter_tools] Add --verify-only flag to flutter upgrade Oct 23, 2020

royarg02 mentioned this pull request Oct 24, 2020

[flutter_tools] A new flutter update should show version info #68936

Open

christopherfujino reviewed Oct 26, 2020

View reviewed changes

Fix nit

c8edc6a

Add changelog url for stable channel

80a6cba

christopherfujino suggested changes Oct 26, 2020

View reviewed changes

Modify the upgrade message and tweak tests accordingly

3f7afa0

royarg02 requested a review from christopherfujino October 26, 2020 17:56

christopherfujino reviewed Oct 26, 2020

View reviewed changes

Fix nit#2

6abe877

royarg02 requested a review from christopherfujino October 26, 2020 18:16

christopherfujino approved these changes Oct 26, 2020

View reviewed changes

christopherfujino added the waiting for tree to go green label Oct 26, 2020

royarg02 commented Oct 26, 2020

View reviewed changes

Correct newline position in message

194557e

royarg02 requested a review from christopherfujino October 26, 2020 19:16

fluttergithubbot removed the waiting for tree to go green label Oct 26, 2020

christopherfujino merged commit cd690f9 into flutter:master Oct 27, 2020

This was referenced Oct 29, 2020

Add current version to the upgrade message of the Flutter tool #68421

Merged

[flutter_tools] flutter upgrade --verify-only shows commit hashes instead of framework version #69417

Closed

royarg02 deleted the flutterupgradeverifyonly branch March 4, 2021 04:47

Conversation

royarg02 commented Oct 23, 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!

royarg02 commented Oct 23, 2020

Uh oh!

christopherfujino commented Oct 23, 2020

Uh oh!

royarg02 commented Oct 23, 2020

Uh oh!

royarg02 commented Oct 24, 2020

Uh oh!

christopherfujino commented Oct 26, 2020

Uh oh!

christopherfujino commented Oct 26, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

royarg02 commented Oct 26, 2020

Uh oh!

christopherfujino commented Oct 26, 2020

Uh oh!

christopherfujino left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

christopherfujino Oct 26, 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!

christopherfujino left a comment

Choose a reason for hiding this comment

Uh oh!

royarg02 Oct 26, 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!

fluttergithubbot commented Oct 26, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

royarg02 commented Oct 23, 2020 •

edited

Loading

christopherfujino Oct 26, 2020 •

edited

Loading

royarg02 Oct 26, 2020 •

edited

Loading