Properly handle relative paths from lockfiles (#1754)

We weren't resolving these paths relative to the lockfile location,
which caused "pub global run" to fail for some path packages.

Closes #1751

Author: nex3

lib/src/lock_file.dart

@@ -154,7 +154,8 @@ class LockFile {
         var source = sources[sourceName];
         var id;
         try {
-          id = source.parseId(name, version, description);
+          id = source.parseId(name, version, description,
+              containingPath: filePath);
         } on FormatException catch (ex) {
           throw new SourceSpanFormatException(
               ex.message, spec.nodes['description'].span);

lib/src/source.dart

@@ -76,10 +76,9 @@ abstract class Source {
   /// the given [description] is well-formed according to this source, and to
   /// give the source a chance to canonicalize the description.
   ///
-  /// [containingPath] is the path to the local file (pubspec or lockfile)
-  /// where this description appears. It may be `null` if the description is
-  /// coming from some in-memory source (such as pulling down a pubspec from
-  /// pub.dartlang.org).
+  /// [containingPath] is the path to the pubspec where this description
+  /// appears. It may be `null` if the description is coming from some in-memory
+  /// source (such as pulling down a pubspec from pub.dartlang.org).
   ///
   /// The description in the returned [PackageRef] need bear no resemblance to
   /// the original user-provided description.
@@ -92,8 +91,13 @@ abstract class Source {
   /// This only accepts descriptions serialized using [serializeDescription]. It
   /// should not be used with user-authored descriptions.
   ///
+  /// [containingPath] is the path to the lockfile where this description
+  /// appears. It may be `null` if the description is coming from some in-memory
+  /// source.
+  ///
   /// Throws a [FormatException] if the description is not valid.
-  PackageId parseId(String name, Version version, description);
+  PackageId parseId(String name, Version version, description,
+      {String containingPath});
 
   /// When a [LockFile] is serialized, it uses this method to get the
   /// [description] in the right format.

lib/src/source/git.dart

@@ -81,7 +81,8 @@ class GitSource extends Source {
         {"url": description["url"], "ref": ref ?? "HEAD", "path": path ?? "."});
   }
 
-  PackageId parseId(String name, Version version, description) {
+  PackageId parseId(String name, Version version, description,
+      {String containingPath}) {
     if (description is! Map) {
       throw new FormatException("The description must be a map with a 'url' "
           "key.");

lib/src/source/hosted.dart

@@ -83,7 +83,8 @@ class HostedSource extends Source {
     return new PackageRef(name, this, description);
   }
 
-  PackageId parseId(String name, Version version, description) {
+  PackageId parseId(String name, Version version, description,
+      {String containingPath}) {
     _parseDescription(description);
     return new PackageId(name, this, version, description);
   }

lib/src/source/path.dart

@@ -81,7 +81,8 @@ class PathSource extends Source {
         name, this, {"path": description, "relative": isRelative});
   }
 
-  PackageId parseId(String name, Version version, description) {
+  PackageId parseId(String name, Version version, description,
+      {String containingPath}) {
     if (description is! Map) {
       throw new FormatException("The description must be a map.");
     }
@@ -96,6 +97,20 @@ class PathSource extends Source {
           "must be a boolean.");
     }
 
+    // Resolve the path relative to the containing file path.
+    if (description["relative"]) {
+      // Relative paths coming from lockfiles that are not on the local file
+      // system aren't allowed.
+      if (containingPath == null) {
+        throw new FormatException('"$description" is a relative path, but this '
+            'isn\'t a local pubspec.');
+      }
+
+      description = new Map.from(description);
+      description["path"] =
+          p.normalize(p.join(p.dirname(containingPath), description["path"]));
+    }
+
     return new PackageId(name, this, version, description);
   }
 

lib/src/source/sdk.dart

@@ -38,7 +38,8 @@ class SdkSource extends Source {
     return new PackageRef(name, this, description);
   }
 
-  PackageId parseId(String name, Version version, description) {
+  PackageId parseId(String name, Version version, description,
+      {String containingPath}) {
     if (description is! String) {
       throw new FormatException("The description must be an SDK name.");
     }

lib/src/source/unknown.dart

@@ -38,7 +38,8 @@ class UnknownSource extends Source {
   PackageRef parseRef(String name, description, {String containingPath}) =>
       new PackageRef(name, this, description);
 
-  PackageId parseId(String name, Version version, description) =>
+  PackageId parseId(String name, Version version, description,
+          {String containingPath}) =>
       new PackageId(name, this, version, description);
 }
 

test/get/path/relative_path_test.dart

@@ -66,6 +66,7 @@ main() {
     var lockfile = new LockFile.load(lockfilePath, new SourceRegistry());
     var description = lockfile.packages["foo"].description;
 
-    expect(path.isRelative(description["path"]), isTrue);
+    expect(description["relative"], isTrue);
+    expect(description["path"], path.join(d.sandbox, "foo"));
   });
 }

test/global/activate/path_package_test.dart

@@ -22,4 +22,36 @@ main() {
         args: ["global", "activate", "--source", "path", "../foo"],
         output: endsWith('Activated foo 1.0.0 at path "$path".'));
   });
+
+  // Regression test for #1751
+  test('activates a package at a local path with a relative path dependency',
+      () async {
+    await d.dir("foo", [
+      d.libPubspec("foo", "1.0.0", deps: {
+        "bar": {"path": "../bar"}
+      }),
+      d.dir("bin", [
+        d.file("foo.dart", """
+        import 'package:bar/bar.dart';
+
+        main() => print(value);
+      """)
+      ])
+    ]).create();
+
+    await d.dir("bar", [
+      d.libPubspec("bar", "1.0.0"),
+      d.dir("lib", [d.file("bar.dart", "final value = 'ok';")])
+    ]).create();
+
+    var path = canonicalize(p.join(d.sandbox, "foo"));
+    await runPub(
+        args: ["global", "activate", "--source", "path", "../foo"],
+        output: endsWith('Activated foo 1.0.0 at path "$path".'));
+
+    await runPub(
+        args: ["global", "run", "foo"],
+        output: "ok",
+        workingDirectory: p.current);
+  });
 }

test/lock_file_test.dart

@@ -22,7 +22,8 @@ class MockSource extends Source {
     return new PackageRef(name, this, description);
   }
 
-  PackageId parseId(String name, Version version, description) {
+  PackageId parseId(String name, Version version, description,
+      {String containingPath}) {
     if (!description.endsWith(' desc')) throw new FormatException('Bad');
     return new PackageId(name, this, version, description);
   }