Revert "Support relative paths in path dependencies."

This reverts commit 0b0da0d44dde213647e7281bf0914c59fc552b27.

BUG=

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

git-svn-id: https://dart.googlecode.com/svn/branches/bleeding_edge/dart@18606 260f80e4-7a28-3924-810f-c04153c831b5
This commit is contained in:
rnystrom@google.com 2013-02-16 02:00:45 +00:00
parent 8514e448b7
commit 5ed85458d1
14 changed files with 92 additions and 319 deletions

View file

@ -82,7 +82,7 @@ class Entrypoint {
}).then((_) {
if (id.source.shouldCache) {
return cache.install(id).then(
(pkg) => createPackageSymlink(id.name, packageDir, pkg.dir));
(pkg) => createPackageSymlink(id.name, pkg.dir, packageDir));
} else {
return id.source.install(id, packageDir).then((found) {
if (found) return null;
@ -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.load(lockFilePath, cache.sources);
return new LockFile.parse(readTextFile(lockFilePath), cache.sources);
}
/// Saves a list of concrete package versions to the `pubspec.lock` file.
@ -231,7 +231,7 @@ class Entrypoint {
// Create the symlink if it doesn't exist.
if (entryExists(linkPath)) return;
ensureDir(packagesDir);
return createPackageSymlink(root.name, linkPath, root.dir,
return createPackageSymlink(root.name, root.dir, linkPath,
isSelfLink: true);
});
}
@ -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 symlink = path.join(dir, 'packages');
if (entryExists(symlink)) return;
return createSymlink(packagesDir, symlink);
var to = path.join(dir, 'packages');
if (entryExists(to)) return;
return createSymlink(packagesDir, to);
});
}
}

View file

