Add support for Pants hosted Go toolchain #20485
Conversation
tgolsson
force-pushed
the
ts/go-toolchain
branch
2 times, most recently
from
eaf3355 to
7754a60
Compare
src/python/pants/backend/go/lint/golangci_lint/rules_integration_test.py
Outdated
Show resolved
Hide resolved
| @@ -819,7 +826,7 @@ async def cgo_compile_request( | |||
| env=cgo_env, | |||
| description=f"Generate Go and C files from CGo files ({request.import_path})", | |||
| input_digest=cgo_input_digest, | |||
| output_directories=(obj_dir_path,), | |||
| output_directories=(obj_dir_path, "!.goroot"), | |||
There was a problem hiding this comment.
Ugly but needed since we capture '.'
| export GOPATH="${{sandbox_root}}/gopath" | ||
| export GOCACHE="${{sandbox_root}}/cache" | ||
| export GOROOT="${{sandbox_root}}/{goroot.path}" |
There was a problem hiding this comment.
has to be absolute here, hence restructure. Could maybe do realpath...
| @@ -141,6 +141,13 @@ def strip_v2_chroot_path(v: bytes | str) -> str: | |||
| return re.sub(r"/.*/pants-sandbox-[a-zA-Z0-9]+/", "", v) | |||
|
|
|||
|
|
|||
| # NB: As above but doesn't convert to str. | |||
| def strip_v2_chroot_path_bytes(v: bytes) -> bytes: | |||
There was a problem hiding this comment.
ijson wants bytes, this satisfies that to avoid keeping sandbox-paths around in the outputs as that breaks later
tgolsson
force-pushed
the
ts/go-toolchain
branch
from
2aa2322 to
0564472
Compare
There was a problem hiding this comment.
I was hesitant about pants providing the go toolchain, but I'm more ok with that now. I'm especially OK with that if we make the go toolchain an ExportableTool (possibly as a symlinked_immutable export?).
I noticed one (inconsequential) inconsistency. Even with that, this looks good to me.
|
When you have a chance, please merge main (or rebase onto it) and add some release notes to docs/notes/2.22.x.md. See #20888 for more info. |
|
@huonw Added notes |
| @@ -66,6 +66,12 @@ The deprecation for the `platforms` field for the `pex_binary` and `pex_binaries | |||
|
|
|||
| Setting [the `orphan_files_behaviour = "ignore"` option](https://www.pantsbuild.org/2.22/reference/subsystems/yamllint#orphan_files_behavior) the `pants.backend.experimental.tools.yamllint` backend is now properly silent. It previously showed spurious warnings. | |||
|
|
|||
| #### Golang | |||
|
|
|||
| Pants will now automatically install a Golang toolchain when needed, removing the need for installing it externally. This can be configured by `[go-toolchain].known_versions`, `[go-toolchain].enabled`, and `[go-toolchain].version`. As of shipping, you can select a version from *1.18.10*, *1.19.13*, *1.20.13*, and *1.21.6*. Other versions can be configured by adding to the known versions. Note that versions after Go 1.21 are currently not official supported due to the coverage system redesign. PRs are welcome here. | |||
There was a problem hiding this comment.
Note that versions after Go 1.21 are currently not official supported due to the coverage system redesign.
I assume the prior coverage system went away in more recent Go versions?
|
|
||
| Pants will now automatically install a Golang toolchain when needed, removing the need for installing it externally. This can be configured by `[go-toolchain].known_versions`, `[go-toolchain].enabled`, and `[go-toolchain].version`. As of shipping, you can select a version from *1.18.10*, *1.19.13*, *1.20.13*, and *1.21.6*. Other versions can be configured by adding to the known versions. Note that versions after Go 1.21 are currently not official supported due to the coverage system redesign. PRs are welcome here. | ||
|
|
||
| The `[go-toolchain].enabled` flag is shipped and immediately deprecated, with the goal of only supporting a Pants-provided Go toolchain in the future. |
There was a problem hiding this comment.
Is this in fact what users want? What if a user still wants to provide their own Go toolchain? Maybe we should not deprecate until we actually decide and intend to remove support for user-provided Go toolchains?
There was a problem hiding this comment.
From a "upgrade/process/release manager" perspective (I know nothing about Go), just reiterating that it might be good to at least have a release or two for it to bake and generally try to make upgrades easier. If we deprecate immediately and there's some bugs:
- it'll be annoying for users to stick with the old version (they'll get warnings they can't action, constantly)
- those bugs may need to be treated as higher severity by us (Pants maintainers) and need "immediate" attention, because they'll potentially be breaking existing code or making it hard for new users to onboard. This might then mean we have to delay releases to wait for fixes... and it's nicer for everyone to not have any of that panic.
One approach might be to: add the option without an explicit default. If it's not set:
- emit a deprecation warning (i.e. tell people that it must be set and that the default will change), and
- fall back to whichever values matches the current behaviour (to ease the upgrade path by not changing behaviour immediately).
Future releases can then continue the phasing out, e.g. a fairly conservative potential path might be:
| release | option | default | old behaviour | new behaviour |
|---|---|---|---|---|
| 2.22 (this PR) | added, deprecation when not set | old | not deprecated, unchanged | added |
| 2.23 | default changed | new | unchanged | unchanged, default |
| 2.24 | no deprecation when not set (just get new behaviour silently) | new | unchanged | unchanged |
| 2.25 | deprecated when set at all | new | deprecated | unchanged |
| 2.26 | waiting... | new | waiting... | unchanged |
| 2.27 | removed | new | removed | unchanged |
It looks like Go backend is experimental, so we can almost certainly do a more aggressive timeline than that... but I think we at least want to have:
- one release where users can opt-in to either the old behaviour or the new behaviour, and doesn't get warnings once they do the opt-in
- one release where the old behaviour exists but has deprecation warnings
What do you think?
| @@ -74,6 +75,7 @@ async def gofmt_fmt( | |||
| output_files=request.files, | |||
| description=f"Run gofmt on {pluralize(len(request.files), 'file')}.", | |||
| level=LogLevel.DEBUG, | |||
| immutable_input_digests={".goroot": goroot.digest}, | |||
There was a problem hiding this comment.
I see .goroot in several places. Maybe it should be a constant?
| If true, Pants will provide a Go distribution instead of requiring one to be installed on the host system. | ||
| """ | ||
| ), | ||
| deprecation_start_version="2.22.0.dev2", |
There was a problem hiding this comment.
See other comment about when to start deprecating.
| @@ -579,6 +580,7 @@ async def resolve_go_stdlib_embed_config( | |||
| input_digest=input_digest, | |||
| description=f"Create embed mapping for {request.package.import_path}", | |||
| level=LogLevel.DEBUG, | |||
| immutable_input_digests={".goroot": goroot.digest}, | |||
There was a problem hiding this comment.
Maybe have a goroot.immutable_input_digests function which returns this dictionary?
| env_result = await Get( # noqa: PNT30: requires triage | ||
| ProcessResult, | ||
| Process( | ||
| (".goroot/go/bin/go", "env", "-json"), |
| ) -> GoRoot: | ||
| if toolchain_subsystem.enabled: | ||
| go_sdk = await Get(GoRoot, InstallGoToolchainRequest()) | ||
|
|
|
We've just branched for |
This adds a new feature where Pants provides a Go toolchain for commands, removing the need for the host system to have one. I've been on the fence about supporting the old method, but opted to keep it for now with a deprecation warning. This will let us iron out any issues that might arise from this without too much risk.
I've been trying to find any significant performance regressions, but there's so much noise in tests that it's hard to tell.
All tests ran on same machine, same command, etc. If anyone wants to do similar tests the command is
pants --process-execution-local-parallelism=8 test --force src/python/pants/backend/go/::I'd ignore most test files during review, then start with goroot.py and take rules after. Most of the changes are the same to include the new goroot input cache. I've not written any new tests -- the new codepath is exercised in every test since I've enabled this by default.
Opinions on default Go version are welcome.