Add github version range support #1335
Conversation
|
I don't think you need to do that many backflips to get "v" versions working. Just add logic to |
computmaxer
force-pushed
the
github-version-ranges
branch
2 times, most recently
from
10803f1 to
3be3412
Compare
Add "use_version_tags" boolean to git source descriptions
computmaxer
force-pushed
the
github-version-ranges
branch
from
3be3412 to
bb0fb5a
Compare
|
Sorry for the delay! Finally had some time to spend on this yesterday. I've implemented @nex3 's suggestion for dealing with "v" version tags. I also fixed a major issue with my initial approach. I was creating bare-bones To help combat that issue I added a flag to enable this feature per dependency. Looks like this: dependencies:
w_flux:
version: "^1.0.0"
git:
url: [email protected]:Workiva/w_flux.git
use_version_tags: truePub will only All unit tests are passing and I have tested this with a number of repositories, with and without "v"s in the version tags. Let me know what you think and what the next steps would be if you want to move forward with this solution. |
|
If checking out each tag is slow, would using |
|
@greglittlefield-wf nice! Didn't know about |
|
@computmaxer If you're fixing your pull request, please rebase against master and do a |
| } on FormatException {} | ||
| } | ||
|
|
||
| if (validVersions.length > 0) { |
|
@kevmoo comments addressed. Would you like all commits squashed into one as well? |
| var ref = _getEffectiveRef(id); | ||
| return git | ||
| .run(["rev-list", "--max-count=1", ref], workingDir: _repoCachePath(id)) | ||
| .then((result) => result.first) |
There was a problem hiding this comment.
By async, I also meant getting red of .then and .catchError and using await, too. 😄
There was a problem hiding this comment.
Ok. Do you guys always prefer async/await over future chaining, then?
There was a problem hiding this comment.
In general, yes. It's easier to read and our debugger actually has special support for async/await – that we don't get if you use the Future API directly.
@nex3 has found a few cases where it's better to use the API directly, but she'd have to comment.
|
@computmaxer I'd wait to squash until reviews are done – but it's good to do once you think you're done |
- Use .isNotEmpty instead of .length > 0 - Use async/await instead of Future chaining
computmaxer
force-pushed
the
github-version-ranges
branch
from
9409905 to
93213d1
Compare
| workingDir: _repoCachePath(id)); | ||
| return result.first; | ||
| } | ||
| throw e; |
There was a problem hiding this comment.
Use rethrow – it preserves the stack trace
| /// 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)) { |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.gitIs there any reason that version can't be under git?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Do you mean PackageDep instead of PackageRef? PackageRef does not have a VersionConstraint available.
|
+1, our team is currently in dependency_overrides hell to work around the issue, we'd definitely be happy to add a flag to every dependency for this solution |
|
@travissanderson-wf @trentgrover-wf I'll chat with @nex3 to see what our options are – reading #1327, it sounds like you'd be open to running your own server |
|
@kevmoo we are open to it, but would rather have support for Git version solving to avoid maintenance headaches and the auth hurdle.
Why doesn't it make sense to add that piping? It seems like |
|
@kevmoo any further thoughts on this issue? |
|
@kevmoo @nex3 I already gave @computmaxer and his team breakfast pizza to thank him for getting this fixed for us, as you know that is a morally binding contract so he is feeling a lot of heat right now since we still can't use version ranges on github dependencies :) |
|
If you're looking to only allow authenticated downloads, I think your best bet is to either run some sort of VPN or to use HTTP basic auth and have people include their credentials in the |
|
So is there no chance of a GitHub based solution (eliminates a bunch of infrastructure requirements)? If you could point us to your specific areas of concern, we're more than happy to devote some dev time to it, but it's sounding like the NO is hardening... |
|
I don't think we're going to find a way to make version range resolution work with GitHub, no. |
|
That kind of sucks to go through so much PR review/work to say "no" to the whole idea of it later. The feature is useful. Hope you reconsider @nex3. |
|
@johncblandii It absolutely sucks. We're very sorry. We try very hard to make sure these types of changes don't have long-term implications or side-effects that can hurt the ecosystem. @nex3 is especially diligent when it comes to these types of changes – although, as she mentioned, we realized there were gnarly edge cases we didn't initially consider when we first encouraged the work to proceed. We'd be more than happy to discuss other approaches to this problem – we know path dependencies are a pain. |
|
Describing it as a pain is a bit of an understatement. We have a number of in-house libraries written in Dart and a handful of apps consuming those libraries. Our options today are either move everything in lockstep (dependencies of dependencies included) or to use a horrific set of dependency_overrides. The other prescribed option of running a private server with VPN access required puts an unnecessary burden of access control and infrastructure on the end-users of the Dart language. None of the other services we use require restricted network access, we do not generally use a VPN and can work with all our other "cloud" services from anywhere. It's a little demoralizing as a company that has evangelized Dart and is offering to even do the work ourselves. I'm sure we're not alone with this being a major difficulty for anyone else trying to use Dart in a medium-large scale company. We would be elated to even use the "cumbersome" flag on every dependency. Our team alone has 40 lines (not exaggerating) of dependency_overrides entries in our primary app - it's almost a mirror of the dependencies section. |
|
In hindsight, knowing this was a dead end would have allowed us to work on an internal pub server. We want to make sure that whatever solution we end up with is solid, but I'm not sure why this can't be accomplished (in principle.) It seems like an elegant way to support private dependencies. |
|
I'll chat w/ @nex3 a bit more tomorrow. We know git (and GitHub) is a really close fit here. Maybe we can get creative. |
|
👍 |
|
Have a good holiday vacation, all! See you when you get back.
|
|
For posterity, here's where we landed:
|
|
Sounds right to me. Just for the record (and you probably already know this):
This is supported already in pub, so there's no work on our end to do. It looks like: dependencies:
transmogrify:
hosted:
name: transmogrify
url: http://your-package-server.com
version: ^1.4.0 |
|
@kevmoo @munificent @nex3 I am having trouble getting this working with the Managed VM GAE deployment. I've added the firewall rule to allow our office IPs in Networking --> Firewall rules, but I can't seem to block access to everything else. I believe this would work if it was just a GCE deployment, but it seems GAE must add some rules or routes to allow traffic to be routed from the internet to the instances. Do you guys know if it is possible to implement an IP whitelist for a GAE Managed VMs deployment? |
|
I'm far from an expert on AppEngine infrastructure, and I think the same is true for Kevin and Bob. Maybe @mkustermann would know; he works on pub.dartlang.org. |
|
Try https://groups.google.com/forum/#!forum/app-engine-managed-vms This is purely outside of the Dart team |
|
@nex3 @kevmoo |
|
I haven't had a chance to work on this yet. I'll try to get it in the works next quarter. |
I know this is not the proper way to submit contributions, but I want to get feedback from dart-lang folks and make sure it is something that could potentially be accepted before I spend any more time on this.
This is a fix for #1250. I work for Workiva (met some of you at the Dart Summit!) and we now have quite a few dart libraries in our Github organization. Some of these are public and OSS, but many of them cannot be. #1250 is a big issue for us for the private libraries as it requires us to update pubspecs all over the place anytime a patch version of some library is released, and consumers end up having a big list of dependency overrides while those transitions are in progress.
This PR implements version ranges for github dependencies with the following in a
pubspec.yaml:It assumes that versions will exist as git tags on the repo, and uses
git tag -lto list the tags. It then collects a list of the tags that are considered valid version strings usingVersion.parse().The biggest issue I have had so far is dealing with git tags that include the "v" in the name, i.e. "v1.0.0". I can strip the "v" before giving it to the version solver, but then when it tries to get the ref (
_getRef) it of course cannot find it without the "v" that was previously removed. At Workiva we do not use the preceding "v" in our tag names, but it seems like many people do so I am assuming this will be a big issue for this feature.It seems like it would be possible for the Version class to accept "v" versions strings in its
parseconstructor, and keep track of it so that it can put it back whentoString()is called. This would require a change to pub_semver, though.I'm hoping to get some feedback from the Dart team on whether or not we should continue down this path. Please let me know your thoughts!
@nex3 @kevmoo @munificent