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

build: enable xz compressed tarballs where possible (v0.12 and v0.10) #4894

Closed
wants to merge 1 commit into from

Conversation

rvagg
Copy link
Member

@rvagg rvagg commented Jan 27, 2016

Continuing on from nodejs/build#284

/cc @chorrell, @mscdex, @ljharb, @nodejs/build

We didn't actually agree on anything there but I'm in a why not mood right now.

This will mean that we get .xz tarballs from v0.12.10 and v0.10.42 onwards for those lines. Backing up and providing .xz tarballs for the others is impossible unless we exclude them from the signed SHASUMS file, which is far from ideal so I'm a strong -1 on completing v0.12 and v0.10. Tools like nvm will just have to hardwire in versions numbers for determining when to use .xz and when to use .gz. We're going to have to do something similar in node-gyp to use header files for older versions now that we have them fixed (the same versions will finally have a proper header tarball that can be used).

@rvagg
Copy link
Member Author

rvagg commented Jan 27, 2016

this logic is the same as master FYI

$ git diff -U0 master -- Makefile| grep -i xz | wc -l
       0

@r-52 r-52 added build Issues and PRs related to build files or the CI. v0.12 labels Jan 27, 2016
@bnoordhuis
Copy link
Member

LGTM FWIW. I'd be okay with making it non-conditional, mandating that xz is installed is not an onerous requirement.

@rvagg
Copy link
Member Author

rvagg commented Jan 27, 2016

@bnoordhuis as per the notes, it's annoying on OSX, hence the conditional

@bnoordhuis
Copy link
Member

I saw that but I assume the OS X buildbots will have it installed so when would it be an issue? People don't generally run it manually, do they?

@rvagg
Copy link
Member Author

rvagg commented Jan 27, 2016

Yeah, mainly just the release build machines and this is one of the criteria for those, I don't think it gets run much in the wild but it can be used to build tarballs if you download source and I suspect there are come companies out there doing that, unlikely that they'd be doing it for OSX, however!

@jbergstroem
Copy link
Member

LGTM here too. I don't mind the conditional much.

@richardlau
Copy link
Member

I'd prefer the conditional to be consistent with the current v4.x.

In addition to OS X, AIX also does not have xz installed by default.

@chorrell
Copy link

Great!

@jasnell
Copy link
Member

jasnell commented Jan 27, 2016

LGTM

@ljharb
Copy link
Member

ljharb commented Jan 27, 2016

so in semver-speak ~0.12.10 || ~0.10.42 || >= 1.0 covers the node versions that definitely will have xz archives available?

@jbergstroem
Copy link
Member

@ljharb I think it should be ~0.12.10 || ~0.10.42 || >= 2.3.2.

@ljharb
Copy link
Member

ljharb commented Jan 27, 2016

@rvagg any possibility of completing >= 1.0.0 with xz?

@jbergstroem
Copy link
Member

@ljharb that'd be painful. Is it important? I mean, 0.10, 0.12 starts midway too.

@ljharb
Copy link
Member

ljharb commented Jan 27, 2016

I don't have visibility into how painful it is for you folks - but every version i have to hardcode into nvm is painful in a long-term sense, whereas creating the xz's seems like it would be painful once.

@jbergstroem
Copy link
Member

@ljharb last I checked we did that xz/version check where we just hardcode lowest required versions. Shouldn't that be an ok compromise?

@ljharb
Copy link
Member

ljharb commented Jan 28, 2016

Yes, currently we hardcode 1 version. The current suggestion seems to indicate I'd have to hardcode 3. Just trying to minimize how many I have to manage.

@rvagg
Copy link
Member Author

rvagg commented Jan 28, 2016

OK, this is a little bit complicated and there are multiple concerns here, both for version managers and for node-gyp. I believe the correct ranges are:

  • Linux (all) and sources with .xz compression: ~0.12.10 || ~0.10.42 || >= 1.0
  • Linux (all), sources and darwin with .xz compression: ~0.12.10 || ~0.10.42 || >= 2.3.2
  • Valid headers tarball (with .xz): ~0.12.10 || ~0.10.42 || >= 2.3.2

i.e.

Clear as mud?

@ljharb
Copy link
Member

ljharb commented Jan 28, 2016

Thank you for documenting it. Any "backfills" you can do that will condense those three rows, or their resulting columns, into a smaller number of combinations than 9, would be appreciated :-)

rvagg added a commit that referenced this pull request Jan 28, 2016
PR-URL: #4894
Reviewed-By: Ben Noordhuis <[email protected]>
Reviewed-By: Johan Bergström <[email protected]>
Reviewed-By: James M Snell <[email protected]>
rvagg added a commit that referenced this pull request Jan 28, 2016
PR-URL: #4894
Reviewed-By: Ben Noordhuis <[email protected]>
Reviewed-By: Johan Bergström <[email protected]>
Reviewed-By: James M Snell <[email protected]>
@rvagg
Copy link
Member Author

rvagg commented Jan 28, 2016

@rvagg rvagg closed this Jan 28, 2016
@rvagg rvagg deleted the xz-all-the-things branch January 28, 2016 09:23
@rvagg
Copy link
Member Author

rvagg commented Jan 28, 2016

rvagg added a commit that referenced this pull request Feb 8, 2016
PR-URL: #4894
Reviewed-By: Ben Noordhuis <[email protected]>
Reviewed-By: Johan Bergström <[email protected]>
Reviewed-By: James M Snell <[email protected]>
rvagg added a commit that referenced this pull request Feb 8, 2016
PR-URL: #4894
Reviewed-By: Ben Noordhuis <[email protected]>
Reviewed-By: Johan Bergström <[email protected]>
Reviewed-By: James M Snell <[email protected]>
jasnell pushed a commit that referenced this pull request Feb 9, 2016
PR-URL: #4894
Reviewed-By: Ben Noordhuis <[email protected]>
Reviewed-By: Johan Bergström <[email protected]>
Reviewed-By: James M Snell <[email protected]>
jasnell pushed a commit that referenced this pull request Feb 9, 2016
PR-URL: #4894
Reviewed-By: Ben Noordhuis <[email protected]>
Reviewed-By: Johan Bergström <[email protected]>
Reviewed-By: James M Snell <[email protected]>
ljharb added a commit to ljharb/nvm that referenced this pull request Sep 5, 2016
jBarz pushed a commit to ibmruntimes/node that referenced this pull request Nov 4, 2016
PR-URL: nodejs/node#4894
Reviewed-By: Ben Noordhuis <[email protected]>
Reviewed-By: Johan Bergström <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
build Issues and PRs related to build files or the CI.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants