Skip to content

Commit

Permalink
[tool] Skip pathified analysis on resolver errors (#4647)
Browse files Browse the repository at this point in the history
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 #4483 (comment) 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.)
  • Loading branch information
stuartmorgan authored Aug 5, 2023
1 parent 992422c commit d7ee75a
Show file tree
Hide file tree
Showing 3 changed files with 61 additions and 0 deletions.
6 changes: 6 additions & 0 deletions .ci/scripts/analyze_pathified.sh
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand Down
22 changes: 22 additions & 0 deletions script/tool/lib/src/analyze_command.dart
Original file line number Diff line number Diff line change
Expand Up @@ -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';

Expand Down Expand Up @@ -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;

Expand Down Expand Up @@ -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, <String>['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(<String>['Unable to get dependencies']);
}
}
Expand Down
33 changes: 33 additions & 0 deletions script/tool/test/analyze_command_test.dart
Original file line number Diff line number Diff line change
Expand Up @@ -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.'),
<String>['pub', 'get']);
processRunner.mockProcessesForExecutable['flutter'] = <FakeProcessInfo>[
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<String> output = await runCapturingPrint(
runner, <String>['analyze', '--skip-if-resolving-fails']);

expect(
output,
containsAllInOrder(<Matcher>[
contains('Skipping package due to pub resolution failure.'),
]),
);
expect(
processRunner.recordedCalls,
orderedEquals(<ProcessCall>[
ProcessCall('flutter', const <String>['pub', 'get'], plugin.path),
ProcessCall('flutter', const <String>['pub', 'get'], plugin.path),
]));
});

test('fails if "pub get" fails', () async {
createFakePlugin('foo', packagesDir);

Expand Down

0 comments on commit d7ee75a

Please sign in to comment.