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 a FAQ #401

Merged
merged 7 commits into from Apr 25, 2017

Conversation

Projects
None yet
4 participants
@carolynvs
Copy link
Collaborator

carolynvs commented Apr 19, 2017

This gives us a place to collect great questions and their helpful answers in one place until #331 is hammered out.

I went through all the issues labeled with doc and quoted like the wind to get it jump started.

Here are the issues that I think are resolved with this PR, because they were kept open in order to get the question documented:

@googlebot googlebot added the cla: yes label Apr 19, 2017

@sdboyer
Copy link
Member

sdboyer left a comment

this is a great start 😄

FAQ.md Outdated
Constraints given in a project's manifest are only applied if the
dependent project is actually imported. Transitive dependencies (dependencies
of your imports) are only updated when the revision in the lockfile no
longer meets the constraints of your direct dependencies.

This comment has been minimized.

@sdboyer

sdboyer Apr 20, 2017

Member

Ah, I think you've flipped the idea from #385 around a bit. While you can only constrain a direct dependency, you can (or at least should be able to) mark any dependency, transitive or direct, for updating.

This comment has been minimized.

@carolynvs

carolynvs Apr 20, 2017

Collaborator

Sorry I don't quite understand what you mean by "you can (or at least should be able to) mark any dependency, transitive or direct, for updating". Can you help me identity which line(s) are missing the mark? 😊

I thought that problem from #385 (and #302 come to think of it) was that because X was not a direct dependency (not imported), running dep ensure -update did not cause X to be updated, even though X is a dependency in the manifest? So the resolution is either:

  • Be okay with it not being updated during dep ensure -update unless forced by the constraints of your direct dependencies.
  • Make X as an override or required package in the manifest.

Though the best solution is to fix #302 and when dep detects this situation, it stops and prints a message to the user on how to fix it.

This comment has been minimized.

@sdboyer

sdboyer Apr 21, 2017

Member

You're in the ballpark there, it's just that you've got it sorta reversed. It seems like you're thinking of it as a one-step process (just the dep ensure -update run). But it tends to be more like this:

  1. User puts a constraint on a transitive dependency in manifest, runs dep ensure
  2. dep doesn't point out to user that the constraint is ineffectual, so user assumes it's working (and maybe the constraint they put in the manifest happened to be the one that ended up in the lock).
  3. Later, user runs dep ensure -update, which appears to not have an effect because it was unconstrained on the previous dep ensure, so the latest version is already what was in the lock.

Yes, I agree that fixing #302 is the thing that's most likely to make this better the fastest.

