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

Extract .tar.gz files using native tar or 7z.exe when possible #7457

Merged
merged 10 commits into from
Jul 29, 2016

Conversation

benjamn
Copy link
Contributor

@benjamn benjamn commented Jul 22, 2016

For the sake of portability, the files.extractTarGz method is currently implemented in terms of a pure-JavaScript library: https://www.npmjs.com/package/tar

Portability is nice, but speed is also nice. On my machine, it takes 25 seconds to extract the fourseven:scss package using the npm tar package, whereas it takes only 3.5 seconds to extract it using the native tar command. On Windows, the time drops from 36 seconds to 8 seconds.

The time savings for even larger packages like meteor-tool are even more dramatic: 45 seconds vs. 4 seconds on my Mac.

This pull request uses the native commands when possible, but falls back to the npm tar library if the native commands don't work for any reason.

For Windows, we now include a standalone 7z.exe binary in the dev bundle (i.e., dev_bundle\bin\7z.exe), though tar will also work on Windows if it's in the $env:PATH. This is the standard 7-Zip 16.02 program available here. I believe this usage is compatible with the 7-zip license, according to the FAQ.

I think these changes may be too risky to ship with Meteor 1.4, given that it's now in the release candidate stage, but if all goes well we will ship them soon after that release (likely 1.4.1).

cc @tmeasday @zol @abernix

var extractor = new tar.Extract({
path: files.convertToOSPath(tempDir)
}).on('entry', function (e) {
if (process.platform === "win32" || options.forceConvert) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

The options.forceConvert boolean is no longer passed by any callers.

@s-devaney
Copy link

Could the status of the extraction be added to the update process? At the moment it says "downloading" rather than "extracting" etc

@abernix
Copy link
Contributor

abernix commented Jul 28, 2016

@s-devaney This is a good idea. I've looked at this recently, but currently they're in the same sub-task so the message doesn't change. I think assuming that this PR works okay, it will be possible to improve the messaging. For now, as Ben mentioned above, meteor run --verbose will be super informative.

@benjamn
Copy link
Contributor Author

benjamn commented Jul 28, 2016

@s-devaney I'll add a commit that reflects that, thanks!

@benjamn
Copy link
Contributor Author

benjamn commented Jul 28, 2016

The build messages now distinguish between downloading, extracting, and loading packages. Thanks to this PR, the extraction is now by far the cheapest part.

@benjamn
Copy link
Contributor Author

benjamn commented Jul 28, 2016

@tmeasday should I hold off on merging this until after 1.4.0.1?

@tmeasday
Copy link
Contributor

Yeah, although I can create a release branch for 1.4.0.1, so merge it whenever

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

Successfully merging this pull request may close these issues.

4 participants