x/build: reconsider the large number of third-party dependencies #29935
Comments
Every single one of those packages coming from me are needed only by a single command in x/build: https://godoc.org/golang.org/x/build/maintner/cmd/maintserve See: https://godoc.org/golang.org/x/build/maintner/cmd/maintserve?imports It’s a small program that reuses a lot of my external code to visualize maintner data offline. It’s very dependency heavy. Before modules, it wasn’t a problem to have it in x/build because you would only fetch its dependencies if you ran If it’s causing problems for people in module world, we can consider removing that command from x/build, or making it be a part of a separate module. /cc @bcmills I’m happy to discuss this. Some additional observations:
|
|
(Responses in no particular order.)
|
I note: Arguably If the official (CC @jadekler @enocom @jba @zombiezen for |
|
Note that
|
|
So I guess that's all to say, the option I prefer is the first one that @mvdan mentioned:
|
|
I'm also fine splitting out useful stuff elsewhere. |
|
About |
|
/cc @aalexand whose team owns the |
|
Thanks all for the replies :)
Thanks - that will help dig through these issues in the future.
I'm of course fine with this solution, as long as it's enforced properly. In that case, I'm happy for this issue to be forked into x/tools and cloud-go issues, if that would be better.
That's how modules and MVS work; #29832 is somewhat related. In short, |
A clarification - note that my issue is about running |
|
It should be totally reasonable to get rid of golang.org/x/build/kubernetes dependency in cloud.google.com/go/profiler/proftest, probably replacing it with k8s.io/client-go. In fact, for now that code can just be removed as the profiler GKE tests are broken and that code needs to be updated anyway. So the immediate action is I think to remove the dependency and the code from proftest.go, then figure out how to do it proper. Another thing to discuss here (and this becomes really cloud-go specific, so not sure how much we want to discuss it here) is that the proftest package is a package that is intended to be used only by tests in profiling agents. Currently it's just use by the Go profiling agent integration test plus by Node.js profiling agent integration test that we own at https://github.com/googleapis/cloud-profiler-nodejs. This package is intended to be useful to us and "invisible" / transparent to the rest of cloud-go users. If it's going to hurt users of cloud-go even with k8s.io/client-go dependency instead of golang.org/x/build/kubernetes, we should re-think the existence of this package. |
|
Can you hide that package behind a build tag? |
|
@bradfitz If I add a build tag in proftest.go, will a test with a dependency on that package like https://github.com/googleapis/cloud-profiler-nodejs/blob/6190f20fa3cb24456d8d9ac37b0891b39ebb122a/testing/integration_test.go#L30 still build / run as is? |
|
Edit: and sorry, not attempting to comment on all that’s been said here. Just wanted to inject one comment on current behavior. |
|
Parameterizing |
In that case, you could consider splitting it out into its own module. |
|
That will happen soon (tm) but we're not ready with tooling for multiple modules in that repository yet, regrettably. By 1.13 we should hopefully be there. |
|
Change https://golang.org/cl/162401 mentions this issue: |
These packages existed only to power cmd/godoc for the purpose of serving the golang.org website. That functionality has moved into x/website as part of golang/go#29206. x/website has become the canonical source of golang.org in CL 162157, the golang.org-serving code was removed from cmd/godoc in CL 162400, and these packages can be deleted too now. This removes the last dependency on the cloud.google.com/go module, which results in a significant reduction of the number of indirect dependencies in x/tools (this is due to issue golang/go#29935, which affects the current version of the cloud.google.com/go module). Run go mod tidy (using Go 1.12 RC 1). Updates golang/go#29206 Updates golang/go#29981 Change-Id: If07e3ccae8538b3ebd51af64b6af5be5463f4906 Reviewed-on: https://go-review.googlesource.com/c/162401 Reviewed-by: Bryan C. Mills <bcmills@google.com> Reviewed-by: Channing Kimble-Brown <channing@golang.org> Reviewed-by: Michael Matloob <matloob@golang.org>
|
@aalexand @jadekler If I understand correctly, the current plan on your side is to make the If so, it'd be helpful to open an issue in https://github.com/googleapis/google-cloud-go/issues to track that task, so people can subscribe to it for further updates on this topic. I can open the issue if that'd be helpful. |
|
@dmitshur Absolutely. Please track in googleapis/google-cloud-go#1344 |
|
Change https://golang.org/cl/167461 mentions this issue: |
Now that a pseudo-version of golang.org/x/build module with maintserve carved out exists, the module golang.org/x/build/maintner/cmd/maintserve can depend on it. Do so now. This is analogous to CL 166857. Updates golang/go#29935 Change-Id: I661f32336ede26fde5581527959d31d164a00b63 Reviewed-on: https://go-review.googlesource.com/c/build/+/167463 Run-TryBot: Dmitri Shuralyov <dmitshur@golang.org> Reviewed-by: Brad Fitzpatrick <bradfitz@golang.org> TryBot-Result: Gobot Gobot <gobot@golang.org>
|
Change https://golang.org/cl/170001 mentions this issue: |
bcmills
added
the
NeedsInvestigation
|
Change https://golang.org/cl/170863 mentions this issue: |
|
On a fresh machine with Go master (cb66462), I tried to I don't have GOPROXY set. It took a long time downloading things and was a much worse experience than GOPATH mode: The actual deps are: But note above that it was downloading So I'm in favor of more sub-modules in x/build, as cmd/go apparently can't do any better. |
|
Most often, setting GOPROXY can help significantly, including because it allows downloading a It could be interesting to check if you see better behavior if you do something like A couple sample datapoints: |
|
@bradfitz Just to give some rough numbers, I ran a quick test of
YMMV. This was with a clean GOPATH each time (and hence no module cache), but not a clean machine. You had cited Sample test steps: |
Should speed up downloads. See golang/go#29935 (comment) for details
Should speed up downloads. See golang/go#29935 (comment) for details
|
Is there anything else we want to do in x/build specifically? I'm not seeing more to be done, and I think the original issue is resolved by now. The overall resolution seems to be:
|
I dunno, it at least seems weird that we have direct dependencies on two different |
|
Both What makes you say they're direct dependencies? We are directly importing and requiring https://github.com/anmitsu/go-shlex/blob/648efa622239a2f6ff949fed78ee37b48d499ba4/example_test.go#L8 I'm not seeing a problem here that we need to fix. |
|
Wanted to briefly bounce this. Is there anything left to do here? |
|
What do you mean by "bounce this"? Close it? Work on it? "Ping"? |
|
My interpretation of "bounce" is the same meaning as "ping".
In #29935 (comment), I concluded that the original issue has been resolved and that I don't see anything actionable left to do. Bryan brought up one concern, and I addressed it. No one has brought anything up since then, and it's been 3 months. I think this is done, so I'll close. Otherwise feel free to speak up or open a new issue. Thanks to everyone who helped with this. |
dmitshur
added
NeedsFix
Today, a coworker and myself noticed how doing a
go get -udownloaded a large number of modules which were completely uninteresting to us, mostly from @dmitshur:You can reproduce this by running
go get -uon https://github.com/xo/usql. What's more worrying, commands likego mod why github.com/shurcooL/httpgzipreport that the main module doesn't need all those packages.After some manual digging, we ended up tracing this to the
cloud.google.com/gomodule: https://github.com/googleapis/google-cloud-go/blob/52299f000fab02ff521d180fd9f101ceb44d0f38/go.modAnd in particular, its requirement on the
golang.org/x/buildmodule: https://github.com/golang/build/blob/ad59fb13d31575de611cdf1252fa3fbfd8f07794/go.modIt's in that last
go.modfile where one can see over fifty third-party module requirements, counting// indirectlines.Now, I would understand that the module which sets up golang.org's CI infrastructure needs to have lots of dependencies. After all, it needs to do lots of work. And that would be fine, as long as extremely few packages required the
x/buildmodule.However,
cloud.google.com/go, and by extension many other popular modules likegoogle.golang.org/grpcandgoogle.golang.org/api, requirex/build. So any developer wishing to use Google Cloud with Go is forced to download dozens of third-party Go modules for no apparent reason. This can add up quickly even on a warm cache, asgo get -uneeds to do much more work.I do understand that, once module mirrors are widespread, we'll only have to download
go.modfiles for these unnecessary modules, and not clone their entire git repositories. Still, I think it's bad design thatcloud.google.com/gois pulling so many module requirements, some of which are exclusively for frontend/JS coding.I can imagine multiple solutions, besides leaving the situation as it is now:
x/build(thoughcloud.google.com/goimports non-trivial chunks ofx/build)x/buildinto a separate modulex/build, bringing it more in line with othergolang.org/x/...reposI realise I could have raised this issue against the google-cloud-go repository, but I think it's best to open it here first. Developers also tend to trust
golang.org/xrepos, so I personally think it's reasonable of them to default to trustingx/build./cc @bradfitz @dmitshur @andybons @bcmills
The text was updated successfully, but these errors were encountered: