build: make .tar.xz creation opt-out, fail if no xz #24551

rvagg · Nov 21, 2018

During a recent release, the 8.x before last I think, we discovered belatedly that the macos release machine didn't have xz installed on it. This change was discussed (somewhere, but I know @MylesBorins was involved) to help resolve such oversights in future.

Change XZ to HAS_XZ and make it 0=no and 1=yes
Introduce SKIP_XZ as a possible user-supplied value, defaulting to 0=no
Check for either HAS_XZ=1 or SKIP_XZ=1 on any release build that produces a tarball of some kind and fail if the system doesn't have xz and SKIP_XZ=1 hasn't been supplied.
Only produce a .tar.xz if both HAS_XZ=1 and SKIP_XZ=0 (i.e. we have xz and haven't explicitly opted to skip)

This would get backported to active lines and we'd have to put a SKIP_XZ=1 for AIX builds (only).

I've tested the main permutations of this and it seems to work fine. I'd appreciate some experienced Makefile eyes though, @nodejs/build @bnoordhuis @joyeecheung?

nodejs-github-bot · Nov 21, 2018

@rvagg build started: https://ci.nodejs.org/blue/organizations/jenkins/node-test-pull-request-lite-pipeline/detail/node-test-pull-request-lite-pipeline/1664/pipeline

refack · Nov 21, 2018

refack approved these changes Nov 21, 2018

View changes

LGTM % quastions

Makefile

refack · Nov 21, 2018

/CC @nodejs/build-files

richardlau · Nov 22, 2018

richardlau approved these changes Nov 22, 2018

View changes

rvagg · Nov 22, 2018

OK, second commit on this PR, 94a2c9c, implements the above behaviour suggested by @refack and the checks around the execution of xz is now simple again, although inverted from previous: ifeq ($(XZ), 1). That should make it easier to understand than the original logical-and within a single ifeq which is pretty opaque.

refack · Nov 22, 2018

refack approved these changes Nov 22, 2018

View changes

richardlau · Nov 22, 2018

richardlau approved these changes Nov 22, 2018

View changes

refack · Nov 22, 2018

Lite-CI (since it's not covered in CI anyway): https://ci.nodejs.org/job/node-test-pull-request-lite-pipeline/1678/

Reviewers, please 👍 if you are in favor of fast-tracking this PR.

richardlau · Nov 22, 2018

Lite-CI (since it's not covered in CI anyway): https://ci.nodejs.org/job/node-test-pull-request-lite-pipeline/1678/

Reviewers, please 👍 if you are in favor of fast-tracking this PR.

Is it worth asking @nodejs/releasers for a test release build?

refack · Nov 22, 2018

Is it worth asking @nodejs/releasers for a test release build?

~~https://ci-release.nodejs.org/job/iojs+release/3954/~~
https://ci-release.nodejs.org/job/iojs+release/3956 ✔️

refack · Nov 22, 2018

The ci-release job finished successfully, the output is at https://nodejs.org/download/nightly/v12.0.0-nightly20181122.2.94a2c9c8e5/ (no .xz for AIX)

rvagg · Nov 23, 2018

@refack you made a nightly with this with a weird date string: https://ci-release.nodejs.org/job/iojs+release/3956/ and now we have screwed up nightly version strings which won't parse with tools that expect a predictable format on this directory:

Please don't do that. Nightlies are strictly not for testing. I'm going to have to figure out if I can clean this one out now.

There's a bunch of good reasons we don't hand out ci-release access liberally, and the ability to make this kind of mess is one of them.

refack · Nov 23, 2018

Please don't do that. Nightlies are strictly not for testing. I'm going to have to figure out if I can clean this one out now.

My bad. What I meant to do was run a test build.

P.S. As a postmortem, we should add some validation code to assert our and our tools format assumptions (nodejs/build#1592)

rvagg · Nov 23, 2018

OK, so this should have failed on AIX but it didn't.

The config doesn't have SKIP_XZ=1 for AIX, and the output https://ci-release.nodejs.org/job/iojs+release/nodes=aix61-ppc64/3956/consoleFull doesn't mention anything about SKIP_XZ. It either hasn't invoked check-xz on there (not sure why it wouldn't) or HAS_XZ=1, which doesn't make sense because it also doesn't invoke xz..

refack · Nov 23, 2018

P.P.S. we also have this one - https://nodejs.org/download/nightly/v11.0.0-nightly201806050300f7c68cffa5de7f0099e3afa95093b0bbbe3c/

rvagg · Nov 23, 2018

Ah, so we have this in iojs+release:

if [[ "X${disttype}" != "Xrelease" ]]; then
  perl -pi -e "s/: release-only/:/g" Makefile
fi

That traces back to the addition of the REPLACEME stuff for API docs which is too strict. I tried to fix that back in #12958 but it got stalled with too much discussion, so the perl replacement has stuck and we're without the release-only checks entirely for release builds.. yay.

refack · Nov 23, 2018

refack reviewed Nov 23, 2018

View changes

refack · Nov 23, 2018

Makefile

 XZ_COMPRESSION ?= 9e
 PKG=$(TARNAME).pkg
 MACOSOUTDIR=out/macos

+ifeq ($(SKIP_XZ), 1)
+check-xz:


Maybe it's because check-xz should be .PHONY?

rvagg · Nov 23, 2018

Proposed fix in #24575 so we can remove the perl release-only stripping.

rvagg · Dec 5, 2018

Discovered some problems, not sure why these weren't picked up with test builds in ci-release for the PR. #24841

rvagg · Dec 5, 2018

@nodejs/build FYI this is now in iojs+release for the ${ENV, var="OSTYPE"} = AIX61 build script:

It's building successfully now:

Results for this test going to https://nodejs.org/download/test/v12.0.0-test201812040ba0e0fd89/

rvagg added the build label Nov 21, 2018

rvagg force-pushed the rvagg:rvagg/xz-opt-out branch from d2e0367 to ba2647a Nov 21, 2018

refack added the aix label Nov 21, 2018

richardlau referenced this pull request Nov 22, 2018
Open
Proposal: remove .gz downloads for some platforms #1584

rvagg force-pushed the rvagg:rvagg/xz-opt-out branch from 14eedc0 to 94a2c9c Nov 22, 2018

refack added fast-track author ready labels Nov 22, 2018

refack referenced this pull request Nov 23, 2018
Open
FR: add some validation to ci-release `iojs+release` #1592

rvagg added a commit to rvagg/io.js that referenced this pull request Nov 23, 2018

build: only check REPLACEME & DEP...X for releases
Ref: nodejs#24551 Ref: nodejs#12958 Ref: nodejs#12957 Ref: nodejs#8325

Loading status checks…

6759a2e

rvagg referenced this pull request Nov 23, 2018
Closed
build: only check REPLACEME & DEP...X for releases #24575

Trott removed author ready fast-track labels Nov 25, 2018

BridgeAR referenced this pull request Dec 5, 2018
Merged
v11.4.0 proposal #24854
4 of 4 tasks complete

refack added a commit to refack/node that referenced this pull request Jan 14, 2019

build: make tar.xz creation opt-out, fail if no xz
PR-URL: nodejs#24551 Reviewed-By: Refael Ackermann <refack@gmail.com> Reviewed-By: Richard Lau <riclau@uk.ibm.com>

8a371cf

BethGriggs referenced this pull request Feb 12, 2019
Merged
v10.15.3 proposal #26063

rvagg added a commit that referenced this pull request Feb 28, 2019

build: make tar.xz creation opt-out, fail if no xz
PR-URL: #24551 Reviewed-By: Refael Ackermann <refack@gmail.com> Reviewed-By: Richard Lau <riclau@uk.ibm.com>

5b4a826

nodejs/node

Perfect your code

build: make .tar.xz creation opt-out, fail if no xz #24551

Conversation

rvagg commented Nov 21, 2018

rvagg added the build label Nov 21, 2018

This comment has been minimized.

nodejs-github-bot commented Nov 21, 2018

rvagg force-pushed the rvagg:rvagg/xz-opt-out branch from d2e0367 to ba2647a Nov 21, 2018

refack approved these changes Nov 21, 2018 View changes

refack left a comment

This comment has been minimized.

refack commented Nov 21, 2018

refack added the aix label Nov 21, 2018

richardlau approved these changes Nov 22, 2018 View changes

richardlau referenced this pull request Nov 22, 2018

rvagg force-pushed the rvagg:rvagg/xz-opt-out branch from 14eedc0 to 94a2c9c Nov 22, 2018

This comment has been minimized.

rvagg commented Nov 22, 2018

refack approved these changes Nov 22, 2018 View changes

richardlau approved these changes Nov 22, 2018 View changes

This comment has been minimized.

refack commented Nov 22, 2018 • edited

refack added fast-track author ready labels Nov 22, 2018

This comment has been minimized.

richardlau commented Nov 22, 2018

This comment has been minimized.

refack commented Nov 22, 2018 • edited

This comment has been minimized.

refack commented Nov 22, 2018 • edited

This comment has been minimized.

rvagg commented Nov 23, 2018 • edited

This comment has been minimized.

refack commented Nov 23, 2018 • edited

This comment has been minimized.

rvagg commented Nov 23, 2018 • edited

This comment has been minimized.

refack commented Nov 23, 2018

refack referenced this pull request Nov 23, 2018

This comment has been minimized.

rvagg commented Nov 23, 2018

refack reviewed Nov 23, 2018 View changes

This comment has been minimized.

rvagg added a commit to rvagg/io.js that referenced this pull request Nov 23, 2018

rvagg referenced this pull request Nov 23, 2018

This comment has been minimized.

rvagg commented Nov 23, 2018

Trott removed author ready fast-track labels Nov 25, 2018

This comment has been minimized.

rvagg commented Dec 5, 2018

This comment has been minimized.

rvagg commented Dec 5, 2018

Trott added a commit to Trott/io.js that referenced this pull request Dec 5, 2018

BridgeAR added a commit that referenced this pull request Dec 5, 2018

BridgeAR added a commit that referenced this pull request Dec 5, 2018

BridgeAR added a commit that referenced this pull request Dec 5, 2018

BridgeAR added a commit that referenced this pull request Dec 5, 2018

BridgeAR referenced this pull request Dec 5, 2018

BridgeAR added a commit that referenced this pull request Dec 7, 2018

BridgeAR added a commit that referenced this pull request Dec 7, 2018

BridgeAR added a commit that referenced this pull request Dec 7, 2018

BridgeAR added a commit that referenced this pull request Dec 7, 2018

This was referenced Dec 7, 2018

MylesBorins added a commit that referenced this pull request Dec 26, 2018

refack added a commit to refack/node that referenced this pull request Jan 14, 2019

refack added a commit to refack/node that referenced this pull request Jan 14, 2019

refack added a commit to refack/node that referenced this pull request Jan 14, 2019

BethGriggs added a commit that referenced this pull request Feb 12, 2019

BethGriggs added a commit that referenced this pull request Feb 12, 2019

BethGriggs referenced this pull request Feb 12, 2019

BethGriggs added a commit that referenced this pull request Feb 20, 2019

BethGriggs added a commit that referenced this pull request Feb 20, 2019

rvagg added a commit that referenced this pull request Feb 28, 2019

rvagg added a commit that referenced this pull request Feb 28, 2019

rvagg added a commit that referenced this pull request Feb 28, 2019

BethGriggs added a commit that referenced this pull request Mar 7, 2019

rvagg force-pushed the rvagg:rvagg/xz-opt-out branch from `d2e0367` to `ba2647a` Nov 21, 2018

refack approved these changes Nov 21, 2018

View changes

richardlau approved these changes Nov 22, 2018

View changes

rvagg force-pushed the rvagg:rvagg/xz-opt-out branch from `14eedc0` to `94a2c9c` Nov 22, 2018

refack approved these changes Nov 22, 2018

View changes

richardlau approved these changes Nov 22, 2018

View changes

refack commented Nov 22, 2018 •

edited

refack commented Nov 22, 2018 •

edited

refack commented Nov 22, 2018 •

edited

rvagg commented Nov 23, 2018 •

edited

refack commented Nov 23, 2018 •

edited

rvagg commented Nov 23, 2018 •

edited

refack reviewed Nov 23, 2018

View changes