Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add github version range support #1335

Closed
wants to merge 12 commits into from
111 changes: 103 additions & 8 deletions lib/src/source/git.dart
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@ library pub.source.git;
import 'dart:async';

import 'package:path/path.dart' as path;
import 'package:pub_semver/pub_semver.dart';

import '../git.dart' as git;
import '../io.dart';
Expand Down Expand Up @@ -40,6 +41,49 @@ class GitSource extends CachedSource {
});
}

/// Gets the list of all versions from git tags.
/// Anything that Version.parse understands is considered a version,
/// it will also attempt to strip off a preceding 'v' e.g. v1.0.0
Future<List<Pubspec>> getVersions(String name, description) async {
if (!_useVersionTags(description)) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm really not a fan of requiring users to explicitly annotate to get this behavior. For the most part, I don't expect people to be using version declarations for Git dependencies at all unless they want some sort of resolution, so requiring them to write more stuff seems annoying. And now that we're using git show, it shouldn't be too bad to get all the pubspecs.

On the other hand, we should short-circuit if the description doesn't have a version listed.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The issue I'm having here is that I can't seem to access the version in this method, as it is not part of the description.

w_transport:
  version: ^1.0.0
  git: [email protected]:Worvia/w_transport.git

description --> "[email protected]:Workiva/w_transport.git"

w_transport:
  version: ^1.0.0
  git: 
    url: [email protected]:Worvia/w_transport.git
    use_version_tags: true

description --> {'url': '[email protected]:Worvia/w_transport.git', 'use_version_tags': true}

Any idea how to access version so I can short-circuit based on it?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It'd be possible to access version if it was in the description under git, like you initially had it, right?

dependencies:
  w_flux:
    version: "^1.0.0"
    git: [email protected]:Workiva/w_flux.git

Is there any reason that version can't be under git?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The version solver (which is abstracted from the package source) won't respect it when it is under git. So you'd have include the same version entry twice.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yikes, good point. I'd just change Source.getVersions() to take a PackageRef rather than a name and description, and check whether the version constraint is any or not.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do you mean PackageDep instead of PackageRef? PackageRef does not have a VersionConstraint available.

// No need to get versions if the useVersionTags option is false
return super.getVersions(name, description);
}

PackageId id =
new PackageId(name, this.name, null, _getDescription(description));
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Passing null for the version here is a smell. It looks like _repoCachePath doesn't actually require a version, so it should be modified to take a PackageRef instead of a PackageId. You can update other call sites by calling PackageId.toRef() to get the ref.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Addressed this in computmaxer@1a9120c

String cachePath = _repoCachePath(id);
if (!entryExists(cachePath)) {
// Must have the repo cloned in order to list its tags
await _clone(_getUrl(id), cachePath);
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is the same logic as in _ensureRevision; consider factoring it out into Future<String> _ensureRepo(PackageRef ref).

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Addressed in 3d9c0ab


List results = await git.run(["tag", "-l"], workingDir: cachePath);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If you pass a pattern to --list, you can cut down on the number of irrelevant tags you have to look at. The pattern *.*.* will match all valid semantic versions with or without the leading v (although obviously it will also match a lot of other stuff).

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Addressed in computmaxer@59f2e6f

List<Pubspec> validVersions = [];
for (String version in results) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Consider using Iterable.where here.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This would require an additional loop - the where which selects the valid version strings, and another that loops through the valid version strings and awaits the pubspec for each.

I sort of prefer it as one loop, but if you would rather have that separation I can change it.

// Strip preceding 'v' character so 'v1.0.0' can be parsed into a Version
if (version.startsWith('v')) {
version = version.substring(1);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should do some de-duplication to ensure that, if a version exists both with and without v, only the canonical one (that is, the one without v) is returned.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Addressed here: c935354

}
try {
// Use Version.parse to determine valid version tags
Version validVersion = new Version.parse(version);
// Fetch the pubspec for this version
Pubspec pubspec = await _getPubspec(cachePath, validVersion.toString());
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

validVersion.toString() here will break if the actual tag started with a v.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Addressed in 8492ba2

if (pubspec != null) {
validVersions.add(pubspec);
}
} on FormatException {}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should definitely catch format exceptions from the version parse, but I don't think we should catch errors caused by an invalid pubpsec. It's a pretty safe assumption that once we're here, the user intentionally opted into version resolution over a given repo, and having some versions silently not appear may be confusing.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Consider the case where a repo initially was tagged 1.0.0 without a pubspec (perhaps it used to be in JS or something ¯_(ツ)_/¯ ). But all tags after 2.0.0 have a pubspec.
If we don't handle invalid pubspec here, this will cause an exception attempting to load the pubspecs for versions <2.0.0 and they won't be able to use version resolution at all for that given repo.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think with my refactor, we'll be able to avoid trying to load pubspecs until they're actually required by the version solver, which should mitigate this.

}

