Skip to content

Commit

Permalink
Fix several package spec bugs.
Browse files Browse the repository at this point in the history
* The package spec's reference to the entrypoint package was pointing to
  the hosted cache rather than the package's directory. This was caused
  by the package graph's instance of the entrypoint package incorrectly
  pointing to the cache; the graph now re-uses `entrypoint.root`.

* The package spec didn't contain relative paths for relative path
  dependencies. This was being missed by our tests because the package
  spec parser was automatically converting all paths to absolute at
  parse-time.

Closes #1294

R=rnystrom@google.com

Review URL: https://codereview.chromium.org//1228093003 .
  • Loading branch information
nex3 committed Jul 9, 2015
1 parent 142885d commit 9cb2334
Show file tree
Hide file tree
Showing 4 changed files with 36 additions and 20 deletions.
2 changes: 1 addition & 1 deletion lib/src/command.dart
Original file line number Diff line number Diff line change
Expand Up @@ -42,7 +42,7 @@ abstract class PubCommand extends Command {
Entrypoint get entrypoint {
// Lazy load it.
if (_entrypoint == null) {
_entrypoint = new Entrypoint(path.current, cache,
_entrypoint = new Entrypoint('.', cache,
packageSymlinks: globalResults['package-symlinks']);
}
return _entrypoint;
Expand Down
2 changes: 2 additions & 0 deletions lib/src/entrypoint.dart
Original file line number Diff line number Diff line change
Expand Up @@ -470,6 +470,8 @@ class Entrypoint {
var graph = await log.progress("Loading package graph", () async {
if (result != null) {
var packages = await Future.wait(result.packages.map((id) async {
if (id.name == root.name) return root;

var dir = await cache.sources[id.source].getDirectory(id);
return new Package(result.pubspecs[id.name], dir);
}));
Expand Down
21 changes: 11 additions & 10 deletions test/descriptor/packages.dart
Original file line number Diff line number Diff line change
Expand Up @@ -44,8 +44,8 @@ class PackagesFileDescriptor extends Descriptor {
// If it's a semver, it's a cache reference.
packagePath = p.join(cachePath, "$package-$version");
} else {
// Otherwise it's a path relative to the .pubspec file,
// which is also the relative path wrt. the .packages file.
// Otherwise it's a path relative to the pubspec file,
// which is also relative to the .packages file.
packagePath = p.fromUri(version);
}
mapping[package] = p.toUri(p.join(packagePath, "lib", ""));
Expand Down Expand Up @@ -74,8 +74,11 @@ class PackagesFileDescriptor extends Descriptor {
/// A function that throws an error if [binaryContents] doesn't match the
/// expected contents of the descriptor.
void _validateNow(List<int> binaryContents, String fullPath) {
var fileUri = p.toUri(fullPath);
var map = packages_file.parse(binaryContents, fileUri);
// Resolve against a dummy URL so that we can test whether the URLs in
// the package file are themselves relative. We can't resolve against just
// "." due to sdk#23809.
var base = "/a/b/c/d/e/f/g/h/i/j/k/l/m/n/o/p";
var map = packages_file.parse(binaryContents, Uri.parse(base));

for (var package in _dependencies.keys) {
if (!map.containsKey(package)) {
Expand All @@ -89,14 +92,12 @@ class PackagesFileDescriptor extends Descriptor {
"Expected $description, found location: ${map[package]}.");
}
} else {
var expected = p.normalize(p.join(
p.dirname(fullPath), p.fromUri(description), 'lib'));
expected = new File(expected).resolveSymbolicLinksSync();
var actual = new File(p.normalize(p.absolute(p.fromUri(map[package]))))
.resolveSymbolicLinksSync();
var expected = p.normalize(p.join(p.fromUri(description), 'lib'));
var actual = p.normalize(p.fromUri(
p.url.relative(map[package].toString(), from: p.dirname(base))));

if (expected != actual) {
fail("Relative path: Expected $description, found ${map[package]}");
fail("Relative path: Expected $expected, found $actual");
}
}
}
Expand Down
31 changes: 22 additions & 9 deletions test/packages_file_test.dart
Original file line number Diff line number Diff line change
Expand Up @@ -16,12 +16,15 @@ main() {
deps: {"bar": "3.2.1"}, contents: [d.dir("lib", [])]);
});

d.appDir({"foo": "1.2.3"}).create();
d.dir(appPath, [
d.appPubspec({"foo": "1.2.3"}),
d.dir('lib')
]).create();

pubCommand(command);

d.dir(appPath, [d.packagesFile({
"foo": "1.2.3", "bar": "3.2.1", "baz": "2.2.2", "myapp": "0.0.0"})])
"foo": "1.2.3", "bar": "3.2.1", "baz": "2.2.2", "myapp": "."})])
.validate();
});

Expand All @@ -34,7 +37,10 @@ main() {
deps: {"bar": "3.2.1"}, contents: [d.dir("lib", [])]);
});

d.appDir({"foo": "1.2.3"}).create();
d.dir(appPath, [
d.appPubspec({"foo": "1.2.3"}),
d.dir('lib')
]).create();

var oldFile = d.dir(appPath, [d.packagesFile({"notFoo": "9.9.9"})]);
oldFile.create();
Expand All @@ -43,12 +49,15 @@ main() {
pubCommand(command);

d.dir(appPath, [d.packagesFile({
"foo": "1.2.3", "bar": "3.2.1", "baz": "2.2.2", "myapp": "0.0.0"})])
"foo": "1.2.3", "bar": "3.2.1", "baz": "2.2.2", "myapp": "."})])
.validate();
});

integration('.packages file is not created if pub command fails', () {
d.appDir({"foo": "1.2.3"}).create();
d.dir(appPath, [
d.appPubspec({"foo": "1.2.3"}),
d.dir('lib')
]).create();

pubCommand(command, args: ['--offline'],
error: "Could not find package foo in cache.\n");
Expand Down Expand Up @@ -76,14 +85,18 @@ main() {
"dependency_overrides": {
"baz": {"path": "../local_baz"},
}
})
}),
d.dir('lib')
]).create();

pubCommand(command);

d.dir(appPath, [d.packagesFile({"myapp": "0.0.0",
"baz": "../local_baz"})]).validate();
d.dir(appPath, [
d.packagesFile({
"myapp": ".",
"baz": "../local_baz"
})
]).validate();
});

});
}

0 comments on commit 9cb2334

Please sign in to comment.