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

nodeenv falls back to /usr/bin/nodejs instead of the node installed by actions/setup-node #905

Open
2 of 5 tasks
godlygeek opened this issue Nov 24, 2023 · 7 comments
Open
2 of 5 tasks
Labels
feature request New feature or request to improve the current logic

Comments

@godlygeek
Copy link

Description:
In a runs-on: ubuntu-latest GitHub hosted runner, I found that even after doing:

      - name: Set up Node
        uses: actions/setup-node@v4
        with:
          node-version: 16

that pre-commit run -a was failing to run a pre-commit hook configured as:

  - repo: https://github.com/pre-commit/mirrors-prettier
    rev: v3.1.0
    hooks:
      - id: prettier
        args: [--no-editorconfig]
        exclude: "^asv\\.conf\\.json$"
        exclude_types: [html]

with an error saying:

prettier requires at least version 14 of Node, please upgrade

Which was surprising, seeing as how we had just installed node version 16.

It turns out that pre-commit runs nodeenv, and nodeenv looks first for a binary named nodejs, and then falls back to looking for one named node:

https://github.com/ekalinin/nodeenv/blob/eaa9de97e561ab4f99458c94633e92547e72d5f1/nodeenv.py#L931-L938

And it seems like actions/setup-node is only installing one named node, and not one named nodejs, so nodeenv found /usr/bin/nodejs first and preferred that over /opt/hostedtoolcache/node/16.20.2/x64/bin/node.

Should the hostedtoolcache contain a nodejs as well? Or perhaps nodeenv should prefer node over nodejs?

Action version:
actions/setup-node@v4

Platform:

  • Ubuntu
  • macOS
  • Windows

Runner type:

  • Hosted
  • Self-hosted

Tools version:

Repro steps:
See https://github.com/bloomberg/memray/actions/runs/6984525297/job/19007471664 for a failed run where /usr/bin/nodejs was chosen instead of /opt/hostedtoolcache/node/16.20.2/x64/bin/node

@nikolai-laevskii
Copy link
Contributor

Than you for creating the issue, we will investigate the source of it and update you on the details!

@aparnajyothi-y
Copy link
Contributor

aparnajyothi-y commented Jan 4, 2024

Hello @godlygeek, Thank you for the issue once again.

We have investigated on the issue and found that the tool is just checking for the name 'node' from the current implementation of actions/setup-node, that is why nodeenv is falling back. Please find the screenshot for reference.
If you want we can implement the compatibility for both node and nodejs as part of feature request.
If you have any question, please feel free to share them.

image

@godlygeek
Copy link
Author

If you want we can implement the compatibility for both node and nodejs as part of feature request.

I'm not very familiar with Node, but it sounds like a good idea to me to include both node and nodejs, considering that nodeenv looks for both executables.

@aparnajyothi-y
Copy link
Contributor

Hello @godlygeek, Thank you for the feature request to consider both node and nodejs to be consider for both executables. This feature request will be considered in the next quarters based on the prioritization.
Hope you have updated nodeenv with node as per the current implementation.
If any clarifications needed, please feel free to share them.

@aparnajyothi-y
Copy link
Contributor

Hello @godlygeek, just a gentle ping.

@godlygeek
Copy link
Author

godlygeek commented Jan 10, 2024

Hello @godlygeek, just a gentle ping.

Sorry, I'm not sure what you're waiting on me for. I'm not sure what you meant by

Hope you have updated nodeenv with node as per the current implementation.

Can you clarify what you're suggesting here? For now, the workaround I've done is this. Note that I'm not directly installing nodeenv and can't update it - it's being installed behind the scenes by https://github.com/pre-commit/pre-commit

@aparnajyothi-y
Copy link
Contributor

Hello @godlygeek! I apologize for the confusion. I just wanted to check with you that the workaround to use node instead of nodejs.
Thank you for the confirmation that you have workaround.
Currently we are looking into the code to see what can be done to include both node and nodejs, considering that nodeenv looks for both executables.

@aparnajyothi-y aparnajyothi-y added feature request New feature or request to improve the current logic and removed bug Something isn't working labels Jan 22, 2024
@aparnajyothi-y aparnajyothi-y removed their assignment Jan 22, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature request New feature or request to improve the current logic
Projects
None yet
Development

No branches or pull requests

3 participants