Make pubspec.lock keep relative path if it is originally specified as relative in pubspec.yaml (iss…

Make pubspec.lock keep relative path if it is originally specified as relative in pubspec.yaml (issue 11441).

BUG=11441
R=rnystrom@google.com

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

[Bob Nystrom, 2013-10-28 22:13:31]

A couple of nits, but otherwise the code looks great.

I would like to see a test for this, though.

Take a look at test/get/path/relative_path_test.dart and
relative_symlink_test.dart for reference.

You can probably just:

1. Create a package with a pubspec with a relative path dep.
2. Run pub get.
3. Validate that the lockfile contains a relative path.

Thanks!

- bob

https://codereview.chromium.org/34803006/diff/1/sdk/lib/_internal/pub/lib/src...
File sdk/lib/_internal/pub/lib/src/lock_file.dart (right):

https://codereview.chromium.org/34803006/diff/1/sdk/lib/_internal/pub/lib/src...
sdk/lib/_internal/pub/lib/src/lock_file.dart:86: String serialize(String
rootPath, SourceRegistry sources) {
"rootPath" -> "packageDir".

Maybe explain it in the doc comment too?

https://codereview.chromium.org/34803006/diff/1/sdk/lib/_internal/pub/lib/src...
File sdk/lib/_internal/pub/lib/src/source.dart (right):

https://codereview.chromium.org/34803006/diff/1/sdk/lib/_internal/pub/lib/src...
sdk/lib/_internal/pub/lib/src/source.dart:233: dynamic
serializeDescription(String rootPath, description) {
"rootPath" -> "containingPath" to match parseDescription().

https://codereview.chromium.org/34803006/diff/1/sdk/lib/_internal/pub/lib/src...
File sdk/lib/_internal/pub/lib/src/source/path.dart (right):

https://codereview.chromium.org/34803006/diff/1/sdk/lib/_internal/pub/lib/src...
sdk/lib/_internal/pub/lib/src/source/path.dart:112: description['path'] =
path.relative(description['path'], from: rootPath);
This modifies the map in place, which might cause problems if we later try to
use the unserialized form.

Should probably return a new map instead.

https://codereview.chromium.org/34803006/diff/1/sdk/lib/_internal/pub/test/te...
File sdk/lib/_internal/pub/test/test_pub.dart (right):

https://codereview.chromium.org/34803006/diff/1/sdk/lib/_internal/pub/test/te...
sdk/lib/_internal/pub/test/test_pub.dart:612: ..register(new PathSource());
Since this is just used in one method, let's just make this local to
createLockFile(), at least for now.

[Anton Moiseev, 2013-11-03 11:11:43]

Bob,

Thanks for the comments. Addressed them in the Patch Set 2.

https://codereview.chromium.org/34803006/diff/1/sdk/lib/_internal/pub/lib/src...
File sdk/lib/_internal/pub/lib/src/lock_file.dart (right):

https://codereview.chromium.org/34803006/diff/1/sdk/lib/_internal/pub/lib/src...
sdk/lib/_internal/pub/lib/src/lock_file.dart:86: String serialize(String
rootPath, SourceRegistry sources) {
On 2013/10/28 22:13:32, Bob Nystrom wrote:
> "rootPath" -> "packageDir".
> 
> Maybe explain it in the doc comment too?

Done.

https://codereview.chromium.org/34803006/diff/1/sdk/lib/_internal/pub/lib/src...
File sdk/lib/_internal/pub/lib/src/source.dart (right):

https://codereview.chromium.org/34803006/diff/1/sdk/lib/_internal/pub/lib/src...
sdk/lib/_internal/pub/lib/src/source.dart:233: dynamic
serializeDescription(String rootPath, description) {
On 2013/10/28 22:13:32, Bob Nystrom wrote:
> "rootPath" -> "containingPath" to match parseDescription().

Done.

https://codereview.chromium.org/34803006/diff/1/sdk/lib/_internal/pub/lib/src...
File sdk/lib/_internal/pub/lib/src/source/path.dart (right):

https://codereview.chromium.org/34803006/diff/1/sdk/lib/_internal/pub/lib/src...
sdk/lib/_internal/pub/lib/src/source/path.dart:112: description['path'] =
path.relative(description['path'], from: rootPath);
On 2013/10/28 22:13:32, Bob Nystrom wrote:
> This modifies the map in place, which might cause problems if we later try to
> use the unserialized form.
> 
> Should probably return a new map instead.

Good catch, thanks! Done.

https://codereview.chromium.org/34803006/diff/1/sdk/lib/_internal/pub/test/te...
File sdk/lib/_internal/pub/test/test_pub.dart (right):

https://codereview.chromium.org/34803006/diff/1/sdk/lib/_internal/pub/test/te...
sdk/lib/_internal/pub/test/test_pub.dart:612: ..register(new PathSource());
On 2013/10/28 22:13:32, Bob Nystrom wrote:
> Since this is just used in one method, let's just make this local to
> createLockFile(), at least for now.

Done.

[Bob Nystrom, 2013-11-07 17:48:49]

LGTM!

I will land this for you. Thank you so much for contributing. :)

[Bob Nystrom, 2013-11-07 18:16:41]

Committed patchset #2 manually as r30060 (presubmit successful).