Handle relative git-url-paths correctly when --directory (#2919)
diff --git a/lib/src/command/add.dart b/lib/src/command/add.dart
index d74dd6c..7d7d19e 100644
--- a/lib/src/command/add.dart
+++ b/lib/src/command/add.dart
@@ -2,6 +2,8 @@
// 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 'package:path/path.dart' as p;
+import 'package:pub/src/source/path.dart';
import 'package:pub_semver/pub_semver.dart';
import 'package:yaml/yaml.dart';
@@ -313,25 +315,45 @@
if (gitUrl == null) {
usageException('The `--git-url` is required for git dependencies.');
}
+ Uri parsed;
+ try {
+ parsed = Uri.parse(gitUrl);
+ } on FormatException catch (e) {
+ usageException('The --git-url must be a valid url: ${e.message}.');
+ }
+ final urlRelativeToEntrypoint = parsed.isAbsolute
+ ? parsed.toString()
+ :
+ // Turn the relative url from current working directory into a relative
+ // url from the entrypoint.
+ p.url.relative(
+ p.url.join(Uri.file(p.absolute(p.current)).toString(),
+ parsed.toString()),
+ from: p.toUri(p.absolute(entrypoint.root.dir)).toString());
/// Process the git options to return the simplest representation to be
/// added to the pubspec.
if (gitRef == null && gitPath == null) {
- git = gitUrl;
+ git = urlRelativeToEntrypoint;
} else {
- git = {'url': gitUrl, 'ref': gitRef, 'path': gitPath};
+ git = {'url': urlRelativeToEntrypoint, 'ref': gitRef, 'path': gitPath};
git.removeWhere((key, value) => value == null);
}
packageRange = cache.sources['git']
- .parseRef(packageName, git)
+ .parseRef(packageName, git, containingPath: entrypoint.pubspecPath)
.withConstraint(constraint ?? VersionConstraint.any);
pubspecInformation = {'git': git};
} else if (path != null) {
+ final relativeToEntryPoint = p.isRelative(path)
+ ? PathSource.relativePathWithPosixSeparators(
+ p.relative(path, from: entrypoint.root.dir))
+ : path;
packageRange = cache.sources['path']
- .parseRef(packageName, path, containingPath: entrypoint.pubspecPath)
+ .parseRef(packageName, relativeToEntryPoint,
+ containingPath: entrypoint.pubspecPath)
.withConstraint(constraint ?? VersionConstraint.any);
- pubspecInformation = {'path': path};
+ pubspecInformation = {'path': relativeToEntryPoint};
} else if (sdk != null) {
packageRange = cache.sources['sdk']
.parseRef(packageName, sdk)
diff --git a/lib/src/global_packages.dart b/lib/src/global_packages.dart
index 726a1a4..595c604 100644
--- a/lib/src/global_packages.dart
+++ b/lib/src/global_packages.dart
@@ -90,9 +90,14 @@
// dependencies. Their executables shouldn't be cached, and there should
// be a mechanism for redoing dependency resolution if a path pubspec has
// changed (see also issue 20499).
+ PackageRef ref;
+ try {
+ ref = cache.git.source.parseRef(name, {'url': repo}, containingPath: '.');
+ } on FormatException catch (e) {
+ throw ApplicationException(e.message);
+ }
await _installInCache(
- cache.git.source
- .refFor(name, repo)
+ ref
.withConstraint(VersionConstraint.any)
.withFeatures(features ?? const {}),
executables,
@@ -282,7 +287,8 @@
var oldPath = p.join(_directory, '$package.lock');
if (fileExists(oldPath)) deleteEntry(oldPath);
- writeTextFile(_getLockFilePath(package), lockFile.serialize(cache.rootDir));
+ writeTextFile(_getLockFilePath(package),
+ lockFile.serialize(p.join(_directory, package)));
}
/// Shows the user the currently active package with [name], if any.
diff --git a/lib/src/source/git.dart b/lib/src/source/git.dart
index c986015..adcfee7 100644
--- a/lib/src/source/git.dart
+++ b/lib/src/source/git.dart
@@ -29,61 +29,44 @@
BoundGitSource bind(SystemCache systemCache) =>
BoundGitSource(this, systemCache);
- /// Returns a reference to a git package with the given [name] and [url].
- ///
- /// If passed, [reference] is the Git reference. It defaults to `"HEAD"`.
- PackageRef refFor(String name, String url, {String reference, String path}) {
- if (path != null) assert(p.url.isRelative(path));
- return PackageRef(name, this,
- {'url': url, 'ref': reference ?? 'HEAD', 'path': path ?? '.'});
- }
-
/// Given a valid git package description, returns the URL of the repository
/// it pulls from.
- String urlFromDescription(description) => description['url'];
+ /// If the url is relative, it will be returned relative to current working
+ /// directory.
+ String urlFromDescription(description) {
+ var url = description['url'];
+ if (description['relative'] == true) {
+ return p.url.relative(url, from: p.toUri(p.current).toString());
+ }
+ return url;
+ }
@override
PackageRef parseRef(String name, description, {String containingPath}) {
- // TODO(rnystrom): Handle git URLs that are relative file paths (#8570).
- if (description is String) description = {'url': description};
-
- if (description is! Map) {
+ dynamic url;
+ dynamic ref;
+ dynamic path;
+ if (description is String) {
+ url = description;
+ } else if (description is! Map) {
throw FormatException('The description must be a Git URL or a map '
"with a 'url' key.");
- }
+ } else {
+ url = description['url'];
- if (description['url'] is! String) {
- throw FormatException("The 'url' field of the description must be a "
- 'string.');
- }
-
- _validateUrl(description['url']);
-
- var ref = description['ref'];
- if (ref != null && ref is! String) {
- throw FormatException("The 'ref' field of the description must be a "
- 'string.');
- }
-
- var path = description['path'];
- if (path != null) {
- if (path is! String) {
- throw FormatException(
- "The 'path' field of the description must be a string.");
- } else if (!p.url.isRelative(path)) {
- throw FormatException(
- "The 'path' field of the description must be relative.");
- } else if (!p.url.isWithin('.', path) && !p.url.equals('.', path)) {
- throw FormatException(
- "The 'path' field of the description must not reach outside the "
- 'repository.');
+ ref = description['ref'];
+ if (ref != null && ref is! String) {
+ throw FormatException("The 'ref' field of the description must be a "
+ 'string.');
}
- _validateUrl(path);
+ path = description['path'];
}
-
- return PackageRef(name, this,
- {'url': description['url'], 'ref': ref ?? 'HEAD', 'path': path ?? '.'});
+ return PackageRef(name, this, {
+ ..._validatedUrl(url, containingPath),
+ 'ref': ref ?? 'HEAD',
+ 'path': _validatedPath(path),
+ });
}
@override
@@ -94,13 +77,6 @@
'key.');
}
- if (description['url'] is! String) {
- throw FormatException("The 'url' field of the description must be a "
- 'string.');
- }
-
- _validateUrl(description['url']);
-
var ref = description['ref'];
if (ref != null && ref is! String) {
throw FormatException("The 'ref' field of the description must be a "
@@ -112,35 +88,78 @@
'must be a string.');
}
- var path = description['path'];
- if (path != null) {
- if (path is! String) {
- throw FormatException(
- "The 'path' field of the description must be a string.");
- } else if (!p.url.isRelative(path)) {
- throw FormatException(
- "The 'path' field of the description must be relative.");
- }
-
- _validateUrl(path);
- }
-
return PackageId(name, this, version, {
- 'url': description['url'],
+ ..._validatedUrl(description['url'], containingPath),
'ref': ref ?? 'HEAD',
'resolved-ref': description['resolved-ref'],
- 'path': path ?? '.'
+ 'path': _validatedPath(description['path'])
});
}
+ /// Serializes path dependency's [description].
+ ///
+ /// For the descriptions where `relative` attribute is `true`, tries to make
+ /// `url` relative to the specified [containingPath].
+ @override
+ dynamic serializeDescription(String containingPath, description) {
+ final copy = Map.from(description);
+ copy.remove('relative');
+ if (description['relative'] == true) {
+ copy['url'] = p.url.relative(description['url'],
+ from: Uri.file(containingPath).toString());
+ }
+ return copy;
+ }
+
/// Throws a [FormatException] if [url] isn't a valid Git URL.
- void _validateUrl(String url) {
+ Map<String, Object> _validatedUrl(dynamic url, String containingDir) {
+ if (url is! String) {
+ throw FormatException("The 'url' field of the description must be a "
+ 'string.');
+ }
+ var relative = false;
// If the URL contains an @, it's probably an SSH hostname, which we don't
// know how to validate.
- if (url.contains('@')) return;
+ if (!url.contains('@')) {
+ // Otherwise, we use Dart's URL parser to validate the URL.
+ final parsed = Uri.parse(url);
+ if (!parsed.hasAbsolutePath) {
+ // Relative paths coming from pubspecs that are not on the local file
+ // system aren't allowed. This can happen if a hosted or git dependency
+ // has a git dependency.
+ if (containingDir == null) {
+ throw FormatException('"$url" is a relative path, but this '
+ 'isn\'t a local pubspec.');
+ }
+ // A relative path is stored internally as absolute resolved relative to
+ // [containingPath].
+ relative = true;
+ url = Uri.file(p.absolute(containingDir)).resolveUri(parsed).toString();
+ }
+ }
+ return {'relative': relative, 'url': url};
+ }
- // Otherwise, we use Dart's URL parser to validate the URL.
- Uri.parse(url);
+ /// Throws a [FormatException] if [path] isn't a relative url or null.
+ String _validatedPath(dynamic path) {
+ path ??= '.';
+ if (path is! String) {
+ throw FormatException("The 'path' field of the description must be a "
+ 'string.');
+ }
+
+ // Use Dart's URL parser to validate the URL.
+ final parsed = Uri.parse(path);
+ if (parsed.isAbsolute) {
+ throw FormatException(
+ "The 'path' field of the description must be relative.");
+ }
+ if (!p.url.isWithin('.', path) && !p.url.equals('.', path)) {
+ throw FormatException(
+ "The 'path' field of the description must not reach outside the "
+ 'repository.');
+ }
+ return p.normalize(parsed.toString());
}
/// If [description] has a resolved ref, print it out in short-form.
@@ -150,7 +169,7 @@
@override
String formatDescription(description) {
if (description is Map && description.containsKey('resolved-ref')) {
- var result = "${description['url']} at "
+ var result = '${urlFromDescription(description)} at '
"${description['resolved-ref'].substring(0, 6)}";
if (description['path'] != '.') result += " in ${description["path"]}";
return result;
@@ -230,12 +249,17 @@
await _ensureRepoCache(ref);
var path = _repoCachePath(ref);
var revision = await _firstRevision(path, ref.description['ref']);
- var pubspec =
- await _describeUncached(ref, revision, ref.description['path']);
+ var pubspec = await _describeUncached(
+ ref,
+ revision,
+ ref.description['path'],
+ source.urlFromDescription(ref.description),
+ );
return [
PackageId(ref.name, source, pubspec.version, {
'url': ref.description['url'],
+ 'relative': ref.description['relative'],
'ref': ref.description['ref'],
'resolved-ref': revision,
'path': ref.description['path']
@@ -249,13 +273,21 @@
@override
Future<Pubspec> describeUncached(PackageId id) {
return _pool.withResource(() => _describeUncached(
- id.toRef(), id.description['resolved-ref'], id.description['path']));
+ id.toRef(),
+ id.description['resolved-ref'],
+ id.description['path'],
+ source.urlFromDescription(id.description),
+ ));
}
/// Like [describeUncached], but takes a separate [ref] and Git [revision]
/// rather than a single ID.
Future<Pubspec> _describeUncached(
- PackageRef ref, String revision, String path) async {
+ PackageRef ref,
+ String revision,
+ String path,
+ String url,
+ ) async {
await _ensureRevision(ref, revision);
var repoPath = _repoCachePath(ref);
@@ -265,18 +297,20 @@
// Git doesn't recognize backslashes in paths, even on Windows.
if (Platform.isWindows) pubspecPath = pubspecPath.replaceAll('\\', '/');
-
List<String> lines;
try {
lines = await git
.run(['show', '$revision:$pubspecPath'], workingDir: repoPath);
} on git.GitException catch (_) {
fail('Could not find a file named "$pubspecPath" in '
- '${ref.description['url']} $revision.');
+ '${source.urlFromDescription(ref.description)} $revision.');
}
- return Pubspec.parse(lines.join('\n'), systemCache.sources,
- expectedName: ref.name);
+ return Pubspec.parse(
+ lines.join('\n'),
+ systemCache.sources,
+ expectedName: ref.name,
+ );
}
/// Clones a Git repo to the local filesystem.
@@ -443,7 +477,6 @@
Future _createRepoCache(PackageRef ref) async {
var path = _repoCachePath(ref);
assert(!_updatedRepos.contains(path));
-
try {
await _clone(ref.description['url'], path, mirror: true);
} catch (_) {
@@ -541,16 +574,23 @@
///
/// If [shallow] is true, creates a shallow clone that contains no history
/// for the repository.
- Future _clone(String from, String to,
- {bool mirror = false, bool shallow = false}) {
+ Future _clone(
+ String from,
+ String to, {
+ bool mirror = false,
+ bool shallow = false,
+ }) {
return Future.sync(() {
// Git on Windows does not seem to automatically create the destination
// directory.
ensureDir(to);
- var args = ['clone', from, to];
-
- if (mirror) args.insert(1, '--mirror');
- if (shallow) args.insertAll(1, ['--depth', '1']);
+ var args = [
+ 'clone',
+ if (mirror) '--mirror',
+ if (shallow) ...['--depth', '1'],
+ from,
+ to
+ ];
return git.run(args);
}).then((result) => null);
diff --git a/test/add/git/git_test.dart b/test/add/git/git_test.dart
index 305b6a6..2577af8 100644
--- a/test/add/git/git_test.dart
+++ b/test/add/git/git_test.dart
@@ -31,6 +31,48 @@
}).validate();
});
+ test('adds a package from git with relative url and --directory', () async {
+ ensureGit();
+
+ await d.git(
+ 'foo.git', [d.libDir('foo'), d.libPubspec('foo', '1.0.0')]).create();
+
+ await d.appDir({}).create();
+
+ await pubAdd(
+ args: ['--directory', appPath, 'foo', '--git-url', 'foo.git'],
+ workingDirectory: d.sandbox,
+ output: contains('Changed 1 dependency in myapp!'),
+ );
+
+ await d.dir(cachePath, [
+ d.dir('git', [
+ d.dir('cache', [d.gitPackageRepoCacheDir('foo')]),
+ d.gitPackageRevisionCacheDir('foo')
+ ])
+ ]).validate();
+
+ await d.appDir({
+ 'foo': {'git': '../foo.git'}
+ }).validate();
+ });
+
+ test('fails with invalid --git-url', () async {
+ ensureGit();
+
+ await d.git(
+ 'foo.git', [d.libDir('foo'), d.libPubspec('foo', '1.0.0')]).create();
+
+ await d.appDir({}).create();
+
+ await pubAdd(
+ args: ['foo', '--git-url', ':'],
+ error:
+ contains('The --git-url must be a valid url: Invalid empty scheme.'),
+ exitCode: exit_codes.USAGE,
+ );
+ });
+
test('adds a package from git with version constraint', () async {
ensureGit();
@@ -76,7 +118,7 @@
]).validate();
});
- test('fails when adding from an invalid url', () async {
+ test('fails when adding from an non-existing url', () async {
ensureGit();
await d.appDir({}).create();
diff --git a/test/add/path/relative_path_test.dart b/test/add/path/relative_path_test.dart
index 6a191a6..8fa1932 100644
--- a/test/add/path/relative_path_test.dart
+++ b/test/add/path/relative_path_test.dart
@@ -26,6 +26,25 @@
}).validate();
});
+ test('can use relative path with --directory', () async {
+ await d
+ .dir('foo', [d.libDir('foo'), d.libPubspec('foo', '0.0.1')]).create();
+
+ await d.appDir({}).create();
+
+ await pubAdd(
+ args: ['--directory', appPath, 'foo', '--path', 'foo'],
+ workingDirectory: d.sandbox,
+ output: contains('Changed 1 dependency in myapp!'),
+ );
+
+ await d.appPackagesFile({'foo': '../foo'}).validate();
+
+ await d.appDir({
+ 'foo': {'path': '../foo'}
+ }).validate();
+ });
+
test('fails if path does not exist', () async {
await d.appDir({}).create();
diff --git a/test/get/git/check_out_transitive_test.dart b/test/get/git/check_out_transitive_test.dart
index 1e897fb..537e670 100644
--- a/test/get/git/check_out_transitive_test.dart
+++ b/test/get/git/check_out_transitive_test.dart
@@ -2,7 +2,9 @@
// 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 'package:path/path.dart' as p;
import 'package:test/test.dart';
+import 'package:pub/src/exit_codes.dart' as exit_codes;
import '../../descriptor.dart' as d;
import '../../test_pub.dart';
@@ -14,7 +16,10 @@
await d.git('foo.git', [
d.libDir('foo'),
d.libPubspec('foo', '1.0.0', deps: {
- 'bar': {'git': '../bar.git'}
+ 'bar': {
+ 'git':
+ p.toUri(p.absolute(d.sandbox, appPath, '../bar.git')).toString()
+ }
})
]).create();
@@ -22,7 +27,9 @@
'bar.git', [d.libDir('bar'), d.libPubspec('bar', '1.0.0')]).create();
await d.appDir({
- 'foo': {'git': '../foo.git'}
+ 'foo': {
+ 'git': p.toUri(p.absolute(d.sandbox, appPath, '../foo.git')).toString()
+ }
}).create();
await pubGet();
@@ -39,4 +46,57 @@
expect(packageSpec('foo'), isNotNull);
expect(packageSpec('bar'), isNotNull);
});
+
+ test('cannot have relative git url packages transitively from Git', () async {
+ ensureGit();
+
+ await d.git('foo.git', [
+ d.libDir('foo'),
+ d.libPubspec('foo', '1.0.0', deps: {
+ 'bar': {'git': '../bar.git'}
+ })
+ ]).create();
+
+ await d.git(
+ 'bar.git', [d.libDir('bar'), d.libPubspec('bar', '1.0.0')]).create();
+
+ await d.appDir({
+ 'foo': {
+ 'git': p.toUri(p.absolute(d.sandbox, appPath, '../foo.git')).toString()
+ }
+ }).create();
+
+ await pubGet(
+ error: contains(
+ '"../bar.git" is a relative path, but this isn\'t a local pubspec.'),
+ exitCode: exit_codes.DATA,
+ );
+ });
+
+ test('cannot have relative path dependencies transitively from Git',
+ () async {
+ ensureGit();
+
+ await d.git('foo.git', [
+ d.libDir('foo'),
+ d.libPubspec('foo', '1.0.0', deps: {
+ 'bar': {'path': '../bar'}
+ })
+ ]).create();
+
+ await d
+ .dir('bar', [d.libDir('bar'), d.libPubspec('bar', '1.0.0')]).create();
+
+ await d.appDir({
+ 'foo': {
+ 'git': p.toUri(p.absolute(d.sandbox, appPath, '../foo.git')).toString()
+ }
+ }).create();
+
+ await pubGet(
+ error: contains(
+ '"../bar" is a relative path, but this isn\'t a local pubspec.'),
+ exitCode: exit_codes.DATA,
+ );
+ });
}
diff --git a/test/upgrade/git/do_not_upgrade_if_unneeded_test.dart b/test/upgrade/git/do_not_upgrade_if_unneeded_test.dart
index 2b948b3..6e8c7df 100644
--- a/test/upgrade/git/do_not_upgrade_if_unneeded_test.dart
+++ b/test/upgrade/git/do_not_upgrade_if_unneeded_test.dart
@@ -3,6 +3,7 @@
// BSD-style license that can be found in the LICENSE file.
import 'package:test/test.dart';
+import 'package:path/path.dart' as p;
import '../../descriptor.dart' as d;
import '../../test_pub.dart';
@@ -16,7 +17,11 @@
await d.git('foo.git', [
d.libDir('foo'),
d.libPubspec('foo', '1.0.0', deps: {
- 'foo_dep': {'git': '../foo_dep.git'}
+ 'foo_dep': {
+ 'git': p
+ .toUri(p.absolute(d.sandbox, appPath, '../foo_dep.git'))
+ .toString()
+ }
})
]).create();
@@ -45,7 +50,11 @@
await d.git('foo.git', [
d.libDir('foo', 'foo 2'),
d.libPubspec('foo', '1.0.0', deps: {
- 'foo_dep': {'git': '../foo_dep.git'}
+ 'foo_dep': {
+ 'git': p
+ .toUri(p.absolute(d.sandbox, appPath, '../foo_dep.git'))
+ .toString()
+ }
})
]).create();
diff --git a/test/version_solver_test.dart b/test/version_solver_test.dart
index 7c1ad45..a7132e7 100644
--- a/test/version_solver_test.dart
+++ b/test/version_solver_test.dart
@@ -240,7 +240,7 @@
await servePackages((builder) {
builder.serve('foo', '1.0.0', deps: {'myapp': 'any'});
builder.serve('bar', '1.0.0', deps: {
- 'myapp': {'git': 'nowhere'}
+ 'myapp': {'git': 'http://nowhere.com/'}
});
});
