Skip to content
This repository has been archived by the owner on Sep 9, 2020. It is now read-only.

Command/flags for updating only named deps #202

Closed
sdboyer opened this issue Jan 31, 2017 · 8 comments
Closed

Command/flags for updating only named deps #202

sdboyer opened this issue Jan 31, 2017 · 8 comments

Comments

@sdboyer
Copy link
Member

sdboyer commented Jan 31, 2017

Right now, dep ensure -update is the way to update existing dependencies - it will disregard the lock, and update dependencies to the latest version allowed by constraints in the manifest. Problem is, -update is global - there's no way to say, "only update this specific dependency; keep the others the same as what's in my lock." (I feel this pain every time I go to update gps in dep)

We need a mechanism for allowing the user to specify only only a subset of dependencies that should be updated.

In the general spec, we described ensure as a combined "fetch/update" command, and indicated that a bare ensure on a dep already present in the project would cause an update. That is:

  $ dep ensure github.com/foo/bar

would update github.com/foo/bar to the latest possible version the solver determines to be possible from looking at constraints in all manifests. However, we never actually got around to implementing that behavior. I think we were put off it because the behavior of a "bare" ensure like this can vary along a number of independent factors:

  • Whether or not the named dependency/ies are actually in the local import statements (or in a manifest require)
  • Whether they're already in the lock or not
  • Whether they're constrained in the manifest or not, and if so, whether it's "possible" to update the type of constraint:
    • Revisions (commit SHA1s) cannot change
    • Non-semver tags can only change if the upstream source force-pushed to the tag
    • Branches can change, but pretty much only to the latest rev from upstream
    • Semver tags can change freely within range constraints

Note: I'm not necessarily saying every combination of these factors results in a significantly different outcome - just that they all bear on it somehow, which made it daunting enough that we kicked the can down the road. But now, it's time to stop kicking.

As far as I've thought it through, we have four basic approaches to solving this:

  • Follow the original model, where a bare ensure argument will result in an update if an import/require is present.
  • Expand -update to allow it to take arguments; all named deps will be updated, and anything not already in the lock is ignored.
  • Make -update behave more like -override - it's paired with the following argument as an indication that the tool should allow an update for just that dependency. It would be an error to specify something that isn't imported/required.
  • Make a separate subcommand - perhaps update.

Note: all of the above choices aren't challenging in terms of the solving mechanics - the ToChange property of gps.SolveParameters lets us specify a list of projects to allow updates to, while ChangeAll lets everything change (current dep ensure -update behavior). So I don't think the implementation work for these options is terrible; it's mostly in parsing and validating the input, and in reporting to the user if the solver returned a solution with changes that weren't explicitly expected.

I figure I'd open this issue so that we can zero in on which one of these we should at least try first, then hop to getting it done!


We're now experimenting with option two:

Expand -update to allow it to take arguments; all named deps will be updated, and anything not already locked is ignored.

@jbrodriguez
Copy link

@sdboyer Just to understand ... is it ok to assume that given a manifest with fixed dependencies constraints (tied to a specific tag or commit), dep ensure -update will produce an exact copy of the vendor folder (thinking about CI environments and reproducible builds) ?

@sdboyer
Copy link
Member Author

sdboyer commented Jan 31, 2017

