[tool] Improve check version ci so that it enforces the version in CHANGELOG and pubspec matches. #3678

cyanglaz · 2021-03-04T21:04:46Z

check version ci always make sure the version in CHANGELOG and pubspec matches for all the plugins.

Pre-launch Checklist

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

cyanglaz · 2021-03-04T22:04:32Z

script/check_version.sh

+# Sets CHANGED_PACKAGE_LIST and CHANGED_PACKAGES
+check_changed_packages
+
+plugin_tools version-check --base_sha="$(get_branch_base_sha)"


This needs to be moved into the if block below before merging

script/incremental_build.sh

blasten · 2021-03-04T23:52:26Z

script/check_version.sh

+# if [[ "${#CHANGED_PACKAGE_LIST[@]}" != 0 ]]; then
+# fi


is this needed?

Yes, before merging, ill move the check into this if block so pre-submit only checks version for changed packages. (also add a block to run everything when on master

Same; I would much rather this be Dart.

blasten · 2021-03-04T23:55:26Z

script/tool/lib/src/version_check_command.dart

-        if (pubspec.publishTo == 'none') {
-          continue;
-        }


this can be handy if a plugin is a WIP state.

this makes testing very hard, because in testing, pubspecs are all created with publishTo = none. See: https://github.com/flutter/plugins/blob/master/script/tool/test/util.dart#L154-L156
So this check basically made tests imposable if we want to test with a pubspec that contains versions.

I was thinking even for WIP plugins, it is still ok to check to see if versions match.

Couldn't we change that test safeguard from none to some made up pub server that doesn't exist, so it wouldn't clash with real none entries?

stuartmorgan-g · 2021-03-08T17:27:27Z

.cirrus.yml

+    - name: version
+      script:
+        - flutter channel master
+        - ./script/check_version.sh


Let's fold this into the publishable task a new script line. There's some overhead to each job we spin up, and this is conceptually part of publishable IMO.

stuartmorgan-g · 2021-03-08T17:28:38Z

script/check_version.sh

+source "$SCRIPT_DIR/common.sh"
+
+# Sets CHANGED_PACKAGE_LIST and CHANGED_PACKAGES
+check_changed_packages


Can't we do this in Dart so we're not adding more logic to shell scripts that we need to deal with?

stuartmorgan-g · 2021-03-08T17:28:49Z

script/check_version.sh

+# if [[ "${#CHANGED_PACKAGE_LIST[@]}" != 0 ]]; then
+# fi


Same; I would much rather this be Dart.

stuartmorgan-g · 2021-03-08T17:29:37Z

script/tool/lib/src/common.dart

@@ -140,6 +142,13 @@ bool isLinuxPlugin(FileSystemEntity entity, FileSystem fileSystem) {
  return pluginSupportsPlatform(kLinux, entity, fileSystem);
 }

+/// Throws a [ToolExit] with `exitCode` and log the `errorMessage` in red.
+void ThrowsToolExit({@required String errorMessage, int exitCode = 1}) {


s/Throws/Throw/; this name sounds like a test function to validate that something throws a ToolExit.

done. Renamed it as PrintErrorAndExit

stuartmorgan-g · 2021-03-08T17:31:44Z

script/tool/lib/src/version_check_command.dart

-        if (pubspec.publishTo == 'none') {
-          continue;
-        }


Couldn't we change that test safeguard from none to some made up pub server that doesn't exist, so it wouldn't clash with real none entries?

stuartmorgan-g · 2021-03-08T17:33:21Z

script/tool/lib/src/version_check_command.dart

+    // get version from CHANGELOG
+    final File changelog = plugin.childFile('CHANGELOG.md');
+    final List<String> lines = changelog.readAsLinesSync();
+    final String firstLine = lines.first;


We should probably skip blank lines.

stuartmorgan-g · 2021-03-08T17:34:10Z

script/tool/lib/src/version_check_command.dart

+    }
+
+    if (fromPubspec != fromChangeLog) {
+      final String error = 'versions for $packageName in CHANGELOG.md and pubspec.yaml do not match.';


Please print the two versions here for debugging if there are issues (e.g., if some accidentally put something after the version on the CHANGELOG line, it might not be obvious why this fails)

stuartmorgan-g · 2021-03-08T17:35:37Z

script/tool/lib/src/version_check_command.dart

+    print('${plugin.basename} passed version check');
+  }
+
+  // ignore: missing_return


Isn't this a real bug you're suppressing, since you're only handling Exception?

stuartmorgan-g · 2021-03-08T17:36:30Z

script/tool/test/version_check_test.dart

@@ -196,6 +196,73 @@ void main() {
      expect(gitDirCommands[2].join(' '),
          equals('show HEAD:packages/plugin_platform_interface/pubspec.yaml'));
    });
