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

Dep ensure without any go code in directories #796

Closed
lclarkmichalek opened this Issue Jun 30, 2017 · 18 comments

Comments

Projects
None yet
8 participants
@lclarkmichalek
Copy link

lclarkmichalek commented Jun 30, 2017

I'd like to be able to do a dep ensure in a directory with just a Gopkg.lock and a Gopkg.toml. All of my dependencies are specified in Gopkg.lock like a good citizen, and I'd like to claim my docker-build-time-tax rebate. This means when I have a Dockerfile like

RUN go get github.com/golang/dep/cmd/dep
ADD Gopkg.lock ./Gopkg.lock
ADD Gopkg.toml ./Gopkg.toml
RUN dep ensure

ADD *.go ./
RUN go install

At the moment, this is nicht sehr gut, as dep replies all dirs lacked any go code. I'd love it if dep could install from my lock file, which is then cachable via docker, before adding my more rapidly changing application code.

Usual apologies if this is a dupe (did a search for 'all dirs lacked any go code' and couldn't find anything other than the PR where the warning was introduced)

@ibrasho

This comment has been minimized.

Copy link
Collaborator

ibrasho commented Jul 1, 2017

Hey, @lclarkmichalek. Thanks for taking the time to open an issue. 😁

You should be able to run dep ensure --vendor-only after #489 is merged. This will populate vendor/ without an attempt to solve again (and thus not requiring the source code).

Edit: As far as I know, this is not possible currently.But @sdboyer should be able to confirm.

@ibrasho

This comment has been minimized.

Copy link
Collaborator

ibrasho commented Jul 1, 2017

Reminder to self: Usage scenarios for build pipeline should be added to the FAQ.

@raykrueger

This comment has been minimized.

Copy link

raykrueger commented Jul 16, 2017

For what it's worth, I got a little farther using

COPY vendor Gopkg.toml Gopkg.lock /go/src/hyatt/astrocreep/
RUN dep ensure

I was attempting to pre-seed the vendor directory in the image with what's local. That resulted in an almost infinite stack of messages such as the following...

Subpackage github.com/docker/docker/daemon/logger is missing. (Package is required by (root).) Subpackage github.com/docker/docker/utils is missing. (Package is required by (root).)Subpackage github.com/docker/docker/volume/drivers is missing. (Package is required by (root).) Subpackage github.com/docker/docker/api/server/middleware is missing. (Package is required by (root).)Subpackage github.com/docker/docker/api/server/router/volume is missing. (Package is required by (root).) Subpackage github.com/docker/docker/daemon/logger/jsonfilelog is missing. (Package is required by (root).)

That --vendor-only support is a must-have.

@sdboyer

This comment has been minimized.

Copy link
Member

sdboyer commented Jul 18, 2017

Edit: As far as I know, this is not possible currently.But @sdboyer should be able to confirm.

confirmed.

yep, we definitely need to get #489 in.

@ethanmick

This comment has been minimized.

Copy link

ethanmick commented Jul 18, 2017

Just came here to voice my support for this feature! Also working with docker, and caching the dependencies would be a huge win. Thank you!

@amacneil

This comment has been minimized.

Copy link

amacneil commented Jul 25, 2017

While we're here, is there a reason this isn't just the default, rather than hiding it behind a --vendor-only flag? Coming from other package managers (bundler, npm) my expected behavior was for the install command (or in dep lingo ensure) to blindly install everything requested by the lock file.

I would think that solving the package dependencies should only be necessary when updating the lock file, which should be a separate command (or at least only happen when adding/updating dependencies).

@treeder

This comment has been minimized.

Copy link

treeder commented Jul 25, 2017

I'm with @amacneil on this, it shouldn't try to be smart except on an update, not on ensure (also wonder why it's called ensure instead if install like everything other package manager)..

@sdboyer

This comment has been minimized.

Copy link
Member

sdboyer commented Jul 25, 2017

@amacneil @treeder so, all of this - names, flags, defaults, etc. - is intentional, and we're sticking with the design unless/until it actually proves inadequate, rather than merely unfamiliar.

I explained the design, and the related/motivating choice of ensure as the verb in my Gophercon talk: https://youtu.be/5LtMb090AZI?t=19m54s

@treeder

This comment has been minimized.

Copy link

treeder commented Jul 25, 2017

Watched the talk, thanks for sharing. While I like the concept of one command doing everything, I feel like that's just not flexible enough. I think various steps could/should be different commands with dep ensure being your "do everything" or as you say: "Hey dep, ensure everything's shipshape, kthx". But why not have something like:

  • dep init - to set things up as it does now (basically dep ensure, but create the Gopkg files too)
  • dep install - just read the lock file as is and install dependencies (same as this --vendor-only flag we're talking about)
  • dep update - only update lock file based on what's found in Gopkg.toml
  • dep ensure - update lock file from source code and toml, adding new first level dependencies to Gopkg.toml, then dep install

That's just off the top of my head, something along those lines. There's clearly issues with just having dep ensure be the one command to rule them all, not too mention how slow it can be. npm install and bundle install are fast and problem free, because they don't try to be smart about anything (for npm, especially now that they added the lock file).

@amacneil

This comment has been minimized.

Copy link

amacneil commented Jul 25, 2017

Thanks for the link, it was helpful. I'm sure you have thought about this much more than we have. I also understand the desire for an ensure command since in some other package managers the install command is overloaded to mean both adding new dependencies, and recreating the vendor directory based on lock file (in this case yarn have actually done a better job than npm by separating the yarn add command from yarn install).

I think you can't get away from the fact that there are two operations needed from a package manager regarding state - per your talk, one which updates the state of the world based on all inputs (code + Gopkg.toml -> Gopkg.lock -> vendor dir), and one which deterministically recreates the requested state of the world (Gopkg.lock -> vendor dir). It seems you have stumbled on this need already by deciding to implement dep ensure --vendor-only flag.

So, at this point it's just a case of naming conventions. I agree with your desire to not have unnecessary additional commands with confusing or overlapping responsibilities. However, I think there is an argument to be made that recreating the vendor directory is a common enough operation (especially in build/CI environments) to warrant a first class operation. It also happens that this operation is named install in at least every other package manager I have worked with (bundler, npm, yarn, composer, cocoapods), ignoring any overloaded functions of the command.

My suggestion would be to add a dep install command which is entirely limited to what you were planning with dep ensure --vendor-only. That is, it installs packages the vendor directory, and has no functionality related to adding dependencies or other tasks. I think this is worth doing because it arguably will cause the least surprise for people coming from other package managers, and this reduces learning curve for dep.

Ultimately it's your choice, but I figured it was worth writing this up since the project readme specifically requests feedback on the developer UX, and this is the most confusing thing I have run into using dep so far.

Also, FWIW I found a workaround for this for now by simply adding a single (or even empty) go file to the project root in your Dockerfile. This allows dep to do its thing while maximizing the changes of a docker cache hit:

COPY Gopkg.* main.go ./
RUN dep ensure
@raykrueger

This comment has been minimized.

Copy link

raykrueger commented Jul 25, 2017

Adding a dummy file is a good idea for a workaround for now. Adding any real source from the app is the problem. You'd have to add a source file that never changes. Maybe even 'RUN echo...' something into place.

EDIT: This doesn't appear to work as dep will still read the sourcecode and then decide there's nothing to do.

my Dockerfile includes the following...

COPY Gopkg.* /go/src/blah/blah
RUN echo "package dummy" > main.go
RUN ls -l
RUN dep ensure -v

COPY . /go/src/blah/blah

Dep isn't falling for it...

Root project is "blah/blah"
 1 transitively valid internal packages
 0 external packages imported from 0 projects
(0)   ✓ select (root)
  ✓ found solution with 0 packages from 0 projects
@sdboyer

This comment has been minimized.

Copy link
Member

sdboyer commented Jul 25, 2017

i hear your points, @treeder and @amacneil, but these sorts of changes just aren't on the table right now. it was either a months-ago thing, when we were first articulating this model, or a months-from-now thing, after the model has been in place for a while and we can either consider a 2.0 refactor, or (vastly preferably) when we're talking about the CLI UI for the go command itself.

there are also other considerations, including:

  • the Go team's expressed desire to minimize the command surface area related to dep mgmt
  • the future possible irrelevancy of vendor, at which point the idea of exposing manual disk state manipulation to the user makes even less sense
  • the basic issue that install has often been a confusing verb, vis-a-vis the other verbs, in these other systems

but, let me address some of y'alls' specific points:

@treeder: But why not have something like:

this set of commands is mostly equivalent to dep ensure, dep ensure -no-vendor -update, and dep ensure -vendor-only. the exception is adding new first level dependencies to Gopkg.toml, which is definitely not on the table, as that's even less of a complete operation.

providing them as flags, vs. top-level commands, becomes a design question, where we're really talking about the user's mental model. more on this at the bottom of this comment.

@treeder: not too mention how slow it can be.

speed is certainly relevant to the feasibility of a sync-based approach, which is exactly why we focus on having fast routines for verifying synchronization. we have some, but not all of those right now, and are working towards them. Some more on this in the FAQ. tl;dr - there are roadmaps for these things, and i believe we have a lot of headroom to make dep very snappy. and that's in large part because of this sync-based model, not in spite of it.

@treeder: npm install and bundle install are fast and problem free

and dep ensure -vendor-only will have the same characteristics

@amacneil: So, at this point it's just a case of naming conventions. I agree with your desire to not have unnecessary additional commands with confusing or overlapping responsibilities. However, I think there is an argument to be made that recreating the vendor directory is a common enough operation (especially in build/CI environments) to warrant a first class operation.

@amacneil: I think this is worth doing because it arguably will cause the least surprise for people coming from other package managers, and this reduces learning curve for dep.

i certainly find this line of argument more convincing. however, i think it just ends up backing us into a fully separated command set - i can imagine similar arguments being made for e.g. dep ensure -no-vendor, which is not the right message to send to the user (per below). and i think it makes the most sense to design the interface primarily around human users, so long as we're not doing undue harm to script's expressiveness (and i don't think there's a strong argument that dep ensure -vendor-only instead of dep install constitutes such harm).

while they certainly have their own interpretation of it, it's worth noting that this sync-based approach is a motivating design behind npm5, and continuing with npm6.

@amacneil: Ultimately it's your choice, but I figured it was worth writing this up since the project readme specifically requests feedback on the developer UX, and this is the most confusing thing I have run into using dep so far.

for sure, and the feedback is welcome! it's def not gone the way i would've liked, as we've had docs that are inconsistent with real behavior for some months now. at this point, we're committed to giving the single-command, sync-based approach a fair shot, and #489 fixes a lot of these docs problems alongside the functionality changes.

@treeder: There's clearly issues with just having dep ensure be the one command to rule them all

so, i think this is the big point: focusing on the individual commands is missing the forest for the trees. there are differences from what people are accustomed to with a single command, but different != wrong.

the sync-based model is one that i originally floated back in Feb 2016, and it was just an idea, so i didn't really know how feasible it was. as i've discussed it with more pkg mgmt system designers, as i've read through hundreds (thousands, now?) of issues people have raised with pkg mgrs, and as dep has gained adoption and we've gathered practical feedback on it, it's seemed to bear out the idea of being sync-based as a strong default interaction pattern. thus, dep ensure.

now i am, of course, biased in favor of my own idea. so, let me guide the opposition: the shape of the argument that would convince me sync-based is an error is one that demonstrates why it is a bad idea that, in the default, user-oriented case (emphasis, default - i am not saying e.g. docker layer construction is not important - just that it is OK to sacrifice that CLI UX a tad for these other principles), the tool should aim to always keep the states in sync; that there are useful interstitial, un-synced states that are the norm during development; that these interstitial states are so valuable as to justify the addition of more complex controls onto the tool in order to facilitate the interaction with a larger known good output space.

@treeder

This comment has been minimized.

Copy link

treeder commented Jul 25, 2017

@sdboyer where is the info on the sync-based approach in npm? I couldn't find anything on it. Just curious to read about it.

the shape of the argument that would convince me sync-based is an error is one that demonstrates why it is a bad idea

I don't think anybody is saying it's a bad idea, I think we're just saying it's not the right idea for all use cases. Maybe all these flags you're talking about will get us there, but we'll see.

@amacneil so I tried your idea of throwing in an empty main file and it doesn't work for me.

ADD ./api/Gopkg.* $D/api/
RUN echo -en "package main\n func main() { }" > $D/api/main.go
RUN cd $D/api && ls -al && cat main.go && cat Gopkg.lock && go build
RUN cd $D/api && dep ensure -v

Output:

...
-rw-r--r--    1 root     root          7363 Jun 30 00:22 Gopkg.lock
-rw-r--r--    1 root     root          1357 Jun 30 00:08 Gopkg.toml
-rw-r--r--    1 root     root            29 Jul 25 15:13 main.go
...
package main
 func main() { }

# This file is autogenerated, do not edit; changes may be undone by the next 'dep ensure'.
[[projects]]
  name = "cloud.google.com/go"
  packages = ["compute/metadata","datastore","internal/atomiccache","internal/fields","internal/version"]
  revision = "d4f8670e9dba7e0c9dd3f6da843d678afd21c587"
  version = "v0.9.0"

[[projects]]
  name = "github.com/Sirupsen/logrus"
  packages = ["."]
  revision = "202f25545ea4cf9b191ff7f846df5d87c9382c2b"
  version = "v1.0.0"
...  BUNCH MORE

Step 8/8 : RUN cd $D/api && dep ensure -v
 ---> Running in da16b65fbd69
Root project is "github.com/treeder/myapp/api"
 1 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: 80.008µs
        other: 14.483µs

  TOTAL: 94.491µs

Notice the last part there, dep ensure doesn't do anything, probably because it reads the source and doesn't see any imports, even though the lock file has a bunch set in it.

@sdboyer

This comment has been minimized.

Copy link
Member

sdboyer commented Jul 25, 2017

where is the info on the sync-based approach in npm? I couldn't find anything on it. Just curious to read about it.

TBH, i haven't had time to explore or play with it much, so i can't give a lot more detail. for now, i can point you to twitter conversations. npm may be bringing me out to SF at some point soonish for an impromptu pkg mgmt summit, at which point i expect i'll have a lot more insight.

the shape of the argument that would convince me sync-based is an error is one that demonstrates why it is a bad idea

I don't think anybody is saying it's a bad idea, I think we're just saying it's not the right idea for all use cases.

If that's the argument, then of course I agree - the rest of that sentence/paragraph after what you clipped is expressly noting that this is about balancing the for that day-to-day user use case.

Maybe all these flags you're talking about will get us there, but we'll see.

indeed!

@ebati

This comment has been minimized.

Copy link
Contributor

ebati commented Aug 4, 2017

with #489 merged, using -vendor-only I still get "all dirs lacked any go code" error

@sdboyer

This comment has been minimized.

Copy link
Member

sdboyer commented Aug 4, 2017

yeah, i forgot to carve out specifically this path. i don't have time to do a PR today, but the fix here should, i think, be pretty simple for someone else to do up and test. basically:

  1. this call to checkErrors should gain a parameter that allows there to be absolutely no go code (the first error case in the func)
  2. set that parameter to true if -vendor-only is passed

we'll also need another test harness case to cover this. it's probably as simple as copying the cmd/dep/testdata/harness_tests/ensure/default/hasheq_vendoronly case and removing the initial/main.go file.

@ebati

This comment has been minimized.

Copy link
Contributor

ebati commented Aug 4, 2017

Instead of adding new parameter to checkErrors, i think, we can separate vendorOnly guarded part from runDefault and add new runVendorOnly function with same signature( no need for params actually but to be consistent with other run** functions) and call it before ListPackages.
i can work on this and send a pr if its ok.

@sdboyer

This comment has been minimized.

Copy link
Member

sdboyer commented Aug 4, 2017

@ebati yeah, i like that more. also, the fact that the arg len checker in runDefault() is differentiated on vendorOnly is another indication that having its own sibling method to the others makes the most sense.

go for it!

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