setup-node icon indicating copy to clipboard operation
setup-node copied to clipboard

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

Open godlygeek opened this issue 2 years ago • 7 comments

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:

  • [X] Ubuntu
  • [ ] macOS
  • [ ] Windows

Runner type:

  • [X] 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

godlygeek avatar Nov 24 '23 21:11 godlygeek

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

nikolai-laevskii avatar Nov 27 '23 09:11 nikolai-laevskii

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

aparnajyothi-y avatar Jan 04 '24 13:01 aparnajyothi-y

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.

godlygeek avatar Jan 05 '24 04:01 godlygeek

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 avatar Jan 05 '24 10:01 aparnajyothi-y

Hello @godlygeek, just a gentle ping.

aparnajyothi-y avatar Jan 10 '24 13:01 aparnajyothi-y

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

godlygeek avatar Jan 10 '24 16:01 godlygeek

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 avatar Jan 11 '24 05:01 aparnajyothi-y