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

x/build: reconsider the large number of third-party dependencies #29935

Open
mvdan opened this Issue Jan 25, 2019 · 18 comments

Comments

Projects
None yet
9 participants
@mvdan
Copy link
Member

mvdan commented Jan 25, 2019

Today, a coworker and myself noticed how doing a go get -u downloaded a large number of modules which were completely uninteresting to us, mostly from @dmitshur:

go: finding github.com/shurcooL/go-goon latest
go: finding github.com/shurcooL/github_flavored_markdown v0.0.0-20181002035957-2122de532470
go: finding github.com/shurcooL/component latest
go: finding github.com/shurcooL/htmlg v0.0.0-20170918183704-d01228ac9e50
go: finding honnef.co/go/tools latest
go: finding github.com/shurcooL/events latest
go: finding github.com/shurcooL/highlight_diff v0.0.0-20170515013008-09bb4053de1b
go: finding github.com/shurcooL/issues v0.0.0-20181008053335-6292fdc1e191
go: finding github.com/gregjones/httpcache latest
go: finding sourcegraph.com/sqs/pbtypes v0.0.0-20180604144634-d3ebe8f20ae4
go: finding github.com/neelance/sourcemap latest
go: finding github.com/shurcooL/httperror v0.0.0-20170206035902-86b7830d14cc
go: finding github.com/jellevandenhooff/dkim latest
go: finding github.com/shurcooL/htmlg latest
go: finding github.com/shurcooL/highlight_diff latest
go: finding github.com/shurcooL/github_flavored_markdown latest
go: finding golang.org/x/exp v0.0.0-20190121172915-509febef88a4
go: finding github.com/shurcooL/go latest
go: finding github.com/russross/blackfriday v1.5.2
go: finding github.com/shurcooL/issues latest
go: finding sourcegraph.com/sqs/pbtypes v1.0.0
go: finding github.com/shurcooL/httperror latest
go: finding github.com/prometheus/procfs latest

You can reproduce this by running go get -u on https://github.com/xo/usql. What's more worrying, commands like go mod why github.com/shurcooL/httpgzip report that the main module doesn't need all those packages.

After some manual digging, we ended up tracing this to the cloud.google.com/go module: https://github.com/googleapis/google-cloud-go/blob/52299f000fab02ff521d180fd9f101ceb44d0f38/go.mod

And in particular, its requirement on the golang.org/x/build module: https://github.com/golang/build/blob/ad59fb13d31575de611cdf1252fa3fbfd8f07794/go.mod

It's in that last go.mod file where one can see over fifty third-party module requirements, counting // indirect lines.

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/build module.

However, cloud.google.com/go, and by extension many other popular modules like google.golang.org/grpc and google.golang.org/api, require x/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, as go get -u needs to do much more work.

I do understand that, once module mirrors are widespread, we'll only have to download go.mod files for these unnecessary modules, and not clone their entire git repositories. Still, I think it's bad design that cloud.google.com/go is 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:

  • Actively discourage "library" modules from requiring x/build (though cloud.google.com/go imports non-trivial chunks of x/build)
  • Split up the library-useful bits of x/build into a separate module
  • Gradually remove the third party dependencies of x/build, bringing it more in line with other golang.org/x/... repos

I 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/x repos, so I personally think it's reasonable of them to default to trusting x/build.

/cc @bradfitz @dmitshur @andybons @bcmills

@gopherbot gopherbot added this to the Unreleased milestone Jan 25, 2019

@gopherbot gopherbot added the Builders label Jan 25, 2019

@dmitshur

This comment has been minimized.

Copy link
Member

dmitshur commented Jan 25, 2019

What's more worrying, commands like go mod why github.com/shurcooL/httpgzip report that the main module doesn't need all those packages.

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 go get -u golang.org/x/build/maintner/cmd/maintserve. They wouldn’t be downloaded if you were installing any other libraries or commands in x/build or things that imported a library from x/build.

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:

  • I’m surprised that google cloud libraries would be relying on x/build module
  • I don’t understand why dependencies of a single command in x/build module need to be downloaded when building something that only imports libraries from x/build
  • it’s unfortunate to hear that go mod why wasn’t more helpful for you to figure out where the httpgzip package is coming from
@bcmills

This comment has been minimized.

Copy link
Member

bcmills commented Jan 25, 2019

(Responses in no particular order.)

commands like go mod why github.com/shurcooL/httpgzip report that the main module doesn't need all those packages.

