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

nvm_get_arch(): fix for arm64 kernel running armv7l userland #2469

Merged
merged 1 commit into from
Apr 15, 2021
Merged

nvm_get_arch(): fix for arm64 kernel running armv7l userland #2469

merged 1 commit into from
Apr 15, 2021

Conversation

Botspot
Copy link
Contributor

@Botspot Botspot commented Mar 22, 2021

Several people in the Raspberry Pi community have had issues with nvm. We realized that everyone affected had one thing in common: they were using armhf (armv7l) Raspberry Pi OS, but had enabled the arm64 kernel.

nvm mistakenly detected the OS to be 64-bit arm, while in reality it was armhf. This pull request solves the problem.

…rect arch

Co-authored-by: Botspot <[email protected]>
Co-authored-by: Jordan Harband <[email protected]>
Co-authored-by: Sladyn Nunes <[email protected]>
Copy link
Member

@ljharb ljharb left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks!

Can we add something to the unit tests for nvm_get_arch to cover this?

nvm.sh Outdated Show resolved Hide resolved
@ljharb ljharb added bugs Oh no, something's broken :-( OS: Raspberry Pi labels Mar 22, 2021
@Botspot
Copy link
Contributor Author

Botspot commented Mar 22, 2021

I don't know if file is POSIX.

In this case, it's only being used to detect if /sbin/init is a 32-bit executable or a 64-bit executable.
While this method has worked for every arm64 OS I've ever tried, it seems like there could be a more direct way to detect that file's bitness.

@ljharb
Copy link
Member

ljharb commented Mar 22, 2021

ah, the other issue is that readlink isn't posix (and readlink -f doesn't work on a Mac at all).

@Botspot
Copy link
Contributor Author

Botspot commented Mar 22, 2021

ah, the other issue is that readlink isn't posix (and readlink -f doesn't work on a Mac at all).

I found an alternative method that may be workable.
Do you know if od is POSIX compliant?

@ljharb
Copy link
Member

ljharb commented Mar 22, 2021

I don't, but it appears to be on my local copies of ksh and dash, which is a good sign.

@Botspot
Copy link
Contributor Author

Botspot commented Mar 22, 2021

I don't, but it appears to be on my local copies of ksh and dash, which is a good sign.

Okay, then this could work:

  # If running a 64bit ARM kernel but a 32bit ARM userland, change NVM_ARCH to 32bit ARM (armv7l)
  if [ "${NVM_ARCH}" = arm64 ] && [ "$(od -An -t x1 -j 4 -N 1 "$(readlink /sbin/init)")" = ' 01' ]; then
    NVM_ARCH=armv7l
  fi

@Itai-Nelken
Copy link

Itai-Nelken commented Mar 22, 2021

/sbin/init doesn't exists on a mac, so something like this is better now that there are ARM macs:

# If Linux and running a 64bit ARM kernel but a 32bit ARM userland, change NVM_ARCH to 32bit ARM (armv7l)
if [[ $(uname) == "Linux" ]]; then
    if [ "${NVM_ARCH}" = arm64 ] && [ "$(od -An -t x1 -j 4 -N 1 "$(readlink /sbin/init)")" = ' 01' ]; then
      NVM_ARCH=armv7l
    fi
fi

@ljharb
Copy link
Member

ljharb commented Mar 23, 2021

If the only systems where uname is Linux and NVM_ARCH is arm64 do in fact have both od and readlink -f, then that would work fine. I'm not sure how to guarantee that.

@Botspot
Copy link
Contributor Author

Botspot commented Mar 23, 2021

If the only systems where uname is Linux and NVM_ARCH is arm64 do in fact have both od and readlink -f, then that would work fine. I'm not sure how to guarantee that.

All systems I've ever encountered will work fine.
Maybe it would be acceptable to implement a fallback, where if od doesn't exist, then skip the if statement altogether?
BTW it appears the readlink -f can be replaced with readlink. At least, this works on my Raspberry Pi.

