From df0f42318422a94e8e09a5bc26fa5aeae82b45f6 Mon Sep 17 00:00:00 2001 From: stuartmorgan Date: Mon, 11 Nov 2024 14:55:30 -0500 Subject: [PATCH] [tools] Run `pub get` before `format` (#8052) The new Dart formatter needs to know the Dart language version of the code it is formatting, and it reads that from a file in `.dart_tool`, not `pubspec.yaml` directly. To avoid it failing to determine the version and assuming the latest (which will almost always be wrong in this repo): - Adds a step to the `format` repo command to ensure that `pub get` appears to have been run, and runs it if not, and - To avoid `pub get` running in `format` in CI, adds a deps-fetching step to the `repo_checks` task, as we have in other tasks that need to `pub get`. This should unblock the roll. --- .ci/targets/repo_checks.yaml | 7 ++ script/tool/lib/src/format_command.dart | 10 +++ .../tool/lib/src/license_check_command.dart | 1 + script/tool/test/format_command_test.dart | 89 +++++++++++++++---- .../tool/test/license_check_command_test.dart | 1 + 5 files changed, 90 insertions(+), 18 deletions(-) diff --git a/.ci/targets/repo_checks.yaml b/.ci/targets/repo_checks.yaml index be48143733..090548718e 100644 --- a/.ci/targets/repo_checks.yaml +++ b/.ci/targets/repo_checks.yaml @@ -2,6 +2,13 @@ tasks: - name: prepare tool script: .ci/scripts/prepare_tool.sh infra_step: true # Note infra steps failing prevents "always" from running. + # format requires that 'pub get' has been run to determine the language + # version, so the tool will auto-run it if necessary. Run it manually first + # so that the network requests are in a separate infra step. + - name: download Dart deps + script: .ci/scripts/tool_runner.sh + args: ["fetch-deps"] + infra_step: true - name: tool unit tests script: .ci/scripts/plugin_tools_tests.sh - name: tool format diff --git a/script/tool/lib/src/format_command.dart b/script/tool/lib/src/format_command.dart index f93f221245..a646cbfde5 100644 --- a/script/tool/lib/src/format_command.dart +++ b/script/tool/lib/src/format_command.dart @@ -12,6 +12,7 @@ import 'package:meta/meta.dart'; import 'common/core.dart'; import 'common/output_utils.dart'; import 'common/package_looping_command.dart'; +import 'common/pub_utils.dart'; import 'common/repository_package.dart'; /// In theory this should be 8191, but in practice that was still resulting in @@ -120,6 +121,15 @@ class FormatCommand extends PackageLoopingCommand { relativeTo: package.directory, ); if (getBoolArg(_dartArg)) { + // Ensure that .dart_tool exists, since without it `dart` doesn't know + // the lanugage version, so the formatter may give different output. + if (!package.directory.childDirectory('.dart_tool').existsSync()) { + if (!await runPubGet(package, processRunner, super.platform)) { + printError('Unable to fetch dependencies.'); + return PackageResult.fail(['unable to fetch dependencies']); + } + } + await _formatDart(files, workingDir: package.directory); } // Success or failure is determined overall in completeRun, since most code diff --git a/script/tool/lib/src/license_check_command.dart b/script/tool/lib/src/license_check_command.dart index 5c401b0918..223735ccbc 100644 --- a/script/tool/lib/src/license_check_command.dart +++ b/script/tool/lib/src/license_check_command.dart @@ -29,6 +29,7 @@ const Set _ignoreBasenameList = { 'flutter_export_environment', 'GeneratedPluginRegistrant', 'generated_plugin_registrant', + 'web_plugin_registrant', }; // File suffixes that otherwise match _codeFileExtensions to ignore. diff --git a/script/tool/test/format_command_test.dart b/script/tool/test/format_command_test.dart index b2c82378a7..670cc0c158 100644 --- a/script/tool/test/format_command_test.dart +++ b/script/tool/test/format_command_test.dart @@ -50,6 +50,12 @@ void main() { runner.addCommand(analyzeCommand); }); + /// Creates the .dart_tool directory for [package] to simulate (as much as + /// this command requires) `pub get` having been run. + void fakePubGet(RepositoryPackage package) { + package.directory.childDirectory('.dart_tool').createSync(); + } + /// Returns a modified version of a list of [relativePaths] that are relative /// to [package] to instead be relative to [packagesDir]. List getPackagesDirRelativePaths( @@ -92,6 +98,7 @@ void main() { packagesDir, extraFiles: files, ); + fakePubGet(plugin); await runCapturingPrint(runner, ['format']); @@ -117,6 +124,7 @@ void main() { unformattedFile, ], ); + fakePubGet(plugin); final p.Context posixContext = p.posix; childFileWithSubcomponents( @@ -140,7 +148,9 @@ void main() { 'lib/src/b.dart', 'lib/src/c.dart', ]; - createFakePlugin('a_plugin', packagesDir, extraFiles: files); + final RepositoryPackage plugin = + createFakePlugin('a_plugin', packagesDir, extraFiles: files); + fakePubGet(plugin); processRunner.mockProcessesForExecutable['dart'] = [ FakeProcessInfo(MockProcess(exitCode: 1), ['format']) @@ -163,7 +173,9 @@ void main() { const List files = [ 'lib/a.dart', ]; - createFakePlugin('a_plugin', packagesDir, extraFiles: files); + final RepositoryPackage plugin = + createFakePlugin('a_plugin', packagesDir, extraFiles: files); + fakePubGet(plugin); await runCapturingPrint(runner, ['format', '--no-dart']); expect(processRunner.recordedCalls, orderedEquals([])); @@ -179,6 +191,7 @@ void main() { packagesDir, extraFiles: files, ); + fakePubGet(plugin); await runCapturingPrint(runner, ['format']); @@ -203,7 +216,9 @@ void main() { 'android/src/main/java/io/flutter/plugins/a_plugin/a.java', 'android/src/main/java/io/flutter/plugins/a_plugin/b.java', ]; - createFakePlugin('a_plugin', packagesDir, extraFiles: files); + final RepositoryPackage plugin = + createFakePlugin('a_plugin', packagesDir, extraFiles: files); + fakePubGet(plugin); processRunner.mockProcessesForExecutable['java'] = [ FakeProcessInfo(MockProcess(exitCode: 1), ['-version']) @@ -229,7 +244,9 @@ void main() { 'android/src/main/java/io/flutter/plugins/a_plugin/a.java', 'android/src/main/java/io/flutter/plugins/a_plugin/b.java', ]; - createFakePlugin('a_plugin', packagesDir, extraFiles: files); + final RepositoryPackage plugin = + createFakePlugin('a_plugin', packagesDir, extraFiles: files); + fakePubGet(plugin); processRunner.mockProcessesForExecutable['java'] = [ FakeProcessInfo( @@ -260,6 +277,7 @@ void main() { packagesDir, extraFiles: files, ); + fakePubGet(plugin); await runCapturingPrint( runner, ['format', '--java-path=/path/to/java']); @@ -284,7 +302,9 @@ void main() { const List files = [ 'android/src/main/java/io/flutter/plugins/a_plugin/a.java', ]; - createFakePlugin('a_plugin', packagesDir, extraFiles: files); + final RepositoryPackage plugin = + createFakePlugin('a_plugin', packagesDir, extraFiles: files); + fakePubGet(plugin); await runCapturingPrint(runner, ['format', '--no-java']); expect(processRunner.recordedCalls, orderedEquals([])); @@ -304,6 +324,7 @@ void main() { packagesDir, extraFiles: files, ); + fakePubGet(plugin); await runCapturingPrint(runner, ['format']); @@ -328,7 +349,9 @@ void main() { 'linux/foo_plugin.cc', 'macos/Classes/Foo.h', ]; - createFakePlugin('a_plugin', packagesDir, extraFiles: files); + final RepositoryPackage plugin = + createFakePlugin('a_plugin', packagesDir, extraFiles: files); + fakePubGet(plugin); processRunner.mockProcessesForExecutable['clang-format'] = [FakeProcessInfo(MockProcess(exitCode: 1))]; @@ -357,6 +380,7 @@ void main() { packagesDir, extraFiles: files, ); + fakePubGet(plugin); processRunner.mockProcessesForExecutable['clang-format'] = [FakeProcessInfo(MockProcess(exitCode: 1))]; @@ -396,6 +420,7 @@ void main() { packagesDir, extraFiles: files, ); + fakePubGet(plugin); await runCapturingPrint(runner, ['format', '--clang-format-path=/path/to/clang-format']); @@ -421,7 +446,9 @@ void main() { 'linux/foo_plugin.cc', 'macos/Classes/Foo.h', ]; - createFakePlugin('a_plugin', packagesDir, extraFiles: files); + final RepositoryPackage plugin = + createFakePlugin('a_plugin', packagesDir, extraFiles: files); + fakePubGet(plugin); processRunner.mockProcessesForExecutable['clang-format'] = [ @@ -448,7 +475,9 @@ void main() { const List files = [ 'linux/foo_plugin.cc', ]; - createFakePlugin('a_plugin', packagesDir, extraFiles: files); + final RepositoryPackage plugin = + createFakePlugin('a_plugin', packagesDir, extraFiles: files); + fakePubGet(plugin); await runCapturingPrint(runner, ['format', '--no-clang-format']); expect(processRunner.recordedCalls, orderedEquals([])); @@ -465,6 +494,7 @@ void main() { packagesDir, extraFiles: files, ); + fakePubGet(plugin); await runCapturingPrint(runner, ['format']); @@ -488,7 +518,9 @@ void main() { 'android/src/main/kotlin/io/flutter/plugins/a_plugin/a.kt', 'android/src/main/kotlin/io/flutter/plugins/a_plugin/b.kt', ]; - createFakePlugin('a_plugin', packagesDir, extraFiles: files); + final RepositoryPackage plugin = + createFakePlugin('a_plugin', packagesDir, extraFiles: files); + fakePubGet(plugin); processRunner.mockProcessesForExecutable['java'] = [ FakeProcessInfo( @@ -513,7 +545,9 @@ void main() { const List files = [ 'android/src/main/kotlin/io/flutter/plugins/a_plugin/a.kt', ]; - createFakePlugin('a_plugin', packagesDir, extraFiles: files); + final RepositoryPackage plugin = + createFakePlugin('a_plugin', packagesDir, extraFiles: files); + fakePubGet(plugin); await runCapturingPrint(runner, ['format', '--no-kotlin']); expect(processRunner.recordedCalls, orderedEquals([])); @@ -530,6 +564,7 @@ void main() { packagesDir, extraFiles: files, ); + fakePubGet(plugin); await runCapturingPrint(runner, [ 'format', @@ -567,11 +602,12 @@ void main() { const List files = [ 'macos/foo.swift', ]; - createFakePlugin( + final RepositoryPackage plugin = createFakePlugin( 'a_plugin', packagesDir, extraFiles: files, ); + fakePubGet(plugin); await runCapturingPrint(runner, ['format', '--no-swift']); @@ -583,7 +619,9 @@ void main() { const List files = [ 'macos/foo.swift', ]; - createFakePlugin('a_plugin', packagesDir, extraFiles: files); + final RepositoryPackage plugin = + createFakePlugin('a_plugin', packagesDir, extraFiles: files); + fakePubGet(plugin); processRunner.mockProcessesForExecutable['swift-format'] = [ @@ -609,7 +647,9 @@ void main() { const List files = [ 'macos/foo.swift', ]; - createFakePlugin('a_plugin', packagesDir, extraFiles: files); + final RepositoryPackage plugin = + createFakePlugin('a_plugin', packagesDir, extraFiles: files); + fakePubGet(plugin); processRunner.mockProcessesForExecutable['swift-format'] = [ @@ -643,7 +683,9 @@ void main() { const List files = [ 'macos/foo.swift', ]; - createFakePlugin('a_plugin', packagesDir, extraFiles: files); + final RepositoryPackage plugin = + createFakePlugin('a_plugin', packagesDir, extraFiles: files); + fakePubGet(plugin); processRunner.mockProcessesForExecutable['swift-format'] = [ @@ -694,6 +736,7 @@ void main() { ...javaFiles, ], ); + fakePubGet(plugin); await runCapturingPrint(runner, ['format']); @@ -732,6 +775,7 @@ void main() { 'example/macos/Flutter/GeneratedPluginRegistrant.swift', ], ); + fakePubGet(plugin); await runCapturingPrint(runner, [ 'format', @@ -773,7 +817,9 @@ void main() { 'linux/foo_plugin.cc', 'macos/Classes/Foo.h', ]; - createFakePlugin('a_plugin', packagesDir, extraFiles: files); + final RepositoryPackage plugin = + createFakePlugin('a_plugin', packagesDir, extraFiles: files); + fakePubGet(plugin); const String changedFilePath = 'packages/a_plugin/linux/foo_plugin.cc'; processRunner.mockProcessesForExecutable['git'] = [ @@ -825,7 +871,9 @@ void main() { 'linux/foo_plugin.cc', 'macos/Classes/Foo.h', ]; - createFakePlugin('a_plugin', packagesDir, extraFiles: files); + final RepositoryPackage plugin = + createFakePlugin('a_plugin', packagesDir, extraFiles: files); + fakePubGet(plugin); processRunner.mockProcessesForExecutable['git'] = [ FakeProcessInfo(MockProcess(exitCode: 1), ['ls-files']) @@ -850,7 +898,9 @@ void main() { 'linux/foo_plugin.cc', 'macos/Classes/Foo.h', ]; - createFakePlugin('a_plugin', packagesDir, extraFiles: files); + final RepositoryPackage plugin = + createFakePlugin('a_plugin', packagesDir, extraFiles: files); + fakePubGet(plugin); const String changedFilePath = 'packages/a_plugin/linux/foo_plugin.cc'; processRunner.mockProcessesForExecutable['git'] = [ @@ -892,6 +942,7 @@ void main() { packagesDir, extraFiles: [...batch1, extraFile], ); + fakePubGet(package); await runCapturingPrint(runner, ['format']); @@ -922,11 +973,12 @@ void main() { // Make the file list one file longer than would fit in a Windows batch. final List batch = get99CharacterPathExtraFiles(batchSize + 1); - createFakePlugin( + final RepositoryPackage plugin = createFakePlugin( pluginName, packagesDir, extraFiles: batch, ); + fakePubGet(plugin); await runCapturingPrint(runner, ['format']); @@ -947,6 +999,7 @@ void main() { packagesDir, extraFiles: [...batch1, extraFile], ); + fakePubGet(package); await runCapturingPrint(runner, ['format']); diff --git a/script/tool/test/license_check_command_test.dart b/script/tool/test/license_check_command_test.dart index bfd12cac57..aee3b3a195 100644 --- a/script/tool/test/license_check_command_test.dart +++ b/script/tool/test/license_check_command_test.dart @@ -115,6 +115,7 @@ void main() { 'GeneratedPluginRegistrant.m', 'generated_plugin_registrant.cc', 'generated_plugin_registrant.cpp', + 'web_plugin_registrant.dart', // Ignored path suffixes. 'foo.g.dart', 'foo.mocks.dart',