From d7ee75ad59ad7bc45e659d0599e935e9e7981ea1 Mon Sep 17 00:00:00 2001 From: stuartmorgan Date: Sat, 5 Aug 2023 07:36:33 -0700 Subject: [PATCH] [tool] Skip pathified analysis on resolver errors (#4647) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit If adding a pathified dependency creates a resolver error, then skip it instead of failing when running pathified analysis. The purpose of pathified analysis it to pre-detect failures that would happen on publishing, and if there's a resolver error that means the publishing even won't affect the package anyway. See https://github.com/flutter/packages/pull/4483#issuecomment-1664468621 for an example case where we need this. (In theory we could get delayed OOB errors that this will miss—e.g., in the case above if the PR would actually break rfw/example/wasm, then if at some later date `wasm` updated to use a newer `ffi`, eliminating the resolver conflict, then suddenly rfw/example/wasm would pick up the PR and break. That seems *extremely* unlikely, however, so I'm not concerned that this will be a problem in practice. We can revisit if that changes.) --- .ci/scripts/analyze_pathified.sh | 6 ++++ script/tool/lib/src/analyze_command.dart | 22 +++++++++++++++ script/tool/test/analyze_command_test.dart | 33 ++++++++++++++++++++++ 3 files changed, 61 insertions(+) diff --git a/.ci/scripts/analyze_pathified.sh b/.ci/scripts/analyze_pathified.sh index 2942cf7d57b8..01c4bd8cc17f 100755 --- a/.ci/scripts/analyze_pathified.sh +++ b/.ci/scripts/analyze_pathified.sh @@ -10,7 +10,13 @@ set -e # This uses --run-on-dirty-packages rather than --packages-for-branch # since only the packages changed by 'make-deps-path-based' need to be # re-checked. +# --skip-if-resolving-fails is used to avoid failing if there's a resolution +# conflict when using path-based dependencies, because that indicates that +# the failing packages won't pick up the new versions of the changed packages +# when they are published anyway, so publishing won't cause an out-of-band +# 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 # Restore the tree to a clean state, to avoid accidental issues if # other script steps are added to the enclosing task. diff --git a/script/tool/lib/src/analyze_command.dart b/script/tool/lib/src/analyze_command.dart index 225e60cbc3d7..15a8ef2e125b 100644 --- a/script/tool/lib/src/analyze_command.dart +++ b/script/tool/lib/src/analyze_command.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 'dart:io' as io; + import 'package:file/file.dart'; import 'package:yaml/yaml.dart'; @@ -34,12 +36,18 @@ class AnalyzeCommand extends PackageLoopingCommand { argParser.addFlag(_libOnlyFlag, help: 'Only analyze the lib/ directory of the main package, not the ' 'entire package.'); + argParser.addFlag(_skipIfResolvingFailsFlag, + help: 'If resolution fails, skip the package. This is only ' + 'intended to be used with pathified analysis, where a resolver ' + 'failure indicates that no out-of-band failure can result anyway.', + hide: true); } static const String _customAnalysisFlag = 'custom-analysis'; static const String _downgradeFlag = 'downgrade'; static const String _libOnlyFlag = 'lib-only'; static const String _analysisSdk = 'analysis-sdk'; + static const String _skipIfResolvingFailsFlag = 'skip-if-resolving-fails'; late String _dartBinaryPath; @@ -134,6 +142,20 @@ class AnalyzeCommand extends PackageLoopingCommand { .pubspecFile .existsSync()) { if (!await _runPubCommand(packageToGet, 'get')) { + if (getBoolArg(_skipIfResolvingFailsFlag)) { + // Re-run, capturing output, to see if the failure was a resolver + // failure. (This is slightly inefficient, but this should be a + // very rare case.) + const String resolverFailureMessage = 'version solving failed'; + final io.ProcessResult result = await processRunner.run( + flutterCommand, ['pub', 'get'], + workingDir: packageToGet.directory); + if ((result.stderr as String).contains(resolverFailureMessage) || + (result.stdout as String).contains(resolverFailureMessage)) { + logWarning('Skipping package due to pub resolution failure.'); + return PackageResult.skip('Resolution failed.'); + } + } return PackageResult.fail(['Unable to get dependencies']); } } diff --git a/script/tool/test/analyze_command_test.dart b/script/tool/test/analyze_command_test.dart index 37f3332d83a4..15da8db22cfb 100644 --- a/script/tool/test/analyze_command_test.dart +++ b/script/tool/test/analyze_command_test.dart @@ -315,6 +315,39 @@ void main() { }); }); + test('skips if requested if "pub get" fails in the resolver', () async { + final RepositoryPackage plugin = createFakePlugin('foo', packagesDir); + + final FakeProcessInfo failingPubGet = FakeProcessInfo( + MockProcess( + exitCode: 1, + stderr: 'So, because foo depends on both thing_one ^1.0.0 and ' + 'thing_two from path, version solving failed.'), + ['pub', 'get']); + processRunner.mockProcessesForExecutable['flutter'] = [ + failingPubGet, + // The command re-runs failures when --skip-if-resolver-fails is passed + // to check the output, so provide the same failing outcome. + failingPubGet, + ]; + + final List output = await runCapturingPrint( + runner, ['analyze', '--skip-if-resolving-fails']); + + expect( + output, + containsAllInOrder([ + contains('Skipping package due to pub resolution failure.'), + ]), + ); + expect( + processRunner.recordedCalls, + orderedEquals([ + ProcessCall('flutter', const ['pub', 'get'], plugin.path), + ProcessCall('flutter', const ['pub', 'get'], plugin.path), + ])); + }); + test('fails if "pub get" fails', () async { createFakePlugin('foo', packagesDir);