Support relative paths in path dependencies.

Support relative paths in path dependencies.


BUG=http://code.google.com/p/dart/issues/detail?id=8527

Committed: https://code.google.com/p/dart/source/detail?r=18605

[nweiz, 2013-02-15 23:09:24]

https://codereview.chromium.org/12285010/diff/1/utils/pub/entrypoint.dart
File utils/pub/entrypoint.dart (right):

https://codereview.chromium.org/12285010/diff/1/utils/pub/entrypoint.dart#new...
utils/pub/entrypoint.dart:213: cache.sources);
Should this be LockFile.load, which just takes a path and reads it itself? Then
you could keep LockFile.parse with its old signature.

https://codereview.chromium.org/12285010/diff/1/utils/pub/git_source.dart
File utils/pub/git_source.dart (right):

https://codereview.chromium.org/12285010/diff/1/utils/pub/git_source.dart#new...
utils/pub/git_source.dart:75: // TODO(rnystrom): Handle git URLs that are
relative file paths.
Add a bug number. Also, it would be nice if we could detect this and throw a
useful error.

https://codereview.chromium.org/12285010/diff/1/utils/pub/git_source.dart#new...
utils/pub/git_source.dart:77: if (description is String) return description;
It would make other code nicer if we canonicalized the format here by returning
{'url': description, 'ref': null}.

https://codereview.chromium.org/12285010/diff/1/utils/pub/io.dart
File utils/pub/io.dart (right):

https://codereview.chromium.org/12285010/diff/1/utils/pub/io.dart#newcode268
utils/pub/io.dart:268: /// [Future] which completes to the symlink file.
"to the path to the symlink file"; same below

https://codereview.chromium.org/12285010/diff/1/utils/pub/io.dart#newcode274
utils/pub/io.dart:274: Future<String> createSymlink(String symlink, String
target,
I don't like the parameter reordering here. It should have the same order as "ln
-s".

https://codereview.chromium.org/12285010/diff/1/utils/pub/path_source.dart
File utils/pub/path_source.dart (right):

https://codereview.chromium.org/12285010/diff/1/utils/pub/path_source.dart#ne...
utils/pub/path_source.dart:41: relative: true);
This should be relative iff the path the user specified was relative.

https://codereview.chromium.org/12285010/diff/1/utils/pub/path_source.dart#ne...
utils/pub/path_source.dart:53: // local file system.
Can this ever happen in practice? If so, we should throw a more useful error
here.

https://codereview.chromium.org/12285010/diff/1/utils/pub/source.dart
File utils/pub/source.dart (right):

https://codereview.chromium.org/12285010/diff/1/utils/pub/source.dart#newcode180
utils/pub/source.dart:180: dynamic parseDescription(String containingPath,
description,
Describe containingPath in the docstring.

[Bob Nystrom, 2013-02-16 00:09:57]

Thanks!

https://codereview.chromium.org/12285010/diff/1/utils/pub/entrypoint.dart
File utils/pub/entrypoint.dart (right):

https://codereview.chromium.org/12285010/diff/1/utils/pub/entrypoint.dart#new...
utils/pub/entrypoint.dart:213: cache.sources);
On 2013/02/15 23:09:24, nweiz wrote:
> Should this be LockFile.load, which just takes a path and reads it itself?
Then
> you could keep LockFile.parse with its old signature.

Done.

https://codereview.chromium.org/12285010/diff/1/utils/pub/git_source.dart
File utils/pub/git_source.dart (right):

https://codereview.chromium.org/12285010/diff/1/utils/pub/git_source.dart#new...
utils/pub/git_source.dart:75: // TODO(rnystrom): Handle git URLs that are
relative file paths.
On 2013/02/15 23:09:24, nweiz wrote:
> Add a bug number. Also, it would be nice if we could detect this and throw a
> useful error.

Added a bug. I don't want to add any code here unless I also add tests, and
that's a bit out of scope for this change.

https://codereview.chromium.org/12285010/diff/1/utils/pub/git_source.dart#new...
utils/pub/git_source.dart:77: if (description is String) return description;
On 2013/02/15 23:09:24, nweiz wrote:
> It would make other code nicer if we canonicalized the format here by
returning
> {'url': description, 'ref': null}.

Added a TODO.

https://codereview.chromium.org/12285010/diff/1/utils/pub/io.dart
File utils/pub/io.dart (right):

