diff --git a/lib/src/command/add.dart b/lib/src/command/add.dart index fda22503d..f5e554451 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'; @@ -311,25 +313,45 @@ class AddCommand extends PubCommand { 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 726a1a418..595c604df 100644 --- a/lib/src/global_packages.dart +++ b/lib/src/global_packages.dart @@ -90,9 +90,14 @@ class GlobalPackages { // 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 @@ To recompile executables, first run `global deactivate ${dep.name}`. 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 c986015c9..adcfee73d 100644 --- a/lib/src/source/git.dart +++ b/lib/src/source/git.dart @@ -29,61 +29,44 @@ class GitSource extends Source { 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."); - } - - 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.'); - } + } else { + url = description['url']; - 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 @@ class GitSource extends Source { '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 @@ class GitSource extends Source { '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 _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 @@ class GitSource extends Source { @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 @@ class BoundGitSource extends CachedSource { 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 @@ class BoundGitSource extends CachedSource { @override Future 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 _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 @@ class BoundGitSource extends CachedSource { // Git doesn't recognize backslashes in paths, even on Windows. if (Platform.isWindows) pubspecPath = pubspecPath.replaceAll('\\', '/'); - List 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 @@ class BoundGitSource extends CachedSource { 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 @@ class BoundGitSource extends CachedSource { /// /// 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 305b6a698..2577af83c 100644 --- a/test/add/git/git_test.dart +++ b/test/add/git/git_test.dart @@ -31,6 +31,48 @@ void main() { }).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 @@ void main() { ]).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 6a191a6ae..8fa193246 100644 --- a/test/add/path/relative_path_test.dart +++ b/test/add/path/relative_path_test.dart @@ -26,6 +26,25 @@ void main() { }).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 1e897fb4e..537e67096 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 @@ void main() { 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 @@ void main() { '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 @@ void main() { 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 2b948b3bc..6e8c7df2e 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 @@ void main() { 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 @@ void main() { 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 7c1ad455b..a7132e7b0 100644 --- a/test/version_solver_test.dart +++ b/test/version_solver_test.dart @@ -240,7 +240,7 @@ void rootDependency() { 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/'} }); });