Upgrade to tar version 3 #299
Conversation
isaacs
force-pushed
the
master
branch
6 times, most recently
from
d0d7c23
to
697d123
Compare
|
I'm kind of confused by the appveyor failures. The tests all pass locally, including on my Windows VM. Can anyone more knowledgeable than me shed some light on what might be going wrong, or how best to debug it? Red CI is disturbing. |
|
Looking at the commit history it seems like Appveyor has almost never passed? I think this PR isn't making it worse at least, but it seems like either that should be fixed, or Appveyor CI removed. |
|
@isaacs thank you for this effort! The appveyor failures are unrelated to your PR. I will work to fix them early next week and then will ping back here so you can rebase and ensure this PR is fully passing. |
|
Turns out getting the tests passing on windows/appveyor was quite a bit more involved than I anticipated. After a couple of days working on this I think the core problems are due to npm bugs (in the npm versions shipped with node v6 and v8) that I've ticketed in #300 and #302. I've confirmed that I can fix them by downgrading npm which I've got working over in #301. So, I think this PR should wait for #301 to be merged. |
|
Would it be worth updating this to use tar 4.0.1? |
|
Yes, I would recommend taking the latest and greatest. The only API difference is that unpacking now throws if the |
|
#301 is merged. Possible to rebase this now? Very sorry for the delay. |
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.
|
Rebased, checks running on travis and appveyor now. @springmeyer Any reason why you chose tape rather than tap? It'd be nice to get coverage and a less granular report output. |
Great, thank you.
I would absolutely welcome a PR moving to tap (ideally separate from this). I'm in a scramble trying to understand/replicate/debug/fix TryGhost/node-sqlite3#866 and wanted to try a new node-pre-gyp tag (based on the notation that maybe the last tag's bundled deps trigger bad behavior in npm). So I merged #301 in a rush trying to get the node-pre-gyp tests passing before tagging and did not feel confident porting to tap in a rush (although I'm sure it is likely very easy). |
|
If you would like to try using tap for tests, it's available on my "use-tap" branch. Here's what the output reporting looks like with coverage:
The diff is pretty small from tape, since you already did the hard part of moving off of mocha. 66a42ed |
|
Ah, yeah, I didn't see your message before posting mine. Separate PR from this one makes total sense. I'll stash it for now and try to debug the appveyor failure if I can. Looks like something weird with an extraneous dep showing up? |
|
Looks like this is passing now. The Thanks! |
|
@isaacs - thank you, this is looking good. The one hesitation I have is the upgrade to |
|
👍 Appveyoring now :) |
|
related issue #318 . |
|
Update on my end: Traveling currently, but when I have time in a few weeks I want to merge this and per #319, create a 1.x series that drops node v0.10.x support and starts targeting only node v4 and above. |
|
@springmeyer Fantastic! Node.js 0.x is a security liability at this point, best to move away from it asap. Let me know if you need any help resolving conflicts or w/e. |
|
@springmeyer We are interested in this being resolved, is there anything we can do to help? |
Starting probably with mapbox/node-pre-gyp#299, the contents of the tarballs are bloated. Rolling back to 0.7.0.
Starting probably with mapbox/node-pre-gyp#299, the contents of the tarballs are bloated. Rolling back to 0.7.0.
Starting probably with mapbox/node-pre-gyp#299, the contents of the tarballs are bloated. Rolling back to 0.7.0.
Upgrade to tar version 3
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 packagewill not result in differentbits on disk.