Skip to content
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

glide get foo does not get dependencies of foo #690

Closed
dt opened this issue Nov 28, 2016 · 8 comments
Closed

glide get foo does not get dependencies of foo #690

dt opened this issue Nov 28, 2016 · 8 comments

Comments

@dt
Copy link

dt commented Nov 28, 2016

In an empty directory, I ran

glide init
glide get --all-dependencies github.com/grpc-ecosystem/grpc-gateway/protoc-gen-grpc-gateway

However, after that, go build ./vendor/github.com/grpc-ecosystem/grpc-gateway/protoc-gen-grpc-gateway fails because protoc-gen-grpc-gateway imports github.com/golang/glog, but glog isn't vendored.

my glide.yaml looks as expected:

package: github.com/dt/demo
import:
- package: github.com/grpc-ecosystem/grpc-gateway
  subpackages:
  - protoc-gen-grpc-gateway

but my glide.lock doesn't have any transitive dependencies in it:

hash: 961f8518d0ae0c9ad9e6af0d491e0a401e22da89e9f39496d377c8c2ed58eb7d
updated: 2016-11-28T21:17:18.379915053Z
imports:
- name: github.com/grpc-ecosystem/grpc-gateway
  version: 84398b94e188ee336f307779b57b3aa91af7063c
  subpackages:
  - protoc-gen-grpc-gateway
testImports: []
@dt dt changed the title glide get foo skips transitive dependencies of foo glide get foo does not get dependencies of foo Nov 28, 2016
@dt
Copy link
Author

dt commented Dec 8, 2016

@sdboyer: any thoughts on this one?

@sdboyer
Copy link
Member

sdboyer commented Dec 8, 2016

I suspect the issue here is basically what's laid out in sdboyer/gps#42, which I've punted on so far - when running glide get on something, it's typically not actually imported yet by the current project. When that's the case, the named package is never visited and analyzed for its own dependencies.

I'm not sure how easy the fix is in the current version of glide. It seems like we might be able to add a parameter to installer.Update() that allows explicitly import paths to be passed in...and that might be sufficient. idk the side effects, but eh, it's probably worth a try...

@dt
Copy link
Author

dt commented Dec 8, 2016

I understand that glide considers imports (of which I indeed have none as it was an empty dir) as roots from which to construct the transitive closure, but surely it also should consider any explicit deps in glide.yaml as additional roots and satisfy their transitive deps as well, right?

@sdboyer
Copy link
Member

sdboyer commented Dec 8, 2016

Nope, it does not consider them additional roots. This is part of what that essay I linked you to a few days ago was grappling with.

At minimum, the issue there is that the expectation is (or will be soon) that a project root is what's listed in glide.yaml under import. Such roots may or may not actually point to real go packages, and even if they do, it's quite possible that only some set of their subpackages are actually imported. There is the subpackages list in glide.yaml, of course, but it's not used (honestly idk the history there); we're getting rid of it, primarily because we consider it a poor design to duplicate state held in the manifest that is readily available in the source code itself. That we can do such analysis actually one of the structural advantages Go has as a lang in this problem domain - I'd go so far as to say that utilizing it is idiomatic.

I'm gonna have a PR you can try in a minute here...lessee if it addresses the problem.

@dt
Copy link
Author

dt commented Dec 8, 2016

Hmm, so I'm probably missing something here. fwiw, here's my understanding, with the hope you can point to where I've misunderstood something:

I'm assuming the intent is that if I glide get foo, I can depend on and build a vendored copy of foo, regardless of wether I import foo somewhere or not. (If that is not intended to be true, what is get intended to do?)

To be able to build and depend on foo, get foo must also vendor foo's transitive dependencies.

get must not eagerly expand foos transitive dependencies into glide.yaml, since if a later version of foo stopped depending on some dep bar, bar should no longer appear in the dep closure.

Thus get foo must add foo as an additional root used to calculate the transitive closure.

@sdboyer
Copy link
Member

sdboyer commented Dec 8, 2016

I don't think you've really misunderstood anything, so much as bumped up against a rough edge that we haven't sanded down yet.

(If that is not intended to be true, what is get intended to do?)

The committee has spent quite some time discussing this very question. It's why there is no get command in the loose spec. Basically, get sits in at an awkward midpoint, and having something like it runs against the grain of the kind of tool we're trying to build.

What it does do for now, at least, is add a constraint to the manifest for you, and IMO should at least put the named project into vendor/ as a result of the first run. (That belief is why I made that PR). That way, you can indeed build the project...or at least, maybe you can. Because:

To be able to build and depend on foo, get foo must also vendor foo's transitive dependencies.

Is vendoring foo's transitive dependencies sufficient? Or, if there are subpackages, e.g. foo/bar, do we also then vendor THEIR transitive deps?

I'm not saying these are unanswerable questions. I'm saying that designing a tool with consistent, orthogonal commands and avoids state duplication requires us to step back and think about how this sort of thing should work.

Beyond that, I'm not sure what else to say without repeating what I've already said, so I'll refer you again to the essay I wrote; that more fully illustrates the tradeoffs at hand.

@dt
Copy link
Author

dt commented Apr 17, 2017

@sdboyer Should this be closed wontfix? Seems like the behavior defined as extracting roots from imports, not the manifest, and that seems unlikely to change with new development focused on `dep.

If someone wants to force a root, a workaround like cockroachdb/cockroach#12166 seems to work.

@sdboyer
Copy link
Member

sdboyer commented Apr 18, 2017

Yep, closing wontfix - thanks for the bump.

@sdboyer sdboyer closed this as completed Apr 18, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants