From dfa32032fdf2af3c23954c5cfa2c66cf5c164b0d Mon Sep 17 00:00:00 2001 From: Janice Collins Date: Fri, 6 Dec 2019 18:01:51 +0000 Subject: [PATCH] Add scaffolding to trial_migration for more package sources. Imports a subprocess helper with JSON parsing borrowed from Dartdoc, which will come in handy both for launching pub and for building the trial migration script out for comparing output between versions. Also puts some basic structure in to allow for fetching packages for multiple sources. Change-Id: I5262b93aa1d60005ee262b7ed00bf534b1c51447 Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/127446 Commit-Queue: Janice Collins Reviewed-by: Brian Wilkerson Reviewed-by: Paul Berry --- pkg/nnbd_migration/tool/src/package.dart | 108 +++++++++++++ .../tool/src/subprocess_launcher.dart | 151 ++++++++++++++++++ pkg/nnbd_migration/tool/trial_migration.dart | 72 +++++---- 3 files changed, 301 insertions(+), 30 deletions(-) create mode 100644 pkg/nnbd_migration/tool/src/package.dart create mode 100644 pkg/nnbd_migration/tool/src/subprocess_launcher.dart diff --git a/pkg/nnbd_migration/tool/src/package.dart b/pkg/nnbd_migration/tool/src/package.dart new file mode 100644 index 000000000000..5b484fd4536c --- /dev/null +++ b/pkg/nnbd_migration/tool/src/package.dart @@ -0,0 +1,108 @@ +// Copyright (c) 2019, the Dart project authors. Please see the AUTHORS file +// for details. All rights reserved. Use of this source code is governed by a +// BSD-style license that can be found in the LICENSE file. + +/// Abstractions for the different sources of truth for different packages. + +import 'dart:convert'; +import 'dart:io'; + +import 'package:path/path.dart' as path; + +/// Returns the path to the SDK repository this script is a part of. +final String thisSdkRepo = () { + var maybeSdkRepoDir = Platform.script.toFilePath(); + while (maybeSdkRepoDir != path.dirname(maybeSdkRepoDir)) { + maybeSdkRepoDir = path.dirname(maybeSdkRepoDir); + if (File(path.join(maybeSdkRepoDir, 'README.dart-sdk')).existsSync()) { + return maybeSdkRepoDir; + } + } + throw UnsupportedError( + 'Script ${Platform.script} using this library must be within the SDK repository'); +}(); + +Uri get thisSdkUri => Uri.file(thisSdkRepo); + +/// Abstraction for a package fetched via Github. +class GitHubPackage extends Package { + GitHubPackage(String name, [String label]) : super(name) { + throw UnimplementedError(); + } + + @override + // TODO: implement packagePath + String get packagePath => null; +} + +/// Abstraction for a package fetched via pub. +class PubPackage extends Package { + PubPackage(String name, [String version]) : super(name) { + throw UnimplementedError(); + } + + @override + // TODO: implement packagePath + String get packagePath => null; +} + +/// Abstraction for a package located within pkg or third_party/pkg. +class SdkPackage extends Package { + /// Where to find packages. Constructor searches in-order. + static List _searchPaths = [ + 'pkg', + path.join('third_party', 'pkg'), + ]; + + SdkPackage(String name) : super(name) { + for (String potentialPath + in _searchPaths.map((p) => path.join(thisSdkRepo, p, name))) { + if (Directory(potentialPath).existsSync()) { + _packagePath = potentialPath; + } + } + if (_packagePath == null) + throw ArgumentError('Package $name not found in SDK'); + } + + /* late final */ String _packagePath; + @override + String get packagePath => _packagePath; + + @override + String toString() => path.relative(packagePath, from: thisSdkRepo); +} + +/// Base class for pub, github, SDK, or possibly other package sources. +abstract class Package { + final String name; + + Package(this.name); + + /// Returns the root directory of the package. + String get packagePath; + + @override + String toString() => name; +} + +/// Abstraction for compiled Dart SDKs (not this repository). +class Sdk { + /// The root of the compiled SDK. + /* late final */ String sdkPath; + + Sdk(String sdkPath) { + this.sdkPath = path.canonicalize(sdkPath); + } + + /// Returns true if the SDK was built with --nnbd. + /// + /// May throw if [sdkPath] is invalid, or there is an error parsing + /// the libraries.json file. + bool get isNnbdSdk { + // TODO(jcollins-g): contact eng-prod for a more foolproof detection method + String libraries = path.join(sdkPath, 'lib', 'libraries.json'); + var decodedJson = JsonDecoder().convert(File(libraries).readAsStringSync()); + return ((decodedJson['comment:1'] as String).contains('sdk_nnbd')); + } +} diff --git a/pkg/nnbd_migration/tool/src/subprocess_launcher.dart b/pkg/nnbd_migration/tool/src/subprocess_launcher.dart new file mode 100644 index 000000000000..1cb38d82afcd --- /dev/null +++ b/pkg/nnbd_migration/tool/src/subprocess_launcher.dart @@ -0,0 +1,151 @@ +// Copyright (c) 2019, the Dart project authors. Please see the AUTHORS file +// for details. All rights reserved. Use of this source code is governed by a +// BSD-style license that can be found in the LICENSE file. + +// TODO(jcollins-g): Merge this with similar utilities in dartdoc +// and extract into a separate package, generate testing and mirrors, and +// reimport that into the SDK, before cut and paste gets out of hand. + +/// This is a modified version of dartdoc's +/// SubprocessLauncher from test/src/utils.dart, for use with the +/// nnbd_migration script. + +import 'dart:convert'; +import 'dart:io'; + +final RegExp quotables = RegExp(r'[ "\r\n\$]'); + +/// SubprocessLauncher manages one or more launched, non-interactive +/// subprocesses. It handles I/O streams, parses JSON output if +/// available, and logs debugging information so the user can see exactly +/// what was run. +class SubprocessLauncher { + final String context; + final Map environmentDefaults; + + String get prefix => context.isNotEmpty ? '$context: ' : ''; + + /// From flutter:dev/tools/dartdoc.dart, modified. + static Future _printStream(Stream> stream, Stdout output, + {String prefix = '', Iterable Function(String line) filter}) { + assert(prefix != null); + if (filter == null) filter = (line) => [line]; + return stream + .transform(utf8.decoder) + .transform(const LineSplitter()) + .expand(filter) + .listen((String line) { + if (line != null) { + output.write('$prefix$line'.trim()); + output.write('\n'); + } + }).asFuture(); + } + + SubprocessLauncher(this.context, [Map environment]) + : this.environmentDefaults = environment ?? {}; + + /// A wrapper around start/await process.exitCode that will display the + /// output of the executable continuously and fail on non-zero exit codes. + /// It will also parse any valid JSON objects (one per line) it encounters + /// on stdout/stderr, and return them. Returns null if no JSON objects + /// were encountered, or if DRY_RUN is set to 1 in the execution environment. + /// + /// Makes running programs in grinder similar to set -ex for bash, even on + /// Windows (though some of the bashisms will no longer make sense). + /// TODO(jcollins-g): refactor to return a stream of stderr/stdout lines + /// and their associated JSON objects. + Future> runStreamed(String executable, List arguments, + {String workingDirectory, + Map environment, + bool includeParentEnvironment = true, + void Function(String) perLine}) async { + environment ??= {}; + environment.addAll(environmentDefaults); + List jsonObjects; + + /// Parses json objects generated by the subprocess. If a json object + /// contains the key 'message' or the keys 'data' and 'text', return that + /// value as a collection of lines suitable for printing. + Iterable jsonCallback(String line) { + if (perLine != null) perLine(line); + Map result; + try { + result = json.decoder.convert(line) as Map; + } catch (FormatException) { + // ignore + } + if (result != null) { + jsonObjects ??= []; + jsonObjects.add(result); + if (result.containsKey('message')) { + line = result['message'] as String; + } else if (result.containsKey('data') && + result['data'] is Map && + (result['data'] as Map).containsKey('key')) { + line = result['data']['text'] as String; + } + } + return line.split('\n'); + } + + stderr.write('$prefix+ '); + if (workingDirectory != null) stderr.write('(cd "$workingDirectory" && '); + if (environment != null) { + stderr.write(environment.keys.map((String key) { + if (environment[key].contains(quotables)) { + return "$key='${environment[key]}'"; + } else { + return "$key=${environment[key]}"; + } + }).join(' ')); + stderr.write(' '); + } + stderr.write('$executable'); + if (arguments.isNotEmpty) { + for (String arg in arguments) { + if (arg.contains(quotables)) { + stderr.write(" '$arg'"); + } else { + stderr.write(" $arg"); + } + } + } + if (workingDirectory != null) stderr.write(')'); + stderr.write('\n'); + + if (Platform.environment.containsKey('DRY_RUN')) return null; + + String realExecutable = executable; + final List realArguments = []; + if (Platform.isLinux) { + // Use GNU coreutils to force line buffering. This makes sure that + // subprocesses that die due to fatal signals do not chop off the + // last few lines of their output. + // + // Dart does not actually do this (seems to flush manually) unless + // the VM crashes. + realExecutable = 'stdbuf'; + realArguments.addAll(['-o', 'L', '-e', 'L']); + realArguments.add(executable); + } + realArguments.addAll(arguments); + + Process process = await Process.start(realExecutable, realArguments, + workingDirectory: workingDirectory, + environment: environment, + includeParentEnvironment: includeParentEnvironment); + Future stdoutFuture = _printStream(process.stdout, stdout, + prefix: prefix, filter: jsonCallback); + Future stderrFuture = _printStream(process.stderr, stderr, + prefix: prefix, filter: jsonCallback); + await Future.wait([stderrFuture, stdoutFuture, process.exitCode]); + + int exitCode = await process.exitCode; + if (exitCode != 0) { + throw ProcessException(executable, arguments, + "SubprocessLauncher got non-zero exitCode: $exitCode", exitCode); + } + return jsonObjects; + } +} diff --git a/pkg/nnbd_migration/tool/trial_migration.dart b/pkg/nnbd_migration/tool/trial_migration.dart index 83b3fe77c89a..891a1fd1c1ed 100644 --- a/pkg/nnbd_migration/tool/trial_migration.dart +++ b/pkg/nnbd_migration/tool/trial_migration.dart @@ -8,7 +8,6 @@ // result of migration, as well as categories (and counts) of exceptions that // occurred. -import 'dart:convert'; import 'dart:io'; import 'package:analyzer/src/dart/analysis/analysis_context_collection.dart'; @@ -19,6 +18,8 @@ import 'package:args/args.dart'; import 'package:nnbd_migration/nnbd_migration.dart'; import 'package:path/path.dart' as path; +import 'src/package.dart'; + main(List args) async { ArgParser argParser = ArgParser(); ArgResults parsedArgs; @@ -31,6 +32,29 @@ main(List args) async { help: 'Select the root of the SDK to analyze against for this run ' '(compiled with --nnbd). For example: ../../xcodebuild/DebugX64NNBD/dart-sdk'); + argParser.addMultiOption( + 'packages', + abbr: 'p', + defaultsTo: [ + 'charcode', + 'collection', + 'logging', + 'meta', + 'path', + 'term_glyph', + 'typed_data', + 'async', + 'source_span', + 'stack_trace', + 'matcher', + 'stream_channel', + 'boolean_selector', + path.join('test', 'pkgs', 'test_api'), + ], + help: 'The list of packages to run the migration against.', + splitCommas: true, + ); + try { parsedArgs = argParser.parse(args); } on ArgParserException { @@ -46,35 +70,25 @@ main(List args) async { throw 'invalid args. Specify *one* argument to get exceptions of interest.'; } - String sdkPath = path.canonicalize(parsedArgs['sdk'] as String); + Sdk sdk = Sdk(parsedArgs['sdk'] as String); warnOnNoAssertions(); - warnOnNoSdkNnbd(sdkPath); + warnOnNoSdkNnbd(sdk); + + List packages = (parsedArgs['packages'] as Iterable) + .map((n) => SdkPackage(n)) + .toList(growable: false); String categoryOfInterest = parsedArgs.rest.isEmpty ? null : parsedArgs.rest.single; - var rootUri = Platform.script.resolve('../../..'); + var listener = _Listener(categoryOfInterest); - for (var testPath in [ - 'third_party/pkg/charcode', - 'third_party/pkg/collection', - 'third_party/pkg/logging', - 'pkg/meta', - 'third_party/pkg/path', - 'third_party/pkg/term_glyph', - 'third_party/pkg/typed_data', - 'third_party/pkg/async', - 'third_party/pkg/source_span', - 'third_party/pkg/stack_trace', - 'third_party/pkg/matcher', - 'third_party/pkg/stream_channel', - 'third_party/pkg/boolean_selector', - 'third_party/pkg/test/pkgs/test_api', - ]) { - print('Migrating $testPath'); - var testUri = rootUri.resolve(testPath); + for (var package in packages) { + print('Migrating $package'); + var testUri = thisSdkUri.resolve(package.packagePath); var contextCollection = AnalysisContextCollectionImpl( - includedPaths: [testUri.toFilePath()], sdkPath: sdkPath); + includedPaths: [testUri.toFilePath()], sdkPath: sdk.sdkPath); + var context = contextCollection.contexts.single; var files = context.contextRoot .analyzedFiles() @@ -133,17 +147,15 @@ void warnOnNoAssertions() { printWarning("You didn't --enable-asserts!"); } -void warnOnNoSdkNnbd(String sdkPath) { - // TODO(jcollins-g): contact eng-prod for a more foolproof detection method - String libraries = path.join(sdkPath, 'lib', 'libraries.json'); +void warnOnNoSdkNnbd(Sdk sdk) { try { - var decodedJson = JsonDecoder().convert(File(libraries).readAsStringSync()); - if ((decodedJson['comment:1'] as String).contains('sdk_nnbd')) return; - } on Exception { + if (sdk.isNnbdSdk) return; + } catch (e) { printWarning('Unable to determine whether this SDK supports NNBD'); return; } - printWarning('SDK at $sdkPath not compiled with --nnbd, use --sdk option'); + printWarning( + 'SDK at ${sdk.sdkPath} not compiled with --nnbd, use --sdk option'); } class _Listener implements NullabilityMigrationListener {