@ -10,7 +10,6 @@ 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';
@ -68,33 +67,24 @@ class GitSource extends Source {
return path.join(systemCacheRoot, revisionCacheName);
});
}
/// Ensures [description] is a Git URL.
dynamic parseDescription(String containingPath, description,
{bool fromLockFile: false}) {
// TODO(rnystrom): Handle git URLs that are relative file paths (#8570).
void validateDescription(description, {bool fromLockFile: false}) {
// A single string is assumed to be a Git URL.
// 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.
if (description is String) return description;
if (description is String) return;
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');
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(', ');
if (!description.isEmpty) {
var plural = description.length > 1;
var keys = description.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(null, yaml, systemCache.sources);
return new Pubspec.parse(yaml, systemCache.sources);
}).catchError((ex) {
_throwFriendlyError(ex, id, parsed.last);
});
@ -114,10 +114,8 @@ 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.
dynamic parseDescription(String containingPath, description,
{bool fromLockFile: false}) {
void validateDescription(description, {bool fromLockFile: false}) {
_parseDescription(description);
return description;
}
/// When an error occurs trying to read something about [package] from [url],

View file

@ -264,23 +264,15 @@ Future _attemptRetryable(Future callback()) {
return makeAttempt(null);
}
/// 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.
/// Creates a new symlink that creates an alias from [from] to [to]. Returns a
/// [Future] which completes to the symlink file (i.e. [to]).
///
/// Note that on Windows, only directories may be symlinked to.
Future<String> createSymlink(String target, String symlink,
{bool relative: false}) {
if (relative) {
target = path.normalize(path.relative(target, from: path.dirname(symlink)));
}
log.fine("Creating $symlink pointing to $target");
Future<String> createSymlink(String from, String to) {
log.fine("Creating symlink ($to is a symlink to $from)");
var command = 'ln';
var args = ['-s', target, symlink];
var args = ['-s', from, to];
if (Platform.operatingSystem == 'windows') {
// Call mklink on Windows to create an NTFS junction point. Only works on
@ -289,29 +281,24 @@ Future<String> createSymlink(String target, String symlink,
// 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', symlink, target];
args = ['/j', to, from];
}
// TODO(rnystrom): Check exit code and output?
return runProcess(command, args).then((result) => symlink);
return runProcess(command, args).then((result) => to);
}
/// 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 symlink, String target,
{bool isSelfLink: false, bool relative: false}) {
/// 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}) {
return defer(() {
// See if the package has a "lib" directory.
target = path.join(target, 'lib');
from = path.join(from, 'lib');
log.fine("Creating ${isSelfLink ? "self" : ""}link for package '$name'.");
if (dirExists(target)) {
return createSymlink(target, symlink, relative: relative);
}
if (dirExists(from)) return createSymlink(from, to);
// 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
@ -321,7 +308,7 @@ Future<String> createPackageSymlink(String name, String symlink, String target,
'you will not be able to import any libraries from it.');
}
return symlink;
return to;
});
}

View file

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

View file

@ -25,77 +25,45 @@ class PathSource extends Source {
Future<Pubspec> describe(PackageId id) {
return defer(() {
_validatePath(id.name, id.description);
return new Pubspec.load(id.name, id.description["path"],
systemCache.sources);
return new Pubspec.load(id.name, id.description, systemCache.sources);
});
}
Future<bool> install(PackageId id, String destination) {
Future<bool> install(PackageId id, String path) {
return defer(() {
try {
_validatePath(id.name, id.description);
} on FormatException catch(err) {
return false;
}
return createPackageSymlink(id.name, destination, id.description["path"],
relative: id.description["relative"]);
return createPackageSymlink(id.name, id.description, path);
}).then((_) => true);
}
/// 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;
}
void validateDescription(description, {bool fromLockFile: false}) {
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 [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"];
/// 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'.");
}
if (fileExists(dir)) {
throw new FormatException(

View file

@ -38,8 +38,7 @@ class Pubspec {
if (!fileExists(pubspecPath)) throw new PubspecNotFoundException(name);
try {
var pubspec = new Pubspec.parse(pubspecPath, readTextFile(pubspecPath),
sources);
var pubspec = new Pubspec.parse(readTextFile(pubspecPath), sources);
if (pubspec.name == null) {
throw new PubspecHasNoNameException(name);
@ -70,12 +69,10 @@ class Pubspec {
bool get isEmpty =>
name == null && version == Version.none && dependencies.isEmpty;
/// 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) {
// 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) {
var name = null;
var version = Version.none;
@ -100,7 +97,7 @@ class Pubspec {
version = new Version.parse(parsedPubspec['version']);
}
var dependencies = _parseDependencies(filePath, sources,
var dependencies = _parseDependencies(sources,
parsedPubspec['dependencies']);
var environmentYaml = parsedPubspec['environment'];
@ -190,8 +187,7 @@ void _validateFieldUrl(url, String field) {
}
}
List<PackageRef> _parseDependencies(String pubspecPath, SourceRegistry sources,
yaml) {
List<PackageRef> _parseDependencies(SourceRegistry sources, yaml) {
var dependencies = <PackageRef>[];
// Allow an empty dependencies key.
@ -237,8 +233,7 @@ List<PackageRef> _parseDependencies(String pubspecPath, SourceRegistry sources,
'Dependency specification $spec should be a string or a mapping.');
}
description = source.parseDescription(pubspecPath, description,
fromLockFile: false);
source.validateDescription(description, fromLockFile: false);
dependencies.add(new PackageRef(
name, source, versionConstraint, description));

View file

@ -41,7 +41,7 @@ class SdkSource extends Source {
var path = _getPackagePath(id);
if (path == null) return false;
return createPackageSymlink(id.name, destPath, path).then((_) => true);
return createPackageSymlink(id.name, path, destPath).then((_) => true);
});
}

View file

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

View file

@ -1,45 +0,0 @@
// 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,19 +2,12 @@
// 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("can use relative path", () {
dir("foo", [
libDir("foo"),
libPubspec("foo", "0.0.1")
]).scheduleCreate();
integration('path dependencies cannot use relative paths', () {
dir(appPath, [
pubspec({
"name": "myapp",
@ -24,49 +17,9 @@ main() {
})
]).scheduleCreate();
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();
schedulePub(args: ['install'],
error: new RegExp("Path dependency for package 'foo' must be an "
"absolute path. Was '../foo'."),
exitCode: exit_codes.DATA);
});
}

View file

@ -1,46 +0,0 @@
// 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 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,10 +17,8 @@ class MockSource extends Source {
final String name = 'mock';
final bool shouldCache = false;
dynamic parseDescription(String filePath, String description,
{bool fromLockFile: false}) {
void validateDescription(String description, {bool fromLockFile: false}) {
if (!description.endsWith(' desc')) throw new FormatException();
return description;
}
String packageName(String description) {

View file

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