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

Use tap #314

Closed
wants to merge 8 commits into from
Closed

Use tap #314

wants to merge 8 commits into from

Conversation

isaacs
Copy link
Contributor

@isaacs isaacs commented Sep 8, 2017

Land after #299
Supersedes #313

This brings node-pre-gyp into a more modern age of tar parsing and
creation.  Node-tar v3 is faster and more reliable, and removes several
race conditions that were inherent in the fstream utility.

This also removes the need for some workarounds to force directories to
be readable.  It also uses the npm-packlist module to decide what to put
into a package, which will be used by npm itself relatively soon.

Tar version 3 is only supported by node v4 and above, as it uses modern
ES class and fat-arrow syntax.  Thus, this is a breaking change, worthy
of a semver major version bump, but it only leaves less than 10% of our
users behind.  (And folks who don't update Node.js out of unmaintained
versions are unlikely to upgrade node-pre-gyp anyway.)

The only change I'm somewhat unsure about is the use of the "portable"
flag in tar.create here.  This excludes system-specific items like
username, uid, ctime, etc., resulting in a tarball that is 100%
dependent on the contents and mtime of the items it includes.  Of
course, mtime will change if the package is rebuilt, but this means that
multiple calls to `node-pre-gyp package` will not result in different
bits on disk.
@springmeyer springmeyer mentioned this pull request Sep 8, 2017
Dane Springmeyer and others added 2 commits September 10, 2017 00:55
Also, coverage!  Not very good right now, but could be improved over time.
@springmeyer
Copy link
Contributor

we've been doing okay with tape, so not planning on working to update/land this.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants