diff --git a/README.md b/README.md index ee8b45b..52c77f6 100644 --- a/README.md +++ b/README.md @@ -94,7 +94,9 @@ with `--yams-name ` persists the full markdown report (not the budget-trimmed brief) to the yams corpus, tagged with the analysed commit and a generation timestamp; `recall --manifest --root ` locate the labelled-bug manifest and the -directory holding the repos it names, and `recall --strict` exits non-zero when a +directory holding the repos it names (without `--manifest`, `recall/manifest.json` +in the working directory, else the one in the pipe checkout the binary was built +from), and `recall --strict` exits non-zero when a case it expects to still find is missed. Every numeric flag rejects a value that is not a positive integer rather than falling back to its default. @@ -115,7 +117,9 @@ Output contract: `verify --expect-resolved`, `recall --strict`, `index --fail-on-error`, or `guide --yams` could not persist); 2 pipe could not do what was asked (a bad path or `--dir`, an unreadable project, a baseline that cannot be read or - written); 64 invalid usage. + written, or an internal error - rerun with `PIPE_DEBUG=1` for its stack + trace); 64 invalid usage, including more than one project path. + `guide --yams --write-baseline` still writes the baseline when yams fails. `pipe` finds the right SDK itself: it reads `flutterRoot` from the target's `.dart_tool/package_config.json`, then `FLUTTER_ROOT`, then `flutter` on `PATH`. diff --git a/bin/pipe.dart b/bin/pipe.dart index 4389825..92abf79 100644 --- a/bin/pipe.dart +++ b/bin/pipe.dart @@ -33,5 +33,16 @@ Future main(List arguments) async { stderr.writeln(error.message); stderr.writeln(error.usage); exitCode = 64; + } catch (error, stack) { + // Anything else is a pipe bug. Say so in one line with the documented + // "could not do what was asked" code instead of a raw trace and exit 255; + // PIPE_DEBUG=1 restores the trace for a bug report. + stderr.writeln('pipe: internal error: $error'); + if (Platform.environment['PIPE_DEBUG'] == '1') { + stderr.writeln(stack); + } else { + stderr.writeln('pipe: rerun with PIPE_DEBUG=1 for a stack trace'); + } + exitCode = 2; } } diff --git a/lib/src/cli.dart b/lib/src/cli.dart index d3e3e8a..b3294b2 100644 --- a/lib/src/cli.dart +++ b/lib/src/cli.dart @@ -122,7 +122,11 @@ void _writeBaseline(ArgResults results, String root, List findings) { final path = results['write-baseline'] as String?; if (path == null) return; try { - File(path).writeAsStringSync(encodeBaseline(root, findings)); + // Record the project root relative to the baseline file, so a committed + // baseline does not carry one machine's absolute path (and does not churn + // when another checkout rewrites it). Matching never reads `root`. + final relativeRoot = p.relative(root, from: p.dirname(p.absolute(path))); + File(path).writeAsStringSync(encodeBaseline(relativeRoot, findings)); } on FileSystemException catch (error) { throw PipeException( 'cannot write baseline $path: ' @@ -169,8 +173,50 @@ Set _readRules(ArgResults results) { return selected; } -String _target(ArgResults results) => - results.rest.isEmpty ? '.' : results.rest.first; +/// The one project path a command analyses, `.` when none is given. +/// +/// Extra positionals used to be dropped silently, so `pipe check a b` analysed +/// only `a` and reported as if both had been checked. +String _target(ArgResults results) { + final rest = results.rest; + if (rest.length > 1) { + throw UsageException( + 'expected one project path, got ${rest.length}: ${rest.join(' ')}', + 'Run "pipe help " for usage; run pipe once per project.', + ); + } + return rest.isEmpty ? '.' : rest.first; +} + +/// Where `recall` looks for its manifest when `--manifest` is not given. +/// +/// The default used to be `recall/manifest.json` relative to the working +/// directory, so `pipe recall` only worked from inside the pipe repo. Prefer that +/// path when it exists, otherwise search upward from the running script and +/// executable (the source checkout for `dart run`, the build bundle for the AOT +/// binary) for the repo's `recall/manifest.json`. +String defaultRecallManifest({String? cwd, List? searchFrom}) { + const relative = 'recall/manifest.json'; + final local = p.join(cwd ?? Directory.current.path, relative); + if (File(local).existsSync()) return local; + final starts = + searchFrom ?? + [ + if (Platform.script.scheme == 'file') Platform.script.toFilePath(), + Platform.resolvedExecutable, + ]; + for (final start in starts) { + var dir = p.dirname(p.absolute(start)); + while (true) { + final candidate = p.join(dir, relative); + if (File(candidate).existsSync()) return candidate; + final parent = p.dirname(dir); + if (parent == dir) break; + dir = parent; + } + } + return local; +} String _encode(Map json) => const JsonEncoder.withIndent(' ').convert(json); @@ -424,8 +470,9 @@ class RecallCommand extends Command { argParser ..addOption( 'manifest', - defaultsTo: 'recall/manifest.json', - help: 'Manifest of labelled known bugs.', + help: + 'Manifest of labelled known bugs (default: recall/manifest.json ' + 'in the working directory, else in the pipe checkout).', ) ..addOption( 'root', @@ -466,7 +513,7 @@ class RecallCommand extends Command { final results = argResults!; final root = (results['root'] as String?) ?? Directory.current.path; final report = await runRecall( - manifestPath: results['manifest'] as String, + manifestPath: (results['manifest'] as String?) ?? defaultRecallManifest(), root: root, sdkOverride: results['sdk'] as String?, concurrency: _readConcurrency(results), @@ -784,6 +831,9 @@ class GuideCommand extends Command { collection: 'pipe', ); if (error != null) { + // Write the baseline anyway: a yams outage must not also lose the + // accepted findings the caller asked to record. + _writeBaseline(results, project.root, analysis.report.findings); stderr.writeln('pipe: could not persist to yams: $error'); return 1; } diff --git a/lib/src/guide.dart b/lib/src/guide.dart index cac9971..4f55fc4 100644 --- a/lib/src/guide.dart +++ b/lib/src/guide.dart @@ -743,15 +743,20 @@ Future emitToYams({ if (collection != null) arguments.addAll(['--collection', collection]); final process = await Process.start(executable, arguments); + // Drain both pipes concurrently. Reading only stderr let a chatty `yams add` + // fill its stdout buffer and block forever. + final stdoutDone = process.stdout.drain(); + final stderrText = process.stderr + .transform(const SystemEncoding().decoder) + .join(); process.stdin.write(content); await process.stdin.flush(); await process.stdin.close(); - final stderrText = await process.stderr - .transform(const SystemEncoding().decoder) - .join(); + await stdoutDone; + final errorText = await stderrText; final exitCode = await process.exitCode; if (exitCode != 0) { - return 'yams add failed (exit $exitCode): ${stderrText.trim()}'; + return 'yams add failed (exit $exitCode): ${errorText.trim()}'; } return null; } diff --git a/lib/src/recall.dart b/lib/src/recall.dart index 0733ad8..09e0895 100644 --- a/lib/src/recall.dart +++ b/lib/src/recall.dart @@ -207,7 +207,7 @@ List loadRecallCases(String manifestPath) { if (!file.existsSync()) { throw PipeException( 'recall manifest not found: $manifestPath', - remedy: 'pass --manifest or run from the pipe repo root', + remedy: 'pass --manifest ', ); } final Object? decoded; diff --git a/test/cli_edges_test.dart b/test/cli_edges_test.dart new file mode 100644 index 0000000..73e20a0 --- /dev/null +++ b/test/cli_edges_test.dart @@ -0,0 +1,117 @@ +import 'dart:convert'; +import 'dart:io'; + +import 'package:args/command_runner.dart'; +import 'package:path/path.dart' as p; +import 'package:pipe/src/cli.dart'; +import 'package:test/test.dart'; + +void main() { + test( + 'a second project path is a usage error, not silently dropped', + () async { + // `pipe check a b` used to analyse only `a` and report as if both ran. + await expectLater( + PipeRunner().run(['check', 'test/fixtures/stuck_loading', 'lib']), + throwsA( + isA().having( + (e) => e.message, + 'message', + contains('expected one project path, got 2'), + ), + ), + ); + }, + ); + + test('recall finds the pipe manifest from outside the repo', () { + // The default used to be cwd-relative, so `pipe recall` failed anywhere + // but the pipe checkout. + final elsewhere = Directory.systemTemp.createTempSync('pipe-recall-cwd'); + final script = p.join(Directory.current.path, 'bin', 'pipe.dart'); + final found = defaultRecallManifest( + cwd: elsewhere.path, + searchFrom: [script], + ); + expect( + p.equals(found, p.join(Directory.current.path, 'recall/manifest.json')), + isTrue, + reason: found, + ); + }); + + test('recall prefers a manifest in the working directory', () { + final temp = Directory.systemTemp.createTempSync('pipe-recall-local'); + final local = File(p.join(temp.path, 'recall', 'manifest.json')) + ..createSync(recursive: true) + ..writeAsStringSync('[]'); + expect( + defaultRecallManifest(cwd: temp.path, searchFrom: const []), + local.path, + ); + }); + + test('guide --yams writes the baseline even when yams fails, and does not ' + 'hang on a chatty yams', () async { + // A fake yams that floods stdout (more than a pipe buffer) and then fails. + // Before: pipe read only stderr, so the child blocked on a full stdout and + // the run hung; and the yams failure returned before --write-baseline ran. + final temp = Directory.systemTemp.createTempSync('pipe-fake-yams'); + final fake = File(p.join(temp.path, 'yams')) + ..writeAsStringSync( + '#!/bin/sh\n' + 'cat >/dev/null\n' + 'head -c 1048576 /dev/zero | tr "\\\\0" x\n' + 'echo "fake yams down" >&2\n' + 'exit 3\n', + ); + await Process.run('chmod', ['+x', fake.path]); + final baseline = p.join(temp.path, 'baseline.json'); + + final result = await Process.run( + Platform.resolvedExecutable, + [ + 'bin/pipe.dart', + 'guide', + 'test/fixtures/stuck_loading', + '--yams', + '--write-baseline', + baseline, + '-c', + '1', + ], + environment: {'PATH': '${temp.path}:/usr/bin:/bin'}, + ).timeout(const Duration(minutes: 2)); + + expect(result.exitCode, 1, reason: '${result.stderr}'); + expect(result.stderr as String, contains('fake yams down')); + expect(File(baseline).existsSync(), isTrue); + }); + + test('a written baseline records its root relative to the file', () async { + // It used to store the absolute project path, so a committed baseline + // leaked one machine's home directory and churned on every checkout. + final temp = Directory.systemTemp.createTempSync('pipe-baseline-root'); + final baseline = p.join(temp.path, 'baseline.json'); + final result = await Process.run(Platform.resolvedExecutable, [ + 'bin/pipe.dart', + 'check', + 'test/fixtures/stuck_loading', + '--write-baseline', + baseline, + '-c', + '1', + ]); + expect(result.exitCode, 0, reason: '${result.stderr}'); + final root = (jsonDecode(File(baseline).readAsStringSync()) as Map)['root']; + expect(p.isAbsolute(root as String), isFalse, reason: root); + expect( + p.equals( + p.normalize(p.join(temp.path, root)), + p.absolute('test/fixtures/stuck_loading'), + ), + isTrue, + reason: root, + ); + }); +}