[All] Expand artifact hub to all plugins by reidbaker · Pull Request #4645 · flutter/packages

reidbaker · 2023-08-04T15:13:34Z

Adds artifact hub check to gradle command
Add tests for build.gradle and settings.gradle check
Update all example build.gradle and settings.gradle files

flutter/flutter/issues/120119

Expansion of #4567

Pre-launch Checklist

stuartmorgan-g

LGTM with some minor comments. I'm looking forward to the reduced flake :)

stuartmorgan-g · 2023-08-04T16:55:21Z

script/tool/lib/src/gradle_check_command.dart

  File _getBuildGradleFile(Directory dir) => dir.childFile('build.gradle');

+  // Returns the settings gradle file in the given directory.
+  File _getSettignsGradleFile(Directory dir) =>


Typo: Settings

stuartmorgan-g · 2023-08-04T16:56:09Z

script/tool/lib/src/gradle_check_command.dart

+    final RegExp keyPresentRegex =
+        RegExp("$keyVariable\\s+=\\s+'ARTIFACT_HUB_REPOSITORY'");
+    final RegExp documentationPresentRegex = RegExp(
+        r'github.com.*wiki.*Plugins-and-Packages-repository-structure.*gradle-structure');


Nit: github\.com

I get lint warnings when I use \ as unnecessary values in a string. I opted for the looser regex instead of ignoring the lint.

Maybe you need \\ even in a raw string then? I forget the rules around \ for raw strings.

I think it is actually a false warning where dart assumes \ is used to escape and it is warning me that I dont need to escape .

... Except I do for regular expressions.

https://dart.dev/tools/linter-rules/unnecessary_string_escapes

stuartmorgan-g · 2023-08-04T16:58:08Z

script/tool/lib/src/gradle_check_command.dart

+    final RegExp documentationPresentRegex = RegExp(
+        r'github.com.*wiki.*Plugins-and-Packages-repository-structure.*gradle-structure');
+    final RegExp keyReadRegex =
+        RegExp('if.*System.getenv.*containsKey.*$keyVariable');


Are the wildcards here just to avoid the readability hit of lots of \s?

yes and I got lint warnings.

Made the regex match a smaller set of inputs with \

stuartmorgan-g · 2023-08-04T16:58:20Z

script/tool/lib/src/gradle_check_command.dart

+    final RegExp documentationPresentRegex = RegExp(
+        r'github.com.*wiki.*Plugins-and-Packages-repository-structure.*gradle-structure');
+    final RegExp keyReadRegex =
+        RegExp('if.*System.getenv.*containsKey.*$keyVariable');


Nit: \.getenv

stuartmorgan-g · 2023-08-04T16:58:43Z

script/tool/lib/src/gradle_check_command.dart

+    final RegExp keyReadRegex =
+        RegExp('if.*System.getenv.*containsKey.*$keyVariable');
+    final RegExp keyUsedRegex =
+        RegExp('maven.*url.*System.getenv.*$keyVariable.*');


stuartmorgan-g · 2023-08-04T16:59:53Z

script/tool/lib/src/gradle_check_command.dart