https://codereview.chromium.org/12285010/diff/1/utils/pub/io.dart#newcode268
utils/pub/io.dart:268: /// [Future] which completes to the symlink file.
On 2013/02/15 23:09:24, nweiz wrote:
> "to the path to the symlink file"; same below

Done.

https://codereview.chromium.org/12285010/diff/1/utils/pub/io.dart#newcode274
utils/pub/io.dart:274: Future<String> createSymlink(String symlink, String
target,
On 2013/02/15 23:09:24, nweiz wrote:
> I don't like the parameter reordering here. It should have the same order as
"ln
> -s".

It's always going to be wrong for one OS. I found both orderings confusing but I
went this way because I think it's more consistent with other create___ methods
here, like createTextFile(path, content).

https://codereview.chromium.org/12285010/diff/1/utils/pub/path_source.dart
File utils/pub/path_source.dart (right):

https://codereview.chromium.org/12285010/diff/1/utils/pub/path_source.dart#ne...
utils/pub/path_source.dart:41: relative: true);
On 2013/02/15 23:09:24, nweiz wrote:
> This should be relative iff the path the user specified was relative.

Done.

https://codereview.chromium.org/12285010/diff/1/utils/pub/path_source.dart#ne...
utils/pub/path_source.dart:53: // local file system.
On 2013/02/15 23:09:24, nweiz wrote:
> Can this ever happen in practice? If so, we should throw a more useful error
> here.

If a hosted package had a path dependency (which is specifically a validation
error) you would hit this path. I don't think it will occur in practice, though
pub the client app itself doesn't currently ensure this.

https://codereview.chromium.org/12285010/diff/1/utils/pub/source.dart
File utils/pub/source.dart (right):

