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.sh: Add more xz platform support checks #2156

Open
wants to merge 3 commits into
base: master
from

Conversation

@DeeDeeG
Copy link

DeeDeeG commented Jan 29, 2020

macOS only supports extracting xz tarballs with tar in 10.9 and up.

GNU tar needs an xz executable on the PATH to extract xz tarballs.

Fixes #2155.

@DeeDeeG

This comment was marked as outdated.

Copy link
Author

DeeDeeG commented Jan 29, 2020

Given my changes, there is a test that I would expect to fail on any system that doesn't have GNU tar:

# set up for a failure by having a minimal toolset available
# but remove xz
ln -s /usr/bin/which $TEST_PATH/which
ln -s /usr/bin/command $TEST_PATH/command
ln -s /usr/bin/awk $TEST_PATH/awk
ln -s $(which rm) $TEST_PATH/rm
export PATH=$TEST_PATH
rm $TEST_PATH/xz
$(nvm_supports_xz "v2.3.2") && \
die "expected 'nvm_supports_xz v2.3.2' with a missing xz binary to exit with 1"

Might make sense to wrap this test in an if [system tar is GNU tar] then; check?

(This test would fail, because I adjusted the check not to worry about xz executable being on the PATH unless the user has GNU tar.)

(I did this because I have no idea if any tar besides GNU tar requires an external xz program on the PATH, and most users are on macOS, Linux, or a GNU user space of some kind on Windows. To be more conservative, I could assume that everyone other than macOS hard requires an xz executable on the path, so that we downgrade to gzip for systems where I personally have no idea how tar works (like on AIX, and on SmartOS (aka SunOS in terms of node tarball filenames).

(Also, it's unclear to me if nvm works in CMD/powershell on Windows, and if so, how tar behaves there or whether it is GNU tar or some other ind of tar.)

@DeeDeeG

This comment was marked as resolved.

Copy link
Author

DeeDeeG commented Jan 29, 2020

I think that same test fails now, despite running on exclusively Linux in Travis CI (presumably the tar in this environment would be GNU tar), because the "minimal tools" available on the PATH for that test don't include any tar at all.

So my platform check if [ -n $(tar --version | grep -o 'GNU') ] isn't satisfied, because the output of trying to run tar (tar: not found) doesn't contain the string "GNU"... and therefore my updated platform checks don't bother to check for an xz executable on the PATH.

I'm starting to see why it may be better to treat xz as the "special" case that is harder to activate, and I'm leaning toward making sure the code falls back to gzip if we aren't on macOS or have GNU tar.

@DeeDeeG

This comment was marked as resolved.

Copy link
Author

DeeDeeG commented Jan 29, 2020

Here's what a more conservative approach could look like (tending more toward using gzip when unsure, rather than using xz):

https://github.com/DeeDeeG/nvm/blob/869ce8d840c58a2518cc91ed13c7fa8598b9e4bf/nvm.sh#L3613-L3622

I can push that to this PR's branch if you like. I believe if would make the tests pass again (other than shellcheck, apparently.)

macOS only supports extracting xz tarballs with `tar` in 10.9 and up.

GNU tar needs an `xz` executable on the `PATH` to extract xz tarballs.

(These are the most common variants of tar, so until further testing
is done, conservatively assume all variants of tar (other than the one
shipped with macOS) need an xz executable on the PATH in order to
decompress xz tarballs.)
@DeeDeeG DeeDeeG force-pushed the DeeDeeG:adjust-xz-platform-checks branch from 5d77836 to bfd4be1 Jan 30, 2020
@DeeDeeG

This comment has been minimized.

Copy link
Author

DeeDeeG commented Jan 30, 2020

I force pushed to update this PR as follows:

  • Correctly get the macOS version with sw_vers -productVersion, rather than plain sw_vers with no flag.
  • Use nvm_version_greater() to check for a minimum macOS version.
  • Do the more conservative check for non-macOS systems; Assume an xz executable is required on the PATH everywhere other than macOS, not just where we can detect tar is GNU tar.
    • This is because I haven't tested on BSD or Windows, etc. (Tested and accurate for macOS and Linux.)
    • Incidentally, should make the test I mentioned above pass.
    • Falling back to gzip if unsure should be a safe bet, as gzip is more-widely supported than xz.
nvm.sh Outdated Show resolved Hide resolved

# macOS 10.9.0 and later support extracting xz with tar
if [ "$(nvm_get_os)" = "darwin" ]; then
MACOS_VERSION="$(sw_vers -productVersion)"

This comment has been minimized.

Copy link
@ljharb

ljharb Feb 2, 2020

Member

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 has been minimized.

Copy link
@DeeDeeG

DeeDeeG Feb 2, 2020

Author

should the previous line also check nvm_has sw_vers?

If you like, that's no problem. I have no strong opinion either way.

The sw_vers utility has been in macOS/OS X since at least as long as there has been support for xz. This page indicates it goes back to OS X 10.3 with support for the -productVersion flag. Unless the user has deleted it, it is available, as a rule, in OS X/macOS that is at all recent.

Also, if the sw_vers program fails to run, and we can't confirm macOS is 10.9 or newer, the nvm_supports_xz() function exits 1 so we fall back to gzip. I don't think it adversely affects the branching logic in that case... IMO it's subjectively a bit over-engineered but not really a bad thing to check for it if you like. Performance penalty would be near-zero and the readability would still be rather good. Edit: Actually, if there is no sw_vers, you get a command not found error, and MACOS_VERSION stays set to empty/null. And if you pass an empty second argument to nvm_version_greater(), nvm_version_greater() exits 0... So the macOS check would pass, as currently written, if there is no sw_vers on the PATH.

If there's a particularly bad consequence to attempting to run sw_vers when it isn't on the PATH, or I suppose just to match the convention in rest of the script, I'd be open to changing it as you suggest.

This comment has been minimized.

Copy link
@DeeDeeG

DeeDeeG Feb 2, 2020

Author

Actually, regarding if there is no sw_vers on the PATH: I'm not sure if we capture an error message about sw_vers not existing, and try to compare the string of the error message as a version against 10.9.0 via nvm_version_greater()? That could be weird, I guess.

Edit: If there is no sw_vers on PATH, MACOS_VERSION is set to null/empty. nvm_version_greater "10.9.0" "[null value here]" exits 0, so the macOS check passes without actually checking properly for xz support, if there is no sw_vers on the PATH.

(macOS not having sw_vers would also be weird, but desn't mean we can't plan for it I suppose.)

Not sure how nvm_version_greater handles letters in strings. I suppose I could test that real quick. Also not sure if the subshell $() would pass the error message if there is no sw_vers as a string to become the value of MACOS_VERSION. I can test that too. Edit: Tested, see above.

This comment has been minimized.

Copy link
@DeeDeeG

DeeDeeG Feb 2, 2020

Author

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

This comment has been minimized.

Copy link
@ljharb

ljharb Feb 2, 2020

Member

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

This comment was marked as outdated.

Copy link
@DeeDeeG

DeeDeeG Feb 15, 2020

Author

I tried the most perf-sensitive (i.e. very fast) test I could think of for macOS.

Note: results are across the board slower on this machine, it's not the script, it's the slower computer I have for running macOS.

Here's a comparison of the run time for the nvm_supports)xz() function (and supporting functions) from master, versus a version that also checks for "OS is Darwin," and if so, "/usr/lib/liblzma.dylib is a file" ([ -f /usr/lib/liblzma.dylib ]):

  • master: 22 to 26 milliseconds typically (one 28 millisecond outlier run)
    • with the "system has xz on PATH" code path active, because I put a dummy xz file at /usr/local/bin/xz with execute permissions
  • basic_macOS_check: 27 to 31 milliseconds typically (one 42 millisecond outlier run)
    • with the "system is macOS and has liblzma.dylib" code path active, because I'm testing on macOS with /usr/lib/liblzma.dylib present

Tried to make it apples to apples, in case all those version comparisons later in the script might slow it down. Didn't want to skip them; The target scenario is if the checks pass. If the tests fail, then nvm_supports_xz() should theoretically return 1 even quicker than it would return 0 after passing the checks throughout the whole function.


For fun, rather than running nvm_supports_xz 10 at the bottom of my mini-script, I tried running through all the version check logic, which happens for any version greater than 0.12.10 but less than 4.0.0. So I ran nvm_supports_xz 2.3.3 instead.

master: 52 to 54 milliseconds (outliers at 65 and 119 milliseconds)
basic_macOS_check: 55 to 61 milliseconds

This comment was marked as outdated.

Copy link
@DeeDeeG

DeeDeeG Feb 15, 2020

Author

code snippet used for basic_macOS_check speed tests above:

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

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

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

This comment was marked as outdated.

Copy link
@DeeDeeG

DeeDeeG Feb 15, 2020

Author

A gist of the actual scripts I ran, if you want to try to reproduce the results.

https://gist.github.com/DeeDeeG/2f32f818991cef953958446cd7aa70f1

(I ran time ./[script_name.sh] and took the "real" value, since waiting for disk or what-have-you is still relevant to the user experience. "Userland" CPU or kernel CPU time basically aren't relevant by themselves, in a vacuum, IMO.)

This comment has been minimized.

Copy link
@DeeDeeG

DeeDeeG Feb 15, 2020

Author

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).

This comment has been minimized.

Copy link
@DeeDeeG

DeeDeeG Feb 15, 2020

Author

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.)

DeeDeeG and others added 2 commits Feb 2, 2020
Co-Authored-By: Jordan Harband <ljharb@gmail.com>
For ksh

Co-Authored-By: Jordan Harband <ljharb@gmail.com>
DeeDeeG added a commit to DeeDeeG/nvm that referenced this pull request Feb 15, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

2 participants
You can’t perform that action at this time.