if (validVersions.isNotEmpty) {
return validVersions;
}

// No valid version tags were found, defer to super
return super.getVersions(name, description);
}

/// Since we don't have an easy way to read from a remote Git repo, this
/// just installs [id] into the system cache, then describes it from there.
Future<Pubspec> describeUncached(PackageId id) {
Expand Down Expand Up @@ -104,6 +148,7 @@ class GitSource extends CachedSource {
var parsed = new Map.from(description);
parsed.remove('url');
parsed.remove('ref');
parsed.remove('use_version_tags');
if (fromLockFile) parsed.remove('resolved-ref');

if (!parsed.isEmpty) {
Expand Down Expand Up @@ -149,6 +194,10 @@ class GitSource extends CachedSource {
Future<PackageId> resolveId(PackageId id) {
return _ensureRevision(id).then((revision) {
var description = {'url': _getUrl(id), 'ref': _getRef(id)};
bool useVersionTags = _useVersionTags(id);
if (useVersionTags) {
description['use_version_tags'] = useVersionTags;
}
description['resolved-ref'] = revision;
return new PackageId(id.name, name, id.version, description);
});
Expand Down Expand Up @@ -253,9 +302,22 @@ class GitSource extends CachedSource {
/// by [id] on the effective ref of [id].
///
/// This assumes that the canonical clone already exists.
Future<String> _getRev(PackageId id) {
return git.run(["rev-list", "--max-count=1", _getEffectiveRef(id)],
workingDir: _repoCachePath(id)).then((result) => result.first);
Future<String> _getRev(PackageId id) async {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If you're changing _getRev rather than _getEffectiveRef (which makes sense), you'll need to update downloadToSystemCache to use the former so that it behaves properly with v-versions.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Addressed in 01e6787

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for linking to the commit w/ the fix. Makes these reviews so much better. 😄

var ref = _getEffectiveRef(id);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Consider making this method async

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

_getEffectiveRef doesn't really make much sense in the new world. It returns "HEAD" if the ID doesn't specify a revision, but that's incorrect if version resolution needs to be done. I think what you want to do here is more like:

Future<String> _getRev(PackageId id) {
  var description = _getDescription(id);

  // If the id is already resolved, there's no need to rev-list to look for it.
  var resolved = description['resolved-ref'];
  if (description is Map && resolved != null) return resolved;

  // If [id] includes a version and not a ref, try "rev-list $version" and
  // "rev-list v$version". Otherwise, try "rev-list ${_getRef(id) ?? 'HEAD'}".
}

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm having an issue with this. Here's what the code roughly looks like:

Future<String> _getRev(PackageId id) {
  print('_getRev called: ${id.toString()}');
  ...

  var ref = description is Map ? description['ref'] : null;
  if (ref == null && id.version != null && id.version != Version.none) {
    print('Trying to use version: ${id.version.toString()}');
   // return the rev-list $version result
  } else {
    print('Returning _getRef(id) or HEAD');
    // return rev-list _getRef(id) ?? 'HEAD';
  }
}

For some reason id.version ends up getting set to something even if no version is specified in the pubspec. So if I put some print statements in this _getRev method the output looks like this when I run a simple test like test/get/git/check_out_test.dart

  | MSG : Resolving dependencies...
  | _getRev called: foo 0.0.0 from git
  | Returning _getRef(id) or HEAD
  | _getRev called: foo 0.0.0 from git
  | Returning _getRef(id) or HEAD
  | _getRev called: foo 0.0.0 from git
  | Returning _getRef(id) or HEAD
  | _getRev called: foo 0.0.0 from git
  | Returning _getRef(id) or HEAD
  | _getRev called: foo 1.0.0 from git
  | Trying to use version: 1.0.0

That's with a description of foo: "../foo.git" (no version specified). So by the logic in the if condition, it will try to get a revision with 1.0.0 which fails as it should. It should have hit the else case instead for this to work properly.

If I leverage _useVersionTags(id) in that if condition I can correctly determine whether or not the version should be used as a ref. This is what the existing code in _getEffectiveRef is doing. However, you mentioned you don't want to rely on an additional annotation for this to work.

I will keep looking for a solution, but let me know if you have any ideas that will solve this. Then I should be able to get rid of the _useVersionTags(id) stuff.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

id.version ends up being set to the version number in the pubspec.yaml after the solver has selected a pubspec: https://github.com/dart-lang/pub/blob/master/lib/src/solver/version_solver.dart#L257

resolveId is called once again after a solution has been reached: https://github.com/dart-lang/pub/blob/master/lib/src/solver/backtracking_solver.dart#L168. resolveId uses _ensureRevision which uses _getRev.

At this point id.version has something even if one wasn't specified in the consumer's pubspec.

So this is why if (ref == null && id.version != null && id.version != Version.none) in _getRev is not a sufficient condition to determine whether or not we should do a rev-list $version.

Open to suggestions :)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, I forgot that PackageId doesn't include the original version constraint because it's not part of the description. This is going to be trickier than I thought.

The underlying issue is that Git IDs are no longer context-independent in the way they used to be; we don't know whether one was created due to new-style version resolution or old-style looking at HEAD. Git does have a means of "resolving" package IDs, but because getVersions() returns Pubspecs rather than PackageIds, those resolved IDs don't make their way into the solver.

This probably calls for a full refactor of getVersions() that changes the signature to List<PackageId> getVersions(PackageRef ref) and moves some caching logic into the hosted source. This is out of the scope of what you're doing, though, so I'll implement it myself; hold tight until then.

try {
var result = await git.run(["rev-list", "--max-count=1", ref],
workingDir: _repoCachePath(id));
return result.first;
} on git.GitException {
if (ref == id.version.toString()) {
// Try again with a "v" before the ref in case this was a version tag
ref = 'v$ref';
var result = await git.run(["rev-list", "--max-count=1", ref],
workingDir: _repoCachePath(id));
return result.first;
}
rethrow;
}
}

/// Clones the repo at the URI [from] to the path [to] on the local
Expand Down Expand Up @@ -284,9 +346,28 @@ class GitSource extends CachedSource {
}

/// Checks out the reference [ref] in [repoPath].
Future _checkOut(String repoPath, String ref) {
return git.run(["checkout", ref], workingDir: repoPath).then(
(result) => null);
Future _checkOut(String repoPath, String ref) async {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ditto

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Note that you won't need to modify this if you change downloadToSystemCache to use _getRef.

try {
await git.run(["checkout", ref], workingDir: repoPath);
} on git.GitException {
// Try again with a "v" before the ref in case this was a version tag
await git.run(["checkout", 'v$ref'], workingDir: repoPath);
}
}

/// Use `git show` to get the pubspec.yaml at a particular ref,
/// then parse it into a Pubspec object
///
/// It is possible that a pubspec didn't always exist, return null if
/// that is the case.
Future<Pubspec> _getPubspec(String repoPath, String ref) async {
try {
var result = await git.run(['show', '$ref:pubspec.yaml'],
workingDir: repoPath);
return new Pubspec.parse(result.join('\n'), systemCache.sources);
} on git.GitException {
return null;
}
}

/// Returns the path to the canonical clone of the repository referred to by
Expand All @@ -313,13 +394,17 @@ class GitSource extends CachedSource {
/// [resolveId].
///
/// [description] may be a description or a [PackageId].
String _getEffectiveRef(description) {
description = _getDescription(description);
String _getEffectiveRef(PackageId id) {
Map description = _getDescription(id);
if (description is Map && description.containsKey('resolved-ref')) {
return description['resolved-ref'];
}

var ref = _getRef(description);
if (_useVersionTags(description) && id.version != null &&
id.version != Version.none) {
return id.version.toString();
}
return ref == null ? 'HEAD' : ref;
}

Expand All @@ -338,4 +423,14 @@ class GitSource extends CachedSource {
if (description is PackageId) return description.description;
return description;
}

/// Returns value of "use_version_tags" in the description
///
/// [description] may be a description or a [PackageId].
bool _useVersionTags(description) {
description = _getDescription(description);
if (description is String) return false;
return description.containsKey('use_version_tags')
? description['use_version_tags'] : false;
}
}