[tools] Improves version-check logic (flutter#6354)

Improves the logic used to determine whether to require a version and/or CHANGELOG change:
- Removes the requirement that dev-only (e.g., test) changes update the CHANGELOG, since in practice we were essentially always overriding in that case.
- Adds file-level analysis of `build.gradle` files to determine whether they are only changing test dependencies.
- Improves the "is this a published example file" logic to better match pub.dev's logic, to fix some false positives and false negatives (e.g., `rfw`'s `example/<foo>/lib/main.dart` being considered published).

Removes the no-longer-necessary special-case handling of some Dependabot PRs, as well as the PR-description-based system it was built on (and that turned out not to be very useful due to the way `CIRRUS_CHANGE_MESSAGE` actually worked). `build.gradle` analysis should not cover all such cases, and without the need to hard-code them by package name.

.cirrus.yml

@@ -85,20 +85,16 @@ task:
         - cd script/tool
         - dart pub run test
     - name: publishable
-      env:
-        CHANGE_DESC: "$TMPDIR/change-description.txt"
       version_check_script:
-        # For pre-submit, pass the PR label, as well as the PR description or
-        # incremental commit message (for Dependabot checks), to the script to
-        # allow for version check overrides.
+        # For pre-submit, pass the PR labels to the script to allow for version
+        # check overrides.
         # For post-submit, ignore platform version breaking version changes and
-        # missing version/CHANGELOG detection since the overrides aren't
+        # missing version/CHANGELOG detection since the labels aren't
         # available outside of the context of the PR.
         - if [[ $CIRRUS_PR == "" ]]; then
         -   ./script/tool_runner.sh version-check --ignore-platform-interface-breaks
         - else
-        -   echo "$CIRRUS_CHANGE_MESSAGE" > "$CHANGE_DESC"
-        -   ./script/tool_runner.sh version-check --check-for-missing-changes --change-description-file="$CHANGE_DESC" --pr-labels="$CIRRUS_PR_LABELS"
+        -   ./script/tool_runner.sh version-check --check-for-missing-changes --pr-labels="$CIRRUS_PR_LABELS"
         - fi
       publish_check_script: ./script/tool_runner.sh publish-check
     - name: format

script/tool/CHANGELOG.md

@@ -1,3 +1,13 @@
+## 0.10.0
+
+* Improves the logic in `version-check` to determine what changes don't require
+  version changes, as well as making any dev-only changes also not require
+  changelog changes since in practice we almost always override the check in
+  that case.
+* Removes special-case handling of Dependabot PRs, and the (fragile)
+  `--change-description-file` flag was only still used for that case, as
+  the improved diff analysis now handles that case more robustly.
+
 ## 0.9.3
 
 * Raises minimum `compileSdkVersion` to 32 for the `all-plugins-app` command.

script/tool/lib/src/common/git_version_finder.dart

@@ -50,6 +50,27 @@ class GitVersionFinder {
     return changedFiles.toList();
   }
 
