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.

Sign up for GitHub

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

Jump to bottom

nvm.sh: Add more xz platform support checks #2156

Merged
merged 1 commit into from
Jul 18, 2020
Merged

nvm.sh: Add more xz platform support checks #2156

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
26 changes: 23 additions & 3 deletions nvm.sh
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3687,10 +3687,32 @@ EOF
}

nvm_supports_xz() {
if [ -z "${1-}" ] || ! command which xz >/dev/null 2>&1; then
if [ -z "${1-}" ]; then
return 1
fi

local NVM_OS
NVM_OS="$(nvm_get_os)"
if [ "_${NVM_OS}" = '_darwin' ]; then
local MACOS_VERSION
MACOS_VERSION="$(sw_vers -productVersion)"
Copy link
Member

Choose a reason for hiding this comment

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

should the previous line also check nvm_has sw_vers?

Suggested change
MACOS_VERSION="$(sw_vers -productVersion)"
local MACOS_VERSION
MACOS_VERSION="$(sw_vers -productVersion)"

(the "local" line is needed for ksh)

This comment was marked as duplicate.

Sign in to view

This comment was marked as duplicate.

Sign in to view

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Semi-tangential point: it could be neat to test to some extent in a macOS Travis CI environment.

Copy link
Member

Choose a reason for hiding this comment

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

If it's not too slow, we could certainly add a MacOS build to the test matrix.

This comment was marked as outdated.

Sign in to view

This comment was marked as outdated.

Sign in to view

This comment was marked as outdated.

Sign in to view

Copy link
Contributor Author

@DeeDeeG DeeDeeG Feb 15, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

Sorry for all the comments at once... Realized my check can still false positive on older macOS where we don't have liblzma.dylib, IF the user also has xz on the PATH such as from homebrew. Updated so the xz on PATH check only happens if we are not on macOS.

Had to redo the performance numbers, so:

For nvm_supports_xz 2.3.3:

  • master: 51 to 52 milliseconds (one outlier run was 53 milliseconds)
  • basic_macOS_check_update: 52 to 54 milliseconds

For nvm_supports_xz 10:

  • master: 22 to 24 milliseconds (one outlier was 29 milliseconds)
  • basic_macOS_check_update: 24 to 25 milliseconds (one outlier was 26, another was 37 milliseconds)

For fun, master with no xz on the PATH, which results in nvm just falling back on gzip:

  • nvm_supports_xz 10: 16 to 17 milliseconds (one outlier was 20 milliseconds)
  • nvm_supports_xz 2.3.3: 16 to 19 milliseconds (should be the same, maybe just background programs producing performance noise).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

New snippet used for basic_macOS_check_update above (this time with no apparent false positive scenarios):

nvm_supports_xz() {
  if [ -z "${1-}" ]; then
    return 1
  fi

  if [ "$(command uname -s)" = 'Darwin' ]; then
    if ! [ -f "/usr/lib/liblzma.dylib" ]; then
      return 1
    fi
  elif ! command which xz >/dev/null 2>&1; then
    return 1
  fi

# [ . . . rest of nvm_supports_xz() as it appears in master . . . ]

(Also added to the previously linked gist, which again is here.)

if nvm_version_greater "10.9.0" "${MACOS_VERSION}"; then
# macOS 10.8 and earlier doesn't support extracting xz-compressed tarballs with tar
return 1
fi
elif [ "_${NVM_OS}" = '_freebsd' ]; then
if ! [ -e '/usr/lib/liblzma.so' ]; then
# FreeBSD without /usr/lib/liblzma.so doesn't support extracting xz-compressed tarballs with tar
return 1
fi
else
if ! command which xz >/dev/null 2>&1; then
# Most OSes without xz on the PATH don't support extracting xz-compressed tarballs with tar
# (Should correctly handle Linux, SmartOS, maybe more)
return 1
fi
fi

# all node versions v4.0.0 and later have xz
if nvm_is_merged_node_version "${1}"; then
return 0
Expand All @@ -3706,8 +3728,6 @@ nvm_supports_xz() {
return 0
fi

local NVM_OS
NVM_OS="$(nvm_get_os)"
case "${NVM_OS}" in
darwin)
# darwin only has xz for io.js v2.3.2 and later
Expand Down