@jbrodriguez that's a wholly separate thing. Reproducibility about making sure that vendor/ has what it's supposed to - aka, what's indicated in the lock. -update is expressly about changing what's in the lock (vendor is then changed as well, but it's more of an afterthought).

I need to make some other issues for e.g. dep ensure --vendor-only, which specifically skips anything that might modify the lock, and just ensures that vendor is written out as the lock specifies it should be.

If, after becoming accustomed to dep, you still feel compelled to pin dependencies in your manifest for safety/control/whatever, then IMO we've done a really poor job with dep. (Aside: it has been my goal to convince @jessfraz of this idea since October 😜)

@jbrodriguez
Copy link

@sdboyer Fair enough ! I'll keep looking how it evolves 👍

@carolynvs
Copy link
Collaborator

@sdboyer What do you mean by "If, after becoming accustomed to dep, you still feel compelled to pin dependencies in your manifest for safety/control/whatever, then IMO we've done a really poor job with dep"?

Am I missing something key about dep? With other dependency managers, I regularly pin to major versions (of libraries that follow semver) and then use update to bump the contents of my lock file to the latest compatible versions. Usually I do this when I know that there are breaking changes in a dependency which I am avoiding, but would still want to (at my discretion) run a single command to pickup bug fixes.

@sdboyer
Copy link
Member Author

sdboyer commented Feb 8, 2017

@carolynvs i think the word "pin" is a bit ambiguous. Maybe I'm missing a clear, commonly accepted meaning, but...in the bit from me you quoted, I'm using it to mean, "specify exactly one allowable version." Needless to say, the tool's update-type behaviors are no-ops in that situation. So many of the go dep managers have historically worked solely with e.g. git commit SHA1s that that's the mentality I'm almost always trying to walk people back from.

However, if the sense is "pin to a major version," as you've used it, then that's basically equivalent to a ^ constraint, and is exactly what I hope people will do 😄

@carolynvs
Copy link
Collaborator

Yeah I'm probably using the word "pin" wrong and as you said, most people think of pinning as selecting with a single version without a range. Thanks for the clarification!

I would like to contribute to this issue, if that's okay? Sounds like there is a bit more planning to be done though. Not sure if my opinion counts but here are my thoughts. 😊

  • "Follow the original model, where a bare ensure argument will result in an update if an import/require is present".

    I would be taken by surprise if running dep ensure foo performed an update. I often use go get <cool-dev-tool> in a script, just to make sure that a dev tool is present, and wouldn't expect it to check for updates and get latest every time I ran it.

  • "Expand -update to allow it to take arguments; all named deps will be updated, and anything not already imported/required is ignored."

    I like this. dep ensure -update github.com/foo/bar github.com/baz/qux is what I would have typed if I decided to skip reading the doc/help and just winged it.

  • "Make -update behave more like -override - it's paired with the following argument as an indication that the tool should allow an update for just that dependency. It would be an error to specify something that isn't imported/required."

    So this would look like this instead? dep ensure -update github.com/foo/bar -update github.com/baz/qux In addition to the repeatable argument, it would return an error if the dependency isn't already in the manifest? I'm not keen on specifying the arg multiple times, vs. providing a list like in the previous option.

    However, I do prefer the error behavior over silently ignoring dependencies. If I told dep to update something, and it did nothing (returning 0) not because it is already at the latest version but because it is not in the manifest, I would be pretty confused. I would rather receive a friendly error message suggesting the appropriate command(s) to run.

  • "Make a separate subcommand - perhaps update."

    With a command like that I always hesitate, wondering if it's supposed to be update or upgrade, and which one updates the tool and which updates the dependency. Having it as flag on ensure make the intent of the command more clear to me.

My vote (based on the above assumptions) would be: dep ensure -update github.com/foo/bar github.com/baz/qux where if any of the dependencies listed is not already managed in the manifest, then an error is returned.

@sdboyer
Copy link
Member Author

sdboyer commented Feb 8, 2017

Yeah I'm probably using the word "pin" wrong and as you said, most people think of pinning as selecting with a single version without a range. Thanks for the clarification!

Absolutely. I think some of the confusion here is also because "pin" gets used a lot in an npm context (where shrinkwrapping isn't necessarily the norm), folks will sometimes attach to a single, specific version. Because the lock is a non-optional part of our system but provides that same class of guarantee, I think of us as "pinning by default."

I would like to contribute to this issue, if that's okay?

✨ ❤️ 👍 👍! 🕺 🥇!! 😄 😄!!!!!

So this would look like this instead? dep ensure -update github.com/foo/bar -update github.com/baz/qux

Yep, that's what I was trying to describe with the third option.

With a command like that I always hesitate, wondering if it's supposed to be update or upgrade, and which one updates the tool and which updates the dependency.

Hah, yeah. Homebrew always gets me with this.

My vote (based on the above assumptions) would be: dep ensure -update github.com/foo/bar github.com/baz/qux

I tend to agree. The big thing we lose by doing this that it's effectively changing the "type" of the ensure command, and you can't combine arbitrary other instructions (e.g., overrides, or just plain fetches) with the updates in a way that you could if it was per-arg. However, doing that seems like a bizarre use pattern anyway and not a terrible loss, so I feel fine about experimentally scrapping it.

One thing to clarify, though:

where if any of the dependencies listed is not already managed in the manifest, then an error is returned.

I think it's not the manifest we're checking against here, but the lock. If it's not in the lock, then there's no "current" version of it selected, and thus nothing to update "from." Also, because we may want to update a transitive dep, and that definitely won't be in the manifest. And lastly, because the way our manifests work are a bit different from other langs - the import graph is still queen. That's its whole own rabbit hole - #213 has a lot of info, if you want to explore.

Any chance I could cajole you into working on this? 😄 😄 🦇 👁

@carolynvs
Copy link
Collaborator

I think of us as "pinning by default."

💯 👍

That's its whole own rabbit hole - #213 has a lot of info, if you want to explore

Excellent, I love rabbit holes. Also a big fan of yaks.

Any chance I could cajole you into working on this?

You had me at ✨ ❤️! Yes, I'd love to take a crack at it.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

No branches or pull requests

3 participants