@sladyn98
Copy link
Contributor

@Botspot I was reading a thread https://groups.google.com/g/comp.unix.shell/c/3s8cuwcVnTk where you could use an alterantive to readlink that would be POSIX, so we might be able to gurantee that part. I guess the alternative is to format the output of ls, you can take a look at it, and see if its feasible, in any case the implementation of the fallback looks good.

@Botspot
Copy link
Contributor Author

Botspot commented Mar 24, 2021

Currently it's using the file command. Would it be preferable to use od to determine if the executable's bitness?

@sladyn98
Copy link
Contributor

I guess od is posix so that should be fine 👍
@ljharb let me know your thoughts and if this is ready to land or might require some extra effort ?

@Botspot
Copy link
Contributor Author

Botspot commented Mar 25, 2021

I guess od is posix so that should be fine 👍

Then it ought to be ready to merge.

@ljharb
Copy link
Member

ljharb commented Mar 26, 2021

It still needs a test.

@Botspot
Copy link
Contributor Author

Botspot commented Mar 29, 2021

It appears you "assigned" this to me. What do I do?

@ljharb
Copy link
Member

ljharb commented Mar 29, 2021

@Botspot yay, it worked :-p for this PR to land, we'd need a test for it. In this case, modifying

run_test x86 smartos x86
run_test x86 smartos x86 no_pkg_info
run_test amd64 smartos x64
run_test amd64 smartos x64 no_pkg_info
run_test x86 osx x86
run_test amd64 osx x64
to cover these architectures should do it.

Copy link
Member

@ljharb ljharb left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks to @sladyn98 for the test :-)

@ljharb ljharb mentioned this pull request Apr 6, 2021
@ljharb
Copy link
Member

ljharb commented Apr 6, 2021

(i'll land this once tests are fixed on master)

@ljharb ljharb merged commit 779a34e into nvm-sh:master Apr 15, 2021
@Botspot Botspot deleted the patch-1 branch April 15, 2021 22:53
@Botspot
Copy link
Contributor Author

Botspot commented Jul 10, 2021

I just tried to install Node on my Raspberry Pi again and noticed that this fix failed to work. Please re-open this PR.

I'm using an armhf OS, but a 64-bit kernel.
The problem can be traced to the od command we used: od -An -t x1 -j 4 -N 1 "${L#*-> }"
The command returns: od: '': No such file or directory. This, in practice, serves to invalidate the entire if statement and make the code inside to not run under any circumstance.
If the od command is changed to this: od -An -t x1 -j 4 -N 1 "$(readlink /sbin/init)", it works correctly and returns the expected exit code of zero, and the expected output of " 01".

Who changed it to "${L#*-> }"? I don't understand it, and I don't think I was the one who changed it to that.

@ljharb
Copy link
Member

ljharb commented Jul 11, 2021

@Botspot thats because this PR hasn’t been released yet.

@ljharb
Copy link
Member

ljharb commented Jul 11, 2021

If you can check with this fix, and it’s still not working, a PR with your suggestion would be appreciated.

@Botspot
Copy link
Contributor Author

Botspot commented Jul 11, 2021

If you can check with this fix, and it’s still not working, a PR with your suggestion would be appreciated.

Actually somehow it did work with this fix. Strange. I wonder why this works flawlessly inside a script, but not in a terminal!

@Botspot
Copy link
Contributor Author

Botspot commented Jul 11, 2021

@Botspot thats because this PR hasn’t been released yet.

How long will it be before there will be a new release with the past several months' changes?

@ljharb
Copy link
Member

ljharb commented Jul 11, 2021

No way to know for sure; i tend to do 1-2 releases a year.

ljharb added a commit to ljharb/nvm that referenced this pull request Aug 5, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bugs Oh no, something's broken :-( OS: Raspberry Pi
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants