Improve OS version detection in nvm debug
#1746
Conversation
nvm.sh
Outdated
| @@ -2405,8 +2406,17 @@ nvm() { | |||
| nvm_err "uname -a: '$(uname -a | awk '{$2=""; print}' | xargs)'" | |||
| if [ "$(nvm_get_os)" = "darwin" ] && nvm_has sw_vers; then | |||
| nvm_err "OS version: $(sw_vers | command awk '{print $2}' | command xargs)" | |||
| elif [ -r "/etc/issue" ]; then | |||
There was a problem hiding this comment.
this seems like it could stay an elif, and the other logic added in an else?
There was a problem hiding this comment.
Nope, as we need to confirm the content of it first
nvm.sh
Outdated
| if [ -r "/etc/issue" ]; then | ||
| nvm_err "OS version: $(command head -n 1 /etc/issue | command sed 's/\\.//g')" | ||
| fi | ||
| if [ -r "/etc/os-release" ] && [ "${OS_VERSION}" = "" ] ; then |
nvm.sh
Outdated
| # shellcheck disable=SC1091 | ||
| OS_VERSION="$(. /etc/os-release && echo "${NAME}" "${VERSION}")" | ||
| fi | ||
| if [ "${OS_VERSION}" != "" ]; then |
nvm.sh
Outdated
| fi | ||
| if [ -r "/etc/os-release" ] && [ "${OS_VERSION}" = "" ] ; then | ||
| # shellcheck disable=SC1091 | ||
| OS_VERSION="$(. /etc/os-release && echo "${NAME}" "${VERSION}")" |
There was a problem hiding this comment.
it seems risky to be sourcing a random file; is this necessary?
There was a problem hiding this comment.
Is it random? I don't think so. It's a built-in of most of the modern Linux distros, and it has clearly definition: https://www.freedesktop.org/software/systemd/man/os-release.html
PeterDaveHello
force-pushed
the
improve-nvm-debug-os-version-info
branch
from
13e710e
to
9e334a9
Compare
nvm.sh
Outdated
| elif [ -r "/etc/issue" ]; then | ||
| nvm_err "OS version: $(command head -n 1 /etc/issue | command sed 's/\\.//g')" | ||
| else | ||
| if [ -r "/etc/issue" ]; then |
There was a problem hiding this comment.
what i mean is, instead of putting three "if"s in this "else", it could be an "elif" for /etc/issue, and then an else for the final two "if"s.
There was a problem hiding this comment.
what if /etc/issue doesn't exist?
There was a problem hiding this comment.
then wouldn't -r return false, and the condition be skipped?
There was a problem hiding this comment.
Sorry that I asked a wrong question.
I wanna say if "/etc/issue" exists, but the content may be empty in some cases, so I put it with the next detection in the same level code block, we can test both of the conditions, otherwise, the existing but empty /etc/issue will cause the next condition and code block not be running.
Will this make sense to you?
Thanks!
There was a problem hiding this comment.
Not sure if I do explain well, I need the conditions below this one also be tested, even if this condition is true.
nvm.sh
Outdated
| if [ -r "/etc/issue" ]; then | ||
| nvm_err "OS version: $(command head -n 1 /etc/issue | command sed 's/\\.//g')" | ||
| fi | ||
| if [ -r "/etc/os-release" ] && [ -z "${OS_VERSION}" ] ; then |
There was a problem hiding this comment.
When would OS_VERSION be set prior to this line?
There was a problem hiding this comment.
should be in the previous condition, will update it in the next push
nvm.sh
Outdated
| nvm_err "OS version: $(command head -n 1 /etc/issue | command sed 's/\\.//g')" | ||
| else | ||
| if [ -r "/etc/issue" ]; then | ||
| nvm_err "OS version: $(command head -n 1 /etc/issue | command sed 's/\\.//g')" |
There was a problem hiding this comment.
is perhaps this line meant to set OS_VERSION instead of logging it with nvm_err?
There was a problem hiding this comment.
yes, that's what I want to do
PeterDaveHello
force-pushed
the
improve-nvm-debug-os-version-info
branch
from
9e334a9
to
28f8870
Compare
PeterDaveHello
force-pushed
the
improve-nvm-debug-os-version-info
branch
from
28f8870
to
d3206f6
Compare
ljharb
force-pushed
the
improve-nvm-debug-os-version-info
branch
from
d3206f6
to
ca66a13
Compare
nvm debugresult in #1744 didn't detect the os version properly, this PR could solve it.