Make pub's binstubs resilient to changes in snapshot format.

sdk/lib/_internal/pub/lib/src/global_packages.dart

Index: sdk/lib/_internal/pub/lib/src/global_packages.dart
diff --git a/sdk/lib/_internal/pub/lib/src/global_packages.dart b/sdk/lib/_internal/pub/lib/src/global_packages.dart
index 73a0c0abffb0038257ee13b6c04a02627d663c6a..1be190cc5dc77ef3b787d7b26a1909a23a9e381b 100644
--- a/sdk/lib/_internal/pub/lib/src/global_packages.dart
+++ b/sdk/lib/_internal/pub/lib/src/global_packages.dart
@@ -13,6 +13,7 @@ import 'package:pub_semver/pub_semver.dart';
 
 import 'barback/asset_environment.dart';
 import 'entrypoint.dart';
+import 'exceptions.dart';
 import 'executable.dart' as exe;
 import 'io.dart';
 import 'lock_file.dart';
@@ -27,10 +28,6 @@ import 'source/path.dart';
 import 'system_cache.dart';
 import 'utils.dart';
 
-/// Matches the package name that a binstub was created for inside the contents
-/// of the shell script.
-final _binStubPackagePattern = new RegExp(r"Package: ([a-zA-Z0-9_-]+)");
-
 /// Maintains the set of packages that have been globally activated.
 ///
 /// These have been hand-chosen by the user to make their executables in bin/
