From 7569ed873ec34170cb919f38d4a4df4724cad482 Mon Sep 17 00:00:00 2001
From: "rnystrom@google.com" <rnystrom@google.com>
Date: Tue, 23 Sep 2014 18:22:25 +0000
Subject: [PATCH] Make binstubs run snapshots directly when possible.
R=nweiz@google.com
Review URL: https://codereview.chromium.org//586173002
git-svn-id: https://dart.googlecode.com/svn/branches/bleeding_edge@40598 260f80e4-7a28-3924-810f-c04153c831b5
---
lib/src/barback/asset_environment.dart | 53 +++++++------
lib/src/entrypoint.dart | 63 +++++++--------
lib/src/global_packages.dart | 79 ++++++++++++-------
.../binstub_runs_executable_test.dart | 57 +++++++++++++
...b_runs_global_run_if_no_snapshot_test.dart | 35 ++++++++
...instub_runs_precompiled_snapshot_test.dart | 33 ++++++++
.../creates_executables_in_pubspec_test.dart | 8 +-
test/test_pub.dart | 43 ++++++----
8 files changed, 264 insertions(+), 107 deletions(-)
create mode 100644 test/global/binstubs/binstub_runs_executable_test.dart
create mode 100644 test/global/binstubs/binstub_runs_global_run_if_no_snapshot_test.dart
create mode 100644 test/global/binstubs/binstub_runs_precompiled_snapshot_test.dart
diff --git a/lib/src/barback/asset_environment.dart b/lib/src/barback/asset_environment.dart
index 8df986b1..330f78d8 100644
--- a/lib/src/barback/asset_environment.dart
+++ b/lib/src/barback/asset_environment.dart
@@ -237,38 +237,45 @@ class AssetEnvironment {
/// [directory].
///
/// If [executableIds] is passed, only those executables are precompiled.
- Future precompileExecutables(String packageName, String directory,
- {Iterable<AssetId> executableIds}) {
+ ///
+ /// Returns a map from executable name to path for the snapshots that were
+ /// successfully precompiled.
+ Future<Map<String, String>> precompileExecutables(String packageName,
+ String directory, {Iterable<AssetId> executableIds}) async {
if (executableIds == null) {
executableIds = graph.packages[packageName].executableIds;
}
- log.fine("executables for $packageName: $executableIds");
- if (executableIds.isEmpty) return null;
+
+ log.fine("Executables for $packageName: $executableIds");
+ if (executableIds.isEmpty) return [];
var package = graph.packages[packageName];
- return servePackageBinDirectory(packageName).then((server) {
- return waitAndPrintErrors(executableIds.map((id) {
+ var server = await servePackageBinDirectory(packageName);
+ try {
+ var precompiled = {};
+ await waitAndPrintErrors(executableIds.map((id) async {
var basename = path.url.basename(id.path);
var snapshotPath = path.join(directory, "$basename.snapshot");
- return runProcess(Platform.executable, [
+ var result = await runProcess(Platform.executable, [
'--snapshot=$snapshotPath',
server.url.resolve(basename).toString()
- ]).then((result) {
- if (result.success) {
- log.message("Precompiled ${_formatExecutable(id)}.");
- } else {
- throw new ApplicationException(
- log.yellow("Failed to precompile "
- "${_formatExecutable(id)}:\n") +
- result.stderr.join('\n'));
- }
- });
- })).whenComplete(() {
- // Don't return this future, since we have no need to wait for the
- // server to fully shut down.
- server.close();
- });
- });
+ ]);
+ if (result.success) {
+ log.message("Precompiled ${_formatExecutable(id)}.");
+ precompiled[path.withoutExtension(basename)] = snapshotPath;
+ } else {
+ throw new ApplicationException(
+ log.yellow("Failed to precompile ${_formatExecutable(id)}:\n") +
+ result.stderr.join('\n'));
+ }
+ }));
+
+ return precompiled;
+ } finally {
+ // Don't await this future, since we have no need to wait for the server
+ // to fully shut down.
+ server.close();
+ }
}
/// Returns the executable name for [id].
diff --git a/lib/src/entrypoint.dart b/lib/src/entrypoint.dart
index 13aa2f63..215dfe28 100644
--- a/lib/src/entrypoint.dart
+++ b/lib/src/entrypoint.dart
@@ -230,7 +230,7 @@ class Entrypoint {
/// Precompiles all executables from dependencies that don't transitively
/// depend on [this] or on a path dependency.
- Future precompileExecutables({Iterable<String> changed}) {
+ Future precompileExecutables({Iterable<String> changed}) async {
if (changed != null) changed = changed.toSet();
var binDir = path.join('.pub', 'bin');
@@ -243,44 +243,41 @@ class Entrypoint {
readTextFile(sdkVersionPath) == "${sdk.version}\n";
if (!sdkMatches) changed = null;
- return loadPackageGraph().then((graph) {
- var executables = new Map.fromIterable(root.immediateDependencies,
- key: (dep) => dep.name,
- value: (dep) => _executablesForPackage(graph, dep.name, changed));
-
- for (var package in executables.keys.toList()) {
- if (executables[package].isEmpty) executables.remove(package);
- }
+ var graph = await loadPackageGraph();
+ var executables = new Map.fromIterable(root.immediateDependencies,
+ key: (dep) => dep.name,
+ value: (dep) => _executablesForPackage(graph, dep.name, changed));
- if (!sdkMatches) deleteEntry(binDir);
- if (executables.isEmpty) return null;
+ for (var package in executables.keys.toList()) {
+ if (executables[package].isEmpty) executables.remove(package);
+ }
- return log.progress("Precompiling executables", () {
- ensureDir(binDir);
+ if (!sdkMatches) deleteEntry(binDir);
+ if (executables.isEmpty) return;
- // Make sure there's a trailing newline so our version file matches the
- // SDK's.
- writeTextFile(sdkVersionPath, "${sdk.version}\n");
+ await log.progress("Precompiling executables", () async {
+ ensureDir(binDir);
- var packagesToLoad =
- unionAll(executables.keys.map(graph.transitiveDependencies))
- .map((package) => package.name).toSet();
- return AssetEnvironment.create(this, BarbackMode.RELEASE,
- packages: packagesToLoad,
- useDart2JS: false).then((environment) {
- environment.barback.errors.listen((error) {
- log.error(log.red("Build error:\n$error"));
- });
+ // Make sure there's a trailing newline so our version file matches the
+ // SDK's.
+ writeTextFile(sdkVersionPath, "${sdk.version}\n");
- return waitAndPrintErrors(executables.keys.map((package) {
- var dir = path.join(binDir, package);
- cleanDir(dir);
- return environment.precompileExecutables(
- package, dir,
- executableIds: executables[package]);
- }));
- });
+ var packagesToLoad =
+ unionAll(executables.keys.map(graph.transitiveDependencies))
+ .map((package) => package.name).toSet();
+ var environment = await AssetEnvironment.create(this, BarbackMode.RELEASE,
+ packages: packagesToLoad,
+ useDart2JS: false);
+ environment.barback.errors.listen((error) {
+ log.error(log.red("Build error:\n$error"));
});
+
+ await waitAndPrintErrors(executables.keys.map((package) async {
+ var dir = path.join(binDir, package);
+ cleanDir(dir);
+ await environment.precompileExecutables(package, dir,
+ executableIds: executables[package]);
+ }));
});
}
diff --git a/lib/src/global_packages.dart b/lib/src/global_packages.dart
index 602f5283..f91f40a9 100644
--- a/lib/src/global_packages.dart
+++ b/lib/src/global_packages.dart
@@ -93,10 +93,9 @@ class GlobalPackages {
// dependencies. Their executables shouldn't be cached, and there should
// be a mechanism for redoing dependency resolution if a path pubspec has
// changed (see also issue 20499).
- var package = await _installInCache(
- new PackageDep(name, "git", VersionConstraint.any, repo));
- _updateBinStubs(package, executables,
- overwriteBinStubs: overwriteBinStubs);
+ await _installInCache(
+ new PackageDep(name, "git", VersionConstraint.any, repo),
+ executables, overwriteBinStubs: overwriteBinStubs);
}
/// Finds the latest version of the hosted package with [name] that matches
@@ -112,10 +111,8 @@ class GlobalPackages {
Future activateHosted(String name, VersionConstraint constraint,
List<String> executables, {bool overwriteBinStubs}) async {
_describeActive(name);
- var package = await _installInCache(
- new PackageDep(name, "hosted", constraint, name));
- _updateBinStubs(package, executables,
- overwriteBinStubs: overwriteBinStubs);
+ await _installInCache(new PackageDep(name, "hosted", constraint, name),
+ executables, overwriteBinStubs: overwriteBinStubs);
}
/// Makes the local package at [path] globally active.
@@ -155,9 +152,8 @@ class GlobalPackages {
}
/// Installs the package [dep] and its dependencies into the system cache.
- ///
- /// Returns the cached root [Package].
- Future<Package> _installInCache(PackageDep dep) async {
+ Future _installInCache(PackageDep dep, List<String> executables,
+ {bool overwriteBinStubs}) async {
var source = cache.sources[dep.source];
// Create a dummy package with just [dep] so we can do resolution on it.
@@ -183,27 +179,31 @@ class GlobalPackages {
// the pubspecs.
var graph = await new Entrypoint.inMemory(root, lockFile, cache)
.loadPackageGraph(result);
- await _precompileExecutables(graph.entrypoint, dep.name);
+ var snapshots = await _precompileExecutables(graph.entrypoint, dep.name);
_writeLockFile(dep.name, lockFile);
- return graph.packages[dep.name];
+ _updateBinStubs(graph.packages[dep.name], executables,
+ overwriteBinStubs: overwriteBinStubs, snapshots: snapshots);
}
/// Precompiles the executables for [package] and saves them in the global
/// cache.
- Future _precompileExecutables(Entrypoint entrypoint, String package) {
- return log.progress("Precompiling executables", () {
+ ///
+ /// Returns a map from executable name to path for the snapshots that were
+ /// successfully precompiled.
+ Future<Map<String, String>> _precompileExecutables(Entrypoint entrypoint,
+ String package) {
+ return log.progress("Precompiling executables", () async {
var binDir = p.join(_directory, package, 'bin');
cleanDir(binDir);
- return AssetEnvironment.create(entrypoint, BarbackMode.RELEASE,
- useDart2JS: false).then((environment) {
- environment.barback.errors.listen((error) {
- log.error(log.red("Build error:\n$error"));
- });
-
- return environment.precompileExecutables(package, binDir);
+ var environment = await AssetEnvironment.create(entrypoint,
+ BarbackMode.RELEASE, useDart2JS: false);
+ environment.barback.errors.listen((error) {
+ log.error(log.red("Build error:\n$error"));
});
+
+ return environment.precompileExecutables(package, binDir);
});
}
@@ -422,11 +422,17 @@ class GlobalPackages {
/// If `null`, all executables in the package will get binstubs. If empty, no
/// binstubs will be created.
///
- /// if [overwriteBinStubs] is `true`, any binstubs that collide with
+ /// If [overwriteBinStubs] is `true`, any binstubs that collide with
/// existing binstubs in other packages will be overwritten by this one's.
/// Otherwise, the previous ones will be preserved.
+ ///
+ /// If [snapshots] is given, it is a map of the names of executables whose
+ /// snapshots that were precompiled to their paths. Binstubs for those will
+ /// run the snapshot directly and skip pub entirely.
void _updateBinStubs(Package package, List<String> executables,
- {bool overwriteBinStubs}) {
+ {bool overwriteBinStubs, Map<String, String> snapshots}) {
+ if (snapshots == null) snapshots = const {};
+
// Remove any previously activated binstubs for this package, in case the
// list of executables has changed.
_deleteBinStubs(package.name);
@@ -447,7 +453,7 @@ class GlobalPackages {
var script = package.pubspec.executables[executable];
var previousPackage = _createBinStub(package, executable, script,
- overwrite: overwriteBinStubs);
+ overwrite: overwriteBinStubs, snapshot: snapshots[script]);
if (previousPackage != null) {
collided[executable] = previousPackage;
@@ -512,10 +518,13 @@ class GlobalPackages {
/// If [overwrite] is `true`, this will replace an existing binstub with that
/// name for another package.
///
+ /// If [snapshot] is non-null, it is a path to a snapshot file. The binstub
+ /// will invoke that directly. Otherwise, it will run `pub global run`.
+ ///
/// If a collision occurs, returns the name of the package that owns the
/// existing binstub. Otherwise returns `null`.
String _createBinStub(Package package, String executable, String script,
- {bool overwrite}) {
+ {bool overwrite, String snapshot}) {
var binStubPath = p.join(_binStubDir, executable);
// See if the binstub already exists. If so, it's for another package
@@ -532,10 +541,20 @@ class GlobalPackages {
}
}
- // TODO(rnystrom): If the script was precompiled to a snapshot, make the
- // script just invoke that directly and skip pub global run entirely.
+ // If the script was precompiled to a snapshot, just invoke that directly
+ // and skip pub global run entirely.
+ var invocation;
+ if (snapshot != null) {
+ // We expect absolute paths from the precompiler since relative ones
+ // won't be relative to the right directory when the user runs this.
+ assert(p.isAbsolute(snapshot));
+ invocation = 'dart "$snapshot"';
+ } else {
+ invocation = "pub global run ${package.name}:$script";
+ }
if (Platform.operatingSystem == "windows") {
+
var batch = """
@echo off
rem This file was created by pub v${sdk.version}.
@@ -543,7 +562,7 @@ rem Package: ${package.name}
rem Version: ${package.version}
rem Executable: ${executable}
rem Script: ${script}
-pub global run ${package.name}:$script "%*"
+$invocation "%*"
""";
writeTextFile(binStubPath, batch);
} else {
@@ -553,7 +572,7 @@ pub global run ${package.name}:$script "%*"
# Version: ${package.version}
# Executable: ${executable}
# Script: ${script}
-pub global run ${package.name}:$script "\$@"
+$invocation "\$@"
""";
writeTextFile(binStubPath, bash);
diff --git a/test/global/binstubs/binstub_runs_executable_test.dart b/test/global/binstubs/binstub_runs_executable_test.dart
new file mode 100644
index 00000000..9abed5ae
--- /dev/null
+++ b/test/global/binstubs/binstub_runs_executable_test.dart
@@ -0,0 +1,57 @@
+// Copyright (c) 2014, 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.
+
+import 'package:path/path.dart' as p;
+import 'package:scheduled_test/scheduled_process.dart';
+
+import '../../descriptor.dart' as d;
+import '../../test_pub.dart';
+
+main() {
+ initConfig();
+ integration("the generated binstub runs a snapshotted executable", () {
+ servePackages((builder) {
+ builder.serve("foo", "1.0.0", pubspec: {
+ "executables": {
+ "foo-script": "script"
+ }
+ }, contents: [
+ d.dir("bin", [
+ d.file("script.dart", "main(args) => print('ok \$args');")
+ ])
+ ]);
+ });
+
+ schedulePub(args: ["global", "activate", "foo"]);
+
+ var process = new ScheduledProcess.start(
+ p.join(sandboxDir, cachePath, "bin/foo-script"), ["arg1", "arg2"]);
+
+ process.stdout.expect("ok [arg1, arg2]");
+ process.shouldExit();
+ });
+
+ integration("the generated binstub runs a non-snapshotted executable", () {
+ d.dir("foo", [
+ d.pubspec({
+ "name": "foo",
+ "executables": {
+ "foo-script": "script"
+ }
+ }),
+ d.dir("bin", [
+ d.file("script.dart", "main(args) => print('ok \$args');")
+ ])
+ ]).create();
+
+ schedulePub(args: ["global", "activate", "-spath", "../foo"]);
+
+ var process = new ScheduledProcess.start(
+ p.join(sandboxDir, cachePath, "bin/foo-script"), ["arg1", "arg2"],
+ environment: getPubTestEnvironment());
+
+ process.stdout.expect("ok [arg1, arg2]");
+ process.shouldExit();
+ });
+}
diff --git a/test/global/binstubs/binstub_runs_global_run_if_no_snapshot_test.dart b/test/global/binstubs/binstub_runs_global_run_if_no_snapshot_test.dart
new file mode 100644
index 00000000..da72ec3f
--- /dev/null
+++ b/test/global/binstubs/binstub_runs_global_run_if_no_snapshot_test.dart
@@ -0,0 +1,35 @@
+// Copyright (c) 2014, 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.
+
+import 'package:scheduled_test/scheduled_test.dart';
+
+import '../../descriptor.dart' as d;
+import '../../test_pub.dart';
+
+main() {
+ initConfig();
+ integration("the binstubs runs pub global run if there is no snapshot", () {
+ d.dir("foo", [
+ d.pubspec({
+ "name": "foo",
+ "executables": {
+ "foo-script": "script"
+ }
+ }),
+ d.dir("bin", [
+ d.file("script.dart", "main() => print('ok');")
+ ])
+ ]).create();
+
+ // Path packages are mutable, so no snapshot is created.
+ schedulePub(args: ["global", "activate", "--source", "path", "../foo"],
+ output: contains("Installed executable foo-script."));
+
+ d.dir(cachePath, [
+ d.dir("bin", [
+ d.matcherFile("foo-script", contains("pub global run foo:script"))
+ ])
+ ]).validate();
+ });
+}
diff --git a/test/global/binstubs/binstub_runs_precompiled_snapshot_test.dart b/test/global/binstubs/binstub_runs_precompiled_snapshot_test.dart
new file mode 100644
index 00000000..cc249115
--- /dev/null
+++ b/test/global/binstubs/binstub_runs_precompiled_snapshot_test.dart
@@ -0,0 +1,33 @@
+// Copyright (c) 2014, 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.
+
+import 'package:scheduled_test/scheduled_test.dart';
+
+import '../../descriptor.dart' as d;
+import '../../test_pub.dart';
+
+main() {
+ initConfig();
+ integration("the binstubs runs a precompiled snapshot if present", () {
+ servePackages((builder) {
+ builder.serve("foo", "1.0.0", pubspec: {
+ "executables": {
+ "foo-script": "script"
+ }
+ }, contents: [
+ d.dir("bin", [
+ d.file("script.dart", "main(args) => print('ok');")
+ ])
+ ]);
+ });
+
+ schedulePub(args: ["global", "activate", "foo"]);
+
+ d.dir(cachePath, [
+ d.dir("bin", [
+ d.matcherFile("foo-script", contains("script.dart.snapshot"))
+ ])
+ ]).validate();
+ });
+}
diff --git a/test/global/binstubs/creates_executables_in_pubspec_test.dart b/test/global/binstubs/creates_executables_in_pubspec_test.dart
index 616ce006..71ae530c 100644
--- a/test/global/binstubs/creates_executables_in_pubspec_test.dart
+++ b/test/global/binstubs/creates_executables_in_pubspec_test.dart
@@ -14,12 +14,12 @@ main() {
builder.serve("foo", "1.0.0", pubspec: {
"executables": {
"one": null,
- "two-renamed": "two"
+ "two-renamed": "second"
}
}, contents: [
d.dir("bin", [
d.file("one.dart", "main(args) => print('one');"),
- d.file("two.dart", "main(args) => print('two');"),
+ d.file("second.dart", "main(args) => print('two');"),
d.file("nope.dart", "main(args) => print('nope');")
])
]);
@@ -30,8 +30,8 @@ main() {
d.dir(cachePath, [
d.dir("bin", [
- d.matcherFile("one", contains("pub global run foo:one")),
- d.matcherFile("two-renamed", contains("pub global run foo:two")),
+ d.matcherFile("one", contains("one")),
+ d.matcherFile("two-renamed", contains("second")),
d.nothing("two"),
d.nothing("nope")
])
diff --git a/test/test_pub.dart b/test/test_pub.dart
index ca1e6679..9719f13f 100644
--- a/test/test_pub.dart
+++ b/test/test_pub.dart
@@ -476,16 +476,35 @@ void confirmPublish(ScheduledProcess pub) {
pub.writeLine("y");
}
+/// Gets the absolute path to [relPath], which is a relative path in the test
+/// sandbox.
+String _pathInSandbox(String relPath) {
+ return p.join(p.absolute(sandboxDir), relPath);
+}
+
+/// Gets the environment variables used to run pub in a test context.
+Map getPubTestEnvironment([Uri tokenEndpoint]) {
+ var environment = {};
+ environment['_PUB_TESTING'] = 'true';
+ environment['PUB_CACHE'] = _pathInSandbox(cachePath);
+
+ // Ensure a known SDK version is set for the tests that rely on that.
+ environment['_PUB_TEST_SDK_VERSION'] = "0.1.2+3";
+
+ if (tokenEndpoint != null) {
+ environment['_PUB_TEST_TOKEN_ENDPOINT'] =
+ tokenEndpoint.toString();
+ }
+
+ return environment;
+}
+
/// Starts a Pub process and returns a [ScheduledProcess] that supports
/// interaction with that process.
///
/// Any futures in [args] will be resolved before the process is started.
ScheduledProcess startPub({List args, Future<Uri> tokenEndpoint}) {
- String pathInSandbox(String relPath) {
- return p.join(p.absolute(sandboxDir), relPath);
- }
-
- ensureDir(pathInSandbox(appPath));
+ ensureDir(_pathInSandbox(appPath));
// Find a Dart executable we can use to spawn. Use the same one that was
// used to run this script itself.
@@ -510,17 +529,7 @@ ScheduledProcess startPub({List args, Future<Uri> tokenEndpoint}) {
if (tokenEndpoint == null) tokenEndpoint = new Future.value();
var environmentFuture = tokenEndpoint.then((tokenEndpoint) {
- var environment = {};
- environment['_PUB_TESTING'] = 'true';
- environment['PUB_CACHE'] = pathInSandbox(cachePath);
-
- // Ensure a known SDK version is set for the tests that rely on that.
- environment['_PUB_TEST_SDK_VERSION'] = "0.1.2+3";
-
- if (tokenEndpoint != null) {
- environment['_PUB_TEST_TOKEN_ENDPOINT'] =
- tokenEndpoint.toString();
- }
+ var environment = getPubTestEnvironment(tokenEndpoint);
// If there is a server running, tell pub what its URL is so hosted
// dependencies will look there.
@@ -535,7 +544,7 @@ ScheduledProcess startPub({List args, Future<Uri> tokenEndpoint}) {
});
return new PubProcess.start(dartBin, dartArgs, environment: environmentFuture,
- workingDirectory: pathInSandbox(appPath),
+ workingDirectory: _pathInSandbox(appPath),
description: args.isEmpty ? 'pub' : 'pub ${args.first}');
}
--
GitLab