-
Notifications
You must be signed in to change notification settings - Fork 18.1k
cmd/go, cmd/distpack: build and run tools that are not necessary for builds as needed and don't include in binary distribution #71867
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
Comments
I agree with the overall goal of this issue, but I wanted to note that currently
Perhaps it is enough to rewrite the |
Does it need to change to |
@seankhliao if the path gets changed to point to a cached binary I guess that would also be fine, but that would be a pretty long and random path, and it wouldn't work if the cache isn't warm - for example if it gets cleaned up manually or after a few days. I would personally prefer the shorter form; it will reproduce the build faithfully as long as none of the inputs like the source code is changed. I don't have a strong opinion here, but @matloob personally, out of all the tools you list, I've only ever run
Could you clarify whether the source for these tools would still be shipped? That is, that the first invocation of e.g. If that's the case, wouldn't changing a tool's source code cause |
@mvdan The |
@mvdan Yes, to preserve the ability to run scripts produced by go build -x, we should list
Yes, the source for the tools would still be shipped in GOROOT. The invocation of the tool wouldn't download anything.
If we default to running a tool that's present in |
This proposal has been added to the active column of the proposals project |
Maybe I'm misunderstanding this, but isn't this true today, too? If I change the source to cmd/compile and run |
How much would this save in the download size (in particular, after compression)? At a glance, it looks like a fair amount, but it would be good to have some hard numbers. |
Looking at pkg/tool, it looks like there are a few more things we could leave out: covdata, dist, distpack. There are a few things that are involved in the build, but aren't required to build themselves: cgo, preprofile. And vet is only needed if you're running tests. |
Yes, it's the current behavior, but the behavior when the tool isn't installed is that we will build from source. So users might get surprised by the change in behavior between when a tool is installed in pkg/tool and when it isn't.
I measured a ~15M difference in the module zips that we use to download and run newer toolchains on darwin/arm64. I think I remember adding doc and zip increases the savings to ~17M, but I'd have to re-check that. |
Dist and distpack should already be removed from the distribution: https://cs.opensource.google/go/go/+/master:src/cmd/distpack/pack.go;l=172;drc=fc5073bc155545dde4856cccdfcbb31880d1eb66 I wasn't sure if we could include covdata because I think we cache its output together with the test output and I think it's a bit more tricky to make guarantees on it. On the other hand, we don't seem to include its toolid as part of the build so maybe I'm misreading it? We do include cgo's and preprofile's toolids (and also vet's) as part of the cache keys so if we want to run |
I can think of a use case that potentially can be impacted by this. Suppose some developer downloads a Go distribution, just wants to build binaries to ship to somewhere else, but doesn't run them locally. Previously they only needs to run downloaded binaries. Now with this proposal, if they also wants to, say, investigate some binary using objdump, they'd need to run a binary that is locally built. I can imagine in some environment (e.g. a corporate profile) it only allows binaries downloaded from trusted sources (e.g. our macOS release binaries are signed) but not locally built binaries, or makes it difficult to do so. So they won't be able to easily run Would this be a problem? This is sort of a weird use case. Do people actually do this? |
That's fair. My sense is that this is fairly advanced usage, so maybe it doesn't matter much?
For reference: go1.24.2.darwin-arm64.tar.gz is 73MB, so this is roughly a 20% savings.
👍
Could we make this conditional on whether cgo is used/a PGO profile is present? IIUC, we don't use these tools otherwise. These sorts of details certainly don't have to hold up the proposal. |
Yeah, I think it's unlikely a user will
Currently the toolid is only included in the action id for an action that uses the tool (for example the preprofile tool id is included in the pgoActionID but not for every action), so it is conditional on whether cgo is used or a pgo profile is present. |
Based on the discussion above, this proposal seems like a likely accept. This proposal is to stop including tools that are not needed for builds in the binary distribution, with the goal of reducing the Go binary distribution size. Instead the tools would be built and run as needed by The install target for main packages in cmd will not change, so If a tool binary is present in that location, it will be run from there, but if not, the go command will fall back to building (if the tool binary isn't cached) and running the tool, similarly to how This is expected to reduce Go binary distribution size by ~20%. |
No change in consensus, so accepted. 🎉 This proposal is to stop including tools that are not needed for builds in the binary distribution, with the goal of reducing the Go binary distribution size. Instead the tools would be built and run as needed by The install target for main packages in cmd will not change, so If a tool binary is present in that location, it will be run from there, but if not, the go command will fall back to building (if the tool binary isn't cached) and running the tool, similarly to how This is expected to reduce Go binary distribution size by ~20%. |
Change https://go.dev/cl/666476 mentions this issue: |
If a tool in cmd is not installed in $GOROOT/pkg/tool/${GOOS}_${GOARCH}, go tool will build (if it's not cached) and run it in a similar way (with some changes) to how tools declared with tool directives are built and run. The main change in how builtin tools are run as compared to mod tools is that they are built "in host mode" using the running go command's GOOS and GOARCH. The "-exec" flag is also ignored and we don't add GOROOT/bin to the PATH. A ForceHost function has been added to the cfg package to force the configuration to runtime.GOOS/runtime.GOARCH. It has to recompute the BuildContext because it's normally determined at init time but we're changing it after we realize we're running a builtin tool. (Detecting that we're running a builtin tool at init time would mean replicating the cmd line parsing logic so recomputing BuildContext sounds like the smaller change.) For #71867 Change-Id: I3b2edf2cb985c1dcf5f845fbf39b7dc11dea4df7 Reviewed-on: https://go-review.googlesource.com/c/go/+/666476 LUCI-TryBot-Result: Go LUCI <[email protected]> Reviewed-by: Michael Pratt <[email protected]> Reviewed-by: Michael Matloob <[email protected]>
Change https://go.dev/cl/666755 mentions this issue: |
Change https://go.dev/cl/673119 mentions this issue: |
This change removes some tools that are not used for builds, or otherwise invoked by the go command (other than through "go tool" itself) from the packaged distributions produced by distpack. When these tools are missing, "go tool" will build and run them as needed. Also update a case where we print a buildid commandline to specify invoking buildid using "go tool" rather than the binary at it's install location, because it may not exist there in packaged distributions anymore. The tools in this CL are the lowest hanging fruit. There are a few more tools that aren't used by builds, but we'd have to get the go command to run them using "go tool" rather than finding them in the tool install directory. For #71867 Change-Id: I217683bd549962a1add87405bf3fb1225e2333c5 Reviewed-on: https://go-review.googlesource.com/c/go/+/666755 LUCI-TryBot-Result: Go LUCI <[email protected]> Reviewed-by: Michael Pratt <[email protected]> Reviewed-by: Michael Matloob <[email protected]>
The "doc", "fix", and "covdata" tools invoked by the go command are not needed for builds. Instead of invoking them directly using the installed binary in the tool directory, use "go tool" to run them, building them if needed. We can then stop distributing those tools in the distribution. covdata is used in tests and can form part of a cached test result, but test results don't have the same requirements as build outputs to be completely determined by the action id. We already don't include a toolid for the covdata tool in the action id for a test run. The more principled way to do things would be to load the covdata package, create the actions to build it, and then depend on the output of that action from the the test action and use that as the covdata tool. For now, it's probably not worth the effort, but, in the future, if we wanted to build a tool like cgo as needed, it would be best to build it in the same action graph. That would introduce a whole bunch of complexity because we'd need to build the tool in the host configuration, and all the configuration parameters are global. For #71867 Change-Id: Id9bbbb5c169296f66c072949f9da552424ecfa2f Reviewed-on: https://go-review.googlesource.com/c/go/+/673119 LUCI-TryBot-Result: Go LUCI <[email protected]> Reviewed-by: Michael Pratt <[email protected]> Reviewed-by: Michael Matloob <[email protected]>
Change https://go.dev/cl/673696 mentions this issue: |
Previously, distpack filtered out tools from the packaged distribution using a list of tools to remove. Instead follow mpratt's suggestion on CL 666755 and instead filter out tools that are not on a list of tools to keep. This will make it easier to tell which tools are actually in the distribution. For #71867 Change-Id: I8336465703ac820028c3381a0a743c457997e78a Reviewed-on: https://go-review.googlesource.com/c/go/+/673696 LUCI-TryBot-Result: Go LUCI <[email protected]> Reviewed-by: Michael Pratt <[email protected]> Reviewed-by: Michael Matloob <[email protected]>
We've submitted the changes to do this. |
Does this need a release note (#71661)? |
This change removes some tools that are not used for builds, or otherwise invoked by the go command (other than through "go tool" itself) from the packaged distributions produced by distpack. When these tools are missing, "go tool" will build and run them as needed. Also update a case where we print a buildid commandline to specify invoking buildid using "go tool" rather than the binary at it's install location, because it may not exist there in packaged distributions anymore. The tools in this CL are the lowest hanging fruit. There are a few more tools that aren't used by builds, but we'd have to get the go command to run them using "go tool" rather than finding them in the tool install directory. For golang#71867 Change-Id: I217683bd549962a1add87405bf3fb1225e2333c5 Reviewed-on: https://go-review.googlesource.com/c/go/+/666755 LUCI-TryBot-Result: Go LUCI <[email protected]> Reviewed-by: Michael Pratt <[email protected]> Reviewed-by: Michael Matloob <[email protected]>
The "doc", "fix", and "covdata" tools invoked by the go command are not needed for builds. Instead of invoking them directly using the installed binary in the tool directory, use "go tool" to run them, building them if needed. We can then stop distributing those tools in the distribution. covdata is used in tests and can form part of a cached test result, but test results don't have the same requirements as build outputs to be completely determined by the action id. We already don't include a toolid for the covdata tool in the action id for a test run. The more principled way to do things would be to load the covdata package, create the actions to build it, and then depend on the output of that action from the the test action and use that as the covdata tool. For now, it's probably not worth the effort, but, in the future, if we wanted to build a tool like cgo as needed, it would be best to build it in the same action graph. That would introduce a whole bunch of complexity because we'd need to build the tool in the host configuration, and all the configuration parameters are global. For golang#71867 Change-Id: Id9bbbb5c169296f66c072949f9da552424ecfa2f Reviewed-on: https://go-review.googlesource.com/c/go/+/673119 LUCI-TryBot-Result: Go LUCI <[email protected]> Reviewed-by: Michael Pratt <[email protected]> Reviewed-by: Michael Matloob <[email protected]>
Previously, distpack filtered out tools from the packaged distribution using a list of tools to remove. Instead follow mpratt's suggestion on CL 666755 and instead filter out tools that are not on a list of tools to keep. This will make it easier to tell which tools are actually in the distribution. For golang#71867 Change-Id: I8336465703ac820028c3381a0a743c457997e78a Reviewed-on: https://go-review.googlesource.com/c/go/+/673696 LUCI-TryBot-Result: Go LUCI <[email protected]> Reviewed-by: Michael Pratt <[email protected]> Reviewed-by: Michael Matloob <[email protected]>
Change https://go.dev/cl/677557 mentions this issue: |
Change https://go.dev/cl/677558 mentions this issue: |
This was an oversight: the pack tool isn't actually used in builds. For #71867 Change-Id: Ib1f1cce0b574cf1d2c1002b2f2ab9ef9d750d0fb Reviewed-on: https://go-review.googlesource.com/c/go/+/677557 Reviewed-by: Michael Matloob <[email protected]> LUCI-TryBot-Result: Go LUCI <[email protected]> Reviewed-by: Michael Pratt <[email protected]> Reviewed-by: Dmitri Shuralyov <[email protected]>
Change https://go.dev/cl/677635 mentions this issue: |
Change https://go.dev/cl/677636 mentions this issue: |
For #71867 Change-Id: Ic4c6304b9a6b35c45bf35342523930924c68545a Reviewed-on: https://go-review.googlesource.com/c/go/+/677635 LUCI-TryBot-Result: Go LUCI <[email protected]> Reviewed-by: Michael Matloob <[email protected]> Reviewed-by: Dmitri Shuralyov <[email protected]> Reviewed-by: Dmitri Shuralyov <[email protected]>
Change https://go.dev/cl/677775 mentions this issue: |
Proposal Details
This proposal is to stop including tools that are not needed for builds in the binary distribution. Instead the tools would be built and run as needed by
go tool
using a similar mechanism to that used to build and run tools declared with atool
directive ingo.mod
. The goal of this proposal is to reduce the go binary distribution size.In cmd/distpack, we should be able to discard the following tools before packaging the binary distribution:
addr2line
,buildid
,nm
,objdump
,pprof
,test2json
, andtrace
. These tools do not seem to be invoked by the go command. We could go a little further and also removedoc
andfix
too because they are only invoked bygo doc
andgo fix
respectively.The install target for main packages in cmd will not change, so
go install
will continue to install binaries to$GOROOT/pkg/tool/$GOOS_$GOARCH
.If a tool binary is present in that location, it will be run from there, but if not, the go command will fall back to building (if the tool binary isn't cached) and running the tool, similarly to how
go tool
builds and runs tools declared with thetool
directive.Potential issues we should watch out for and which I would be interested in hearing feedback about:
go tool
to run it may be surprised that the tool that's run doesn't include their changes. So we may want to think a little more about what to do in that case.go test
andgo run
wouldn't work on either.cc @dmitshur
The text was updated successfully, but these errors were encountered: