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

[Fix]: Fix NodeJS download link for armv8l #3102

Merged
merged 1 commit into from
May 6, 2023
Merged

Conversation

dewren99
Copy link
Contributor

@dewren99 dewren99 commented Apr 28, 2023

Fixes #3035. Fixes #2845.

I wasn't sure what would be the best solution. Right now, if NVM_ARCH is armv8l, it falls back to armv7l since ARMv8 is backward compatible with ARMv7.

I think other approaches are

  • Changing aarch64) NVM_ARCH="arm64" ;; pattern to aarch64 | armv8l) NVM_ARCH="arm64" ;;. That way it would step into the following code block and also fall back to armv7l.
  # If running a 64bit ARM kernel but a 32bit ARM userland,
  # change ARCH to 32bit ARM (armv7l) if /sbin/init is 32bit executable
  if [ "$(uname)" = "Linux" ] \
    && ( [ "${NVM_ARCH}" = arm64 ] || [ "${NVM_ARCH}" = armv8l ] ) \
    && [ "$(command od -An -t x1 -j 4 -N 1 "/sbin/init" 2>/dev/null)" = ' 01' ]\
  ; then
    NVM_ARCH=armv7l
    HOST_ARCH=armv7l
  fi
  • Instead of falling back to armv7l, create another if block to use arm64, something like
  if [ "$(uname)" = "Linux" ] \
    && [ "${NVM_ARCH}" = armv8l ] \
  ; then
    NVM_ARCH=arm64
    HOST_ARCH=arm64
  fi

Please let me know if you have any suggestions, feedback, or concerns!

@ljharb
Copy link
Member

ljharb commented Apr 29, 2023

I'd love a regression test - even a unit test that mocks out the arch - that can illustrate what this is fixing :-)

@dewren99
Copy link
Contributor Author

Thanks for the suggestion, that was helpful!
I added a mock for the armv8l arch. With the current change, it should behave as if the arch was armv7l.

@ljharb
Copy link
Member

ljharb commented May 6, 2023

The mock is good, but we'd also need a test in test/fast/Unit tests/nvm_get_arch that exercises it. I'll see if i can add one.

@ljharb ljharb merged commit 5410ae5 into nvm-sh:master May 6, 2023
84 checks passed
@Rehu199

This comment was marked as spam.

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

Successfully merging this pull request may close these issues.

Broken nodejs download link nvm doesn't load nodejs v16.15.1
3 participants