+
+     test('Throws if first line in change log is empty', () async{


Why do we want to do this? Isn't an empty first line a harmless cosmetic thing that we should just ignore?

stuartmorgan-g · 2021-03-08T17:37:04Z

script/tool/test/version_check_test.dart

+          'No version check errors found!'
+        ]),
+      );
+    });
  });


Please include a test where the version matches an older entry in the CHANGELOG (an actual bug that's happened).

stuartmorgan-g

Looking good, just some small things left. Yay for less logic in bash! (And a clear path to migrate some of our other bash easily.)

.cirrus.yml

stuartmorgan-g · 2021-03-09T20:41:40Z

.cirrus.yml

@@ -8,6 +8,8 @@ task:
    memory: 16G
  env:
    INTEGRATION_TEST_PATH: "./packages/integration_test"
+    PLUGIN_TOOLS: "dart run ./script/tool/lib/src/main.dart"
+    BRANCH: "${BRANCH:-'$(git rev-parse --abbrev-ref HEAD)'}"


Isn't this just $CIRRUS_BRANCH?

stuartmorgan-g · 2021-03-09T20:49:15Z

script/tool/lib/src/common.dart

+        changedFilesCommand.stdout.toString().isEmpty) {
+      return <String>[];
+    }
+    final List<String> changedFiles = changedFilesCommand.stdout


changedFilesCommand.stdout.toString() is used three times; let's make it a local variable. (You could avoid the early return and its two checks by just setting that string with an ?? '' and then proceeding; an empty string fed to the logic below should give you an empty list).

stuartmorgan-g · 2021-03-09T20:50:35Z

script/tool/lib/src/common.dart

+      baseShaFromMergeBase = await baseGitDir
+          .runCommand(<String>['merge-base', 'FETCH_HEAD', 'HEAD']);
+    }
+    return (baseShaFromMergeBase.stdout as String).replaceAll('\n', '');


Why not trim()?

stuartmorgan-g · 2021-03-09T20:51:15Z

script/tool/lib/src/version_check_command.dart


  @override
  final String name = 'version-check';

  @override
  final String description =
-      'Checks if the versions of the plugins have been incremented per pub specification.\n\n'
+      'Checks if the versions of the plugins have been incremented per pub specification.\n'
+      'Also checks if the version in CHANGELOG matches the version in pubspec.\n\n'


s/the version/the latest version/

stuartmorgan-g · 2021-03-09T20:51:44Z

script/tool/lib/src/common.dart

@@ -140,6 +144,13 @@ bool isLinuxPlugin(FileSystemEntity entity, FileSystem fileSystem) {
  return pluginSupportsPlatform(kLinux, entity, fileSystem);
 }

