Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

chore: pin Node.JS pre-commit hooks to use LTS #13785

Merged
merged 1 commit into from
Oct 20, 2024

Conversation

acolombier
Copy link
Member

nodeenv (used by pre-commit to install Node env for pre-commit hooks using JavaScript) default to install the latest Node.JS runtime.
v23.0.0 was just released and it contains a bug preventing npm pack. This works around the issue by pinning the LTS version instead.

@JoergAtGithub
Copy link
Member

LGTM! Thank you for fixing this!

@JoergAtGithub JoergAtGithub merged commit fe9608f into mixxxdj:main Oct 20, 2024
13 checks passed
@daschuer
Copy link
Member

daschuer commented Nov 4, 2024

This breaks pre-commit on main for me:

[INFO] Installing environment for https://github.com/DavidAnson/markdownlint-cli2.
[INFO] Once installed this environment will be reused.
[INFO] This may take a few minutes...
An unexpected error has occurred: CalledProcessError: command: ('/usr/bin/python3', '-mnodeenv', '--prebuilt', '--clean-src', '/home/daniel/.cache/pre-commit/repoqogur6rg/node_env-lts', '-n', 'lts')
return code: 1
stdout: (none)
stderr:
     * Install prebuilt node (lts) .
    Traceback (most recent call last):
      File "/usr/lib/python3.8/runpy.py", line 194, in _run_module_as_main
        return _run_code(code, main_globals, None,
      File "/usr/lib/python3.8/runpy.py", line 87, in _run_code
        exec(code, run_globals)
      File "/home/daniel/.local/lib/python3.8/site-packages/nodeenv.py", line 1471, in <module>
        main()
      File "/home/daniel/.local/lib/python3.8/site-packages/nodeenv.py", line 1065, in main
        create_environment(env_dir, opt)
      File "/home/daniel/.local/lib/python3.8/site-packages/nodeenv.py", line 951, in create_environment
        install_node(env_dir, src_dir, opt)
      File "/home/daniel/.local/lib/python3.8/site-packages/nodeenv.py", line 716, in install_node
        install_node_wrapped(env_dir, src_dir, opt)
      File "/home/daniel/.local/lib/python3.8/site-packages/nodeenv.py", line 738, in install_node_wrapped
        download_node_src(node_url, src_dir, opt)
      File "/home/daniel/.local/lib/python3.8/site-packages/nodeenv.py", line 565, in download_node_src
        dl_contents = io.BytesIO(urlopen(node_url).read())
      File "/home/daniel/.local/lib/python3.8/site-packages/nodeenv.py", line 601, in urlopen
        return urllib2.urlopen(req)
      File "/usr/lib/python3.8/urllib/request.py", line 222, in urlopen
        return opener.open(url, data, timeout)
      File "/usr/lib/python3.8/urllib/request.py", line 531, in open
        response = meth(req, response)
      File "/usr/lib/python3.8/urllib/request.py", line 640, in http_response
        response = self.parent.error(
      File "/usr/lib/python3.8/urllib/request.py", line 569, in error
        return self._call_chain(*args)
      File "/usr/lib/python3.8/urllib/request.py", line 502, in _call_chain
        result = func(*args)
      File "/usr/lib/python3.8/urllib/request.py", line 649, in http_error_default
        raise HTTPError(req.full_url, code, msg, hdrs, fp)
    urllib.error.HTTPError: HTTP Error 404: Not Found
    
Check the log at /home/daniel/.cache/pre-commit/pre-commit.log

@ronso0
Copy link
Member

ronso0 commented Nov 5, 2024

oh, I'm also using python 3.8, so this change may be the reason for https://mixxx.zulipchat.com/#narrow/channel/247620-development-help/topic/pre-commit.20404.20for.20markdownlint-cli2
??

@daschuer
Copy link
Member

daschuer commented Nov 5, 2024

@acolombier What would be the downside to revert this PR? Is there a workaround?

@acolombier
Copy link
Member Author

If Node.JS has fixed the bug, it should be fine to revert. To test, delete pre-commit hooks and cache. Note that the CI should already do so.

@ronso0
Copy link
Member

ronso0 commented Nov 5, 2024

Looks like this has been fixed in Node.js 23.1.0
I didn't test, yet.

edit
the ix is nodejs/node#55414 and it's in the 23.1.0 changelog

@ronso0
Copy link
Member

ronso0 commented Nov 5, 2024

No issues with 23.1.0 here (using python 3.8).
#13839

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants