-
Notifications
You must be signed in to change notification settings - Fork 3.8k
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
vendor: add fake imports for glide's benefit #12166
Conversation
c4f6e52
to
efb72d1
Compare
yep. till glide adds explicit |
Reviewed 5 of 5 files at r1. build/README.md, line 45 at r1 (raw file):
seems like we don't advise this at all now build/tool_imports.go, line 31 at r1 (raw file):
change checkdeps to look at this file? ...or what's your plan for checkdeps? Comments from Reviewable |
Review status: all files reviewed at latest revision, 2 unresolved discussions, all commit checks successful. build/README.md, line 45 at r1 (raw file): Previously, tamird (Tamir Duberstein) wrote…
Hm. You need something to fetch the dep, so you can start developing / importing it, and I don't have a better suggestion for now? build/tool_imports.go, line 31 at r1 (raw file): Previously, tamird (Tamir Duberstein) wrote…
That said, this shouldn't make Comments from Reviewable |
Review status: all files reviewed at latest revision, 2 unresolved discussions, all commit checks successful. build/README.md, line 45 at r1 (raw file): Previously, dt (David Taylor) wrote…
would build/tool_imports.go, line 31 at r1 (raw file): Previously, dt (David Taylor) wrote…
Wait, what does buildutil have to do with anything? checkdeps is implemented using Comments from Reviewable |
Review status: all files reviewed at latest revision, 2 unresolved discussions, all commit checks successful. build/README.md, line 45 at r1 (raw file): Previously, tamird (Tamir Duberstein) wrote…
I don't think so -- build/tool_imports.go, line 31 at r1 (raw file): Previously, tamird (Tamir Duberstein) wrote…
How are you suggesting we use Comments from Reviewable |
Review status: all files reviewed at latest revision, 2 unresolved discussions, all commit checks successful. build/README.md, line 45 at r1 (raw file):
I don't know, this is what I'm asking you to test. build/tool_imports.go, line 31 at r1 (raw file): Previously, dt (David Taylor) wrote…
I have no idea what you're referring to here - the current code in checkdeps already does a Comments from Reviewable |
I'm going to add the changes I suggested here to avoid the back-and-forth. |
59101b2
to
fd917c7
Compare
We've repeatedly encountered issues arising from the fact that entries in `glide.yaml` are not treated as the, or even as additional, roots in the calulation of the transitive dependency closure. Previously in cockroachdb#11797, we attempted to clarify the situation by pruning all derivable specs from `glide.yaml`, so that it would be clear where the dependency roots were indeed coming from. We however left vendored tools, and their transitive dependencies, there, as they were not derivable from any imports (e.g. some are not importable due to `package main`). As we've already found though, simply adding a tool to `glide.yaml`, or even `glide get`'ing them, doesn't work. Thus, to reliably vendor tools and their dependencies, we need to introduce artifical imports of them. We get around the `package main` issue with by hiding the imports in a file that is build-tagged `+glide` so we never attempt to build it. --- The `glide up` used to test this picked up a test-only raft change, etcd-io/etcd#6935, and: - golang/tools@ae1141f...3d92dd6 - cockroachdb/cockroach-go@2c874f1...140a8c5 - etcd-io/etcd@cfd10b4...b713113 - distribution/distribution@67095fb...844b928 - google/go-github@171a931...43e7458 - opencontainers/runc@5974b4c...34f23cb
We've repeatedly encountered issues arising from the fact that entries in
glide.yaml
are not treated as the, or even as additional, roots in the calculation of the transitive dependency closure.Previously in #11797, we attempted to clarify the situation by pruning all derivable specs from
glide.yaml
, so that it would be clear where the dependency roots were indeed coming from.We however left vendored tools, and their transitive dependencies, there, as they were not derivable from any imports (e.g. some are not importable due to
package main
).As we've already found though, simply adding a tool to
glide.yaml
, or evenglide get
'ing them, doesn't work: Masterminds/glide#690.Thus, to reliably vendor tools and their dependencies, we need to introduce artifical imports of them. We get around the
package main
issue with by hiding the imports in a file that is build-tagged+glide
so we never attempt to build it.The
glide up
used to test this also picked up the following uninteresting changes:Note: The etcd bump does include a raft change, but it is test only
This change is