vendor: switch to gvt #11732
vendor: switch to gvt #11732
Conversation
dt
force-pushed
the
gvt
branch
2 times, most recently
from
1bb2901 to
ce89c49
Compare
|
should glide-up.sh have been removed? the manifest is now in the submodule, which makes it impossible to see what versions are being changed in a given PR. can it not be? Reviewed 4 of 4 files at r1. Comments from Reviewable |
all files reviewed at latest revision, all discussions resolved, all commit checks successful.
|
Review status: all files reviewed at latest revision, 1 unresolved discussion, all commit checks successful. vendor, line 1 at r1 (raw file):
I don't see this SHA in the vendored repo. How do I review this? Comments from Reviewable |
|
That SHA is on a branch called `gvt` in the vendor repo.
…On Wed, Nov 30, 2016 at 8:48 PM, Peter Mattis ***@***.***> wrote:
Review status: all files reviewed at latest revision, 1 unresolved
discussion, all commit checks successful.
------------------------------
*vendor, line 1 at r1
<https://reviewable.io:443/reviews/cockroachdb/cockroach/11732#-KXs3BB4SRhrr9SdvncP:-KXs3BB4SRhrr9SdvncQ:bf9qcrp>
(raw file
<https://github.com/cockroachdb/cockroach/blob/a485d76f2a40be9affbdfca44d0df4fca959d751/vendor#L1>):*
Subproject commit ab8b3274eedfe6c3b1e8fb3df4566af83dee0f6f
I don't see this SHA in the vendored repo. How do I review this?
------------------------------
*Comments from Reviewable
<https://reviewable.io:443/reviews/cockroachdb/cockroach/11732>*
—
You are receiving this because you were assigned.
Reply to this email directly, view it on GitHub
<#11732 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/ABdsPNmWA0k9cDqhK5yqqj3Ne_x30ePkks5rDidRgaJpZM4LAvnb>
.
|
dt
force-pushed
the
gvt
branch
2 times, most recently
from
4684678 to
b51b649
Compare
|
@tamird deleted |
|
OK, last question - how does one regenerated gvt-manifest.json, if needed? Reviewed 3 of 3 files at r2. Comments from Reviewable |
|
Note that the |
|
Yeah, so we should check in the script that does that, perhaps mod the manual version pinning (though if it did the version pinning that would be great as well). |
|
if we're in the steady-state of having a sufficient (e.g. capturing all transitive deps) That said, I added the |
|
Sure. The point of adding that script is also auditability. The diff in the vendor repo is over a million lines, and the manifest checked in here is over 1700 lines; not exactly feasible to review either. lgtm, i guess, though i'm really taking this on faith because there's no feasible way to review the actual dependencies changed here. Reviewed 1 of 1 files at r3. scripts/3rdparty-imports.sh, line 9 at r3 (raw file):
missing newline. also noticed there is a superset of this logic in teamcity-checkdeps, can you consolidate? Comments from Reviewable |
|
Review status: all files reviewed at latest revision, 2 unresolved discussions, all commit checks successful. scripts/3rdparty-imports.sh, line 9 at r3 (raw file): Previously, tamird (Tamir Duberstein) wrote…
wait a second, it seems to me that both this and teamcity-checkdeps have the wrong logic. shouldn't we also include Comments from Reviewable |
|
Review status: all files reviewed at latest revision, 2 unresolved discussions, all commit checks successful. scripts/3rdparty-imports.sh, line 9 at r3 (raw file): Previously, tamird (Tamir Duberstein) wrote…
Hm, yeah, I think this should be: The difference in output: diff --git a/old b/old
index 8b71e55..11e789f 100644
--- a/old
+++ b/old
@@ -1,32 +1,92 @@
+github.com/VividCortex/ewma
+github.com/abourget/teamcity
+github.com/biogo/store/interval
github.com/biogo/store/llrb
github.com/cenk/backoff
github.com/chzyer/readline
+github.com/cockroachdb/c-jemalloc
+github.com/cockroachdb/c-protobuf
+github.com/cockroachdb/c-rocksdb
+github.com/cockroachdb/cmux
+github.com/cockroachdb/cockroach-go/crdb
+github.com/cockroachdb/pq
+github.com/codahale/hdrhistogram
github.com/coreos/etcd/raft
github.com/coreos/etcd/raft/raftpb
github.com/docker/docker/api/types
github.com/docker/docker/api/types/container
+github.com/docker/docker/api/types/events
+github.com/docker/docker/api/types/filters
+github.com/docker/docker/api/types/network
+github.com/docker/docker/client
+github.com/docker/docker/pkg/jsonmessage
github.com/docker/docker/pkg/namesgenerator
+github.com/docker/docker/pkg/stdcopy
+github.com/docker/go-connections/nat
+github.com/dustin/go-humanize
+github.com/elastic/gosigar
+github.com/elazarl/go-bindata-assetfs
+github.com/facebookgo/clock
github.com/go-sql-driver/mysql
github.com/gogo/protobuf/jsonpb
github.com/gogo/protobuf/proto
+github.com/gogo/protobuf/protoc-gen-gogo/descriptor
+github.com/gogo/protobuf/sortkeys
+github.com/gogo/protobuf/vanity
+github.com/gogo/protobuf/vanity/command
+github.com/golang/protobuf/proto
github.com/google/btree
github.com/google/go-github/github
+github.com/grpc-ecosystem/grpc-gateway/runtime
+github.com/grpc-ecosystem/grpc-gateway/third_party/googleapis/google/api
+github.com/grpc-ecosystem/grpc-gateway/utilities
+github.com/jeffjen/datefmt
+github.com/kisielk/gotool
github.com/kr/pretty
+github.com/kr/text
+github.com/leekchan/timeutil
github.com/lib/pq
github.com/lib/pq/oid
+github.com/lightstep/lightstep-tracer-go
+github.com/mattn/go-isatty
github.com/montanaflynn/stats
github.com/olekukonko/tablewriter
github.com/opentracing/basictracer-go
github.com/opentracing/opentracing-go
+github.com/opentracing/opentracing-go/ext
github.com/opentracing/opentracing-go/log
+github.com/petermattis/goid
github.com/pkg/errors
github.com/prometheus/client_model/go
+github.com/prometheus/common/expfmt
+github.com/rcrowley/go-metrics
+github.com/rcrowley/go-metrics/exp
github.com/rubyist/circuitbreaker
+github.com/satori/go.uuid
+github.com/spf13/cobra
+github.com/spf13/cobra/doc
github.com/spf13/pflag
+github.com/tebeka/go2xunit/lib
github.com/termie/go-shutil
+golang.org/x/crypto/bcrypt
+golang.org/x/crypto/ssh/terminal
golang.org/x/net/context
+golang.org/x/net/http2
golang.org/x/net/trace
+golang.org/x/oauth2
+golang.org/x/text/collate
+golang.org/x/text/language
+golang.org/x/text/unicode/norm
google.golang.org/grpc
+google.golang.org/grpc/codes
google.golang.org/grpc/credentials
+google.golang.org/grpc/grpclog
+google.golang.org/grpc/peer
+google.golang.org/grpc/transport
gopkg.in/inf.v0
gopkg.in/yaml.v2
+honnef.co/go/lint
+honnef.co/go/lint/lintutil
+honnef.co/go/simple
+honnef.co/go/staticcheck
+honnef.co/go/unused
Comments from Reviewable |
|
Review status: all files reviewed at latest revision, 2 unresolved discussions, all commit checks successful. scripts/3rdparty-imports.sh, line 9 at r3 (raw file): Previously, tamird (Tamir Duberstein) wrote…
Oh, this is still incorrect because it doesn't consider all platforms' dependencies. In other words, this is affected by robfig/glock#29. Comments from Reviewable |
|
The Review status: all files reviewed at latest revision, 2 unresolved discussions, all commit checks successful. Comments from Reviewable |
|
Unfortunately using buildutil and |
|
That said, looking at pruning the excessive superset of our deps that |
|
@petermattis Digging into Glide's workflow for bumping a single dependency (bumping the version in |
|
hello! glide person here. also, a pm committee member. no, i'm not here to argue against this decision you've made - i promise! first, i'd love it if someone could hop on voice with me for 20 minutes or so to describe the workflow challenges you've had with glide. we've spent a long time (both glide and the committee) planning and laying out user stories, but this is a complex domain and it's disturbingly easy to miss stuff. it's especially helpful to get input from a more complex project - and cockroach certainly qualifies as that. second, i thought i'd address/clarify some of the things raised here...
yep, we're in the process of moving glide to use an entirely new engine.
yeah, this is a significant issue, one that everyone who works in this space is painfully aware of. there are definitely solutions for dealing with the finite sets of build tags (so, GOOS and GOARCH info). there might be one for the infinite set that is other build tags...but afaik, nobody's figured that one out. in general though, yes, those solutions start with parsing all .go files directly and not using
this wasn't the original reason that information was included (in the lock), but it's one of the good reasons to keep it sticking around :)
more or less, yeah. at the very least, the fact that glide allows for the possibility of revising its choice of dependency as it moves through is what makes updating just one dependency at a time a hard problem. that's a well-known issue, too - Masterminds/glide#252, Masterminds/glide#328, Masterminds/glide#368. (it's also one that's definitively solved by gps 😎 ) |
|
@sdboyer FWIW, we've decided to stick with glide for the moment. I'd like to mention that another problem I have with glide is that it tracks everything in the glide.yaml (not just glide.lock), which can result in weird rot where a package is no longer used and yet Previously, when we were using glock, we could just Also:
This development seems to have stalled, no? At least, https://github.com/Masterminds/glide/tree/gps-integration hasn't seen any movement in over a month. |
|
@sdboyer another thing: glide's behaviour where it resolves dependencies of the top-level package to versions used by other dependencies is infuriating and, well, stupid. Concretely, we depend on github.com/coreos/etcd/raft, which has a bunch of vendored depdencies that overlap with ours (e.g. grpc) - glide will force us to use etcd's version of grpc, even though we are not doing vendor flattening. That is horrible. |
ah, ok! i missed that. good to know, though it changes nothing about my intentions here :)
yep, definitely a known annoyance. those package statements in the manifest will definitely be going away in the refactor. (in fact, they're already gone - Masterminds/glide#565). probably not before, though.
cool - post-refactor behavior should be much closer to this.
It has been a slow month - things have picked up at my dayjob, the committee's taken up a lot of my discretionary time, and I am a tad blocked on one particular design question right now. But we are still moving forward.
ah yeah, so, this is a biggie. concrete first: under glide's new engine, you'll be able to issue an override for grpc, so you can supercede etcd and control the version of grpc that gets used. so at least you won't be subject to the vagaries of your deps. the general issue of flattening, though, is really knotty. this is the diamond dep problem. There's basically two approaches to this: either you flatten, trying to reconcile everything together (my friends and I like to call this as the "highlander" option), or you let things encapsulate their own deps (a la npm). The former leads you to an NP-complete problem; the latter veers towards a metastasizing state monster. More concretely:
As with all things engineering, these issues aren't necessarily disqualifiers of the approach - they're just factors to consider when making tradeoffs. My own view is that highlander ought to be the baseline behavior, because that's the generally safer path for users...but that finding ways to make specific exceptions can be very valuable. For self-contained things like grpc - where risks 1 and 2 are significantly lessened - not having the choice to make an exception can really rankle. glide, and the tool the committee is working on, are both following the highlander path right now, but I don't think we're ruling out the possibility of exceptions at some future point. In fact, @spf13 brought a suggestion to the committee a couple months ago that could get us a long way towards it. |
|
On Fri, Dec 2, 2016 at 12:51 AM, sam boyer ***@***.***> wrote:
ah yeah, so, this is a biggie. concrete first: under glide's new engine,
you'll be able to issue an override for grpc, so you can supercede etcd and
control the version of grpc that gets used. so at least you won't be
subject to the vagaries of your deps.
the general issue of flattening, though, is really knotty. this is the diamond
dep problem <http://www.well-typed.com/blog/9/>. There's basically two
approaches to this: either you flatten, trying to reconcile everything
together (my friends and I like to call this as the "highlander"
<http://blogs.forrester.com/f/b/users/JGOWNDER/highlander_0.jpg> option),
or you let things encapsulate their own deps (a la npm). The former leads
you to an NP-complete problem; the latter veers towards a metastasizing
state monster. More concretely:
1. You have the type incompatibility issues described in the
aforelinked article; Peter Bourgon first raised these publicly in a Go
context last March
<https://groups.google.com/forum/#!searchin/golang-nuts/bourgon%7Csort:date/golang-nuts/AnMr9NL6dtc/PWM6RgpdCAAJ>.
It's even a bit worse for Go, as encapsulating the same version twice would
still cause the type incompatibilities.
2. There's potential correctness issues as well, if any of the
duplicated packages have global/package-level state that shouldn't be
duplicated (e.g. db connections).
3. If reproducibility is something you value, then managing and
maintaining the encapsulated tree is a more difficult task than dealing
with a flattened list. npm has had reproducibility and nondeterminism
problems for years; here's their docs on it
<https://docs.npmjs.com/how-npm-works/npm3-nondet>. Yarn's grappling
<yarnpkg/yarn#579> with this
<yarnpkg/yarn#838>, too, of course.
4. There's also the (relatively) minor costs of binary bloat.
As with all things engineering, these issues aren't necessarily
disqualifiers of the approach - they're just factors to consider when
making tradeoffs. My own view is that highlander ought to be the baseline
behavior, because that's the generally safer path for users...but that
finding ways to make specific exceptions can be very valuable. For
self-contained things like grpc - where risks 1 and 2 are significantly
lessened - not having the choice to make an exception can really rankle.
glide, and the tool the committee is working on, are both following the
highlander path right now, but I don't think we're ruling out the
possibility of exceptions at some future point. In fact, @spf13
<https://github.com/spf13> brought a suggestion to the committee a couple
months ago that could get us a long way towards it.
Yes, I understand this motivation, but we do not actually use the bits of
etcd that depend on grpc - glide still blindly tries to resolve the
dependencies. We should be able to opt out of this, but we can't. Note that
manually pinning grpc is already possible, but it's a pain because it
effectively breaks `glide up`.
|
If "do not actually use" means "do not import the packages in etcd that import grpc," then that sounds like a bug. Otherwise, we'll get there when we can. It's unsafe in the general case, and that's what we have to prioritize first.
Does "manually pinning" here mean specifying the rev in |
|
Just added a test to doubly confirm that if the issue is that you're not importing the packages in etcd that import grpc, then you'll no longer have this problem once glide switches to gps. ("Doubly" because the preceding test already covered this, but in a slightly different root-vs-dep configuration.) |
Correct, we do not transitively import grpc via etcd.
Yes.
Thanks. Any chance of getting this behaviour change prior to the transition to gps? Seems like that change isn't imminent. |
|
@sdboyer Thanks for stopping by and for the clarification / guidance! Going back a bit, this change was originally motivated by a couple usability bumps we hit (more below) and because were struggling to understand why we sometimes did or didn't have a particular dependency resolved / vendored after running Specifically, initially I assumed that However this hasn't appeared to be what happens in practice. Similarly, in #11759, a dependency was removed from glide.yaml but was still imported in, and only in, cockroachdb/cockroach/pkg/... (e.g. it was not a transitive dependency of something in glide.yaml) but it wasn't removed from I filed issues for the minor usability bumps we ran into: Finally, while I don't think it is currently hurting us, it looked like there was a correctness concern, where glide wasn't fetching/vendoring the content of submodules used in dependencies: |
Sure! Sadly, it's pretty unlikely. Things like this are a fairly fundamental algorithm change, and changing glide's existing one to address this specific case could easily end up requiring a significant teardown and rebuild. And even then, it would be unwise to do and introduce change/instability for our users when we're already planning on doing that by introducing gps, where we know the problem is solved. https://gist.github.com/sdboyer/b0813bf2b9dba58a335a85092085472f
Yeah, this specific issue is...well, I'm not sure why that bug exists the way it does. Early glide code decided to allow for the possibility of not doing transitive resolution, and get appears to follow that path. That will no longer be the case (it won't even be possible) after the shift. (thanks for the issue, btw!) Similarly, in #11759, a dependency was removed from glide.yaml but was still imported in, and only in, cockroachdb/cockroach/pkg/... (e.g. it was not a transitive dependency of something in glide.yaml) but it wasn't removed from glide.lock by glide upuntil the import was removed from our source. Ah, right. So, this one is unlikely to change after the gps transition, because imports are treated as the end-all-be-all of what needs to be there. You should have other recourse, though, if needed. This goes to a really heavily tradeoff-laden problem of whether something appearing in
Ah, yeah. There's a few issues related to the underlying problem here. It does make me a little squirmy, but this particular one shouldn't be that bad.
Uuuuugh, these are gonna put a kink in my day 😼 yeah, we'll discuss over there. Thanks for filing these, and for the thoughtful and specific response! It's tremendously helpful. |
|
@sdboyer we're definitely in the That said, a command that can be run when desired, that updated Anyway, thanks again for dropping in here and helping clarify things for us, and as you suggested, we'll continue discussions of the individual issues brought up over on them. Cheers! |
|
(if there's an issue/thread where we should contribute our use cases for explicit, non-import deps, I can copy-paste the above there too). |
|
@sdboyer another issue i often see with glide: This is miraculously fixed upon re-rerunning glide (typically |
Ah, so, that essay may have been slightly misleading - sorry, it was just one snapshot of a very large conversation. The plan, certainly for the committee's tool, and probably for the next version of glide, is to have both These have to be separate directives from constraints, really, because we express constraints at a project level, but imports work on a package level. Without package-level granularity in the requirements information, it would be impossible to solve one of the other problems we've discussed here...unless we were to list out those subpackages explicitly in the manifest, as well. Which, ugh.
I think they're pretty well-understood, but please feel free 😄 Masterminds/glide#418 sdboyer/gps#42 (this is one of now only a couple things i haven't actually implemented in gps yet)
Honestly, there seem to be a fair number of these, and my eyes kinda glaze over. i'm not as familiar with the existing glide engine as others are, but because all of it is slated to go away in favor of gps (which does not have those issues, at least as far as the test suites show), it's kinda hard to summon the motivation to go spelunking for these sorts of things. That said, there isn't much concurrency in the process of picking deps in glide right now - the algorithm is generally not amenable to it. These issues often end up being state tripping over itself in good ol' serial execution. |
Glide workflows have proved painful in practice. It looks like they're doing some significant refactoring right now and moving to a new rewrite at some point, but in the mean time the slightly simpler
gvtmight be easier for us.Our fork has the submodule and new-deps-from-update fixes, so we can use that for now.
gvtdoesn't attempt to determine dependencies, but rather just fetches what it is told to -- this ends up working better e.g. for vendoring our tools as well as code deps.This change is