Fix and update version checks #3792

stuartmorgan · 2021-04-07T01:03:36Z

Currently our version update checks aren't actually working; the script doesn't work correctly if no explicit --base-sha is passed, but that's always how CI is calling it.

Fixes flutter/flutter#79823 (and version checks in general)

This makes a number of changes:

Fixes it to work without --base-sha
Adds tests that it works in that mode
- And tightens existing tests to require ToolExit, not just any error, to reduce false-positive test success
Adds verbose logging of the checks being done, to make it easier to debug this kind of issue in the future
Tightens the exception handling for missing previous versions to just the line that's expected to fail in that case
Only allows missing versions when "publish_to: none" is set
- Adds that everywhere it's missing
- Standardize the format in the repo to "none" (instead of also having "'none'").
Allows the use of NEXT in CHANGELOG as a way of gathering changes that are worth noting, but not
doing a publish cycle for. (Replaces the plan of using -dev versions, since that's actually harder to implement,
and more confusing.)
- Ensures that we don't forget to clean up NEXT entries when bumping versions

Pre-launch Checklist

If you need help, consider asking for advice on the #hackers-new channel on Discord.

stuartmorgan · 2021-04-07T01:04:17Z

I imagine that now that this works, we'll run into things we need to relax or tweak over time, but getting it actually working is a good first step to getting it working the way we want :)

cyanglaz · 2021-04-07T02:47:57Z

script/tool/lib/src/version_check_command.dart