FAQ.md Outdated
> However, before taking either of those steps, I'd say it's worth asking if you actually need to use master of github.com/gorilla/context. I imagine it's imported by github.com/gorilla/mux - and if that package is OK with using the tagged release instead of master (which is the preferred mode of operation anyway), then maybe that should be good enough for you? If you really needed something out of github.com/gorilla/context, then you'd probably be importing it directly and doing something with it
-[@sdboyer in #385](https://github.com/golang/dep/issues/385#issuecomment-294361087)
* Is package X included in the `ignored` list in your manifest?

This comment has been minimized.

@sdboyer

sdboyer Apr 20, 2017

Member

If a package is ignored, then it shouldn't be showing up at all in lock or vendor - so there shouldn't be anything to update.

This comment has been minimized.

@carolynvs

carolynvs Apr 20, 2017

Collaborator

Doh! This wasn't meant to be in here. I thought there may be more scenarios to troubleshoot where you hoped that a package would be updated, and it wasn't. I'll remove this.

FAQ.md Outdated
-[@sdboyer in #371](https://github.com/golang/dep/issues/371#issuecomment-293246832)
## Does `dep` replace `go get`?

This comment has been minimized.

@sdboyer

sdboyer Apr 20, 2017

Member

Probably would be good to do a bit more caveating here - this is one of the lesser-explored areas with @rsc so far. So, even though the opinions expressed here are tentative, only having them doesn't really paint a representative picture moving forward.

I'll think about some other verbiage...

This comment has been minimized.

@carolynvs

carolynvs Apr 20, 2017

Collaborator

I'd appreciate any wording you may have for this. Obviously I'm still learning and don't always summarize things right! 😀

This comment has been minimized.

@carolynvs

carolynvs Apr 21, 2017

Collaborator

Okay I took a stab at being coy and vague about dep and go proper. 😀

@@ -0,0 +1,112 @@
# FAQ

_The first rule of FAQ is don't bikeshed the FAQ, leave that for

This comment has been minimized.

@sdboyer

sdboyer Apr 20, 2017

Member

❤️

FAQ.md Outdated
> We prefer to treat the use of vendor/ as an implementation detail.
-[@sdboyer on go package management list](https://groups.google.com/d/msg/go-package-management/et1qFUjrkP4/LQFCHP4WBQAJ)
## Unable to update checked out version: fatal: reference is not a tree

This comment has been minimized.

@sdboyer

sdboyer Apr 20, 2017

Member

I think this one actually should be fixed now. ...well, arguably not "fixed", as the outcome might not be right - it just silently ignores the commit from on-disk and falls back to the latest version from upstream...but at least, there's no error anymore 😄

We should double check on that before putting this into the FAQ

This comment has been minimized.

@carolynvs

carolynvs Apr 20, 2017

Collaborator

Oooh yeah you're right. Here is the new behavior #405.

FAQ.md Outdated
Remove it from `ignored` and try again.

## Why is `dep` ignoring the version specified in the manifest?
Only direct dependencies can be managed with a `depenencies` entry

This comment has been minimized.

@sdboyer

sdboyer Apr 20, 2017

Member

typo

@peterbourgon

This comment has been minimized.

Copy link
Member

peterbourgon commented Apr 21, 2017

Suggestion, since this question comes in very frequently in #vendor:

## Should I commit my vendor directory?

Committing the vendor directory is totally up to you. There is no general advice
that applies in all cases. Pros: it's the only way to get truly reproducible
builds, as it guards against upstream renames and deletes; and you don't need an
extra `dep ensure` step on fresh clones to build your repo. Cons: your repo will
be bigger, potentially a lot bigger; and PR diffs are more annoying.

@daenney daenney referenced this pull request Apr 21, 2017

Closed

docs: Create a FAQ #406

carolynvs added some commits Apr 21, 2017

Update answer for “revision is not a tree”
The behavior has changed, and now instead of a failing, dep picks a new commit.

@carolynvs carolynvs force-pushed the carolynvs:faq branch from 46f64fa to 3617095 Apr 21, 2017

@carolynvs

This comment has been minimized.

Copy link
Collaborator

carolynvs commented Apr 21, 2017

Okeedokee!

  • Taken another crack at explaining why dep ensure -update didn't work as expected. If I am still getting it wrong, I vote for either removing it for now or you giving me the exact verbiage to use in the FAQ for this question. 😀
  • Added @peterbourgon 's "Should I commit my vendor directory"?
  • Updated answer to reflect new behavior for "revision is not a tree"
  • Added caveats to dep vs. go get
@sdboyer

This comment has been minimized.

Copy link
Member

sdboyer commented Apr 25, 2017

Cool. I'm gonna merge this as it stands now, then push up some more tweaks tonight :)

@sdboyer

This comment has been minimized.

Copy link
Member

sdboyer commented Apr 25, 2017

thanks!

@sdboyer sdboyer merged commit a28d05c into golang:master Apr 25, 2017

3 checks passed

cla/google All necessary CLAs are signed
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

ibrasho pushed a commit to ibrasho-forks/dep that referenced this pull request May 10, 2017

@carolynvs carolynvs deleted the carolynvs:faq branch Jun 11, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment