Fix process running during package creation (#14508)
There were some problems I introduced with the last PR for this. It passed the test, but failed in practice.
This adds tests for those failure cases, adds a "--help" and fixes the test so that it doesn't try and actually download MinGit as part of the Windows test.
I added package:platform as a dependency, so I did a force upgrade on the packages.
Also, re-enabling 'create package' in the cache warming code, now that #14448 is fixed.
diff --git a/dev/bots/prepare_package.dart b/dev/bots/prepare_package.dart
index ad3c7fa..e5f66ee 100644
--- a/dev/bots/prepare_package.dart
+++ b/dev/bots/prepare_package.dart
@@ -13,11 +13,15 @@
import 'package:process/process.dart';
import 'package:platform/platform.dart' show Platform, LocalPlatform;
-const String CHROMIUM_REPO =
+const String chromiumRepo =
'https://chromium.googlesource.com/external/github.com/flutter/flutter';
-const String GITHUB_REPO = 'https://github.com/flutter/flutter.git';
-const String MINGIT_FOR_WINDOWS_URL = 'https://storage.googleapis.com/flutter_infra/mingit/'
+const String githubRepo = 'https://github.com/flutter/flutter.git';
+const String mingitForWindowsUrl = 'https://storage.googleapis.com/flutter_infra/mingit/'
'603511c649b00bbef0a6122a827ac419b656bc19/mingit.zip';
+const String gsBase = 'gs://flutter_infra';
+const String releaseFolder = '/releases';
+const String gsReleaseFolder = '$gsBase$releaseFolder';
+const String baseUrl = 'https://storage.googleapis.com/flutter_infra';
/// Exception class for when a process fails to run, so we can catch
/// it and provide something more readable than a stack trace.
@@ -74,11 +78,11 @@
/// properly without dropping any.
class ProcessRunner {
ProcessRunner({
- this.processManager: const LocalProcessManager(),
+ ProcessManager processManager,
this.subprocessOutput: true,
this.defaultWorkingDirectory,
this.platform: const LocalPlatform(),
- }) {
+ }) : processManager = processManager ?? const LocalProcessManager() {
environment = new Map<String, String>.from(platform.environment);
}
@@ -167,6 +171,8 @@
}
}
+typedef Future<Uint8List> HttpReader(Uri url, {Map<String, String> headers});
+
/// Creates a pre-populated Flutter archive from a git repo.
class ArchiveCreator {
/// [tempDir] is the directory to use for creating the archive. The script
@@ -185,8 +191,10 @@
ProcessManager processManager,
bool subprocessOutput: true,
this.platform: const LocalPlatform(),
+ HttpReader httpReader,
}) : assert(revision.length == 40),
flutterRoot = new Directory(path.join(tempDir.path, 'flutter')),
+ httpReader = httpReader ?? http.readBytes,
_processRunner = new ProcessRunner(
processManager: processManager,
subprocessOutput: subprocessOutput,
@@ -200,15 +208,36 @@
_processRunner.environment['PUB_CACHE'] = path.join(flutterRoot.absolute.path, '.pub-cache');
}
+ /// The platform to use for the environment and determining which
+ /// platform we're running on.
final Platform platform;
+
+ /// The branch to build the archive for. The branch must contain [revision].
final Branch branch;
+
+ /// The git revision hash to build the archive for. This revision has
+ /// to be available in the [branch], although it doesn't have to be
+ /// at HEAD, since we clone the branch and then reset to this revision
+ /// to create the archive.
final String revision;
+
+ /// The flutter root directory in the [tempDir].
final Directory flutterRoot;
+
+ /// The temporary directory used to build the archive in.
final Directory tempDir;
+
+ /// The directory to write the output file to.
final Directory outputDir;
- final Uri _minGitUri = Uri.parse(MINGIT_FOR_WINDOWS_URL);
+
+ final Uri _minGitUri = Uri.parse(mingitForWindowsUrl);
final ProcessRunner _processRunner;
+ /// Used to tell the [ArchiveCreator] which function to use for reading
+ /// bytes from a URL. Used in tests to inject a fake reader. Defaults to
+ /// [http.readBytes].
+ final HttpReader httpReader;
+
File _outputFile;
String _version;
String _flutter;
@@ -255,12 +284,12 @@
// We want the user to start out the in the specified branch instead of a
// detached head. To do that, we need to make sure the branch points at the
// desired revision.
- await _runGit(<String>['clone', '-b', branchName, CHROMIUM_REPO], workingDirectory: tempDir);
+ await _runGit(<String>['clone', '-b', branchName, chromiumRepo], workingDirectory: tempDir);
await _runGit(<String>['reset', '--hard', revision]);
// Make the origin point to github instead of the chromium mirror.
await _runGit(<String>['remote', 'remove', 'origin']);
- await _runGit(<String>['remote', 'add', 'origin', GITHUB_REPO]);
+ await _runGit(<String>['remote', 'add', 'origin', githubRepo]);
}
/// Retrieve the MinGit executable from storage and unpack it.
@@ -268,7 +297,7 @@
if (!platform.isWindows) {
return;
}
- final Uint8List data = await http.readBytes(_minGitUri);
+ final Uint8List data = await httpReader(_minGitUri);
final File gitFile = new File(path.join(tempDir.absolute.path, 'mingit.zip'));
await gitFile.writeAsBytes(data, flush: true);
@@ -289,10 +318,7 @@
// Create each of the templates, since they will call 'pub get' on
// themselves when created, and this will warm the cache with their
// dependencies too.
- // TODO(gspencer): 'package' is broken on dev branch right now!
- // Add it back in once the following is fixed:
- // https://github.com/flutter/flutter/issues/14448
- for (String template in <String>['app', 'plugin']) {
+ for (String template in <String>['app', 'package', 'plugin']) {
final String createName = path.join(tempDir.path, 'create_$template');
await _runFlutter(
<String>['create', '--template=$template', createName],
@@ -381,11 +407,6 @@
subprocessOutput: subprocessOutput,
);
- static String gsBase = 'gs://flutter_infra';
- static String releaseFolder = '/releases';
- static String gsReleaseFolder = '$gsBase$releaseFolder';
- static String baseUrl = 'https://storage.googleapis.com/flutter_infra';
-
final Platform platform;
final String platformName;
final String metadataGsPath;
@@ -498,14 +519,24 @@
argParser.addFlag(
'publish',
defaultsTo: false,
- help: 'The path to the directory where the output archive should be '
- 'written. If --output is not specified, the archive will be written to '
- "the current directory. If the output directory doesn't exist, it, and "
- 'the path to it, will be created.',
+ help: 'If set, will publish the archive to Google Cloud Storage upon '
+ 'successful creation of the archive. Will publish under this '
+ 'directory: $baseUrl$releaseFolder',
+ );
+ argParser.addFlag(
+ 'help',
+ defaultsTo: false,
+ negatable: false,
+ help: 'Print help for this command.',
);
final ArgResults args = argParser.parse(argList);
+ if (args['help']) {
+ print(argParser.usage);
+ exit(0);
+ }
+
void errorExit(String message, {int exitCode = -1}) {
stderr.write('Error: $message\n\n');
stderr.write('${argParser.usage}\n');
diff --git a/dev/bots/pubspec.yaml b/dev/bots/pubspec.yaml
index 18cd046..6f9b64b 100644
--- a/dev/bots/pubspec.yaml
+++ b/dev/bots/pubspec.yaml
@@ -3,8 +3,9 @@
dependencies:
path: 1.5.1
- args: 1.2.0
+ args: 1.3.0
process: 2.0.7
+ platform: 2.1.1
dev_dependencies:
test: 0.12.30+1
@@ -26,7 +27,7 @@
http_multi_server: 2.0.4 # TRANSITIVE DEPENDENCY
http_parser: 3.1.1 # TRANSITIVE DEPENDENCY
intl: 0.15.2 # TRANSITIVE DEPENDENCY
- io: 0.3.1 # TRANSITIVE DEPENDENCY
+ io: 0.3.2+1 # TRANSITIVE DEPENDENCY
isolate: 1.1.0 # TRANSITIVE DEPENDENCY
js: 0.6.1 # TRANSITIVE DEPENDENCY
logging: 0.11.3+1 # TRANSITIVE DEPENDENCY
@@ -37,7 +38,6 @@
node_preamble: 1.4.0 # TRANSITIVE DEPENDENCY
package_config: 1.0.3 # TRANSITIVE DEPENDENCY
package_resolver: 1.0.2 # TRANSITIVE DEPENDENCY
- platform: 2.1.1 # TRANSITIVE DEPENDENCY
plugin: 0.2.0+2 # TRANSITIVE DEPENDENCY
pool: 1.3.4 # TRANSITIVE DEPENDENCY
pub_semver: 1.3.2 # TRANSITIVE DEPENDENCY
diff --git a/dev/bots/test/prepare_package_test.dart b/dev/bots/test/prepare_package_test.dart
index c517ae6..320feee 100644
--- a/dev/bots/test/prepare_package_test.dart
+++ b/dev/bots/test/prepare_package_test.dart
@@ -2,8 +2,10 @@
// Use of this source code is governed by a BSD-style license that can be
// found in the LICENSE file.
+import 'dart:async';
import 'dart:convert';
import 'dart:io' hide Platform;
+import 'dart:typed_data';
import 'package:mockito/mockito.dart';
import 'package:test/test.dart';
@@ -20,6 +22,39 @@
operatingSystem: platformName,
environment: <String, String>{},
);
+ group('ProcessRunner for $platform', () {
+ test('Defaults to local process manager, can actually run a command', () async {
+ final ProcessRunner processRunner =
+ new ProcessRunner(subprocessOutput: false, platform: platform);
+ // We want to test that we can actually run a process and obtain stdout.
+ // The command 'echo test' works on all platforms.
+ final String output = await processRunner.runProcess(<String>['echo', 'test']);
+ expect(output, equals('test'));
+ });
+ test('Returns stdout', () async {
+ final FakeProcessManager fakeProcessManager = new FakeProcessManager();
+ fakeProcessManager.fakeResults = <String, List<ProcessResult>>{
+ 'echo test': <ProcessResult>[new ProcessResult(0, 0, 'output', 'error')],
+ };
+ final ProcessRunner processRunner = new ProcessRunner(
+ subprocessOutput: false, platform: platform, processManager: fakeProcessManager);
+ final String output = await processRunner.runProcess(<String>['echo', 'test']);
+ expect(output, equals('output'));
+ });
+ test('Throws on process failure', () async {
+ final FakeProcessManager fakeProcessManager = new FakeProcessManager();
+ fakeProcessManager.fakeResults = <String, List<ProcessResult>>{
+ 'echo test': <ProcessResult>[new ProcessResult(0, -1, 'output', 'error')],
+ };
+ final ProcessRunner processRunner = new ProcessRunner(
+ subprocessOutput: false, platform: platform, processManager: fakeProcessManager);
+ expect(
+ expectAsync1((List<String> commandLine) async {
+ return processRunner.runProcess(commandLine);
+ })(<String>['echo', 'test']),
+ throwsA(const isInstanceOf<ProcessRunnerException>()));
+ });
+ });
group('ArchiveCreator for $platformName', () {
ArchiveCreator creator;
Directory tmpDir;
@@ -29,6 +64,10 @@
final List<Map<Symbol, dynamic>> namedArgs = <Map<Symbol, dynamic>>[];
String flutter;
+ Future<Uint8List> fakeHttpReader(Uri url, {Map<String, String> headers}) {
+ return new Future<Uint8List>.value(new Uint8List(0));
+ }
+
setUp(() async {
processManager = new FakeProcessManager();
args.clear();
@@ -44,6 +83,7 @@
processManager: processManager,
subprocessOutput: false,
platform: platform,
+ httpReader: fakeHttpReader,
);
flutter = path.join(creator.flutterRoot.absolute.path, 'bin', 'flutter');
});
@@ -119,9 +159,7 @@
'$flutter precache': null,
'$flutter ide-config': null,
'$flutter create --template=app ${createBase}app': null,
- // TODO(gspencer): Re-enable this when package works again:
- // https://github.com/flutter/flutter/issues/14448
- // '$flutter create --template=package ${createBase}package': null,
+ '$flutter create --template=package ${createBase}package': null,
'$flutter create --template=plugin ${createBase}plugin': null,
'git clean -f -X **/.packages': null,
});
@@ -141,6 +179,7 @@
processManager: processManager,
subprocessOutput: false,
platform: platform,
+ httpReader: fakeHttpReader,
);
await creator.initializeRepo();
await creator.createArchive();
