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

download headers tarball for ~0.12.10 || ~0.10.42 #877

Closed
wants to merge 1 commit into from

Conversation

rvagg
Copy link
Member

@rvagg rvagg commented Feb 12, 2016

/R= @nodejs/build @ljharb I guess

@rvagg
Copy link
Member Author

rvagg commented Feb 12, 2016

I'd consider this a semver-minor and warrants introduction into npm@2 if we can manage it. There's some queued up changes that need to go out with the next 0.10 and 0.12 so that could be done fairly soon. So /cc @zkat.

@@ -93,7 +95,7 @@ function processRelease (argv, gyp, defaultVersion, defaultRelease) {
// making the bold assumption that anything with a version number >3.0.0 will
// have a *-headers.tar.gz file in its dist location, even some frankenstein
// custom version
tarballUrl = url.resolve(baseUrl, name + '-v' + version + (versionSemver.major >= 3 ? '-headers' : '') + '.tar.gz')
tarballUrl = url.resolve(baseUrl, name + '-v' + version + (semver.satisfies(versionSemver, headersTarballRange) ? '-headers' : '') + '.tar.gz')
Copy link
Member

Choose a reason for hiding this comment

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

Overlong line.

@bnoordhuis
Copy link
Member

LGTM sans style nit. The commit log could go into more detail.

Properly created -headers.tar.gz files were not available until
v0.12.10 and v0.10.42 (in addition to >= 3).
@rvagg rvagg force-pushed the headers-0.10-0.12 branch from 9ecd001 to 941e24f Compare February 13, 2016 01:50
@rvagg
Copy link
Member Author

rvagg commented Feb 13, 2016

thanks @bnoordhuis, both addressed

This was referenced Feb 13, 2016
@bnoordhuis
Copy link
Member

LGTM but can you s/^download/Download before you land it?

@jbergstroem
Copy link
Member

Do we care about xz at this stage? Benefitting from headers only is good enough? Anyway, I can likely represent the Internet Bandwidth Committee stating LGTM.

@rvagg
Copy link
Member Author

rvagg commented Feb 14, 2016

@jbergstroem I'd love to use xz for these but we don't have xz support from zlib so uncompressing them without invoking system tools is tricky. I've considered pulling in a JS implementation but haven't done any research on whether they exist or whether they are any good. Given that we're talking about ~500kb the effort in researching and coding it up is unlikely to yield enough benefit to warrant me bothering!

@rvagg
Copy link
Member Author

rvagg commented Feb 14, 2016

^ that's not to say I wouldn't support someone else doing this work if they feel it's an interesting enough challenge and worth their time, it's just not worth my time

@rvagg rvagg closed this Feb 15, 2016
@rvagg rvagg deleted the headers-0.10-0.12 branch February 15, 2016 00:18
rvagg added a commit that referenced this pull request Feb 15, 2016
Properly created -headers.tar.gz files were not available until
v0.12.10 and v0.10.42 (in addition to >= 3).

PR-URL: #877
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Johan Bergström <bugs@bergstroem.nu>
@rvagg
Copy link
Member Author

rvagg commented Feb 15, 2016

landed @ 070fe69

rvagg added a commit that referenced this pull request Feb 15, 2016
Properly created -headers.tar.gz files were not available until
v0.12.10 and v0.10.42 (in addition to >= 3).

PR-URL: #877
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Johan Bergström <bugs@bergstroem.nu>
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.

3 participants