Masterminds / glide

Package Management for Golang
https://glide.sh
Other
8.15k stars 540 forks source link

[UX] Transitive vs direct deps #485

Open lucab opened 8 years ago

lucab commented 8 years ago

Moving the discussion from here, /cc @sdboyer @sgotti

I wish it was easier to distinguish direct deps from transitive deps. I mistakenly tried to remove a transitive deps and that (correctly) didn't affect lockfile. I'm not sure if glide has some knobs to warn about unconstrained deps, but it would make this behavior clearer.

I'm a little unclear about this - when you say "tried to remove a transitive dep", do you mean tried to manually remove it from the vendor dir?

I started from a populated .yaml+.lock+vendor/ and tried to remove a tag-constrained dependency via glide rm. It was removed from manifest but, to my surprise, it was still in lockfile and under vendor/. I later realized that it was conceptually being removed+reintroduced due to being a transitive dependency.

And, "knobs to warn about unconstrained deps" - meaning, the deps are in the code (in the real import graph), but not mentioned with a constraint in a glide.yaml? or they are in a glide.yaml, but no constraint is specified? Both? Something else?

Both would be nice, but I was actually referring to the first case you mention. In a tightly constrained manifest (eg. rkt one), I can foresee some new unconstrained deps appearing in lockfile as a result of being new transitive deps. In that case, I'd like to quickly discover which additional entries I may need to pin in my manifest, without having to manually compare it with the lockfile.

mattfarina commented 8 years ago

@lucab I'm happy to try and improve this.

Let's start with the deps directly referenced in the local codebase. I was already planning on noting those that are in the codebase but not in the glide.yaml and offer the option to add them. I was considering adding elements of the wizard to ask about using a version/range.

What do you think makes sense beyond that? Providing a warning or other message about packages that do not have a version/range specified?

Then there is the matter of transitive dependencies. Any thoughts on those?

lucab commented 8 years ago

What do you think makes sense beyond that? Providing a warning or other message about packages that do not have a version/range specified?

In the context of rkt workflow, it may make sense to have a glide switch that warns (or abort without side-effects, I'm not sure) at update time whenever a lockfile entry does not have its counterpart in manifest.

In my näif view, this would cover both direct and transitive deps (but I may be missing some crucial detail here).

sdboyer commented 8 years ago

tl;dr

I think there are maybe just two situations we really want to uncover and potentially warn the user about:

In the context of rkt workflow, it may make sense to have a glide switch that warns (or abort without side-effects, I'm not sure) at update time whenever a lockfile entry does not have its counterpart in manifest.

Hmm...I'm not sure this is the best approach to promote. Maybe it's fine, but let me explain.

Once glide switches to vsolver with #384, there are a handful of notably different contexts under which a package can enter the depgraph (this may also be true of glide right now, but I haven't applied the same rigorous analysis there):

Lemme split this up.

The Ideal

In an ideal world - one that I hope glide well help us to achieve - pretty much everything is coming from the first or second. We're a long way off from there, but this is one of those "be the change you want to see in the world", or at least "fake it till you make it" situations - the only way we get there is if everyone does it.

It's important to understand that constraints expressed through either the first or second route have the same effect. If your dep, say A, already expresses a constraint on its dep, say B, there is no benefit to re-expressing that constraint on A in your manifest. In fact, it's harmful.

When you pull up that constraint on B, you're effectively preventing A from being able to change its own constraint. Even if your manifest allows newer versions of A, and even if you explicitly request an upgrade of A, if newer versions of A have changed that constraint on B that you copied up to your manifest, then you'll be stuck with your original version. In other words, duplicating constraints silently masks the natural avenue you would otherwise have for discovering the possibility (not necessity!) of bringing in a new version of A.

If you take an even wider view, things get much worse. Say that B depends on C, which depends on D. If everyone in that chain uses "tightly constrained manifests," and there's a security update to D, then C has to release a new manifest which takes the new version of D, then B on C, then A on B. That'd only take...what, weeks? months?

This is a subtle issue, and can be hard to appreciate from the perspective of any one project in the ecosystem. However, if you look at the bigger picture, too many projects being too restrictive in their manifest constraints can cause the entire system to seize up. I suspect this is the reason why cargo defaults to interpreting e.g. 2.0.0 as ^2.0.0. At that point, the only option you personally have is setting override constraints - where the root project dictates the constraint for packages, and everyone else's constraint is ignored - which, in turn, creates its own tragedy of the commons.

The bottom line is, you're doing more harm than good if you just reflexively pull up all your deps into your manifest. That's really not what manifests are for - it's what lock files are for. Trust your lock file! Though...

Gritty Reality

First and foremost, you can't really trust your lock file right now, because glide does not allow you to do minimal, targeted updates - #252. (This is already fixed by vsolver; #384). So for now, at least, you may have to pull up some of those constraints to defend yourself against that problem. I'm hoping we have vsolver in before gophercon, though, so that concern should be short-lived.

The bigger issue is when you have dependencies entering in the third or fourth context - via a godep-style lock, or with no constraint at all. Now, the third isn't quite so bad - we get a lot of stability out of that, and we were just discussing its implications the other day in #479. But still, it's generally unwise to have something in your depgraph that's totally unconstrained.

Today, when unconstrained, glide will pick the first available version it sees (idk the exact algorithm for what determines that), and stick with that version. Once the switch to vsolver is made, it will attempt all available versions, in the following order:

  1. Semantic version tags, in descending order (with prereleases - alpha, beta, etc. - forced to the bottom)
  2. Branches, in descending lexicographic order
  3. Non-semver tags, in descending lexicographic order

Clear rules and ordering are helpful, but it's still probably a good idea to add some constraints in your root. Problem is...projects might, at any time, adopt glide, at which point we're suddenly back in the 'ideal' camp, where you want to avoid duplicating deps.

The Upshot

Given all of this, it seems to me that there are two things we really want to uncover and potentially warn the user about. (this reiterates the tl;dr):

Thoughts?

I realize this is a lot. Hopefully it makes decent sense. Happy to try to clarify further if needed 😄

lucab commented 8 years ago

[Sorry, I thought I had already replied here since long time, but looks like my memory is faulty]

Your ideal scenario makes sense in an environment where versioning is already properly handled. However, in the context of rkt, we have many dependencies which are not even tagged (let alone semversioned) and that's why we would need some warnings from the toolchain. But I think that once #252 is fixed, your TLDR proposal should be enough to help us in most cases.

sdboyer commented 8 years ago

@lucab no worries on the delay, thanks for the response. Also, @thockin or maybe @vishh, it'd be great to have your feedback on this.

I think this may be too tricky to solve in glide's current architecture given the difficulties around #252, but @mattfarina might have some ideas about how it could be accomplished.

But, for the post-vsolver glide world, I've opened sdboyer/vsolver#46. I don't think the implementation should be terribly difficult.

sdboyer commented 8 years ago

Oh, though:

your TLDR proposal should be enough to help us in most cases.

Is there a particular case you can think of that I've missed? Or is that just standard, totally-reasonable "That sounds nice and all, but I won't actually trust this wild idea until it's covered in battle scars" engineer hedging? 😄

lucab commented 8 years ago

Boilerplate for "only time will tell" :smile:

No, I don't have any missing case that I can think of right now.