disable automatic toolchain switching for golang hooks#3304
disable automatic toolchain switching for golang hooks#3304asottile merged 1 commit intopre-commit:mainfrom
Conversation
|
Unrelated to this PR, but mypy fails with pre-commit/pre_commit/xargs.py Line 31 in de85900 Replacing try/except with |
| env = no_git_env(dict(os.environ, GOPATH=gopath)) | ||
| env.pop('GOBIN', None) | ||
| if version != 'system': | ||
| env['GOTOOLCHAIN'] = 'local' |
There was a problem hiding this comment.
I think (?) this should be part of get_env_patch ?
There was a problem hiding this comment.
From what I can see get_env_patch is only called from in_env which is used when running the hook and not during the installation. This should only be relevant during installation because that's when the build happens.
I ran the test to make sure it works this way and it breaks when I move it.
There was a problem hiding this comment.
hmmm I guess? but what about for hooks that just call gofmt -- I know the rust toolchain gets grumbly when the versions mismatch -- does something like this survive?
- repo: local
hooks:
- id: gofmt
name: gofmt
language: golang
entry: gofmt -l -w
types: [go]There was a problem hiding this comment.
gofmt doesn't seem to care about this option, but go fmt performs the toolchain switch. Looking at the code it seems that any go command will try to perform a toolchain switch.
I guess the variable should be set in both places then.
tests/languages/golang_test.py
Outdated
| exe='golang-version-test', | ||
| ) | ||
|
|
||
| assert 'go.mod requires go >= 1.23.1' in excinfo.value |
There was a problem hiding this comment.
this line is never run because it is indented
tests/languages/golang_test.py
Outdated
| language=golang, | ||
| version='1.22.0', | ||
| exe='go fmt', | ||
| file_args=(str(main_file)), |
There was a problem hiding this comment.
this is just a parenthesized string and should be a tuple I believe -- I'm kinda surprised the type checker was ok with this? (oh duh, str is a Sequence[str] so it just kinda lets it happen)
|
|
||
| return ( | ||
| ('GOROOT', os.path.join(venv, '.go')), | ||
| ('GOTOOLCHAIN', 'local'), |
There was a problem hiding this comment.
does this break with language_version: system? this seems different than the install code below
There was a problem hiding this comment.
If the language_version is system the function is going to return before reaching that line, so it's consistent with the install code.
tests/languages/golang_test.py
Outdated
| with monkeypatch.context() as m: | ||
| m.chdir(str(test_dir)) |
There was a problem hiding this comment.
there's a helper for this I believe (I think it's called with chdir(...)? or with cwd(...))
asottile
force-pushed
the
go-toolchain
branch
from
122528a to
109628c
Compare
3 participants
Resolves #3300
I only disabled the toolchain switching if the version is not
system. I did this becausesystemis the default if go is installed globally and nolanguage_versionis explicitly specified. In this case users likely doesn't care which version of go is used as long as their hooks are working, so I think it makes more sense to allow the toolchain switching since it doesn't break user expectations and other than a small performance hit on hook installation doesn't have negative impact.If we were to disable toolchain switching regardless of the version, many hooks would break since many environments lag with their go version. For example, in GitHub-hosted runners for GitHub Actions the pre-installed go version is
1.21which is two versions behind the current version, so it's likely that a lot of hooks work thanks to automatic toolchain switching without even knowing.