diff --git a/.ci/scripts/analyze_legacy.sh b/.ci/scripts/analyze_legacy.sh index 391d520b4737..0a0ffcef5ea5 100755 --- a/.ci/scripts/analyze_legacy.sh +++ b/.ci/scripts/analyze_legacy.sh @@ -15,8 +15,7 @@ set -e # since only the packages changed by 'make-deps-path-based' need to be # re-checked. .ci/scripts/tool_runner.sh analyze --lib-only \ - --skip-if-not-supporting-flutter-version="$CHANNEL" \ - --custom-analysis=script/configs/custom_analysis.yaml + --skip-if-not-supporting-flutter-version="$CHANNEL" # Restore the tree to a clean state, to avoid accidental issues if # other script steps are added to the enclosing task. diff --git a/.ci/scripts/analyze_pathified.sh b/.ci/scripts/analyze_pathified.sh index 13939d8bd007..5cd98229d40a 100755 --- a/.ci/scripts/analyze_pathified.sh +++ b/.ci/scripts/analyze_pathified.sh @@ -17,7 +17,7 @@ set -e # failure regardless. dart ./script/tool/bin/flutter_plugin_tools.dart analyze --run-on-dirty-packages \ --skip-if-resolving-fails \ - --log-timing --custom-analysis=script/configs/custom_analysis.yaml + --log-timing # Restore the tree to a clean state, to avoid accidental issues if # other script steps are added to the enclosing task. git checkout . diff --git a/.ci/targets/analyze.yaml b/.ci/targets/analyze.yaml index 3d4f4716cff3..b2a393125b80 100644 --- a/.ci/targets/analyze.yaml +++ b/.ci/targets/analyze.yaml @@ -12,9 +12,7 @@ tasks: infra_step: true - name: analyze script: .ci/scripts/tool_runner.sh - # DO NOT change the custom-analysis argument here without changing the Dart repo. - # See the comment in script/configs/custom_analysis.yaml for details. - args: ["analyze", "--custom-analysis=script/configs/custom_analysis.yaml"] + args: ["analyze"] # Re-run analysis with path-based dependencies to ensure that publishing # the changes won't break analysis of other packages in the respository # that depend on it. diff --git a/.ci/targets/analyze_downgraded.yaml b/.ci/targets/analyze_downgraded.yaml index e4d1756665d4..2c0c46606364 100644 --- a/.ci/targets/analyze_downgraded.yaml +++ b/.ci/targets/analyze_downgraded.yaml @@ -7,4 +7,4 @@ tasks: # those APIs are introduced. - name: analyze - downgraded script: .ci/scripts/tool_runner.sh - args: ["analyze", "--downgrade", "--custom-analysis=script/configs/custom_analysis.yaml"] + args: ["analyze", "--downgrade"] diff --git a/packages/animations/ci_config.yaml b/packages/animations/ci_config.yaml new file mode 100644 index 000000000000..646cdfe9a3e2 --- /dev/null +++ b/packages/animations/ci_config.yaml @@ -0,0 +1,3 @@ +# Opts out of unawaited_futures, matching flutter/flutter's disabling due +# to interactions with animation. +allow_custom_analysis_options: true diff --git a/packages/camera/camera_android_camerax/ci_config.yaml b/packages/camera/camera_android_camerax/ci_config.yaml index c71f578cf0e6..4e65781cc456 100644 --- a/packages/camera/camera_android_camerax/ci_config.yaml +++ b/packages/camera/camera_android_camerax/ci_config.yaml @@ -1 +1,3 @@ analyze_skills: true +# Uses custom analysis to enable the cyclomatic complexity linter plugin. +allow_custom_analysis_options: true diff --git a/packages/cupertino_ui/ci_config.yaml b/packages/cupertino_ui/ci_config.yaml index d3c078296f34..bcc591cbbd47 100644 --- a/packages/cupertino_ui/ci_config.yaml +++ b/packages/cupertino_ui/ci_config.yaml @@ -2,3 +2,7 @@ exempt_from_excerpts: true release: batch: false + +# Temporarily ignore tests that have yet to have their cross imports fixed. +# See https://github.com/flutter/flutter/issues/177028. +allow_custom_analysis_options: true diff --git a/packages/flutter_lints/ci_config.yaml b/packages/flutter_lints/ci_config.yaml new file mode 100644 index 000000000000..64434b3e9556 --- /dev/null +++ b/packages/flutter_lints/ci_config.yaml @@ -0,0 +1,2 @@ +# Deliberately uses flutter_lints, as that's what it is demonstrating. +allow_custom_analysis_options: true diff --git a/packages/go_router/ci_config.yaml b/packages/go_router/ci_config.yaml index 5784c6eb1d56..a9786cb5cf55 100644 --- a/packages/go_router/ci_config.yaml +++ b/packages/go_router/ci_config.yaml @@ -1,2 +1,6 @@ release: batch: true + +# Has some test files that are intentionally broken to conduct dart fix tests. +# Also opts out of unawaited_futures, matching flutter/flutter. +allow_custom_analysis_options: true diff --git a/packages/go_router_builder/ci_config.yaml b/packages/go_router_builder/ci_config.yaml index f7160fb4571b..de0be5423080 100644 --- a/packages/go_router_builder/ci_config.yaml +++ b/packages/go_router_builder/ci_config.yaml @@ -1,3 +1,7 @@ release: # TODO(chunhtai): Opt in when ready. batch: false + +# Uses default formatter settings instead of custom repo settings since it +# serves as a golden for builder output tests. +allow_custom_analysis_options: true diff --git a/packages/material_ui/ci_config.yaml b/packages/material_ui/ci_config.yaml index d3c078296f34..bcc591cbbd47 100644 --- a/packages/material_ui/ci_config.yaml +++ b/packages/material_ui/ci_config.yaml @@ -2,3 +2,7 @@ exempt_from_excerpts: true release: batch: false + +# Temporarily ignore tests that have yet to have their cross imports fixed. +# See https://github.com/flutter/flutter/issues/177028. +allow_custom_analysis_options: true diff --git a/packages/rfw/ci_config.yaml b/packages/rfw/ci_config.yaml new file mode 100644 index 000000000000..d1cbe6ba2f2a --- /dev/null +++ b/packages/rfw/ci_config.yaml @@ -0,0 +1,2 @@ +# Has some constructions that are currently handled poorly by dart format. +allow_custom_analysis_options: true diff --git a/packages/web_benchmarks/ci_config.yaml b/packages/web_benchmarks/ci_config.yaml new file mode 100644 index 000000000000..c40b6aaf2a89 --- /dev/null +++ b/packages/web_benchmarks/ci_config.yaml @@ -0,0 +1,2 @@ +# Disables docs requirements, as it is test code. +allow_custom_analysis_options: true diff --git a/script/configs/custom_analysis.yaml b/script/configs/custom_analysis.yaml deleted file mode 100644 index b4539c78a0e1..000000000000 --- a/script/configs/custom_analysis.yaml +++ /dev/null @@ -1,39 +0,0 @@ -# Packages that deliberately use their own analysis_options.yaml. -# -# Please consult with #hackers-ecosystem before adding anything to this list, -# as the goal is to have consistent analysis options across all packages. -# -# All entries here shoud include a comment explaining why they use different -# options. - -# DO NOT move or delete this file without updating -# https://github.com/dart-lang/sdk/blob/main/tools/bots/flutter/analyze_flutter_packages.sh -# and -# https://github.com/flutter/flutter/blob/main/dev/bots/test.dart -# which reference this file from source, but out-of-repo. -# Contact stuartmorgan or devoncarew for assistance if necessary. - -# Opts out of unawaited_futures, matching flutter/flutter's disabling due -# to interactions with animation. -- animations -# Temporarily ignore tests that have yet to have their cross imports fixed. -# See https://github.com/flutter/flutter/issues/177028. -- cupertino_ui -# Deliberately uses flutter_lints, as that's what it is demonstrating. -- flutter_lints/example -# Has some test files that are intentionally broken to conduct dart fix tests. -# Also opts out of unawaited_futures, matching flutter/flutter. -- go_router -# Uses default formatter settings instead of custom repo settings since it -# serves as a golden for builder output tests. -- go_router_builder/example -# Temporarily ignore tests that have yet to have their cross imports fixed. -# See https://github.com/flutter/flutter/issues/177028. -- material_ui -# Has some constructions that are currently handled poorly by dart format. -- rfw/example -# Disables docs requirements, as it is test code. -- web_benchmarks/testing/test_app -# Uses custom analysis to enable the cyclomatic complexity linter plugin. -- camera/camera_android_camerax - diff --git a/script/tool/CHANGELOG.md b/script/tool/CHANGELOG.md index 55aeacaf445b..aecc2e551d93 100644 --- a/script/tool/CHANGELOG.md +++ b/script/tool/CHANGELOG.md @@ -1,3 +1,9 @@ +## NEXT + +* Deprecates the `--custom-analysis` argument to `analyze`, which is now + ignored, in favor of `allow_custom_analysis_options: true` in package-level + `ci_config.yaml` files. + ## 0.14.2 * Ensures that pub commands use `flutter` or `dart` depending on whether the diff --git a/script/tool/lib/src/analyze_command.dart b/script/tool/lib/src/analyze_command.dart index 9db7abd6fb5d..b1363a646ee0 100644 --- a/script/tool/lib/src/analyze_command.dart +++ b/script/tool/lib/src/analyze_command.dart @@ -30,14 +30,12 @@ class AnalyzeCommand extends PackageLoopingCommand { argParser.addFlag(platformMacOS, help: "Runs 'xcodebuild analyze' on macOS code"); // Dart options. + // TODO(stuartmorgan): Remove this entirely once the flutter/flutter and dart-lang repo scripts + // that run flutter/packages analysis have been updated not to pass it. argParser.addMultiOption( - _customAnalysisFlag, - help: - 'Directories (comma separated) that are allowed to have their own ' - 'analysis options.\n\n' - 'Alternately, a list of one or more YAML files that contain a list ' - 'of allowed directories.', - defaultsTo: [], + 'custom-analysis', + help: 'Ignored; exists for legacy compatibility only.', + hide: true, ); argParser.addOption( _analysisSdk, @@ -85,7 +83,6 @@ class AnalyzeCommand extends PackageLoopingCommand { } static const String _dartFlag = 'dart'; - static const String _customAnalysisFlag = 'custom-analysis'; static const String _downgradeFlag = 'downgrade'; static const String _libOnlyFlag = 'lib-only'; static const String _analysisSdk = 'analysis-sdk'; @@ -95,8 +92,6 @@ class AnalyzeCommand extends PackageLoopingCommand { late String _dartBinaryPath; - Set _allowedCustomAnalysisDirectories = const {}; - @override final String name = 'analyze'; @@ -105,8 +100,8 @@ class AnalyzeCommand extends PackageLoopingCommand { 'Analyzes all packages using dart analyze.\n\n' 'This command requires "dart" and "flutter" to be in your path.'; - /// Checks that there are no unexpected analysis_options.yaml files. - bool _hasUnexpectedAnalysisOptions(RepositoryPackage package) { + /// Checks that there are no package-local analysis_options.yaml files. + bool _hasLocalAnalysisOptions(RepositoryPackage package) { final List files = package.directory.listSync( recursive: true, followLinks: false, @@ -121,19 +116,12 @@ class AnalyzeCommand extends PackageLoopingCommand { continue; } - final bool allowed = _allowedCustomAnalysisDirectories.any( - (String directory) => - directory.isNotEmpty && - path.isWithin(packagesDir.childDirectory(directory).path, file.path), - ); - if (allowed) { - continue; - } - - printError('Found an extra analysis_options.yaml at ${file.absolute.path}.'); + printError('Found an unexpected analysis_options.yaml at ${file.absolute.path}.'); printError( - 'If this was deliberate, pass the package to the analyze command ' - 'with the --$_customAnalysisFlag flag and try again.', + 'If this is an intentional exception to the general repository guidance against having ' + 'custom analysis_options.yaml files, add a ci_config.yaml to this package that ' + 'sets "allow_custom_analysis_options: true", with a comment explaining why an exception ' + 'is needed.', ); return true; } @@ -170,8 +158,6 @@ class AnalyzeCommand extends PackageLoopingCommand { @override Future initializeRun() async { - _allowedCustomAnalysisDirectories = getYamlListArg(_customAnalysisFlag); - // Use the Dart SDK override if one was passed in. final dartSdk = argResults![_analysisSdk] as String?; _dartBinaryPath = dartSdk == null ? 'dart' : path.join(dartSdk, 'bin', 'dart'); @@ -303,7 +289,10 @@ class AnalyzeCommand extends PackageLoopingCommand { } } - if (_hasUnexpectedAnalysisOptions(package)) { + // Require an explicit opt-in to use custom analysis options, since it's very easy to + // accidentally introduce them to the repo (e.g., with 'flutter create'). + if (!(package.parseCIConfig()?.allowCustomAnalysisOptions ?? false) && + _hasLocalAnalysisOptions(package)) { return PackageResult.fail(['Unexpected local analysis options']); } final int mainExitCode = await processRunner.runAndStream(_dartBinaryPath, [ diff --git a/script/tool/lib/src/common/ci_config.dart b/script/tool/lib/src/common/ci_config.dart index f7c9ca34e641..8c2a368443ba 100644 --- a/script/tool/lib/src/common/ci_config.dart +++ b/script/tool/lib/src/common/ci_config.dart @@ -4,6 +4,12 @@ import 'package:yaml/yaml.dart'; +const String _releaseModeKey = 'release'; +const String _isBatchModeKey = 'batch'; +const String _exemptFromExcerptsKey = 'exempt_from_excerpts'; +const String _analyzeSkillsKey = 'analyze_skills'; +const String _allowCustomAnalysisOptionsKey = 'allow_custom_analysis_options'; + /// A class representing the parsed content of a `ci_config.yaml` file. class CIConfig { /// Creates a [CIConfig] from a parsed YAML map. @@ -11,6 +17,7 @@ class CIConfig { required this.isBatchRelease, required this.requiresExcerpts, required this.analyzeSkills, + required this.allowCustomAnalysisOptions, }); /// Parses a [CIConfig] from a YAML string. @@ -25,30 +32,34 @@ class CIConfig { _checkCIConfigEntries(loaded, syntax: _validCIConfigSyntax); var isBatchRelease = false; - final Object? release = loaded['release']; + final Object? release = loaded[_releaseModeKey]; if (release is Map) { - isBatchRelease = release['batch'] == true; + isBatchRelease = release[_isBatchModeKey] == true; } // Any package that hasn't been explicitly exempted is assumed to require // excerpts. - final requiresExcerpts = loaded['exempt_from_excerpts'] != true; + final requiresExcerpts = loaded[_exemptFromExcerptsKey] != true; + + final analyzeSkills = loaded[_analyzeSkillsKey] == true; - final analyzeSkills = loaded['analyze_skills'] == true; + final allowCustomAnalysisOptions = loaded[_allowCustomAnalysisOptionsKey] == true; return CIConfig._( isBatchRelease: isBatchRelease, requiresExcerpts: requiresExcerpts, analyzeSkills: analyzeSkills, + allowCustomAnalysisOptions: allowCustomAnalysisOptions, ); } static const Map _validCIConfigSyntax = { - 'release': { - 'batch': {true, false}, + _releaseModeKey: { + _isBatchModeKey: {true, false}, }, - 'exempt_from_excerpts': {true, false}, - 'analyze_skills': {true, false}, + _exemptFromExcerptsKey: {true, false}, + _analyzeSkillsKey: {true, false}, + _allowCustomAnalysisOptionsKey: {true, false}, }; /// Returns true if the package is configured for batch release. @@ -60,6 +71,9 @@ class CIConfig { /// Returns true if the package has its agent skills analyzed. final bool analyzeSkills; + /// Returns true if the package is allowed to have its own analysis options. + final bool allowCustomAnalysisOptions; + static void _checkCIConfigEntries( YamlMap config, { required Map syntax, diff --git a/script/tool/test/analyze_command_test.dart b/script/tool/test/analyze_command_test.dart index 57124c2ebaa5..6f1dacbbac26 100644 --- a/script/tool/test/analyze_command_test.dart +++ b/script/tool/test/analyze_command_test.dart @@ -315,7 +315,7 @@ void main() { }); group('verifies analysis settings', () { - test('fails analysis_options.yaml', () async { + test('fails for unexpected analysis_options.yaml', () async { createFakePlugin('foo', packagesDir, extraFiles: ['analysis_options.yaml']); Error? commandError; @@ -331,7 +331,9 @@ void main() { expect( output, containsAllInOrder([ - contains('Found an extra analysis_options.yaml at /packages/foo/analysis_options.yaml'), + contains( + 'Found an unexpected analysis_options.yaml at /packages/foo/analysis_options.yaml', + ), contains( ' foo:\n' ' Unexpected local analysis options', @@ -340,7 +342,7 @@ void main() { ); }); - test('fails .analysis_options', () async { + test('fails on .analysis_options', () async { createFakePlugin('foo', packagesDir, extraFiles: ['.analysis_options']); Error? commandError; @@ -356,7 +358,9 @@ void main() { expect( output, containsAllInOrder([ - contains('Found an extra analysis_options.yaml at /packages/foo/.analysis_options'), + contains( + 'Found an unexpected analysis_options.yaml at /packages/foo/.analysis_options', + ), contains( ' foo:\n' ' Unexpected local analysis options', @@ -365,30 +369,13 @@ void main() { ); }); - test('takes an allow list', () async { - final RepositoryPackage plugin = createFakePlugin( - 'foo', - packagesDir, - extraFiles: ['analysis_options.yaml'], - ); - - await runCapturingPrint(runner, ['analyze', '--custom-analysis', 'foo']); - - expect( - processRunner.recordedCalls, - orderedEquals([ - ProcessCall('flutter', const ['pub', 'get'], plugin.path), - ProcessCall('dart', const ['analyze', '--fatal-infos'], plugin.path), - ]), - ); - }); - test('ignores analysis options in the plugin .symlinks directory', () async { final RepositoryPackage plugin = createFakePlugin( 'foo', packagesDir, extraFiles: ['analysis_options.yaml'], ); + plugin.ciConfigFile.writeAsStringSync('allow_custom_analysis_options: true'); final RepositoryPackage includingPackage = createFakePlugin('bar', packagesDir); // Simulate the local state of having built 'bar' if it includes 'foo'. includingPackage.directory @@ -397,19 +384,18 @@ void main() { .childLink('.symlinks') .createSync(plugin.directory.path, recursive: true); - await runCapturingPrint(runner, ['analyze', '--custom-analysis', 'foo']); + await runCapturingPrint(runner, ['analyze']); }); - test('takes an allow config file', () async { + test('allows analysis options when configured', () async { final RepositoryPackage plugin = createFakePlugin( 'foo', packagesDir, extraFiles: ['analysis_options.yaml'], ); - final File allowFile = packagesDir.childFile('custom.yaml'); - allowFile.writeAsStringSync('- foo'); + plugin.ciConfigFile.writeAsStringSync('allow_custom_analysis_options: true'); - await runCapturingPrint(runner, ['analyze', '--custom-analysis', allowFile.path]); + await runCapturingPrint(runner, ['analyze']); expect( processRunner.recordedCalls, @@ -419,27 +405,6 @@ void main() { ]), ); }); - - test('allows an empty config file', () async { - createFakePlugin('foo', packagesDir, extraFiles: ['analysis_options.yaml']); - final File allowFile = packagesDir.childFile('custom.yaml'); - allowFile.createSync(); - - await expectLater( - () => runCapturingPrint(runner, ['analyze', '--custom-analysis', allowFile.path]), - throwsA(isA()), - ); - }); - - // See: https://github.com/flutter/flutter/issues/78994 - test('takes an empty allow list', () async { - createFakePlugin('foo', packagesDir, extraFiles: ['analysis_options.yaml']); - - await expectLater( - () => runCapturingPrint(runner, ['analyze', '--custom-analysis', '']), - throwsA(isA()), - ); - }); }); group('dart_code_linter', () { @@ -654,6 +619,7 @@ void main() { isFlutter: true, ); _writeFakePubspecWithLinter(package, inDevDependencies: true); + package.ciConfigFile.writeAsStringSync('allow_custom_analysis_options: true'); package.directory.childFile('analysis_options.yaml').writeAsStringSync(''' dart_code_linter: metrics: @@ -670,7 +636,7 @@ dart_code_linter: Error? commandError; final List output = await runCapturingPrint( runner, - ['analyze', '--custom-analysis', 'a_package'], + ['analyze'], errorHandler: (Error e) { commandError = e; }, @@ -830,7 +796,7 @@ dart_code_linter: Error? commandError; final List output = await runCapturingPrint( runner, - ['analyze', '--custom-analysis', 'a_package'], + ['analyze'], errorHandler: (Error e) { commandError = e; }, @@ -966,13 +932,10 @@ dart_code_linter: // modify the script above, as it is run from source, but out-of-repo. // Contact stuartmorgan or devoncarew for assistance. test('Dart repo analyze command works', () async { - final RepositoryPackage plugin = createFakePlugin( - 'foo', - packagesDir, - extraFiles: ['analysis_options.yaml'], - ); + final RepositoryPackage plugin = createFakePlugin('foo', packagesDir); final File allowFile = packagesDir.childFile('custom.yaml'); - allowFile.writeAsStringSync('- foo'); + // Intentionally do not create the file; this ensures that removing the legacy file from this + // repository won't break the other callers. await runCapturingPrint(runner, [ // DO NOT change this call; see comment above.