+/// Throws a [ToolExit] with `exitCode` and log the `errorMessage` in red.
+void PrintErrorAndExit({@required String errorMessage, int exitCode = 1}) {


Why is this Print rather than print?

Haha, it is probably caused by context switching between CPP and dart. Will fix

stuartmorgan-g · 2021-03-09T20:55:13Z

script/tool/lib/src/version_check_command.dart

+    String firstLineWithText;
+    final Iterator iterator = lines.iterator;
+    while (iterator.moveNext()) {
+      final String currentStriptEmptySpaces =


How about just:

if ((iterator.current as String).trim()?.isNotEmpty == true) { ... }

?

stuartmorgan-g · 2021-03-09T20:56:52Z

script/tool/lib/src/version_check_command.dart

+    Version fromChangeLog = Version.parse(versionString);
+    if (fromChangeLog == null) {
+      final String error =
+          'Cannot find version on the first line of CHANGELOG.md';


This path isn't printing what it found and tried to parse (related to previous review comment).

cyanglaz

updated based on your review, PTAL @stuartmorgan

cyanglaz · 2021-03-09T21:12:04Z

script/tool/lib/src/common.dart

@@ -140,6 +144,13 @@ bool isLinuxPlugin(FileSystemEntity entity, FileSystem fileSystem) {
  return pluginSupportsPlatform(kLinux, entity, fileSystem);
 }

+/// Throws a [ToolExit] with `exitCode` and log the `errorMessage` in red.
+void PrintErrorAndExit({@required String errorMessage, int exitCode = 1}) {


Haha, it is probably caused by context switching between CPP and dart. Will fix

cyanglaz · 2021-03-09T21:37:09Z

script/tool/lib/src/common.dart

+        changedFilesCommand.stdout.toString().isEmpty) {
+      return <String>[];
+    }
+    final List<String> changedFiles = changedFilesCommand.stdout


cyanglaz · 2021-03-09T21:38:08Z

script/tool/lib/src/common.dart

+      baseShaFromMergeBase = await baseGitDir
+          .runCommand(<String>['merge-base', 'FETCH_HEAD', 'HEAD']);
+    }
+    return (baseShaFromMergeBase.stdout as String).replaceAll('\n', '');


cyanglaz · 2021-03-09T21:38:33Z

script/tool/lib/src/version_check_command.dart


  @override
  final String name = 'version-check';

  @override
  final String description =
-      'Checks if the versions of the plugins have been incremented per pub specification.\n\n'
+      'Checks if the versions of the plugins have been incremented per pub specification.\n'
+      'Also checks if the version in CHANGELOG matches the version in pubspec.\n\n'


cyanglaz · 2021-03-09T21:39:23Z

script/tool/lib/src/version_check_command.dart

+    String firstLineWithText;
+    final Iterator iterator = lines.iterator;
+    while (iterator.moveNext()) {
+      final String currentStriptEmptySpaces =


cyanglaz · 2021-03-09T21:40:32Z

script/tool/lib/src/version_check_command.dart

+    Version fromChangeLog = Version.parse(versionString);
+    if (fromChangeLog == null) {
+      final String error =
+          'Cannot find version on the first line of CHANGELOG.md';


stuartmorgan-g

LGTM

…on in CHANGELOG and pubspec matches. (flutter/plugins#3678)

…on in CHANGELOG and pubspec matches. (flutter/plugins#3678) (#77854)

* master: (49 commits) Prep for alignment with Flutter analysis options (flutter#3703) [google_maps_flutter_platform_interface] Mark constructors as const for ids (flutter#3718) [image_picker] Endorse image_picker_for_web (flutter#3717) Add missing licenses, and add a check (flutter#3720) [ci] Add libgcrypt to Docker image. (flutter#3711) Reorder the checkboxes in the PR template (flutter#3666) Re-endorse connectivity_for_web (flutter#3708) Fix missing declaration of windows' default_package (flutter#3705) Typos in comments (flutter#2846) Skip flutter upgrade for pod linting Cirrus task (flutter#3700) [cross_file] Delete. (flutter#3698) [tool] Improve check version ci so that it enforces the version in CHANGELOG and pubspec matches. (flutter#3678) Streamline CI setup, and reenable macOS credits (flutter#3697) [video_player] fixed misleading size and aspect ratio documentation (flutter#3668) [image_picker] Implemented 2860 and added Unit Test to test functionality (flutter#3685) [shared_preferences] Fix concurrent modification of the shared preferences on Android (flutter#3684) [extension_google_sign_in_as_googleapis_auth] Deleted. (flutter#3694) Skip pod lint tests (flutter#3692) [Video_Player] Remove the deprecated API reference. (flutter#3669) [google_sign_in] fix test(flutter#3690) ...

Chris Yang added 4 commits March 4, 2021 12:22

version check

b6eb46d

tests

7ae7c82

clea n

1918666

enable version-check

4618e63

google-cla bot added the cla: yes label Mar 4, 2021

Chris Yang added 8 commits March 4, 2021 13:05

remove todo

d7d03ef

version check for all plugins

98dd162

check version

8bd1bb0

remove other tests

7cac875

addtional log

0fecf79

chmod

e3a1bc5

try run all

d2a505c

update log

f8f1ed0

cyanglaz requested a review from stuartmorgan-g March 4, 2021 22:02

Chris Yang added 2 commits March 4, 2021 14:03

remove todo

e1f64e9

revert .cirrus change

351d467

cyanglaz commented Mar 4, 2021

View reviewed changes

blasten reviewed Mar 4, 2021

View reviewed changes

refactor throw tool exit

071ccfc

stuartmorgan-g suggested changes Mar 8, 2021

View reviewed changes

Chris Yang added 9 commits March 8, 2021 10:22

review 1

025a910

format

3e9e74e

allow empty lines

94566ee

get changed plugins

75c6e46

fix typo

697b4bf

rename file and bug fixes

744b7d1

auto get base sha and tests

30b8677

remove sh script and add the script to cirrus

3e0ccc6

add log to cirrus

848b67f

Chris Yang added 4 commits March 9, 2021 12:29

add run-on-changed-packages option

6469c21

fix cirrus

19253b6

fix typo

6c47afa

revert unwanted changes

0bd32d2

cyanglaz requested a review from stuartmorgan-g March 9, 2021 20:35

stuartmorgan-g suggested changes Mar 9, 2021

View reviewed changes

Chris Yang added 2 commits March 9, 2021 13:01

set branch

02de427

add export

f1314f6

cyanglaz commented Mar 9, 2021

View reviewed changes

cyanglaz force-pushed the check_version branch from 4ad0e9e to f97251a Compare March 9, 2021 23:48

review

e2c3a8e

cyanglaz force-pushed the check_version branch from f97251a to e2c3a8e Compare March 9, 2021 23:49

cyanglaz requested a review from stuartmorgan-g March 9, 2021 23:52

Chris Yang added 3 commits March 9, 2021 16:12

fix directory

7ece242

use cirrus branch

6cd6f84

remove testing echo

bc83daf

stuartmorgan-g approved these changes Mar 10, 2021

View reviewed changes

revert device_info change

9cfc0b9

cyanglaz added waiting for tree to go green (Use "autosubmit") This PR is approved and tested, but waiting for the tree to be green to land. and removed p: device_info labels Mar 10, 2021

Chris Yang added 4 commits March 10, 2021 11:25

Merge branch 'master' into check_version

2bd1bfc

merge master

4f3e800

fix merge error

1ff0046

remove unnecessary channel swtich

fa5240e

fluttergithubbot merged commit 8feb2e7 into flutter:master Mar 10, 2021

cyanglaz deleted the check_version branch March 10, 2021 22:15

engine-flutter-autoroll mentioned this pull request Mar 10, 2021

Roll Plugins from 79dd06a1eb1d to 8feb2e78070f (1 revision) flutter/flutter#77854

Merged

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

8feb2e7 [tool] Improve check version ci so that it enforces the versi…

8e227ff

…on in CHANGELOG and pubspec matches. (flutter/plugins#3678)

fluttergithubbot pushed a commit to flutter/flutter that referenced this pull request Mar 11, 2021

8feb2e7 [tool] Improve check version ci so that it enforces the versi…

a640c14

…on in CHANGELOG and pubspec matches. (flutter/plugins#3678) (#77854)

[tool] Improve check version ci so that it enforces the version in CHANGELOG and pubspec matches. #3678

[tool] Improve check version ci so that it enforces the version in CHANGELOG and pubspec matches. #3678

Conversation

cyanglaz commented Mar 4, 2021 • edited Loading

Pre-launch Checklist

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

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

Choose a reason for hiding this comment

stuartmorgan-g left a 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

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

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

stuartmorgan-g left a comment

Choose a reason for hiding this comment

cyanglaz commented Mar 4, 2021 •

edited

Loading