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

dep ensure -add should not depend on Go code #1312

Closed
FiloSottile opened this issue Oct 25, 2017 · 13 comments
Closed

dep ensure -add should not depend on Go code #1312

FiloSottile opened this issue Oct 25, 2017 · 13 comments

Comments

@FiloSottile
Copy link

What version of dep are you using (dep version)?

dep:
 version     : devel
 build date  :
 git hash    :
 go version  : go1.9
 go compiler : gc
 platform    : darwin/amd64

What dep command did you run?

dep ensure -add github.com/square/go-jose

I ran this in an empty project, so I could start my main.go file.

What did you expect to see?

The usual warning about this dep being ephemeral.

What did you see instead?

no dirs contained any Go code

dep ensure -add is already a "stop trying to be smarter than me, I mean what I say" command, it shouldn't complain about an empty project.

@sdboyer sdboyer added the bug label Oct 25, 2017
@sdboyer
Copy link
Member

sdboyer commented Oct 25, 2017

eek, that's definitely an oversight. it makes sense that the requirements would be different for dep ensure -add.

dep ensure -add is already a "stop trying to be smarter than me, I mean what I say" command, it shouldn't complain about an empty project.

while i think i know the general sentiment behind this - the essential model of "keeping things in sync" instead of allowing arbitrary state mutations - if there are any specific things you're chafing against, please do let us know. while we're settled on the base model, we're now increasingly entering a phase of sanding off the particularly onerous edges.

@FiloSottile
Copy link
Author

if there are any specific things you're chafing against, please do let us know

I'm fairly at peace with dep ensure -add, modulo #1313.

@devinus
Copy link

devinus commented Nov 25, 2017

This is important to me because I want to use dep ensure before I've even started a project or when I'm working with projects that depend on me installing Go dependencies without actually having Go code in them.

@arbourd
Copy link
Contributor

arbourd commented Nov 25, 2017

dep ensure -add won't work if the project can't build, either:

all dirs contained build errors

@sdboyer
Copy link
Member

sdboyer commented Nov 28, 2017

dep ensure -add won't work if the project can't build, either:

this is usually a bit of a different case, as having build errors in all your dirs usually occurs when you're mid-refactor. the problem with continuing through in that case is that to skip all of those packages with errors would mean skipping all the imports that you probably picked up in previous dep ensure runs...which likely means that you'd end up with a Gopkg.lock and vendor directory that's basically empty except for what you -added. i don't think that's generally the desired outcome.

(it's important to note that it's fairly hard to reach this case, as there must be Go files that fail validation prior to the end of the import statements, as we use ImportsOnly|ParseComments)

@niondir
Copy link

niondir commented Dec 5, 2017

I took a quick look into this.

When the error is just handled as non fatal, the user will get the following message when adding a dependency to an empty project:

"github.com/sdboyer/deptest" is not imported by your project, and has been temporarily added to Gopkg.lock and vendor/.
If you run "dep ensure" again before actually importing it, it will disappear from Gopkg.lock and vendor/.

This is true for all calls, even for additional and dep ensure -add, so you will only be able to add a single dependency before actually importing it (This could be solved in a separate issue)

Still it feels correct to allow running dep ensure on an empty project.

The log output without having a fatal error is not even confusing:

dep ensure -v
no dirs contained any Go code
Root project is "github.com/golang/test"
 0 transitively valid internal packages
 0 external packages imported from 0 projects
(0)   ✓ select (root)
  ✓ found solution with 0 packages from 0 projects

Solver wall times by segment:
  select-root: 1.0007ms
        other:       0s

  TOTAL: 1.0007ms

@sdboyer
Copy link
Member

sdboyer commented Dec 5, 2017

This is true for all calls, even for additional and dep ensure -add, so you will only be able to add a single dependency before actually importing it

you can pass multiple arguments to dep ensure -add, but...

(This could be solved in a separate issue)

that's the only option. anything else would require hanging on to additional state in a sidecar somewhere, which means more files on disk. if you want to permanently guarantee that something not in your imports is still included, you need a required entry in Gopkg.toml.

Still it feels correct to allow running dep ensure on an empty project.

eh, i could be convinced either way? on the one hand, it is a harmless no-op; on the other, what's the benefit of having it not complain to the user when we know the inputs are nonsensical? that seems like a useful guardrail that could help inform the user as to dep's intended use pattern.

@niondir
Copy link

niondir commented Dec 5, 2017

IMHO the log output from the no-op explains way more than the short error message. Of course, than the fix could be a better error message as well ;)

I don't really have a strong opinion on this, but I took this as a chance to get my hands dirty on the code. I made a small PR out of it but can't judge my self if it should go through.

@trushton
Copy link

I'm running into a case where this would be beneficial (to run dep ensure on an empty project).
I'm using a docker multistep build and building a base image which is then be used in each of my microservices, and it would be helpful if I could simply add the Gopkg.toml and Gopkg.lock to the base image, and then run dep ensure to get my packages into that base image.

This way, dep ensure only needs to be run once, then each of the microservices just copies in everything from that base image and continues from there. As it currently stands I have to copy in my code from that base image and then run dep ensure for each of the 10+ microservices.

@sdboyer
Copy link
Member

sdboyer commented Dec 13, 2017

@trushton that's a slightly different case, and should be covered by dep ensure -vendor-only.

@kenperkins
Copy link

kenperkins commented Feb 2, 2018

An additional data point: the docs contradict what the current functionality is (emphasis mine):

At this point, our project is initialized, and we're ready to start writing code. You can open up a .go file in an editor and start hacking away. Or, if you already know some projects you'll need, you can pre-populate your vendor directory with them:

$ dep ensure -add github.com/foo/bar github.com/baz/quux

https://github.com/golang/dep/blob/master/docs/new-project.md

@sdboyer
Copy link
Member

sdboyer commented Feb 2, 2018

crap. I'd forgotten that we hadn't fixed this yet when i wrote that. thank you for calling attention to it.

if you wanted to submit a PR with a parenthetical caveat link to this issue, similar to elsewhere in the docs when there's an outstanding bug... i wouldn't cry about it 🙏🎉

@ORESoftware
Copy link

If the project won't build, maybe it will build after the dependency is added/sourced? (shrug) lol

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

Successfully merging a pull request may close this issue.

10 participants