Support relative paths in path dependencies.

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

Review URL: https://codereview.chromium.org//12285010


Review URL: https://codereview.chromium.org//12294039

git-svn-id: https://dart.googlecode.com/svn/branches/bleeding_edge/dart@18715 260f80e4-7a28-3924-810f-c04153c831b5
This commit is contained in:
rnystrom@google.com 2013-02-19 19:49:46 +00:00
parent cfff419935
commit a1702c3578
13 changed files with 337 additions and 91 deletions

View file

@ -209,7 +209,7 @@ class Entrypoint {
LockFile loadLockFile() {
var lockFilePath = path.join(root.dir, 'pubspec.lock');
if (!fileExists(lockFilePath)) return new LockFile.empty();
return new LockFile.parse(readTextFile(lockFilePath), cache.sources);
return new LockFile.load(lockFilePath, cache.sources);
}
/// Saves a list of concrete package versions to the `pubspec.lock` file.
@ -294,9 +294,9 @@ class Entrypoint {
/// Creates a symlink to the `packages` directory in [dir] if none exists.
Future _linkSecondaryPackageDir(String dir) {
return defer(() {
var to = path.join(dir, 'packages');
if (entryExists(to)) return;
return createSymlink(packagesDir, to);
var symlink = path.join(dir, 'packages');
if (entryExists(symlink)) return;
return createSymlink(packagesDir, symlink);
});
}
}

View file

@ -10,6 +10,7 @@ import '../../pkg/path/lib/path.dart' as path;
import 'git.dart' as git;
import 'io.dart';
import 'log.dart' as log;
import 'package.dart';
import 'source.dart';
import 'source_registry.dart';
@ -67,24 +68,33 @@ class GitSource extends Source {
return path.join(systemCacheRoot, revisionCacheName);
});
}
/// Ensures [description] is a Git URL.
void validateDescription(description, {bool fromLockFile: false}) {
dynamic parseDescription(String containingPath, description,
{bool fromLockFile: false}) {
// TODO(rnystrom): Handle git URLs that are relative file paths (#8570).
// TODO(rnystrom): Now that this function can modify the description, it
// may as well canonicalize it to a map so that other code in the source
// can assume that.
// A single string is assumed to be a Git URL.
if (description is String) return;
if (description is String) return description;
if (description is! Map || !description.containsKey('url')) {
throw new FormatException("The description must be a Git URL or a map "
"with a 'url' key.");
}
description = new Map.from(description);
description.remove('url');
description.remove('ref');
if (fromLockFile) description.remove('resolved-ref');
if (!description.isEmpty) {
var plural = description.length > 1;
var keys = description.keys.join(', ');
var parsed = new Map.from(description);
parsed.remove('url');
parsed.remove('ref');
if (fromLockFile) parsed.remove('resolved-ref');
if (!parsed.isEmpty) {
var plural = parsed.length > 1;
var keys = parsed.keys.join(', ');
throw new FormatException("Invalid key${plural ? 's' : ''}: $keys.");
}
return description;
}
/// Two Git descriptions are equal if both their URLs and their refs are

View file

@ -56,7 +56,7 @@ class HostedSource extends Source {
"${id.version}.yaml";
return httpClient.read(fullUrl).then((yaml) {
return new Pubspec.parse(yaml, systemCache.sources);
return new Pubspec.parse(null, yaml, systemCache.sources);
}).catchError((ex) {
_throwFriendlyError(ex, id, parsed.last);
});
@ -114,8 +114,10 @@ class HostedSource extends Source {
/// There are two valid formats. A plain string refers to a package with the
/// given name from the default host, while a map with keys "name" and "url"
/// refers to a package with the given name from the host at the given URL.
void validateDescription(description, {bool fromLockFile: false}) {
dynamic parseDescription(String containingPath, description,
{bool fromLockFile: false}) {
_parseDescription(description);
return description;
}
/// When an error occurs trying to read something about [package] from [url],

View file

@ -264,15 +264,32 @@ Future _attemptRetryable(Future callback()) {
return makeAttempt(null);
}
/// Creates a new symlink that creates an alias from [from] to [to]. Returns a
/// [Future] which completes to the symlink file (i.e. [to]).
/// Creates a new symlink at path [symlink] that points to [target]. Returns a
/// [Future] which completes to the path to the symlink file.
///
/// If [relative] is true, creates a symlink with a relative path from the
/// symlink to the target. Otherwise, uses the [target] path unmodified.
///
/// Note that on Windows, only directories may be symlinked to.
Future<String> createSymlink(String from, String to) {
log.fine("Creating symlink ($to is a symlink to $from)");
Future<String> createSymlink(String target, String symlink,
{bool relative: false}) {
if (relative) {
// Relative junction points are not supported on Windows. Instead, just
// make sure we have a clean absolute path because it will interpret a
// relative path to be relative to the cwd, not the symlink, and will be
// confused by forward slashes.
if (Platform.operatingSystem == 'windows') {
target = path.normalize(path.absolute(target));
} else {
target = path.normalize(
path.relative(target, from: path.dirname(symlink)));
}
}
log.fine("Creating $symlink pointing to $target");
var command = 'ln';
var args = ['-s', from, to];
var args = ['-s', target, symlink];
if (Platform.operatingSystem == 'windows') {
// Call mklink on Windows to create an NTFS junction point. Only works on
@ -281,24 +298,29 @@ Future<String> createSymlink(String from, String to) {
// link (/d) because the latter requires some privilege shenanigans that
// I'm not sure how to specify from the command line.
command = 'mklink';
args = ['/j', to, from];
args = ['/j', symlink, target];
}
// TODO(rnystrom): Check exit code and output?
return runProcess(command, args).then((result) => to);
return runProcess(command, args).then((result) => symlink);
}
/// Creates a new symlink that creates an alias from the `lib` directory of
/// package [from] to [to]. Returns a [Future] which completes to the symlink
/// file (i.e. [to]). If [from] does not have a `lib` directory, this shows a
/// warning if appropriate and then does nothing.
Future<String> createPackageSymlink(String name, String from, String to,
{bool isSelfLink: false}) {
/// Creates a new symlink that creates an alias at [symlink] that points to the
/// `lib` directory of package [target]. Returns a [Future] which completes to
/// the path to the symlink file. If [target] does not have a `lib` directory,
/// this shows a warning if appropriate and then does nothing.
///
/// If [relative] is true, creates a symlink with a relative path from the
/// symlink to the target. Otherwise, uses the [target] path unmodified.
Future<String> createPackageSymlink(String name, String target, String symlink,
{bool isSelfLink: false, bool relative: false}) {
return defer(() {
// See if the package has a "lib" directory.
from = path.join(from, 'lib');
target = path.join(target, 'lib');
log.fine("Creating ${isSelfLink ? "self" : ""}link for package '$name'.");
if (dirExists(from)) return createSymlink(from, to);
if (dirExists(target)) {
return createSymlink(target, symlink, relative: relative);
}
// It's OK for the self link (i.e. the root package) to not have a lib
// directory since it may just be a leaf application that only has
@ -308,7 +330,7 @@ Future<String> createPackageSymlink(String name, String from, String to,
'you will not be able to import any libraries from it.');
}
return to;
return symlink;
});
}

View file

@ -5,6 +5,7 @@
library lock_file;
import 'dart:json' as json;
import 'io.dart';
import 'package.dart';
import 'source_registry.dart';
import 'utils.dart';
@ -21,8 +22,19 @@ class LockFile {
LockFile.empty()
: packages = <String, PackageId>{};
/// Parses the lockfile whose text is [contents].
/// Loads a lockfile from [filePath].
factory LockFile.load(String filePath, SourceRegistry sources) {
return LockFile._parse(filePath, readTextFile(filePath), sources);
}
/// Parses a lockfile whose text is [contents].
factory LockFile.parse(String contents, SourceRegistry sources) {
return LockFile._parse(null, contents, sources);
}
/// Parses the lockfile whose text is [contents].
static LockFile _parse(String filePath, String contents,
SourceRegistry sources) {
var packages = <String, PackageId>{};
if (contents.trim() == '') return new LockFile.empty();
@ -54,7 +66,8 @@ class LockFile {
throw new FormatException('Package $name is missing a description.');
}
var description = spec['description'];
source.validateDescription(description, fromLockFile: true);
description = source.parseDescription(filePath, description,
fromLockFile: true);
var id = new PackageId(name, source, version, description);

View file

@ -25,45 +25,77 @@ class PathSource extends Source {
Future<Pubspec> describe(PackageId id) {
return defer(() {
_validatePath(id.name, id.description);
return new Pubspec.load(id.name, id.description, systemCache.sources);
return new Pubspec.load(id.name, id.description["path"],
systemCache.sources);
});
}
Future<bool> install(PackageId id, String path) {
Future<bool> install(PackageId id, String destination) {
return defer(() {
try {
_validatePath(id.name, id.description);
} on FormatException catch(err) {
return false;
}
return createPackageSymlink(id.name, id.description, path);
return createPackageSymlink(id.name, id.description["path"], destination,
relative: id.description["relative"]);
}).then((_) => true);
}
void validateDescription(description, {bool fromLockFile: false}) {
/// Parses a path dependency. This takes in a path string and returns a map.
/// The "path" key will be the original path but resolves relative to the
/// containing path. The "relative" key will be `true` if the original path
/// was relative.
///
/// A path coming from a pubspec is a simple string. From a lock file, it's
/// an expanded {"path": ..., "relative": ...} map.
dynamic parseDescription(String containingPath, description,
{bool fromLockFile: false}) {
if (fromLockFile) {
if (description is! Map) {
throw new FormatException("The description must be a map.");
}
if (description["path"] is! String) {
throw new FormatException("The 'path' field of the description must "
"be a string.");
}
if (description["relative"] is! bool) {
throw new FormatException("The 'relative' field of the description "
"must be a boolean.");
}
return description;
}
if (description is! String) {
throw new FormatException("The description must be a path string.");
}
// Resolve the path relative to the containing file path, and remember
// whether the original path was relative or absolute.
bool isRelative = path.isRelative(description);
if (path.isRelative(description)) {
// Can't handle relative paths coming from pubspecs that are not on the
// local file system.
assert(containingPath != null);
description = path.join(path.dirname(containingPath), description);
}
return {
"path": description,
"relative": isRelative
};
}
/// Ensures that [dir] is a valid path. It must be an absolute path that
/// points to an existing directory. Throws a [FormatException] if the path
/// is invalid.
void _validatePath(String name, String dir) {
// Relative paths are not (currently) allowed because the user would expect
// them to be relative to the pubspec where the dependency appears. That in
// turn means that two pubspecs in different locations with the same
// relative path dependency could refer to two different packages. That
// violates pub's rule that a description should uniquely identify a
// package.
//
// At some point, we may want to loosen this, but it will mean tracking
// where a given PackageId appeared.
if (!path.isAbsolute(dir)) {
throw new FormatException(
"Path dependency for package '$name' must be an absolute path. "
"Was '$dir'.");
}
/// Ensures that [description] is a valid path description. It must be a map,
/// with a "path" key containing a path that points to an existing directory.
/// Throws a [FormatException] if the path is invalid.
void _validatePath(String name, description) {
var dir = description["path"];
if (fileExists(dir)) {
throw new FormatException(
@ -75,4 +107,4 @@ class PathSource extends Source {
throw new FormatException("Could not find package '$name' at '$dir'.");
}
}
}
}

View file

@ -38,7 +38,8 @@ class Pubspec {
if (!fileExists(pubspecPath)) throw new PubspecNotFoundException(name);
try {
var pubspec = new Pubspec.parse(readTextFile(pubspecPath), sources);
var pubspec = new Pubspec.parse(pubspecPath, readTextFile(pubspecPath),
sources);
if (pubspec.name == null) {
throw new PubspecHasNoNameException(name);
@ -69,10 +70,14 @@ class Pubspec {
bool get isEmpty =>
name == null && version == Version.none && dependencies.isEmpty;
// TODO(rnystrom): Make this a static method to match corelib.
/// Parses the pubspec whose text is [contents]. If the pubspec doesn't define
/// version for itself, it defaults to [Version.none].
factory Pubspec.parse(String contents, SourceRegistry sources) {
// TODO(rnystrom): Instead of allowing a null argument here, split this up
// into load(), parse(), and _parse() like LockFile does.
/// Parses the pubspec stored at [filePath] whose text is [contents]. If the
/// pubspec doesn't define version for itself, it defaults to [Version.none].
/// [filePath] may be `null` if the pubspec is not on the user's local
/// file system.
factory Pubspec.parse(String filePath, String contents,
SourceRegistry sources) {
var name = null;
var version = Version.none;
@ -97,7 +102,7 @@ class Pubspec {
version = new Version.parse(parsedPubspec['version']);
}
var dependencies = _parseDependencies(sources,
var dependencies = _parseDependencies(filePath, sources,
parsedPubspec['dependencies']);
var environmentYaml = parsedPubspec['environment'];
@ -187,7 +192,8 @@ void _validateFieldUrl(url, String field) {
}
}
List<PackageRef> _parseDependencies(SourceRegistry sources, yaml) {
List<PackageRef> _parseDependencies(String pubspecPath, SourceRegistry sources,
yaml) {
var dependencies = <PackageRef>[];
// Allow an empty dependencies key.
@ -233,7 +239,8 @@ List<PackageRef> _parseDependencies(SourceRegistry sources, yaml) {
'Dependency specification $spec should be a string or a mapping.');
}
source.validateDescription(description, fromLockFile: false);
description = source.parseDescription(pubspecPath, description,
fromLockFile: false);
dependencies.add(new PackageRef(
name, source, versionConstraint, description));

View file

@ -169,13 +169,23 @@ abstract class Source {
/// When a [Pubspec] or [LockFile] is parsed, it reads in the description for
/// each dependency. It is up to the dependency's [Source] to determine how
/// that should be interpreted. This will be called during parsing to validate
/// that the given [description] is well-formed according to this source. It
/// should return if the description is valid, or throw a [FormatException] if
/// not.
/// that 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).
///
/// It should return if a (possibly modified) valid description, or throw a
/// [FormatException] if not valid.
///
/// [fromLockFile] is true when the description comes from a [LockFile], to
/// allow the source to use lockfile-specific descriptions via [resolveId].
void validateDescription(description, {bool fromLockFile: false}) {}
dynamic parseDescription(String containingPath, description,
{bool fromLockFile: false}) {
return description;
}
/// Returns whether or not [description1] describes the same package as
/// [description2] for this source. This method should be light-weight. It
@ -199,7 +209,7 @@ abstract class Source {
/// the resolved id.
///
/// The returned [PackageId] may have a description field that's invalid
/// according to [validateDescription], although it must still be serializable
/// according to [parseDescription], although it must still be serializable
/// to JSON and YAML. It must also be equal to [id] according to
/// [descriptionsEqual].
///

View file

@ -0,0 +1,45 @@
// Copyright (c) 2013, the Dart project authors. Please see the AUTHORS file
// for details. All rights reserved. Use of this source code is governed by a
// BSD-style license that can be found in the LICENSE file.
import '../../../../../pkg/path/lib/path.dart' as path;
import '../../../../pub/exit_codes.dart' as exit_codes;
import '../../test_pub.dart';
main() {
initConfig();
integration("generates a symlink with an absolute path if the dependency "
"path was absolute", () {
dir("foo", [
libDir("foo"),
libPubspec("foo", "0.0.1")
]).scheduleCreate();
dir(appPath, [
pubspec({
"name": "myapp",
"dependencies": {
"foo": {"path": path.join(sandboxDir, "foo")}
}
})
]).scheduleCreate();
schedulePub(args: ["install"],
output: new RegExp(r"Dependencies installed!$"));
dir("moved").scheduleCreate();
// Move the app but not the package. Since the symlink is absolute, it
// should still be able to find it.
scheduleRename(appPath, path.join("moved", appPath));
dir("moved", [
dir(packagesPath, [
dir("foo", [
file("foo.dart", 'main() => "foo";')
])
])
]).scheduleValidate();
});
}

View file

@ -2,12 +2,19 @@
// for details. All rights reserved. Use of this source code is governed by a
// BSD-style license that can be found in the LICENSE file.
import '../../../../../pkg/path/lib/path.dart' as path;
import '../../../../pub/exit_codes.dart' as exit_codes;
import '../../test_pub.dart';
main() {
initConfig();
integration('path dependencies cannot use relative paths', () {
integration("can use relative path", () {
dir("foo", [
libDir("foo"),
libPubspec("foo", "0.0.1")
]).scheduleCreate();
dir(appPath, [
pubspec({
"name": "myapp",
@ -17,9 +24,49 @@ main() {
})
]).scheduleCreate();
schedulePub(args: ['install'],
error: new RegExp("Path dependency for package 'foo' must be an "
"absolute path. Was '../foo'."),
exitCode: exit_codes.DATA);
schedulePub(args: ["install"],
output: new RegExp(r"Dependencies installed!$"));
dir(packagesPath, [
dir("foo", [
file("foo.dart", 'main() => "foo";')
])
]).scheduleValidate();
});
}
integration("path is relative to containing pubspec", () {
dir("relative", [
dir("foo", [
libDir("foo"),
libPubspec("foo", "0.0.1", deps: [
{"path": "../bar"}
])
]),
dir("bar", [
libDir("bar"),
libPubspec("bar", "0.0.1")
])
]).scheduleCreate();
dir(appPath, [
pubspec({
"name": "myapp",
"dependencies": {
"foo": {"path": "../relative/foo"}
}
})
]).scheduleCreate();
schedulePub(args: ["install"],
output: new RegExp(r"Dependencies installed!$"));
dir(packagesPath, [
dir("foo", [
file("foo.dart", 'main() => "foo";')
]),
dir("bar", [
file("bar.dart", 'main() => "bar";')
])
]).scheduleValidate();
});
}

View file

@ -0,0 +1,54 @@
// Copyright (c) 2013, the Dart project authors. Please see the AUTHORS file
// for details. All rights reserved. Use of this source code is governed by a
// BSD-style license that can be found in the LICENSE file.
import 'dart:io';
import '../../../../../pkg/path/lib/path.dart' as path;
import '../../../../pub/exit_codes.dart' as exit_codes;
import '../../test_pub.dart';
main() {
// Pub uses NTFS junction points to create links in the packages directory.
// These (unlike the symlinks that are supported in Vista and later) do not
// support relative paths. So this test, by design, will not pass on Windows.
// So just skip it.
if (Platform.operatingSystem == "windows") return;
initConfig();
integration("generates a symlink with a relative path if the dependency "
"path was relative", () {
dir("foo", [
libDir("foo"),
libPubspec("foo", "0.0.1")
]).scheduleCreate();
dir(appPath, [
pubspec({
"name": "myapp",
"dependencies": {
"foo": {"path": "../foo"}
}
})
]).scheduleCreate();
schedulePub(args: ["install"],
output: new RegExp(r"Dependencies installed!$"));
dir("moved").scheduleCreate();
// Move the app and package. Since they are still next to each other, it
// should still be found.
scheduleRename("foo", path.join("moved", "foo"));
scheduleRename(appPath, path.join("moved", appPath));
dir("moved", [
dir(packagesPath, [
dir("foo", [
file("foo.dart", 'main() => "foo";')
])
])
]).scheduleValidate();
});
}

View file

@ -17,8 +17,10 @@ class MockSource extends Source {
final String name = 'mock';
final bool shouldCache = false;
void validateDescription(String description, {bool fromLockFile: false}) {
dynamic parseDescription(String filePath, String description,
{bool fromLockFile: false}) {
if (!description.endsWith(' desc')) throw new FormatException();
return description;
}
String packageName(String description) {

View file

@ -15,8 +15,10 @@ import '../../pub/version.dart';
class MockSource extends Source {
final String name = "mock";
final bool shouldCache = false;
void validateDescription(description, {bool fromLockFile: false}) {
dynamic parseDescription(String filePath, description,
{bool fromLockFile: false}) {
if (description != 'ok') throw new FormatException('Bad');
return description;
}
String packageName(description) => 'foo';
}
@ -29,12 +31,12 @@ main() {
sources.register(new MockSource());
expectFormatError(String pubspec) {
expect(() => new Pubspec.parse(pubspec, sources),
expect(() => new Pubspec.parse(null, pubspec, sources),
throwsFormatException);
}
test("allows a version constraint for dependencies", () {
var pubspec = new Pubspec.parse('''
var pubspec = new Pubspec.parse(null, '''
dependencies:
foo:
mock: ok
@ -49,7 +51,7 @@ dependencies:
});
test("allows an empty dependencies map", () {
var pubspec = new Pubspec.parse('''
var pubspec = new Pubspec.parse(null, '''
dependencies:
''', sources);
@ -74,8 +76,8 @@ dependencies:
});
test("throws if 'homepage' doesn't have an HTTP scheme", () {
new Pubspec.parse('homepage: http://ok.com', sources);
new Pubspec.parse('homepage: https://also-ok.com', sources);
new Pubspec.parse(null, 'homepage: http://ok.com', sources);
new Pubspec.parse(null, 'homepage: https://also-ok.com', sources);
expectFormatError('homepage: ftp://badscheme.com');
expectFormatError('homepage: javascript:alert("!!!")');
@ -89,8 +91,8 @@ dependencies:
});
test("throws if 'documentation' doesn't have an HTTP scheme", () {
new Pubspec.parse('documentation: http://ok.com', sources);
new Pubspec.parse('documentation: https://also-ok.com', sources);
new Pubspec.parse(null, 'documentation: http://ok.com', sources);
new Pubspec.parse(null, 'documentation: https://also-ok.com', sources);
expectFormatError('documentation: ftp://badscheme.com');
expectFormatError('documentation: javascript:alert("!!!")');
@ -99,8 +101,8 @@ dependencies:
});
test("throws if 'authors' is not a string or a list of strings", () {
new Pubspec.parse('authors: ok fine', sources);
new Pubspec.parse('authors: [also, ok, fine]', sources);
new Pubspec.parse(null, 'authors: ok fine', sources);
new Pubspec.parse(null, 'authors: [also, ok, fine]', sources);
expectFormatError('authors: 123');
expectFormatError('authors: {not: {a: string}}');
@ -108,7 +110,7 @@ dependencies:
});
test("throws if 'author' is not a string", () {
new Pubspec.parse('author: ok fine', sources);
new Pubspec.parse(null, 'author: ok fine', sources);
expectFormatError('author: 123');
expectFormatError('author: {not: {a: string}}');
@ -120,7 +122,7 @@ dependencies:
});
test("allows comment-only files", () {
var pubspec = new Pubspec.parse('''
var pubspec = new Pubspec.parse(null, '''
# No external dependencies yet
# Including for completeness
# ...and hoping the spec expands to include details about author, version, etc
@ -132,12 +134,12 @@ dependencies:
group("environment", () {
test("defaults to any SDK constraint if environment is omitted", () {
var pubspec = new Pubspec.parse('', sources);
var pubspec = new Pubspec.parse(null, '', sources);
expect(pubspec.environment.sdkVersion, equals(VersionConstraint.any));
});
test("allows an empty environment map", () {
var pubspec = new Pubspec.parse('''
var pubspec = new Pubspec.parse(null, '''
environment:
''', sources);
expect(pubspec.environment.sdkVersion, equals(VersionConstraint.any));
@ -150,7 +152,7 @@ environment: []
});
test("allows a version constraint for the sdk", () {
var pubspec = new Pubspec.parse('''
var pubspec = new Pubspec.parse(null, '''
environment:
sdk: ">=1.2.3 <2.3.4"
''', sources);