Make Golang a first class language#2651
Conversation
d251ed6 to
9d155c4
Compare
tests/git_test.py
Outdated
|
|
||
|
|
||
| def test_get_remote_url(tmpdir): | ||
| git.init_repo(str(tmpdir), remote='dne') | ||
| assert git.get_remote_url(tmpdir) == 'dne' |
There was a problem hiding this comment.
if this function is dead it should just be removed -- originally this was building a GOPATH-like directory -- if that's not needed any more then there's a bunch of stuff that can be cleaned up I think ?
There was a problem hiding this comment.
Only get_remote_url is left I think. Done!
pre_commit/languages/golang.py
Outdated
| def _get_arch() -> str: # pragma: no cover | ||
| arch = platform.machine().lower() | ||
| if arch in ('x86_64',): | ||
| return 'amd64' | ||
| if arch in ('i386',): | ||
| return '386' | ||
| if arch in ('armv6l', 'armv7l'): | ||
| return 'armv6l' | ||
| if arch in ('arm64', 'aarch64', 'armv8'): | ||
| return 'arm64' | ||
| return arch |
pre_commit/languages/golang.py
Outdated
| if language_version == 'system': | ||
| return ( | ||
| ('PATH', (os.path.join(venv, 'bin'), os.pathsep, Var('PATH'))), | ||
| ) |
There was a problem hiding this comment.
this is potentially going to install stuff outside of pre-commit's management isn't it?
There was a problem hiding this comment.
No, since I've set a GOPATH later in the code for system which now I think can be moved to here I've moved here.
pre_commit/languages/golang.py
Outdated
| if helpers.exe_exists('go'): | ||
| return 'system' | ||
| else: | ||
| return '1.19.4' # C.DEFAULT |
There was a problem hiding this comment.
I was wondering if:
- I should return directly a default version
X.Y.Z? - or I should return
C.DEFAULTand translate it somewhere in the code to a default versionX.Y.Z?
There was a problem hiding this comment.
yeah this should just be 'default' -- we absolutely cannot have a specific version hardcoded because I do not want to deal with spammy PRs bumping it all the time
There was a problem hiding this comment.
yeah this should just be
'default'-- we absolutely cannot have a specific version hardcoded because I do not want to deal with spammy PRs bumping it all the time
and translate it somewhere in the code to a default version X.Y.Z? (this won't shoo spammy PRs, and I don't think Go provides an alias for stable or latest version)
There was a problem hiding this comment.
yeah I can't accept that -- you'll have to figure some way to determine the latest available version
There was a problem hiding this comment.
Looks good now? -- The only way I could think of is to fetch the list of versions
pre_commit/languages/golang.py
Outdated
| shutil.unpack_archive( | ||
| tmp_file.name, | ||
| dest, | ||
| format='zip' if sys.platform == 'win32' else 'gztar', | ||
| ) |
There was a problem hiding this comment.
hmm I thought the whole point of unpack_archive was that it's automatic?
There was a problem hiding this comment.
That's what I thought at first, but when not provided, the format is only inferred using the filename extension.
There was a problem hiding this comment.
should be easy to preserve the original extension right?
There was a problem hiding this comment.
The only way I can think of is to add it as a suffix to the named temp file (basically move the whole ternary there)
There was a problem hiding this comment.
the url has the extension, no?
There was a problem hiding this comment.
I've suffixed the whole name, might be useful for someone someday inspecting the temp dir :D
d128ccd to
37edd59
Compare
|
@asottile Could you please approve me running workflows? Congrats on the migration btw 🎉 |
|
if we don't need to build special |
a1b4a06 to
efe2ca1
Compare
df6239b to
ecd44ab
Compare
| return ( | ||
| ('GOROOT', os.path.join(venv, '.go')), | ||
| ( | ||
| 'PATH', ( | ||
| os.path.join(venv, 'bin'), os.pathsep, | ||
| os.path.join(venv, '.go', 'bin'), os.pathsep, Var('PATH'), | ||
| ), | ||
| ), | ||
| ) |
There was a problem hiding this comment.
I'm not sure this is needed at all? the extra go installation is only needed during build right?
There was a problem hiding this comment.
All I can think of are some builtin commands that are useful to hookify (😆) and require access to a Go instance e.g. gofmt, go generate, go mod ... , go vet.
The question now is; do we want to give each an independent env? or expect/require the user to have a system installation? what if he uses a language version manager -- will it be cumbersome to activate?
If no to independent env, then we can yeet it.
There was a problem hiding this comment.
I've been thinking about this a bit -- and it probably benefits pre-commit to store languages separate from the environments maybe? that way they can all be shared instead of having N copies of go -- I'm not sure what it would look like yet (probably another entry in the store database?)
There was a problem hiding this comment.
I had the same thought when I started working on this PR. It will mainly benefit the end user, thus pre-commit, especially when dealing with e.g. large sized languages in this case Go. Speaking of which, Go is a good candidate in this case, as it offers a simple way of achieving what we're looking for. I'm not sure if other languages offer the same experience -- I think that we can leverage virtualenv and rbenv for python and ruby -- if it's too complex or not worth it, we can have both structures coexist, maybe.
As a start, I think a very simple solution would be to pass a computed path to a language dir, install the language there, and make available a helper function that returns the language binary, this won't require a new database entry, but again this is a very simple solution based on my little knowledge of the tool internals.
There was a problem hiding this comment.
yeah I think I'll design something separate from this -- I'm imaginging any "first class" language will support this (so ruby, js, rust, and now go) -- and any second class language (python) will continue doing nothing. I'll need to think of the right api for this though (and hopefully something that makes it ~easy for pre-commit.ci to use as well)
There was a problem hiding this comment.
Oh! python is a second class language, I forgot that, knowing that I've read it in the CONTRIBUTING docs
pre_commit/languages/golang.py
Outdated
| gopath = env_dir | ||
| env = dict(os.environ, GOPATH=gopath) | ||
| env.pop('GOBIN', None) | ||
| with in_env(prefix, version): |
There was a problem hiding this comment.
I had factored this out in your other patch -- this shouldn't be needed here I think
There was a problem hiding this comment.
Mainly because both hook install and run steps have shared env vars since I kept go accessible.
I've refactored it now after fixing #2677.
| module golang-hello-world | ||
|
|
||
| go 1.18 | ||
|
|
||
| require github.com/BurntSushi/toml v1.1.0 |
There was a problem hiding this comment.
you can probably take the existing local go test and set language_version -- that'd be simpler than a whole separate repo
There was a problem hiding this comment.
done, now we only have one repo!
tests/repository_test.py
Outdated
| def test_golang_hook(tempdir_factory, store): | ||
| @pytest.fixture | ||
| def _golang_lru_cache_clear(): | ||
| golang.get_default_version.cache_clear() | ||
| yield | ||
| golang.get_default_version.cache_clear() |
There was a problem hiding this comment.
I have added two tests, where I test default version resolving to system and to fetching latest version, so I needed to clear the get_default_version cache. Probably might be a stretch since we're already testing get_default_version, let me know if you think I should get rid of the tests.
There was a problem hiding this comment.
you can set the language version directly for these tests -- you can pass in both default and system
ca2ec9d to
dac33b0
Compare
pre_commit/languages/golang.py
Outdated
| ) | ||
|
|
||
|
|
||
| def _get_arch() -> str: # pragma: no cover |
There was a problem hiding this comment.
I don't think this needs no cover any more
pre_commit/languages/golang.py
Outdated
| if version != C.DEFAULT: | ||
| return version | ||
| resp = urllib.request.urlopen('https://go.dev/dl/?mode=json') | ||
| return json.loads(resp.read())[0]['version'].lstrip('go') |
There was a problem hiding this comment.
json.load(resp) is ~slightly more efficient -- lstrip isn't quite right here -- you probably want removeprefix but that's new in python 3.9
pre_commit/languages/golang.py
Outdated
| os_name = platform.system().lower() | ||
| ext = 'zip' if os_name == 'windows' else 'tar.gz' |
There was a problem hiding this comment.
the rest of the checks in the codebase use sys.platform -- just to be consistent let's do that here as well
pre_commit/languages/golang.py
Outdated
| try: | ||
| url = _get_url(version) | ||
| resp = urllib.request.urlopen(url) | ||
| except urllib.error.HTTPError as exec: # pragma: no cover |
There was a problem hiding this comment.
exec is a bit of a weird name here -- if it's short for exception idk where the second e comes from 😆 -- I would call it just e or exc personally
There was a problem hiding this comment.
Muscle memory typo probably 😅 -- done!
pre_commit/languages/golang.py
Outdated
| raise | ||
| else: | ||
| with tempfile.NamedTemporaryFile( | ||
| delete=False, |
There was a problem hiding this comment.
I don't think this is right? this leaves the archive around after
There was a problem hiding this comment.
On windows, we can't open the named temp file a second time without first closing it, hence the delete=False. I've delegated the cleanup to the OS.
There was a problem hiding this comment.
the store database has the same problem -- check out what's done there.
some people are ok delegating temp cleanup to the OS, I am not :P
There was a problem hiding this comment.
Today I learned that Windows doesn't automatically cleanup the temp dir. I've made a little context manager to perform the cleanup.
pre_commit/languages/golang.py
Outdated
| with envcontext(get_env_patch(envdir)): | ||
| def in_env( | ||
| prefix: Prefix, | ||
| language_version: str, |
There was a problem hiding this comment.
in a WIP branch I've standardized this function and with the name version: str (it'll also fit on one line) -- if you could do that here that'll save me a semantic conflict :D
pre_commit/languages/golang.py
Outdated
| gopath = cmd_output('cygpath', '-w', env_dir)[1].strip() | ||
| else: | ||
| gopath = env_dir | ||
| gopath = os.path.join(env_dir) |
There was a problem hiding this comment.
os.path.join isn't doing anything here
pre_commit/languages/golang.py
Outdated
| env['PATH'] = ( | ||
| os.path.join(env_dir, '.go', 'bin') + | ||
| os.pathsep + os.environ['PATH'] | ||
| ) |
There was a problem hiding this comment.
I would do env['PATH'] = os.pathsep.join((os.path.join(env_dir, '.go', 'bin'), os.environ['PATH']))
tests/languages/golang_test.py
Outdated
| resp = urllib.request.urlopen(golang._get_url(version)) | ||
| assert resp.code == 200 |
There was a problem hiding this comment.
this is going to be pretty slow -- not sure it improves the test -- maybe just assert that it looks like a version?
tests/repository_test.py
Outdated
| def test_golang_hook(tempdir_factory, store): | ||
| @pytest.fixture | ||
| def _golang_lru_cache_clear(): | ||
| golang.get_default_version.cache_clear() | ||
| yield | ||
| golang.get_default_version.cache_clear() |
There was a problem hiding this comment.
you can set the language version directly for these tests -- you can pass in both default and system
916cec3 to
523f2ae
Compare
pre_commit/languages/golang.py
Outdated
| @contextlib.contextmanager | ||
| def _named_temp_file( | ||
| delete: bool = True, **kwargs: Any, | ||
| ) -> Generator[tempfile._TemporaryFileWrapper[Any], None, None]: | ||
| """On Windows, we can't open a named temp file a second time, | ||
| while it's still open. | ||
| - https://github.com/python/cpython/issues/58451 | ||
| """ | ||
| f = tempfile.NamedTemporaryFile(**kwargs, delete=False) | ||
| try: | ||
| yield f | ||
| finally: # pragma: no cover | ||
| try: | ||
| if delete: | ||
| os.unlink(f.name) | ||
| except OSError: | ||
| pass |
There was a problem hiding this comment.
there's a different pattern in pre_commit/store.py -- also we shouldn't no cover here -- that should be reserved for things which aren't testable
There was a problem hiding this comment.
also I don't think this is necessary -- you can seek(0) on the original temporary file without having to close and reopen it
There was a problem hiding this comment.
I'll give it a look -- the no cover was necessary due to a coverage bug on the finally and if delete coveragepy/coveragepy#867
I'm not sure if seek(0) will do the trick -- shutil.unpack_archive only accepts the filename.
There was a problem hiding this comment.
It should be good now
pre_commit/languages/golang.py
Outdated
| gopath = env_dir | ||
| if sys.platform == 'cygwin': # pragma: no cover | ||
| gopath = cmd_output('cygpath', '-w', env_dir)[1].strip() | ||
| else: | ||
| gopath = env_dir |
tests/repository_test.py
Outdated
|
|
||
|
|
||
| def _test_hook_repo( | ||
| def _try_hook_repo( |
There was a problem hiding this comment.
this should just use the original function
tests/repository_test.py
Outdated
| def test_golang_default_to_version_hook(tempdir_factory, store): | ||
| with mock.patch.object(helpers, 'exe_exists') as exe_exists_mck: | ||
| with mock.patch.object(golang, '_infer_go_version') as _infer_go_mck: | ||
| exe_exists_mck.return_value = False | ||
| _infer_go_mck.return_value = '1.18.4' | ||
| _test_hook_repo( | ||
| tempdir_factory, store, 'golang_hooks_repo', | ||
| 'golang-hook', [], b'hello world from go1.18.4\n', | ||
| config_kwargs={ | ||
| 'hooks': [{ | ||
| 'id': 'golang-hook', | ||
| 'language_version': 'default', | ||
| }], | ||
| }, | ||
| ) | ||
| _infer_go_mck.assert_called_once_with(C.DEFAULT) |
There was a problem hiding this comment.
I don't think the mocks are useful here? can't this just make an assertion based on the real execution and output? this test is just a reimplementation of the implementation so it's not actually testing anything (same for the mock above as well)
There was a problem hiding this comment.
I think I'll remove the test as I'm not convinced by it -- I wanted to test that the program fetches a go version and run it, when no version is passed and no system binary is available -- having test_golang_versionned_hook and testing the helper functions is enough I think
97af889 to
d8841b5
Compare
d8841b5 to
9afd639
Compare
|
I got rid of unpack_archive -- it's way simpler without it |
This PR contains the following updates: | Package | Type | Update | Change | |---|---|---|---| | [pre-commit](https://github.com/pre-commit/pre-commit) | dev-dependencies | major | `^2.21.0` -> `^3.0.0` | --- ### Release Notes <details> <summary>pre-commit/pre-commit</summary> ### [`v3.2.2`](https://github.com/pre-commit/pre-commit/blob/HEAD/CHANGELOG.md#​322---2023-04-03) [Compare Source](pre-commit/pre-commit@v3.2.1...v3.2.2) \================== ##### Fixes - Fix support for swift >= 5.8. - [#​2836](pre-commit/pre-commit#2836) PR by [@​edelabar](https://github.com/edelabar). - [#​2835](pre-commit/pre-commit#2835) issue by [@​kgrobelny-intive](https://github.com/kgrobelny-intive). ### [`v3.2.1`](https://github.com/pre-commit/pre-commit/blob/HEAD/CHANGELOG.md#​321---2023-03-25) [Compare Source](pre-commit/pre-commit@v3.2.0...v3.2.1) \================== ##### Fixes - Fix `language_version` for `language: rust` without global `rustup`. - [#​2823](pre-commit/pre-commit#2823) issue by [@​daschuer](https://github.com/daschuer). - [#​2827](pre-commit/pre-commit#2827) PR by [@​asottile](https://github.com/asottile). ### [`v3.2.0`](https://github.com/pre-commit/pre-commit/blob/HEAD/CHANGELOG.md#​320---2023-03-17) [Compare Source](pre-commit/pre-commit@v3.1.1...v3.2.0) \================== ##### Features - Allow `pre-commit`, `pre-push`, and `pre-merge-commit` as `stages`. - [#​2732](pre-commit/pre-commit#2732) issue by [@​asottile](https://github.com/asottile). - [#​2808](pre-commit/pre-commit#2808) PR by [@​asottile](https://github.com/asottile). - Add `pre-rebase` hook support. - [#​2582](pre-commit/pre-commit#2582) issue by [@​BrutalSimplicity](https://github.com/BrutalSimplicity). - [#​2725](pre-commit/pre-commit#2725) PR by [@​mgaligniana](https://github.com/mgaligniana). ##### Fixes - Remove bulky cargo cache from `language: rust` installs. - [#​2820](pre-commit/pre-commit#2820) PR by [@​asottile](https://github.com/asottile). ### [`v3.1.1`](https://github.com/pre-commit/pre-commit/blob/HEAD/CHANGELOG.md#​311---2023-02-27) [Compare Source](pre-commit/pre-commit@v3.1.0...v3.1.1) \================== ##### Fixes - Fix `rust` with `language_version` and a non-writable host `RUSTUP_HOME`. - [pre-commit-ci/issues#​173](pre-commit-ci/issues#173) by [@​Swiftb0y](https://github.com/Swiftb0y). - [#​2788](pre-commit/pre-commit#2788) by [@​asottile](https://github.com/asottile). ### [`v3.1.0`](https://github.com/pre-commit/pre-commit/blob/HEAD/CHANGELOG.md#​310---2023-02-22) [Compare Source](pre-commit/pre-commit@v3.0.4...v3.1.0) \================== ##### Fixes - Fix `dotnet` for `.sln`-based hooks for dotnet>=7.0.200. - [#​2763](pre-commit/pre-commit#2763) PR by [@​m-rsha](https://github.com/m-rsha). - Prevent stashing when `diff` fails to execute. - [#​2774](pre-commit/pre-commit#2774) PR by [@​asottile](https://github.com/asottile). - [#​2773](pre-commit/pre-commit#2773) issue by [@​strubbly](https://github.com/strubbly). - Dependencies are no longer sorted in repository key. - [#​2776](pre-commit/pre-commit#2776) PR by [@​asottile](https://github.com/asottile). ##### Updating - Deprecate `language: python_venv`. Use `language: python` instead. - [#​2746](pre-commit/pre-commit#2746) PR by [@​asottile](https://github.com/asottile). - [#​2734](pre-commit/pre-commit#2734) issue by [@​asottile](https://github.com/asottile). ### [`v3.0.4`](https://github.com/pre-commit/pre-commit/blob/HEAD/CHANGELOG.md#​304---2023-02-03) [Compare Source](pre-commit/pre-commit@v3.0.3...v3.0.4) \================== ##### Fixes - Fix hook diff detection for files affected by `--textconv`. - [#​2743](pre-commit/pre-commit#2743) PR by [@​adamchainz](https://github.com/adamchainz). - [#​2743](pre-commit/pre-commit#2743) issue by [@​adamchainz](https://github.com/adamchainz). ### [`v3.0.3`](https://github.com/pre-commit/pre-commit/blob/HEAD/CHANGELOG.md#​303---2023-02-01) [Compare Source](pre-commit/pre-commit@v3.0.2...v3.0.3) \================== ##### Fixes - Revert "Prevent local `Gemfile` from interfering with hook execution.". - [#​2739](pre-commit/pre-commit#2739) issue by [@​Roguelazer](https://github.com/Roguelazer). - [#​2740](pre-commit/pre-commit#2740) PR by [@​asottile](https://github.com/asottile). ### [`v3.0.2`](https://github.com/pre-commit/pre-commit/blob/HEAD/CHANGELOG.md#​302---2023-01-29) [Compare Source](pre-commit/pre-commit@v3.0.1...v3.0.2) \================== ##### Fixes - Prevent local `Gemfile` from interfering with hook execution. - [#​2727](pre-commit/pre-commit#2727) PR by [@​asottile](https://github.com/asottile). - Fix `language: r`, `repo: local` hooks - [pre-commit-ci/issues#​107](pre-commit-ci/issues#107) by [@​lorenzwalthert](https://github.com/lorenzwalthert). - [#​2728](pre-commit/pre-commit#2728) PR by [@​asottile](https://github.com/asottile). ### [`v3.0.1`](https://github.com/pre-commit/pre-commit/blob/HEAD/CHANGELOG.md#​301---2023-01-26) [Compare Source](pre-commit/pre-commit@v3.0.0...v3.0.1) \================== ##### Fixes - Ensure coursier hooks are available offline after install. - [#​2723](pre-commit/pre-commit#2723) PR by [@​asottile](https://github.com/asottile). ### [`v3.0.0`](https://github.com/pre-commit/pre-commit/blob/HEAD/CHANGELOG.md#​300---2023-01-23) [Compare Source](pre-commit/pre-commit@v2.21.0...v3.0.0) \================== ##### Features - Make `language: golang` bootstrap `go` if not present. - [#​2651](pre-commit/pre-commit#2651) PR by [@​taoufik07](https://github.com/taoufik07). - [#​2649](pre-commit/pre-commit#2649) issue by [@​taoufik07](https://github.com/taoufik07). - `language: coursier` now supports `additional_dependencies` and `repo: local` - [#​2702](pre-commit/pre-commit#2702) PR by [@​asottile](https://github.com/asottile). - Upgrade `ruby-build` to `20221225`. - [#​2718](pre-commit/pre-commit#2718) PR by [@​jalessio](https://github.com/jalessio). ##### Fixes - Improve error message for invalid yaml for `pre-commit autoupdate`. - [#​2686](pre-commit/pre-commit#2686) PR by [@​asottile](https://github.com/asottile). - [#​2685](pre-commit/pre-commit#2685) issue by [@​CarstenGrohmann](https://github.com/CarstenGrohmann). - `repo: local` no longer provisions an empty `git` repo. - [#​2699](pre-commit/pre-commit#2699) PR by [@​asottile](https://github.com/asottile). ##### Updating - Drop support for python<3.8 - [#​2655](pre-commit/pre-commit#2655) PR by [@​asottile](https://github.com/asottile). - Drop support for top-level list, use `pre-commit migrate-config` to update. - [#​2656](pre-commit/pre-commit#2656) PR by [@​asottile](https://github.com/asottile). - Drop support for `sha` to specify revision, use `pre-commit migrate-config` to update. - [#​2657](pre-commit/pre-commit#2657) PR by [@​asottile](https://github.com/asottile). - Remove `pre-commit-validate-config` and `pre-commit-validate-manifest`, use `pre-commit validate-config` and `pre-commit validate-manifest` instead. - [#​2658](pre-commit/pre-commit#2658) PR by [@​asottile](https://github.com/asottile). - `language: golang` hooks must use `go.mod` to specify dependencies - [#​2672](pre-commit/pre-commit#2672) PR by [@​taoufik07](https://github.com/taoufik07). </details> --- ### Configuration 📅 **Schedule**: Branch creation - At any time (no schedule defined), Automerge - At any time (no schedule defined). 🚦 **Automerge**: Disabled by config. Please merge this manually once you are satisfied. ♻ **Rebasing**: Whenever PR becomes conflicted, or you tick the rebase/retry checkbox. 🔕 **Ignore**: Close this PR and you won't be reminded about this update again. --- - [ ] <!-- rebase-check -->If you want to rebase/retry this PR, check this box --- This PR has been generated by [Renovate Bot](https://github.com/renovatebot/renovate). <!--renovate-debug:eyJjcmVhdGVkSW5WZXIiOiIzNS42MS4wIiwidXBkYXRlZEluVmVyIjoiMzUuNjEuMCJ9--> Co-authored-by: Renovate Bot <[email protected]> Reviewed-on: https://git.goatpr0n.de/public/doxy/pulls/2 Co-authored-by: renovate <[email protected]> Co-committed-by: renovate <[email protected]>

Following #2672
Closes #2649