+      }
+      final Pubspec pubspec = Pubspec.parse(pubspecFile.readAsStringSync());
+      if (pubspec.publishTo == 'none') {
+        print('  Found "publish_to: none"; skipping.');


Is the indentation intended? I imagine you wanted the output look more organized?

Yes, this is instead of doing the ---------- between each section like the CHANGELOG check; it's more compact, and I think it's more readable.

Updated to use an indentation variable to make it clearer and ensure consistency.

cyanglaz · 2021-04-07T02:53:09Z

script/tool/lib/src/version_check_command.dart

      } on io.ProcessException {
-        print('Unable to find pubspec in master for $pubspecPath.'
-            ' Safe to ignore if the project is new.');
+        print('  Unable to find pubspec in master. '


Now looking at this code again. I think we should handle it in getPackageVersion. Then getPackageVersion would just return null if a version is not available. WDYT?

Yes, that makes way more sense; the type of exception here is entirely dependent on the implementation details of that method, which is odd. Fixed.

script/tool/lib/src/common.dart

cyanglaz · 2021-04-07T03:00:16Z

script/tool/lib/src/version_check_command.dart

+
+    // Skip validation for the special NEXT version that's used to accumulate
+    // changes that don't warrant publishing on their own.
+    if (versionString == 'NEXT') {


We also need to make sure that the version in pubspec equals to the next version after NEXT in CHANGELOG.

This is to prevent someone updated the version in pubspec but forgot to change the "NEXT"

If we only cares about presubmit, which looks like so in the current code (base-sha not configurable), we can easily do

if (versionString == 'NEXT' && pubspecVersion not changed)

Good call, added validation for that and added a test.

script/tool/lib/src/version_check_command.dart

cyanglaz · 2021-04-07T17:00:20Z

script/tool/lib/src/version_check_command.dart

+      // CHANGELOG. That means the next version entry in the CHANGELOG pass the
+      // normal validation.
+      while (iterator.moveNext()) {
+        if (iterator.current.trim().startsWith('## ')) {


some of our CHANGELOG.md files do no follow the standard format, see: https://github.com/flutter/plugins/blob/master/packages/url_launcher/url_launcher_web/CHANGELOG.md

This would fail. :(

I'm fine with a model where trying to use NEXT won't work until the format has been fixed to follow our standard format.

We can also do a one-time sweep if that becomes annoying.

Sounds good!

cyanglaz

LGTM

ditman

Looks good to me, but there's a small typo that slipped through?

script/tool/lib/src/version_check_command.dart

packages/path_provider/path_provider_macos/example/pubspec.yaml

Co-authored-by: David Iglesias <[email protected]>

ditman

format CI check is taking issue, a pass of dartfmt -w . should fix it!

* master: (397 commits) [in_app_purchase] Implementation of platform interface (flutter#3781) [google_sign_in] Add todo WRT correctly setting X-Goog-AuthUser header (flutter#3819) [tools] fix version check command not working for new packages (flutter#3818) [camera] android-rework part 1: Base classes to support Android Camera features (flutter#3795) fix MD (flutter#3815) Path provider windows crash fix (flutter#3814) [local_auth] docs update (flutter#3103) Update PULL_REQUEST_TEMPLATE.md (flutter#3801) [quick_actions] handle cold start on iOS correctly (flutter#3811) Replace path_provider_linux widget tests with simple unit tests (flutter#3812) [sensors] format dart code based on the new dart formatter (flutter#3809) [google_sign_in] Fix "pick account" on iOS (flutter#3805) [image_picker_platform_interface] Added pickMultiImage (flutter#3782) [in_app_purchase] Added currency code and numerical price to product detail model. (flutter#3794) [local_auth] Fix iOS crash when no localizedReason (flutter#3780) Fix and update version checks (flutter#3792) [in_app_purchase] Configured example app to use StoreKit Testing on iOS 14 (flutter#3772) [local_auth] Unnecessary reassignment in example removed (flutter#2983) [flutter_webview] Fix `allowsInlineMediaPlayback` ignored on iOS (flutter#3791) Switch script/tools over to the new analysis options (flutter#3777) ...

Currently our version update checks aren't actually working; the script doesn't work correctly if no explicit --base-sha is passed, but that's always how CI is calling it. Fixes flutter/flutter#79823 (and version checks in general) This makes a number of changes: - Fixes it to work without --base-sha - Adds tests that it works in that mode - And tightens existing tests to require ToolExit, not just any error, to reduce false-positive test success - Adds verbose logging of the checks being done, to make it easier to debug this kind of issue in the future - Tightens the exception handling for missing previous versions to just the line that's expected to fail in that case - Only allows missing versions when "publish_to: none" is set - Adds that everywhere it's missing - Standardize the format in the repo to "none" (instead of also having "'none'"). - Allows the use of NEXT in CHANGELOG as a way of gathering changes that are worth noting, but not doing a publish cycle for. (Replaces the plan of using -dev versions, since that's actually harder to implement, and more confusing.) - Ensures that we don't forget to clean up NEXT entries when bumping versions

stuartmorgan added 3 commits April 6, 2021 20:36

Add publish_to: none to all examples, and standardize format

93d66df

Update script

9fd0641

Add tests without base-sha

04cc372

stuartmorgan requested a review from cyanglaz April 7, 2021 01:03

stuartmorgan requested review from bparrishMines, ditman, gaaclarke and LHLL as code owners April 7, 2021 01:03

google-cla bot added the cla: yes label Apr 7, 2021

github-actions bot added p: shared_preferences p: url_launcher p: video_player p: webview_flutter Edits files for a webview_flutter plugin p: wifi_info_flutter platform-linux platform-macos platform-windows labels Apr 7, 2021

cyanglaz suggested changes Apr 7, 2021

View reviewed changes

Address review comments

f12de47

cyanglaz suggested changes Apr 7, 2021

View reviewed changes

cyanglaz approved these changes Apr 7, 2021

View reviewed changes

ditman suggested changes Apr 7, 2021

View reviewed changes

script/tool/lib/src/version_check_command.dart Show resolved Hide resolved

script/tool/lib/src/version_check_command.dart Show resolved Hide resolved

packages/path_provider/path_provider_macos/example/pubspec.yaml Outdated Show resolved Hide resolved

Fix typo

8de68b2

Co-authored-by: David Iglesias <[email protected]>

ditman approved these changes Apr 7, 2021

View reviewed changes

dartftm changes from Flutter update

fbe4c1a

github-actions bot added the platform-web label Apr 8, 2021

stuartmorgan merged commit 89ccc0e into flutter:master Apr 8, 2021

stuartmorgan deleted the version-check-additions branch April 8, 2021 20:22

github-actions bot added the needs-publishing label Apr 8, 2021

engine-flutter-autoroll mentioned this pull request Apr 8, 2021

Roll Plugins from d5ebaa536834 to 89ccc0eadb2c (1 revision) flutter/flutter#80077

Merged

engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Apr 8, 2021

89ccc0e Fix and update version checks (flutter/plugins#3792)

c3f33a3

fluttergithubbot pushed a commit to flutter/flutter that referenced this pull request Apr 8, 2021

89ccc0e Fix and update version checks (flutter/plugins#3792) (#80077)

0ee4656

cyanglaz removed the needs-publishing label Apr 20, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix and update version checks #3792

Fix and update version checks #3792

stuartmorgan commented Apr 7, 2021

stuartmorgan commented Apr 7, 2021

cyanglaz Apr 7, 2021

stuartmorgan Apr 7, 2021

stuartmorgan Apr 7, 2021

cyanglaz Apr 7, 2021

stuartmorgan Apr 7, 2021

cyanglaz Apr 7, 2021

cyanglaz Apr 7, 2021

stuartmorgan Apr 7, 2021

cyanglaz Apr 7, 2021

stuartmorgan Apr 7, 2021

cyanglaz Apr 7, 2021

cyanglaz left a comment

ditman left a comment

ditman left a comment

Fix and update version checks #3792

Fix and update version checks #3792

Conversation

stuartmorgan commented Apr 7, 2021

Pre-launch Checklist

stuartmorgan commented Apr 7, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cyanglaz left a comment

Choose a reason for hiding this comment

ditman left a comment

Choose a reason for hiding this comment

ditman left a comment

Choose a reason for hiding this comment