GV

GeezerVison

The World Wide Web, restored to 800 pixels of glory.

Home|About|Gaming cabinet|Archives|How it works|Starter sites

On this site

  • Sign in
  • ActionsCI/CD and automation
  • CodespacesCloud development environments
  • All Features
  • Documentation
  • Training
  • Certifications
  • Build with Copilot
  • Customize Copilot
  • Integrations
  • Trending repos
  • GitHub Sponsors
  • Developer articles
  • Enterprises

This view

  • Original page
  • New address

GeezerVison is showing github.com — disable automatic toolchain switching for golang hooks by AleksaC · Pull Request #3304 · pre-commit/pre-commit

Conversation

@AleksaC

@AleksaC AleksaC commented Sep 18, 2024

Copy link
Copy Markdown
Contributor

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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@AleksaC

AleksaC commented Sep 19, 2024

Copy link
Copy Markdown
Contributor Author

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

return len(os.sched_getaffinity(0))

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.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
asottile
asottile reviewed Sep 28, 2024
View reviewed changes
Comment thread pre_commit/languages/golang.py
env = no_git_env(dict(os.environ, GOPATH=gopath))
env.pop('GOBIN', None)
if version != 'system':
env['GOTOOLCHAIN'] = 'local'

@asottile asottile Sep 28, 2024

Copy link
Copy Markdown
Member

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 of get_env_patch ?

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@AleksaC AleksaC Sep 28, 2024

Copy link
Copy Markdown
Contributor Author

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 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.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@asottile asottile Sep 30, 2024

Copy link
Copy Markdown
Member

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 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.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@AleksaC AleksaC Sep 30, 2024

Copy link
Copy Markdown
Contributor Author

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, 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.

Uh oh!

There was an error while loading. Please reload this page.
asottile reacted with thumbs up emoji
All reactions
  • 👍 1 reaction
asottile
asottile reviewed Oct 26, 2024
View reviewed changes
Comment thread tests/languages/golang_test.py Outdated
exe='golang-version-test',
)

assert 'go.mod requires go >= 1.23.1' in excinfo.value

@asottile asottile Oct 26, 2024

Copy link
Copy Markdown
Member

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
asottile
asottile reviewed Oct 26, 2024
View reviewed changes
Comment thread tests/languages/golang_test.py Outdated
language=golang,
version='1.22.0',
exe='go fmt',
file_args=(str(main_file)),

@asottile asottile Oct 26, 2024

Copy link
Copy Markdown
Member

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 a Sequence[str] so it just kinda lets it happen)

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
asottile
asottile reviewed Nov 9, 2024
View reviewed changes
Comment thread pre_commit/languages/golang.py

return (
('GOROOT', os.path.join(venv, '.go')),
('GOTOOLCHAIN', 'local'),

@asottile asottile Nov 9, 2024

Copy link
Copy Markdown
Member

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 with language_version: system? this seems different than the install code below

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@AleksaC AleksaC Nov 9, 2024

Copy link
Copy Markdown
Contributor Author

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 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.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
Comment thread tests/languages/golang_test.py Outdated
Comment on lines +225 to +226
with monkeypatch.context() as m:
m.chdir(str(test_dir))

@asottile asottile Nov 9, 2024

Copy link
Copy Markdown
Member

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 called with chdir(...)? or with cwd(...))

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@AleksaC @asottile
disable automatic toolchain switching for golang hooks
109628c
@asottile
asottile force-pushed the go-toolchain branch from 122528a to 109628c Compare November 25, 2024 23:47
asottile
asottile approved these changes Nov 25, 2024
View reviewed changes

@asottile asottile left a comment

Copy link
Copy Markdown
Member

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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@asottile
asottile enabled auto-merge November 25, 2024 23:48
@asottile
asottile merged commit cb14bc2 into pre-commit:main Nov 25, 2024
WojasKrk
WojasKrk suggested changes Jan 21, 2025
View reviewed changes

@WojasKrk WojasKrk left a comment

Copy link
Copy Markdown

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@pre-commit pre-commit locked as off-topic and limited conversation to collaborators Jan 21, 2025
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

@asottile asottile asottile approved these changes
+1 more reviewer
@WojasKrk WojasKrk WojasKrk requested changes
Reviewers whose approvals may not affect merge requirements

Uh oh!

There was an error while loading. Please reload this page.

Assignees

No one assigned

Labels

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Disable automatic toolchain switching in golang

Uh oh!

There was an error while loading. Please reload this page.

3 participants

@AleksaC @asottile @WojasKrk
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
You are visitor 01402503 — GeezerVison does not store the pages you look up.
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.