Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork931
disable automatic toolchain switching for golang hooks#3304
disable automatic toolchain switching for golang hooks#3304asottile merged 1 commit intopre-commit:mainfrom
Conversation
AleksaC commentedSep 19, 2024
Unrelated to this PR, but mypy fails with pre-commit/pre_commit/xargs.py Line 31 inde85900
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
I think (?) this should be part ofget_env_patch ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
From what I can seeget_env_patch is only called fromin_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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
hmmm I guess? but what about for hooks that just callgofmt -- I know the rust toolchain gets grumbly when the versions mismatch -- does something like this survive?
-repo:localhooks: -id:gofmtname:gofmtlanguage:golangentry:gofmt -l -wtypes:[go]
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
gofmt doesn't seem to care about this option, butgo fmt performs the toolchain switch. Looking at the code it seems that anygo command will try to perform atoolchain 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
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 aSequence[str] so it just kinda lets it happen)
| return ( | ||
| ('GOROOT', os.path.join(venv, '.go')), | ||
| ('GOTOOLCHAIN', 'local'), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
does this break withlanguage_version: system? this seems different than the install code below
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
If thelanguage_version issystem 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
there's a helper for this I believe (I think it's calledwith chdir(...)? orwith cwd(...))
asottile left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
WojasKrk left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
X
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.