@@ -381,25 +378,31 @@ class GlobalPackages {
   void listActivePackages() {
     if (!dirExists(_directory)) return;
 
-    // Loads lock [file] and returns [PackageId] of the activated package.
-    loadPackageId(file, name) {
-      var lockFile = new LockFile.load(p.join(_directory, file), cache.sources);
-      return lockFile.packages[name];
-    }
-
-    var packages = listDir(_directory).map((entry) {
-      if (fileExists(entry)) {
-        return loadPackageId(entry, p.basenameWithoutExtension(entry));
-      } else {
-        return loadPackageId(p.join(entry, 'pubspec.lock'), p.basename(entry));
-      }
-    }).toList();
-
-    packages
+    return listDir(_directory).map(_loadPackageId).toList()
         ..sort((id1, id2) => id1.name.compareTo(id2.name))
         ..forEach((id) => log.message(_formatPackage(id)));
   }
 
+  /// Returns the [PackageId] for the globally-activated package at [path].
+  ///
+  /// [path] should be a path within [_directory]. It can either be an old-style
+  /// path to a single lockfile or a new-style path to a directory containing a
+  /// lockfile.
+  PackageId _loadPackageId(String path) {
+    var name = p.basenameWithoutExtension(path);
+    if (!fileExists(path)) path = p.join(path, 'pubspec.lock');
+
+    var id = new LockFile.load(p.join(_directory, path), cache.sources)
+        .packages[name];
+
+    if (id == null) {
+      throw new FormatException("Pubspec for activated package $name didn't "
+          "contain an entry for itself.");
+    }
+
+    return id;
+  }
+
   /// Returns formatted string representing the package [id].
   String _formatPackage(PackageId id) {
     if (id.source == 'git') {
@@ -413,6 +416,83 @@ class GlobalPackages {
     }
   }
 
+  /// Repairs any corrupted globally-activated packages and their binstubs.
+  ///
+  /// Returns a pair of two [int]s. The first indicates how many packages were
+  /// successfully re-activated; the second indicates how many failed.
+  Future<Pair<int, int>> repairActivatedPackages() async {
+    var executables = {};
+    if (dirExists(_binStubDir)) {
+      for (var entry in listDir(_binStubDir)) {
+        try {
+          var binstub = readTextFile(entry);
+          var package = _binStubProperty(binstub, "Package");
+          if (package == null) {
+            throw new ApplicationException("No 'Package' property.");
+          }
+
+          var executable = _binStubProperty(binstub, "Executable");
+          if (executable == null) {
+            throw new ApplicationException("No 'Executable' property.");
+          }
+
+          executables.putIfAbsent(package, () => []).add(executable);
+        } catch (error, stackTrace) {
+          log.error(

[nweiz, 2014/11/21 01:09:55]

I'm not sure whether this should delete the binstub. On the one hand, the
existing "cache repair" stuff doesn't and it seems scary to just delete stuff,
but on the other hand not doing so risks leaving the cache in a broken
state—running the script will continue to be busted. What do you think?

[Bob Nystrom, 2014/11/21 19:13:41]

I like the idea of deleting it. It doesn't contain any unique data that can't be
reconstituted, and it avoids leaving something known bad on disk.

[nweiz, 2014/11/22 02:32:55]

Done. Should we plan to do the same for cached packages?

[Bob Nystrom, 2014/12/01 18:29:24]

Yeah, I think so. We probably want to handle the deletion failing too.

[nweiz, 2014/12/01 23:44:39]

Done.

+              "Error reading binstub for "
+                  "\"${p.basenameWithoutExtension(entry)}\"",
+              error, stackTrace);
+        }
+      }
+    }
+
+    var successes = 0;
+    var failures = 0;
+    if (dirExists(_directory)) {
+      await Future.wait(listDir(_directory).map((entry) async {

[Bob Nystrom, 2014/11/21 19:13:41]

I think you should use a loop here. Otherwise, it's possible to interleave
output between "Reactivating..." and a later error.

[nweiz, 2014/11/22 02:32:56]

That's contrary to how repairing cached packages works. I'm not opposed to
changing it, but it should be consistent either way.

[Bob Nystrom, 2014/12/01 18:29:24]

Does repairing have any other output aside from the initial log of the package
name? Either way, I agree on consistency. I'd be fine if repairing used a loop
too.

[nweiz, 2014/12/01 23:44:39]

Repairing could also have an error.
Done.

+        var id;
+        try {
+          id = _loadPackageId(entry);
+          log.message("Reactivating ${log.bold(id.name)} ${id.version}...");
+
+          var entrypoint = await find(id.name);
+
+          var graph = await entrypoint.loadPackageGraph();
+          var snapshots = await _precompileExecutables(entrypoint, id.name);
+          var packageExecutables = executables.remove(id.name);
+          if (packageExecutables == null) packageExecutables = [];
+          _updateBinStubs(graph.packages[id.name], packageExecutables,
+              overwriteBinStubs: true, snapshots: snapshots,
+              pathSuggestion: false);
+          successes++;
+        } catch (error, stackTrace) {
+          var message = "Failed to reactivate "
+              "${log.bold(p.basenameWithoutExtension(entry))}";
+          if (id != null) {
+            message += " ${id.version}";
+            if (id.source != "hosted") message += " from ${id.source}";
+          }
+
+          log.error(message, error, stackTrace);
+          failures++;
+        }
+      }));
+    }
+
+    if (executables.isNotEmpty) {
+      var packages = pluralize("package", executables.length);
+      var message = new StringBuffer("Binstubs exist for non-activated "
+          "packages:\n");

[Bob Nystrom, 2014/11/21 19:13:41]

What are these from? Should they be deleted?

[nweiz, 2014/11/22 02:32:56]

They could be from any number of things, but most likely is probably a
deactivate gone wrong. If we're deleting corrupted binstubs, we should delete
these as well.

+      executables.forEach((package, executableNames) {
+        message.writeln("  From ${log.bold(package)}: "
+            "${toSentence(executableNames)}");
+      });
+      log.error(message);
+    }
+
+    return new Pair(successes, failures);
+  }
+
   /// Updates the binstubs for [package].
   ///
   /// A binstub is a little shell script in `PUB_CACHE/bin` that runs an
@@ -430,10 +510,14 @@ class GlobalPackages {
   /// 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.
+  /// snapshots were precompiled to the paths of those snapshots. Binstubs for
+  /// those will run the snapshot directly and skip pub entirely.
+  ///
+  /// If [pathSuggestion] is `true` (the default), this will warn the user if
+  /// the bin directory isn't on their path.
   void _updateBinStubs(Package package, List<String> executables,
-      {bool overwriteBinStubs, Map<String, String> snapshots}) {
+      {bool overwriteBinStubs, Map<String, String> snapshots,
+       bool pathSuggestion: true}) {

[Bob Nystrom, 2014/11/21 19:13:41]

"suggestion" is a weird name for a boolean to me. How about
"suggestIfNotOnPath"?

[nweiz, 2014/11/22 02:32:56]

Done.

     if (snapshots == null) snapshots = const {};
 
     // Remove any previously activated binstubs for this package, in case the
@@ -513,7 +597,9 @@ class GlobalPackages {
       }
     }
 
-    if (installed.isNotEmpty) _suggestIfNotOnPath(installed);
+    if (pathSuggestion && installed.isNotEmpty) {
+      _suggestIfNotOnPath(installed.first);
+    }
   }
 
   /// Creates a binstub named [executable] that runs [script] from [package].
@@ -537,25 +623,25 @@ class GlobalPackages {
     var previousPackage;
     if (fileExists(binStubPath)) {
       var contents = readTextFile(binStubPath);
-      var match = _binStubPackagePattern.firstMatch(contents);
-      if (match != null) {
-        previousPackage = match[1];
-        if (!overwrite) return previousPackage;
-      } else {
+      previousPackage = _binStubProperty(contents, "Package");
+      if (previousPackage == null) {
         log.fine("Could not parse binstub $binStubPath:\n$contents");
+      } else if (!overwrite) {
+        return previousPackage;
       }
     }
 
     // If the script was precompiled to a snapshot, just invoke that directly
     // and skip pub global run entirely.
     var invocation;
+    var pubGlobalInvocation = "pub global run ${package.name}:$script";
     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";
+      invocation = pubGlobalInvocation;
     }
 
     if (Platform.operatingSystem == "windows") {
@@ -568,6 +654,18 @@ rem Executable: ${executable}
 rem Script: ${script}
 $invocation %*
 """;
+
+      if (snapshot != null) {
+        batch += """
+
+if errorlevel 255 (

[Bob Nystrom, 2014/11/21 19:13:41]

Document this.

[nweiz, 2014/11/22 02:32:55]

Done.

+  exit /b %errorlevel%
+)
+

[Bob Nystrom, 2014/11/21 19:13:41]

Delete the snapshot here.

[nweiz, 2014/11/22 02:32:55]

Actually, deleting the snapshot is wrong; it prevents "pub global" from
rebuilding the snapshot. Removed from the Bash version.

+$pubGlobalInvocation
+""";
+      }
+
       writeTextFile(binStubPath, batch);
     } else {
       var bash = """
@@ -579,6 +677,20 @@ $invocation %*
 # Script: ${script}
 $invocation "\$@"
 """;
+
+      if (snapshot != null) {
+        bash += """
+
+exit_code=\$?
+if [[ \$exit_code != 255 ]]; then
+  exit \$exit_code
+fi
+
+rm "$snapshot"
+$pubGlobalInvocation
+""";
+      }
+
       writeTextFile(binStubPath, bash);
 
       // Make it executable.
@@ -606,13 +718,13 @@ $invocation "\$@"
 
     for (var file in listDir(_binStubDir, includeDirs: false)) {
       var contents = readTextFile(file);
-      var match = _binStubPackagePattern.firstMatch(contents);
-      if (match == null) {
+      var binStubPackage = _binStubProperty(contents, "Package");
+      if (binStubPackage == null) {
         log.fine("Could not parse binstub $file:\n$contents");
         continue;
       }
 
-      if (match[1] == package) {
+      if (binStubPackage == package) {
         log.fine("Deleting old binstub $file");
         deleteEntry(file);
       }
@@ -621,11 +733,14 @@ $invocation "\$@"
 
   /// Checks to see if the binstubs are on the user's PATH and, if not, suggests
   /// that the user add the directory to their PATH.
-  void _suggestIfNotOnPath(List<String> installed) {
+  ///
+  /// [installed] should be the name of an installed executable that can be used
+  /// to test whether accessing it on the path works.
+  void _suggestIfNotOnPath(String installed) {
     if (Platform.operatingSystem == "windows") {
       // See if the shell can find one of the binstubs.
       // "\q" means return exit code 0 if found or 1 if not.
-      var result = runProcessSync("where", [r"\q", installed.first + ".bat"]);
+      var result = runProcessSync("where", [r"\q", installed + ".bat"]);
       if (result.exitCode == 0) return;
 
       log.warning(
@@ -636,7 +751,7 @@ $invocation "\$@"
           'A web search for "configure windows path" will show you how.');
     } else {
       // See if the shell can find one of the binstubs.
-      var result = runProcessSync("which", [installed.first]);
+      var result = runProcessSync("which", [installed]);
       if (result.exitCode == 0) return;
 
       var binDir = _binStubDir;
@@ -655,4 +770,12 @@ $invocation "\$@"
           "\n");
     }
   }
+
+  /// Returns the value of the property named [name] in the bin stub script
+  /// [source].
+  String _binStubProperty(String source, String name) {
+    var pattern = new RegExp(quoteRegExp(name) + r": ([a-zA-Z0-9_-]+)");
+    var match = pattern.firstMatch(source);
+    return match == null ? null : match[1];
+  }
 }