+  /// Get a list of all the changed files.
+  Future<List<String>> getDiffContents({
+    String? targetPath,
+    bool includeUncommitted = false,
+  }) async {
+    final String baseSha = await getBaseSha();
+    final io.ProcessResult diffCommand = await baseGitDir.runCommand(<String>[
+      'diff',
+      baseSha,
+      if (!includeUncommitted) 'HEAD',
+      if (targetPath != null) ...<String>['--', targetPath],
+    ]);
+    final String diffStdout = diffCommand.stdout.toString();
+    if (diffStdout.isEmpty) {
+      return <String>[];
+    }
+    final List<String> changedFiles = diffStdout.split('\n')
+      ..removeWhere((String element) => element.isEmpty);
+    return changedFiles.toList();
+  }
+
   /// Get the package version specified in the pubspec file in `pubspecPath` and
   /// at the revision of `gitRef` (defaulting to the base if not provided).
   Future<Version?> getPackageVersion(String pubspecPath,

script/tool/lib/src/common/package_state_utils.dart

@@ -2,6 +2,8 @@
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
+import 'package:file/file.dart';
+import 'package:flutter_plugin_tools/src/common/git_version_finder.dart';
 import 'package:meta/meta.dart';
 import 'package:path/path.dart' as p;
 
@@ -14,6 +16,7 @@ class PackageChangeState {
   const PackageChangeState({
     required this.hasChanges,
     required this.hasChangelogChange,
+    required this.needsChangelogChange,
     required this.needsVersionChange,
   });
 
@@ -26,6 +29,10 @@ class PackageChangeState {
   /// True if any changes in the package require a version change according
   /// to repository policy.
   final bool needsVersionChange;
+
+  /// True if any changes in the package require a CHANGELOG change according
+  /// to repository policy.
+  final bool needsChangelogChange;
 }
 
 /// Checks [package] against [changedPaths] to determine what changes it has
@@ -38,18 +45,23 @@ class PackageChangeState {
 /// and `getRelativePosixPath(package.directory, gitDir.path)` respectively;
 /// they are arguments mainly to allow for caching the changed paths for an
 /// entire command run.
-PackageChangeState checkPackageChangeState(
+///
+/// If [git] is provided, [changedPaths] must be repository-relative
+/// paths, and change type detection can use file diffs in addition to paths.
+Future<PackageChangeState> checkPackageChangeState(
   RepositoryPackage package, {
   required List<String> changedPaths,
   required String relativePackagePath,
-}) {
+  GitVersionFinder? git,
+}) async {
   final String packagePrefix = relativePackagePath.endsWith('/')
       ? relativePackagePath
       : '$relativePackagePath/';
 
   bool hasChanges = false;
   bool hasChangelogChange = false;
   bool needsVersionChange = false;
+  bool needsChangelogChange = false;
   for (final String path in changedPaths) {
     // Only consider files within the package.
     if (!path.startsWith(packagePrefix)) {
@@ -62,34 +74,131 @@ PackageChangeState checkPackageChangeState(
     if (components.isEmpty) {
       continue;
     }
-    final bool isChangelog = components.first == 'CHANGELOG.md';
-    if (isChangelog) {
+
+    if (components.first == 'CHANGELOG.md') {
       hasChangelogChange = true;
+      continue;
     }
 
-    if (!needsVersionChange &&
-        !isChangelog &&
-        // One of a few special files example will be shown on pub.dev, but for
-        // anything else in the example publishing has no purpose.
-        !(components.first == 'example' &&
-            !<String>{'main.dart', 'readme.md', 'example.md'}
-                .contains(components.last.toLowerCase())) &&
-        // Changes to tests don't need to be published.
-        !components.contains('test') &&
-        !components.contains('androidTest') &&
-        !components.contains('RunnerTests') &&
-        !components.contains('RunnerUITests') &&
-        // The top-level "tool" directory is for non-client-facing utility code,
-        // so doesn't need to be published.
-        components.first != 'tool' &&
-        // Ignoring lints doesn't affect clients.
-        !components.contains('lint-baseline.xml')) {
-      needsVersionChange = true;
+    if (!needsVersionChange) {
+      // Developer-only changes don't need version changes or changelog changes.
+      if (await _isDevChange(components, git: git, repoPath: path)) {
+        continue;
+      }
+
+      // Some other changes don't need version changes, but might benefit from
+      // changelog changes.
+      needsChangelogChange = true;
+      if (
+          // One of a few special files example will be shown on pub.dev, but
+          // for anything else in the example publishing has no purpose.
+          !_isUnpublishedExampleChange(components, package)) {
+        needsVersionChange = true;
+      }
     }
   }
 
   return PackageChangeState(
       hasChanges: hasChanges,
       hasChangelogChange: hasChangelogChange,
+      needsChangelogChange: needsChangelogChange,
       needsVersionChange: needsVersionChange);
 }
+
+bool _isTestChange(List<String> pathComponents) {
+  return pathComponents.contains('test') ||
+      pathComponents.contains('androidTest') ||
+      pathComponents.contains('RunnerTests') ||
+      pathComponents.contains('RunnerUITests');
+}
+
+// True if the given file is an example file other than the one that will be
+// published according to https://dart.dev/tools/pub/package-layout#examples.
+//
+// This is not exhastive; it currently only handles variations we actually have
+// in our repositories.
+bool _isUnpublishedExampleChange(
+    List<String> pathComponents, RepositoryPackage package) {
+  if (pathComponents.first != 'example') {
+    return false;
+  }
+  final List<String> exampleComponents = pathComponents.sublist(1);
+  if (exampleComponents.isEmpty) {
+    return false;
+  }
+
+  final Directory exampleDirectory =
+      package.directory.childDirectory('example');
+
+  // Check for example.md/EXAMPLE.md first, as that has priority. If it's
+  // present, any other example file is unpublished.
+  final bool hasExampleMd =
+      exampleDirectory.childFile('example.md').existsSync() ||
+          exampleDirectory.childFile('EXAMPLE.md').existsSync();
+  if (hasExampleMd) {
+    return !(exampleComponents.length == 1 &&
+        exampleComponents.first.toLowerCase() == 'example.md');
+  }
+
+  // Most packages have an example/lib/main.dart (or occasionally
+  // example/main.dart), so check for that. The other naming variations aren't
+  // currently used.
+  const String mainName = 'main.dart';
+  final bool hasExampleCode =
+      exampleDirectory.childDirectory('lib').childFile(mainName).existsSync() ||
+          exampleDirectory.childFile(mainName).existsSync();
+  if (hasExampleCode) {
+    // If there is an example main, only that example file is published.
+    return !((exampleComponents.length == 1 &&
+            exampleComponents.first == mainName) ||
+        (exampleComponents.length == 2 &&
+            exampleComponents.first == 'lib' &&
+            exampleComponents[1] == mainName));
+  }
+
+  // If there's no example code either, the example README.md, if any, is the
+  // file that will be published.
+  return exampleComponents.first.toLowerCase() != 'readme.md';
+}
+
+// True if the change is only relevant to people working on the plugin.
+Future<bool> _isDevChange(List<String> pathComponents,
+    {GitVersionFinder? git, String? repoPath}) async {
+  return _isTestChange(pathComponents) ||
+      // The top-level "tool" directory is for non-client-facing utility
+      // code, such as test scripts.
+      pathComponents.first == 'tool' ||
+      // Ignoring lints doesn't affect clients.
+      pathComponents.contains('lint-baseline.xml') ||
+      await _isGradleTestDependencyChange(pathComponents,
+          git: git, repoPath: repoPath);
+}
+
+Future<bool> _isGradleTestDependencyChange(List<String> pathComponents,
+    {GitVersionFinder? git, String? repoPath}) async {
+  if (git == null) {
+    return false;
+  }
+  if (pathComponents.last != 'build.gradle') {
+    return false;
+  }
+  final List<String> diff = await git.getDiffContents(targetPath: repoPath);
+  final RegExp changeLine = RegExp(r'[+-] ');
+  final RegExp testDependencyLine =
+      RegExp(r'[+-]\s*(?:androidT|t)estImplementation\s');
+  bool foundTestDependencyChange = false;
+  for (final String line in diff) {
+    if (!changeLine.hasMatch(line) ||
+        line.startsWith('--- ') ||
+        line.startsWith('+++ ')) {
+      continue;
+    }
+    if (!testDependencyLine.hasMatch(line)) {
+      return false;
+    }
+    foundTestDependencyChange = true;
+  }
+  // Only return true if a test dependency change was found, as a failsafe
+  // against having the wrong (e.g., incorrectly empty) diff output.
+  return foundTestDependencyChange;
+}

script/tool/lib/src/update_release_info_command.dart

@@ -133,7 +133,7 @@ class UpdateReleaseInfoCommand extends PackageLoopingCommand {
           packagesDir.fileSystem.directory((await gitDir).path);
       final String relativePackagePath =
           getRelativePosixPath(package.directory, from: gitRoot);
-      final PackageChangeState state = checkPackageChangeState(package,
+      final PackageChangeState state = await checkPackageChangeState(package,
           changedPaths: _changedFiles,
           relativePackagePath: relativePackagePath);