https://codereview.chromium.org/12285010/diff/1/utils/pub/source.dart#newcode180
utils/pub/source.dart:180: dynamic parseDescription(String containingPath,
description,
On 2013/02/15 23:09:24, nweiz wrote:
> Describe containingPath in the docstring.

Done.

[nweiz, 2013-02-16 01:02:14]

https://codereview.chromium.org/12285010/diff/1/utils/pub/io.dart
File utils/pub/io.dart (right):

https://codereview.chromium.org/12285010/diff/1/utils/pub/io.dart#newcode274
utils/pub/io.dart:274: Future<String> createSymlink(String symlink, String
target,
On 2013/02/16 00:09:57, Bob Nystrom wrote:
> On 2013/02/15 23:09:24, nweiz wrote:
> > I don't like the parameter reordering here. It should have the same order as
> "ln
> > -s".
> 
> It's always going to be wrong for one OS.

The way it is now is wrong for all but one OS, and the OS it's right for is the
one that uses symlinks the least.

> I found both orderings confusing but I
> went this way because I think it's more consistent with other create___
methods
> here, like createTextFile(path, content).

I'd rather be consistent with the most-commonly-used symlink utility. Don't
think of creating a symlink as creating a file as content; think of it as an
analogue to copying a file from one place to another. The argument order for "ln
-s" is the same as it would be if you were using "cp" instead.

https://codereview.chromium.org/12285010/diff/1/utils/pub/path_source.dart
File utils/pub/path_source.dart (right):

https://codereview.chromium.org/12285010/diff/1/utils/pub/path_source.dart#ne...
utils/pub/path_source.dart:53: // local file system.
On 2013/02/16 00:09:57, Bob Nystrom wrote:
> On 2013/02/15 23:09:24, nweiz wrote:
> > Can this ever happen in practice? If so, we should throw a more useful error
> > here.
> 
> If a hosted package had a path dependency (which is specifically a validation
> error) you would hit this path. I don't think it will occur in practice,
though
> pub the client app itself doesn't currently ensure this.

What if a remote git dependency has a path dependency?

https://codereview.chromium.org/12285010/diff/7001/utils/pub/path_source.dart
File utils/pub/path_source.dart (right):

https://codereview.chromium.org/12285010/diff/7001/utils/pub/path_source.dart...
utils/pub/path_source.dart:53: // an expanded {"path": ..., "relative": ...}
map.
This might be a little more helpful if it were in the docstring.

https://codereview.chromium.org/12285010/diff/5002/utils/pub/git_source.dart
File utils/pub/git_source.dart (right):

https://codereview.chromium.org/12285010/diff/5002/utils/pub/git_source.dart#...
utils/pub/git_source.dart:77: // TODO(rnystrom): Now that this function that
modify the description, it
"that" -> "can"

[Bob Nystrom, 2013-02-16 01:19:03]

Thanks!

https://codereview.chromium.org/12285010/diff/1/utils/pub/io.dart
File utils/pub/io.dart (right):

https://codereview.chromium.org/12285010/diff/1/utils/pub/io.dart#newcode274
utils/pub/io.dart:274: Future<String> createSymlink(String symlink, String
target,
On 2013/02/16 01:02:14, nweiz wrote:
> On 2013/02/16 00:09:57, Bob Nystrom wrote:
> > On 2013/02/15 23:09:24, nweiz wrote:
> > > I don't like the parameter reordering here. It should have the same order
as
> > "ln
> > > -s".
> > 
> > It's always going to be wrong for one OS.
> 
> The way it is now is wrong for all but one OS, and the OS it's right for is
the
> one that uses symlinks the least.
> 
> > I found both orderings confusing but I
> > went this way because I think it's more consistent with other create___
> methods
> > here, like createTextFile(path, content).
> 
> I'd rather be consistent with the most-commonly-used symlink utility. Don't
> think of creating a symlink as creating a file as content; think of it as an
> analogue to copying a file from one place to another. The argument order for
"ln
> -s" is the same as it would be if you were using "cp" instead.

Done.

https://codereview.chromium.org/12285010/diff/1/utils/pub/path_source.dart
File utils/pub/path_source.dart (right):

https://codereview.chromium.org/12285010/diff/1/utils/pub/path_source.dart#ne...
utils/pub/path_source.dart:53: // local file system.
On 2013/02/16 01:02:14, nweiz wrote:
> On 2013/02/16 00:09:57, Bob Nystrom wrote:
> > On 2013/02/15 23:09:24, nweiz wrote:
> > > Can this ever happen in practice? If so, we should throw a more useful
error
> > > here.
> > 
> > If a hosted package had a path dependency (which is specifically a
validation
> > error) you would hit this path. I don't think it will occur in practice,
> though
> > pub the client app itself doesn't currently ensure this.
> 
> What if a remote git dependency has a path dependency?

I believe that will get installed locally, and then it'll resolve the path dep
from there.

https://codereview.chromium.org/12285010/diff/7001/utils/pub/path_source.dart
File utils/pub/path_source.dart (right):

https://codereview.chromium.org/12285010/diff/7001/utils/pub/path_source.dart...
utils/pub/path_source.dart:53: // an expanded {"path": ..., "relative": ...}
map.
On 2013/02/16 01:02:14, nweiz wrote:
> This might be a little more helpful if it were in the docstring.

Done.

https://codereview.chromium.org/12285010/diff/5002/utils/pub/git_source.dart
File utils/pub/git_source.dart (right):

https://codereview.chromium.org/12285010/diff/5002/utils/pub/git_source.dart#...
utils/pub/git_source.dart:77: // TODO(rnystrom): Now that this function that
modify the description, it
On 2013/02/16 01:02:14, nweiz wrote:
> "that" -> "can"

Done.

[nweiz, 2013-02-16 01:33:11]

lgtm

https://codereview.chromium.org/12285010/diff/1/utils/pub/path_source.dart
File utils/pub/path_source.dart (right):

https://codereview.chromium.org/12285010/diff/1/utils/pub/path_source.dart#ne...
utils/pub/path_source.dart:53: // local file system.
On 2013/02/16 01:19:03, Bob Nystrom wrote:
> On 2013/02/16 01:02:14, nweiz wrote:
> > On 2013/02/16 00:09:57, Bob Nystrom wrote:
> > > On 2013/02/15 23:09:24, nweiz wrote:
> > > > Can this ever happen in practice? If so, we should throw a more useful
> error
> > > > here.
> > > 
> > > If a hosted package had a path dependency (which is specifically a
> validation
> > > error) you would hit this path. I don't think it will occur in practice,
> > though
> > > pub the client app itself doesn't currently ensure this.
> > 
> > What if a remote git dependency has a path dependency?
> 
> I believe that will get installed locally, and then it'll resolve the path dep
> from there.

That sounds like something else we should throw a meaningful error for.