go mod why gives you a path through the package-import graph, not through the module graph. (See #27900.)

@bcmills

This comment has been minimized.

Copy link
Member

bcmills commented Jan 25, 2019

After some manual digging, we ended up tracing this to the cloud.google.com/go module

I note:

~/src/cloud.google.com/go$ go mod why -m golang.org/x/build
# golang.org/x/build
cloud.google.com/go/profiler/proftest
golang.org/x/build/kubernetes

Arguably cloud.google.com/go should use the official Kubernetes client (k8s.io/client-go) instead of the one from golang.org/x/build (which is specifically tailored for use within x/build, and to my knowledge carries no compatibility guarantees).

If the official kubernetes client is too heavy-weight for cloud.google.com, that's probably something they should take up with Kubernetes folks directly.

(CC @jadekler @enocom @jba @zombiezen for cloud.google.com; CC @liggitt for k8s.io/client-go)

@bcmills

This comment has been minimized.

Copy link
Member

bcmills commented Jan 25, 2019

Note that x/build is almost entirely project-internal infrastructure.

x/tools is the more user-facing “utility junk drawer”, and it does depend on x/build (via golang.org/x/tools/cmd/tip), but we should arguably solve that by carving cmd/tip out of x/tools rather than worrying too much about the dependencies of x/build.

@bcmills

This comment has been minimized.

Copy link
Member

bcmills commented Jan 25, 2019

So I guess that's all to say, the option I prefer is the first one that @mvdan mentioned:

  • Actively discourage "library" modules from requiring x/build
@bradfitz

This comment has been minimized.

Copy link
Member

bradfitz commented Jan 25, 2019

I'm also fine splitting out useful stuff elsewhere.

@dmitshur

This comment has been minimized.

Copy link
Member

dmitshur commented Jan 25, 2019

About x/tools/cmd/tip, we plan to move that out of x/tools (into x/website or x/build) as part of #29206. /cc @cnoellekb

@enocom

This comment has been minimized.

Copy link

enocom commented Jan 25, 2019

/cc @aalexand whose team owns the cloud.google.com/go/profiler package.

@bcmills bcmills added the modules label Jan 25, 2019

@mvdan

This comment has been minimized.

Copy link
Member Author

mvdan commented Jan 25, 2019

Thanks all for the replies :)

not through the module graph. (See #27900.)

Thanks - that will help dig through these issues in the future.

Note that x/build is almost entirely project-internal infrastructure.

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.

I don’t understand why dependencies of a single command in x/build module need to be downloaded when building something that only imports libraries from x/build

That's how modules and MVS work; #29832 is somewhat related. In short, go get -u needs to get all the go.mod files in the entire module graph. Perhaps @bcmills can clarify if I said anything wrong.

@mvdan

This comment has been minimized.

Copy link
Member Author

mvdan commented Jan 25, 2019

when building something that only imports libraries from x/build

A clarification - note that my issue is about running go get -u, not go build or go test. Having to fetch the latest version of all these unnecessary modules is what's causing slowness and pain.

@aalexand

This comment has been minimized.

Copy link
Contributor

aalexand commented Jan 25, 2019

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.

@nolanmar511 @jqll @kalyanac FYI

@bradfitz

This comment has been minimized.

Copy link
Member

bradfitz commented Jan 25, 2019

Can you hide that package behind a build tag?

@aalexand

This comment has been minimized.

Copy link
Contributor

aalexand commented Jan 25, 2019

@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?

@thepudds

This comment has been minimized.

Copy link

thepudds commented Jan 25, 2019

go mod tidy looks across all build tags, so build tag alone might be insufficient to address the initial concern raised by @mvdan.

Edit: and sorry, not attempting to comment on all that’s been said here. Just wanted to inject one comment on current behavior.

@bcmills

This comment has been minimized.

Copy link
Member

bcmills commented Jan 25, 2019

Parameterizing mod tidy on build tags is #25873.

@bcmills

This comment has been minimized.

Copy link
Member

bcmills commented Jan 25, 2019

This package is intended to be useful to us and "invisible" / transparent to the rest of cloud-go users.

In that case, you could consider splitting it out into its own module.

@jadekler

This comment has been minimized.

Copy link
Contributor

jadekler commented Jan 25, 2019

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.

@gopherbot

This comment has been minimized.

Copy link

gopherbot commented Feb 13, 2019

Change https://golang.org/cl/162401 mentions this issue: godoc/dl, godoc/proxy, godoc/short, internal/memcache: delete

gopherbot pushed a commit to golang/tools that referenced this issue Feb 20, 2019

godoc/dl, godoc/proxy, godoc/short, internal/memcache: delete
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>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment