disable automatic toolchain switching for golang hooks - #3304
Conversation
Sorry, something went wrong.
|
Unrelated to this PR, but mypy fails with Module has no attribute "sched_getaffinity" likely because it's not available on MacOS: pre-commit/pre_commit/xargs.py Line 31 in de85900 Replacing try/except with hasattr would fix it, but I didn't want to change it because it's outside the scope of the current PR. |
Sorry, something went wrong.
| 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 ?
Sorry, something went wrong.
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.
Sorry, something went wrong.
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]
Sorry, something went wrong.
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.
Sorry, something went wrong.
| 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
Sorry, something went wrong.
| 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)
Sorry, something went wrong.
|
|
||
| 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
Sorry, something went wrong.
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.
Sorry, something went wrong.
| 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(...))
Sorry, something went wrong.
asottile
left a comment
There was a problem hiding this comment.
Sorry, something went wrong.
WojasKrk
left a comment
There was a problem hiding this comment.
X
Sorry, something went wrong.
Resolves #3300
I only disabled the toolchain switching if the version is not system. I did this because system is the default if go is installed globally and no language_version is 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.21 which 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.