From a1139b80c0230a429a468f9f411556a3fc183bba Mon Sep 17 00:00:00 2001
From: Natalie Weizenbaum <nweiz@google.com>
Date: Wed, 28 Oct 2015 14:24:12 -0700
Subject: [PATCH] Fix error detection for a non-existent global script.
When we tried to run a global script that didn't exist, we would try to
load an AssetEnvironment, which would try to compute the transformers
for it. Because the script doesn't exist, the computation would
conservatively assume that all transformers had to run in case one of
them generated it.
To find all the scripts, it iterated through the package's dependencies.
Since it's notionally the entrypoint, it tried to include its dev
dependencies, but this broke because dev dependencies aren't installed
for globally-activated packages.
R=rnystrom@google.com
Review URL: https://codereview.chromium.org//1413713010 .
---
lib/src/barback/dependency_computer.dart | 18 ++++++++++++++----
lib/src/entrypoint.dart | 12 +++++++++---
lib/src/executable.dart | 2 +-
lib/src/global_packages.dart | 11 ++++++-----
test/global/run/nonexistent_script_test.dart | 9 +++++++--
5 files changed, 37 insertions(+), 15 deletions(-)
diff --git a/lib/src/barback/dependency_computer.dart b/lib/src/barback/dependency_computer.dart
index 4134e800..46d5f03e 100644
--- a/lib/src/barback/dependency_computer.dart
+++ b/lib/src/barback/dependency_computer.dart
@@ -43,6 +43,10 @@ class DependencyComputer {
/// This is precomputed before any package computers are loaded.
final _untransformedPackages = new Set<String>();
+ /// Creates a dependency computer for [graph].
+ ///
+ /// If [rootDevDependencies] is true, this includes the root package's dev
+ /// dependencies in the computation.
DependencyComputer(this._graph) {
for (var package in ordered(_graph.packages.keys)) {
if (_graph.transitiveDependencies(package).every((dependency) =>
@@ -185,8 +189,11 @@ class DependencyComputer {
}
}
- var dependencies = packageName == _graph.entrypoint.root.name ?
- package.immediateDependencies : package.dependencies;
+ var dependencies =
+ !_graph.entrypoint.isGlobal &&
+ packageName == _graph.entrypoint.root.name
+ ? package.immediateDependencies
+ : package.dependencies;
for (var dep in dependencies) {
try {
traversePackage(dep.name);
@@ -335,8 +342,11 @@ class _PackageDependencyComputer {
var externalDirectives = _getTransitiveExternalDirectives(library);
if (externalDirectives == null) {
var rootName = _dependencyComputer._graph.entrypoint.root.name;
- var dependencies = _package.name == rootName ?
- _package.immediateDependencies : _package.dependencies;
+ var dependencies =
+ !_dependencyComputer._graph.entrypoint.isGlobal &&
+ _package.name == rootName
+ ? _package.immediateDependencies
+ : _package.dependencies;
// If anything transitively imported/exported by [library] within this
// package is modified by a transformer, we don't know what it will
diff --git a/lib/src/entrypoint.dart b/lib/src/entrypoint.dart
index fd88b8da..60f07eea 100644
--- a/lib/src/entrypoint.dart
+++ b/lib/src/entrypoint.dart
@@ -56,6 +56,9 @@ class Entrypoint {
/// real directory on disk.
final bool _inMemory;
+ /// Whether this is an entrypoint for a globally-activated package.
+ final bool isGlobal;
+
/// The lockfile for the entrypoint.
///
/// If not provided to the entrypoint, it will be loaded lazily from disk.
@@ -111,20 +114,23 @@ class Entrypoint {
/// If [packageSymlinks] is `true`, this will create a "packages" directory
/// with symlinks to the installed packages. This directory will be symlinked
/// into any directory that might contain an entrypoint.
- Entrypoint(String rootDir, SystemCache cache, {bool packageSymlinks: true})
+ Entrypoint(String rootDir, SystemCache cache, {bool packageSymlinks: true,
+ this.isGlobal: false})
: root = new Package.load(null, rootDir, cache.sources),
cache = cache,
_packageSymlinks = packageSymlinks,
_inMemory = false;
/// Creates an entrypoint given package and lockfile objects.
- Entrypoint.inMemory(this.root, this._lockFile, this.cache)
+ Entrypoint.inMemory(this.root, this._lockFile, this.cache,
+ {this.isGlobal: false})
: _packageSymlinks = false,
_inMemory = true;
/// Creates an entrypoint given a package and a [solveResult], from which the
/// package graph and lockfile will be computed.
- Entrypoint.fromSolveResult(this.root, this.cache, SolveResult solveResult)
+ Entrypoint.fromSolveResult(this.root, this.cache, SolveResult solveResult,
+ {this.isGlobal: false})
: _packageSymlinks = false,
_inMemory = true {
_packageGraph = new PackageGraph.fromSolveResult(this, solveResult);
diff --git a/lib/src/executable.dart b/lib/src/executable.dart
index bd21b4d8..83880392 100644
--- a/lib/src/executable.dart
+++ b/lib/src/executable.dart
@@ -105,7 +105,7 @@ Future<int> runExecutable(Entrypoint entrypoint, String package,
if (executableUrl == null) {
var message = "Could not find ${log.bold(executable)}";
- if (package != entrypoint.root.name) {
+ if (isGlobal || package != entrypoint.root.name) {
message += " in package ${log.bold(package)}";
}
log.error("$message.");
diff --git a/lib/src/global_packages.dart b/lib/src/global_packages.dart
index c58ed54c..d3f0112b 100644
--- a/lib/src/global_packages.dart
+++ b/lib/src/global_packages.dart
@@ -123,7 +123,7 @@ class GlobalPackages {
/// Otherwise, the previous ones will be preserved.
Future activatePath(String path, List<String> executables,
{bool overwriteBinStubs}) async {
- var entrypoint = new Entrypoint(path, cache);
+ var entrypoint = new Entrypoint(path, cache, isGlobal: true);
// Get the package's dependencies.
await entrypoint.acquireDependencies(SolveType.GET);
@@ -172,7 +172,8 @@ class GlobalPackages {
// Load the package graph from [result] so we don't need to re-parse all
// the pubspecs.
- var entrypoint = new Entrypoint.fromSolveResult(root, cache, result);
+ var entrypoint = new Entrypoint.fromSolveResult(root, cache, result,
+ isGlobal: true);
var snapshots = await _precompileExecutables(entrypoint, dep.name);
_writeLockFile(dep.name, lockFile);
writeTextFile(_getPackagesFilePath(dep.name), lockFile.packagesFile());
@@ -312,14 +313,14 @@ class GlobalPackages {
// lockfile is the one we just loaded.
var dir = cache.sources[id.source].getDirectory(id);
var package = new Package.load(name, dir, cache.sources);
- return new Entrypoint.inMemory(package, lockFile, cache);
+ return new Entrypoint.inMemory(package, lockFile, cache, isGlobal: true);
}
// For uncached sources (i.e. path), the ID just points to the real
// directory for the package.
assert(id.source == "path");
- return new Entrypoint(PathSource.pathFromDescription(id.description),
- cache);
+ return new Entrypoint(
+ PathSource.pathFromDescription(id.description), cache, isGlobal: true);
}
/// Runs [package]'s [executable] with [args].
diff --git a/test/global/run/nonexistent_script_test.dart b/test/global/run/nonexistent_script_test.dart
index db7f099c..84571747 100644
--- a/test/global/run/nonexistent_script_test.dart
+++ b/test/global/run/nonexistent_script_test.dart
@@ -9,12 +9,17 @@ import '../../test_pub.dart';
main() {
integration('errors if the script does not exist.', () {
- servePackages((builder) => builder.serve("foo", "1.0.0"));
+ servePackages((builder) => builder.serve("foo", "1.0.0", pubspec: {
+ // Make sure barback doesn't try to look at *all* dependencies when
+ // determining which transformers to load.
+ "dev_dependencies": {"bar": "1.0.0"}
+ }));
schedulePub(args: ["global", "activate", "foo"]);
var pub = pubRun(global: true, args: ["foo:script"]);
- pub.stderr.expect("Could not find ${p.join("bin", "script.dart")}.");
+ pub.stderr.expect(
+ "Could not find ${p.join("bin", "script.dart")} in package foo.");
pub.shouldExit(exit_codes.NO_INPUT);
});
}
--
GitLab