Skip to content
This repository has been archived by the owner on Mar 28, 2021. It is now read-only.

Enable xz by default on macOS 11+ #12

Merged
merged 1 commit into from
Jun 26, 2020

Conversation

DeeDeeG
Copy link
Contributor

@DeeDeeG DeeDeeG commented Jun 24, 2020

Pull Request

Problem

See tj/n#623.

This PR is the same implementation done in tj/n#624.

macOS 11 has been announced. At the moment, the system check in can_use_xz() only cares about the OS minor version being greater than 8, so as it stands, n would think the upcoming macOS 11.0.0 doesn't support xz decompression via tar. Downloads would fall back to fetching gzip tarballs.

(I think it's reasonable to assume Apple will keep xz support in tar, as opposed to abruptly dropping it.)

Solution

In this PR, the check in can_use_xz() now allows any major macOS version 11+, in addition to allowing any macOS with minor version 8 or greater.

An aside about `sw_vers` and why this loose logic is good enough (Click to expand.)

We check the macOS version with the sw_vers utility. The only Mac operating system versions with the sw_vers utility are versions 10+. On old enough OS X/Mac OS, sw_vers command doesn't exist; As such, I'd expect to see this on stderr:

$ macos_version=$(sw_vers -productVersion)
sw_vers: command not found

As that output is on stderr and nothing is in stdout, $macos_version holds an empty string and fails the version check. In this scenario, I would expect n to fall back to using gzip.

(I simulated that scenario by being on Ubuntu (no sw_vers command here!) and setting macos_version="$(sw_vers -productVersion)" and running the version checks against this $macos_version, which indeed was an empty string and failed the version checks.)

My mental "truth table" of this check, considering all that, is as follows:

OS X/macOS major version 10 macOS major version 11+ Some older Mac OS version <= 9
Minor version 8 or less ❌ No xz (use gzip) ✔️ Use xz ❌ No xz (use gzip)
Minor version 9+ ✔️ Use xz ✔️ Use xz ❌ No xz (use gzip)

This all looks correct to me, and I'm satisfied the logic theoretically works as intended. Typos or bugs notwithstanding.

ChangeLog

Enable xz support by default for macOS 11+

macOS 11 has been announced, and is expected to continue to support xz
@@ -606,9 +606,14 @@ function can_use_xz() {
if [[ "${uname_s}" = "Linux" ]] && command -v xz &> /dev/null ; then
# tar on linux is likely to support xv if it is available separately
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Unrelated to my PR, but I noticed this typo ("xv") still exists here.

"xv" --> "xz"


tj/n has this comment now:

# tar on linux is likely to support xz if it is available as a command

Copy link
Owner

@shadowspawn shadowspawn left a comment

Choose a reason for hiding this comment

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

LGTM, thanks @DeeDeeG

@shadowspawn shadowspawn merged commit a75f15e into shadowspawn:develop Jun 26, 2020
@shadowspawn shadowspawn added the pending release Merged into a branch for a future release, but not released yet label Jun 26, 2020
@shadowspawn shadowspawn removed the pending release Merged into a branch for a future release, but not released yet label Jul 7, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants