support fetching xz tarballs #10

DeeDeeG · 2019-09-21T23:16:47Z

Closes #8

This is the xz feature. As I see it...

Technical goals include:

Basic ability to download xz tarballs
Ability to fall back to gzip if necessary

Preferences already identified:

xz should be able to be on by default, without this change causing problems for existing end-users

Nice to have:

Reasonable way to configure this by environment variable
Reasonable way to configure this by command-line flags

Totally optional:

Backward compatibility for environment variable with the prior implementation at tj/n (Feature: xz compression option tj/n#571)
- This is totally optional, since implementation can be tweaked upon porting to n, or the old n environment variable could be tweaked/deprecated, etc.

I believe this PR as initially posted meets those goals, but could perhaps be refined. (Although I note it is quite possible to bikeshed the minor things, so would prefer to settle on something soonish, rather than extend discussion too indefinitely...)

meta: commits are very spaced out/tiny, to aid in some rebasing and tweaks I have been doing. Can consolidate a bit if/as desired.

Set "NVH_USE_XZ" to "true" to use xz compression.

--use-xz for xz, --no-use-xz for gzip. Overrides environment variable.

Node.js has had xz support on Linux and macOS since 4.0.0 (io.js had xz starting from 1.0+ for Linux, from 2.3.2+ for macOS. Node.js picks up where io.js left off, so 4.0+ has xz for both Linux and macOS as well.) There are some versions of Node.js in the 0.10 and 0.12 series that also have xz tarballs, but these aren't downloaded often, and checking just the major version is simpler than checking major, minor and patch versions.

shadowspawn · 2019-09-22T00:17:23Z

Nice work after all the hard research. I could run with this and make a few changes myself. I'll let you know what I am thinking. Some code comments coming about smaller details.

I don't like the complication of g_xz_force_enabled. I feel this is adding support for a use-case I do not see being likely?

However, I suggest you do not change the code for that because I have some opinions about the normalising and can_use_xz call, and I'll make a branch and try some code to see if my ideas turn into reasonable structure.

DeeDeeG · 2019-09-22T00:17:58Z

Tiny updates in force push:

Re-order a commit for more readable commit history (better grouping)
remove &> /dev/null from is_ok.
- This was from a branch where I had two is_oks, and the first curl 404 not found error message seemed out of place when it showed up every time nvh safely fell back to gzip.
Increase minimum node version for xz from 3 to 4, given that io.js is not supported.
Reverted incorrect use of double-quotes rather than single-quotes for "$resolved_version" --> '$resolved_version' in the error message for preflight checks.
Refactored an if-then for handling "xz forced on" mode (whether to run can_use_xz in install()), for readability, and for consistency with other if-thens to do with "xz forced on" mode.

shadowspawn · 2019-09-22T00:23:59Z

remove &> /dev/null from is_ok

That was something I was going to comment on, already addressed. 👍

bin/nvh

Determine whether xz can be used, based on platform/system details.

DeeDeeG · 2019-09-22T01:32:43Z

Updated can_use_xz to address the two most-recent comments.

If can_use_xz() checks fail, revert to using gzip.

Adapted from this comment: shadowspawn#8 (comment) Co-authored-by: John Gee <john@ruru.gen.nz>

Skips node version checks, which would otherwise gate xz usage. In other words: Overrides any checks that would normally be run, so that nvh always attempts to use xz. --- Also updates "--use-xz" to not set the "xz forced on" state. (This matches the flag's description in the help messages.) --- Partly adapted from this comment (with liberties taken on implementation): shadowspawn#8 (comment) Co-authored-by: John Gee <john@ruru.gen.nz>

Activates "force use xz" state, which skips xz-related sanity checks.

DeeDeeG · 2019-09-22T03:08:20Z

More tiny changes in force-push:

Edit the commit titled "Use can_use_xz to gate xz usage"
- This commit harmlessly passed the node version to can_use_xz, which was unnecessary, because in this PR, can_use_xz() doesn't operate on arguments at all. Just some code cleanup after all the rebasing.
Edit the same commit's commit title to have parentheses for "can_use_xz()"

DeeDeeG · 2019-09-22T04:02:31Z

bin/nvh

+    local resolved_major_version
+    resolved_major_version="$(echo ${resolved_version#v} | cut -d '.' -f '1')"
+    if [[ "${resolved_major_version}" -lt 4 ]]; then
+      NVH_USE_XZ="false"


Just a thought: the Node version check could be moved back into can_use_xz().

Not sure which I prefer, but putting it in can_use_xz() could be a bit more DRY, due to not having to check for NVH_USE_XZ again.

Could gate any of the checks in can_use_xz() (e.g. just the platform checks) with: if [[ "${g_xz_force_enabled}" = "false" ]]; then if need be.

shadowspawn · 2019-09-22T04:04:08Z

Working on code now, almost done...

shadowspawn · 2019-09-22T04:12:26Z

Pushed to a branch on nvh. I'll open a PR back to you!

DeeDeeG · 2019-09-22T04:14:39Z

It is getting a bit late in my time zone, but I may have a look before I log off for the evening. Happy to take a look if not this evening then tomorrow.

Edit: I take it this is the branch in question: https://github.com/shadowspawn/nvh/tree/feature/auto-xz Taking a quick look now, but if it is much of a change I probably should wait until tomorrow to really give it a proper look.

Edit 2: Looks more in line with the coding style of the rest of nvh. 👍 (I couldn't tell if the branch changes scenarios where xz will be used. on my TODO for tomorrow would be to take a closer look at that, etc.) Logging off for the night. Thanks for the back and forth!

shadowspawn · 2019-09-22T04:16:33Z

No worries, no hurry. Thanks.

shadowspawn

Planning some refactoring as per changes on feature/auto-xz, but happy with PR as stands.

Thanks for the deep investigations!

DeeDeeG · 2019-09-22T14:59:46Z

Thanks for the review!

Miscellaneous comments regarding the branch at feature/auto-xz (as of commit dbb996b):

I could drop the last two commits from this PR for a neater commit history. Those are the commits that added the xz "force enabled" mode.
Comment in setup still references xz force-enabled mode: "set to anything else to force-enable"
Looking at the comments for can_use_xz(), "Unfortunately we can't query tar itself." We sort of can, just that we would need to check all of:
- the version number of tar
- whether tar is GNU tar or (bsd)tar from libarchive
- still have to check for xz on PATH on Linux
- Now need to additionally check for /usr/lib/liblzma.dylib on macOS
  So it's more testing logic, for what amounts to checks that more than likely only work on Linux or macOS anyway. (So I guess "we can't [just] query tar itself." is, by and large, accurate without being long-winded.)
- Normalizing NVH_USE_XZ in main() looks practical, as it can call can_use_xz().
  - ~~Probably makes the normalization in the setup phase redundant, or vice-versa.~~
    - okay, I see this just sets NVH_USE_XZ to "true" if unset, so it's not redundant.
    - could potentially refactor the normalization/on-by-default logic into a single if-elif-else in main() if desired.
  - Noticed the comment in main() reads: "# Default to using nvh" which probably should be "# Default to using xz"
    - I've also done this a few times. 😄

In any case I am happy to ship this feature off to wider, bluer waters (upstream). Thanks for the rounds of feedback and review.

shadowspawn · 2019-09-23T00:17:23Z

Good tip about dropping the two commits, I can do that. And I'll update the errata in the comments, thanks.

shadowspawn · 2019-10-03T03:37:43Z

Merged and tweaked on develop. Preparing for release.

Thank you for your contributions.

DeeDeeG added 2 commits September 19, 2019 12:28

bin/nvh: xz compression (w/ environment variable)

85cf20c

Set "NVH_USE_XZ" to "true" to use xz compression.

bin/nvh: Command-line argument for xz compression

890ae1b

--use-xz for xz, --no-use-xz for gzip. Overrides environment variable.

DeeDeeG changed the base branch from master to develop September 21, 2019 23:17

DeeDeeG force-pushed the xz-pr branch from dd7d9b1 to 48a9f35 Compare September 22, 2019 00:17

shadowspawn reviewed Sep 22, 2019

View reviewed changes

bin/nvh Outdated Show resolved Hide resolved

bin/nvh Outdated Show resolved Hide resolved

bin/nvh: Add "can_use_xz()" function

8ec7b71

Determine whether xz can be used, based on platform/system details.

DeeDeeG force-pushed the xz-pr branch from 48a9f35 to 3c1481a Compare September 22, 2019 01:32

DeeDeeG and others added 4 commits September 21, 2019 23:03

bin/nvh: Use can_use_xz() to gate xz usage

0c17df1

If can_use_xz() checks fail, revert to using gzip.

bin/nvh: Normalize "NVH_USE_XZ" from environment

40883c1

Adapted from this comment: shadowspawn#8 (comment) Co-authored-by: John Gee <john@ruru.gen.nz>

bin/nvh: Add --force-use-xz command-line flag

ea52a42

Activates "force use xz" state, which skips xz-related sanity checks.

DeeDeeG force-pushed the xz-pr branch from 3c1481a to ea52a42 Compare September 22, 2019 03:09

DeeDeeG commented Sep 22, 2019

View reviewed changes

shadowspawn approved these changes Sep 22, 2019

View reviewed changes

shadowspawn closed this Oct 3, 2019

DeeDeeG mentioned this pull request Jan 25, 2020

Consider porting the auto-xz feature from nvh tj/n#606

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

support fetching xz tarballs #10

support fetching xz tarballs #10

DeeDeeG commented Sep 21, 2019 •

edited

Loading

shadowspawn commented Sep 22, 2019

DeeDeeG commented Sep 22, 2019

shadowspawn commented Sep 22, 2019

DeeDeeG commented Sep 22, 2019

DeeDeeG commented Sep 22, 2019

DeeDeeG Sep 22, 2019

shadowspawn commented Sep 22, 2019

shadowspawn commented Sep 22, 2019

DeeDeeG commented Sep 22, 2019 •

edited

Loading

shadowspawn commented Sep 22, 2019

shadowspawn left a comment

DeeDeeG commented Sep 22, 2019 •

edited

Loading

shadowspawn commented Sep 23, 2019

shadowspawn commented Oct 3, 2019

support fetching xz tarballs #10

support fetching xz tarballs #10

Conversation

DeeDeeG commented Sep 21, 2019 • edited Loading

shadowspawn commented Sep 22, 2019

DeeDeeG commented Sep 22, 2019

shadowspawn commented Sep 22, 2019

DeeDeeG commented Sep 22, 2019

DeeDeeG commented Sep 22, 2019

DeeDeeG Sep 22, 2019

Choose a reason for hiding this comment

shadowspawn commented Sep 22, 2019

shadowspawn commented Sep 22, 2019

DeeDeeG commented Sep 22, 2019 • edited Loading

shadowspawn commented Sep 22, 2019

shadowspawn left a comment

Choose a reason for hiding this comment

DeeDeeG commented Sep 22, 2019 • edited Loading

shadowspawn commented Sep 23, 2019

shadowspawn commented Oct 3, 2019

DeeDeeG commented Sep 21, 2019 •

edited

Loading

DeeDeeG commented Sep 22, 2019 •

edited

Loading

DeeDeeG commented Sep 22, 2019 •

edited

Loading