+        gradleLines.any((String line) => keyUsedRegex.hasMatch(line));
+
+    if (!documentationPresent) {
+      printError('Does not link artifact hub documentation.');


This should print out the comment to add.

After seeing this output run on packages missing artifact hub implementation I removed documentation as a separate printed error since it is included in the combined string.

stuartmorgan-g · 2023-08-04T17:02:03Z

script/tool/lib/src/gradle_check_command.dart

+  /// configuration that enables artifact hub env variable.
+  @visibleForTesting
+  static String exampleRootSettingsArtifactHubString = '''
+// See https://github.com/flutter/flutter/wiki/Plugins-and-Packages-repository-structure#gradle-structure for more info.


Let's extract this URL to a constant that's used in both statics.

stuartmorgan-g · 2023-08-04T17:02:30Z

script/tool/lib/src/gradle_check_command.dart

+  bool _validateArtifactHubSettingsUsage(
+      RepositoryPackage example, List<String> gradleLines) {
+    final RegExp documentationPresentRegex = RegExp(
+        r'github.com.*wiki.*Plugins-and-Packages-repository-structure.*gradle-structure');


Same comments in these strings about escaping literal .s

stuartmorgan-g · 2023-08-04T17:02:50Z

script/tool/lib/src/gradle_check_command.dart

+        .any((String line) => artifactRegistryPluginApplyRegex.hasMatch(line));
+
+    if (!documentationPresent) {
+      printError('Does not link artifact hub documentation.');


As above, please include the link to add.

flutter/packages@d7ee75a...ac41376 2023-08-08 engine-flutter-autoroll@skia.org Roll Flutter from ad0aa8d to 436df69 (17 revisions) (flutter/packages#4663) 2023-08-08 10687576+bparrishMines@users.noreply.github.com [webview_flutter_wkwebview] Repeatedly pump WebViews until one is garbage collected (flutter/packages#4662) 2023-08-08 erikgerman917@gmail.com [xdg_directories] Add example app (flutter/packages#4554) 2023-08-08 70025277+Nitin-Poojary@users.noreply.github.com [pigeon] Recursively create output target files (flutter/packages#4458) 2023-08-07 jpnurmi@gmail.com [path_provider] Add getApplicationCachePath() (flutter/packages#4483) 2023-08-07 10687576+bparrishMines@users.noreply.github.com [flutter_markdown] Adopt code excerpts in README (flutter/packages#4656) 2023-08-07 reidbaker@google.com [All] Expand artifact hub to all plugins (flutter/packages#4645) 2023-08-07 engine-flutter-autoroll@skia.org Roll Flutter from 2ba9f7b to ad0aa8d (31 revisions) (flutter/packages#4659) If this roll has caused a breakage, revert this CL and stop the roller using the controls here: https://autoroll.skia.org/r/flutter-packages-flutter-autoroll Please CC flutter-ecosystem@google.com,rmistry@google.com on the revert to ensure that a human is aware of the problem. To file a bug in Flutter: https://github.com/flutter/flutter/issues/new/choose To report a problem with the AutoRoller itself, please file a bug: https://bugs.chromium.org/p/skia/issues/entry?template=Autoroller+Bug Documentation for the AutoRoller is here: https://skia.googlesource.com/buildbot/+doc/main/autoroll/README.md

reidbaker added 8 commits August 3, 2023 11:19

Adds artifact hub check to gradle command

2ef3551

passing tests

2519427

Verify regex against actual packages

05f76af

All camera packages use artifact hub

d7dd201

All passing tests

f01bad5

Add validation for settings.gradle but no tests

b916ecf

Add tests for settings.gradle check

b7e8bea

Update all example build.gradle and settings.gradle files

5d37c2a

reidbaker marked this pull request as ready for review August 4, 2023 15:28

reidbaker requested review from Hixie, Piinks, bparrishMines, camsim99, chunhtai, ditman, domesticmouse, gmackall, goderbauer, gspencergoog, stuartmorgan-g and tarrinneal as code owners August 4, 2023 15:28

stuartmorgan-g approved these changes Aug 4, 2023

View reviewed changes

reidbaker added 3 commits August 4, 2023 14:38

pr feedback

d82c744

code review feedback

a7dee37

Merge branch 'main' into i120119-expand-artifiact-hub-to-all-plugins

b1511d1

reidbaker added the autosubmit Merge PR when tree becomes green via auto submit App label Aug 7, 2023

auto-submit bot merged commit 6cb2a32 into flutter:main Aug 7, 2023

engine-flutter-autoroll mentioned this pull request Aug 8, 2023

Roll Packages from d7ee75ad59ad to 269118d2284e (7 revisions) flutter/flutter#132131

Closed

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

6cb2a32 [All] Expand artifact hub to all plugins (flutter/packages#4645)

8c7c9b7

engine-flutter-autoroll mentioned this pull request Aug 8, 2023

Roll Packages from d7ee75ad59ad to ac4137624a13 (8 revisions) flutter/flutter#132133

Merged

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

6cb2a32 [All] Expand artifact hub to all plugins (flutter/packages#4645)

35df659

reidbaker mentioned this pull request Sep 13, 2023

Refactor "app plugin loader" Gradle Plugin so it can be applied using the declarative plugins {} block flutter/flutter#127897

Merged

8 tasks

gmackall mentioned this pull request Oct 12, 2023

Mac module_custom_host_app_name_test is 2.02% flaky flutter/flutter#136327

Closed

camsim99 mentioned this pull request May 30, 2024

Gradle wrapper download failures should be classified as an infra flake. flutter/flutter#122031

Open

Comments

Conversation

reidbaker commented Aug 4, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Pre-launch Checklist

Uh oh!

stuartmorgan-g left a comment

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!

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!

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!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

reidbaker commented Aug 4, 2023 •

edited

Loading