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: make .tar.xz creation opt-out, fail if no xz #24551

Closed
wants to merge 2 commits into from

Conversation

rvagg
Copy link
Member

@rvagg rvagg commented 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?

@rvagg rvagg added the build Issues and PRs related to build files or the CI. label Nov 21, 2018
Copy link
Contributor

@refack refack left a comment

Choose a reason for hiding this comment

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

LGTM % quastions

Makefile Show resolved Hide resolved
Makefile Show resolved Hide resolved
@refack
Copy link
Contributor

refack commented Nov 21, 2018

/CC @nodejs/build-files

@refack refack added the aix Issues and PRs related to the AIX platform. label Nov 21, 2018
@rvagg
Copy link
Member Author

rvagg commented Nov 22, 2018

OK, second commit on this PR, 94a2c9c8e5, 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
Copy link
Contributor

refack commented 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.

@refack refack added fast-track PRs that do not need to wait for 48 hours to land. author ready PRs that have at least one approval, no pending requests for changes, and a CI started. labels Nov 22, 2018
@richardlau
Copy link
Member

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
Copy link
Contributor

refack commented 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
Copy link
Contributor

refack commented 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
Copy link
Member Author

rvagg commented 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:

screenshot 2018-11-23 13 11 26

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
Copy link
Contributor

refack commented 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
Copy link
Member Author

rvagg commented 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
Copy link
Contributor

refack commented Nov 23, 2018

@rvagg
Copy link
Member Author

rvagg commented 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.

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

ifeq ($(SKIP_XZ), 1)
check-xz:
Copy link
Contributor

Choose a reason for hiding this comment

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

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

@rvagg
Copy link
Member Author

rvagg commented Nov 23, 2018

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

@Trott Trott removed author ready PRs that have at least one approval, no pending requests for changes, and a CI started. fast-track PRs that do not need to wait for 48 hours to land. labels Nov 25, 2018
@rvagg
Copy link
Member Author

rvagg commented 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
Copy link
Member Author

rvagg commented Dec 5, 2018

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

screenshot 2018-12-05 11 11 00

It's building successfully now:

screenshot 2018-12-05 11 12 30

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

Trott pushed a commit to Trott/io.js that referenced this pull request Dec 5, 2018
5e80a9a introduced check-xz, using `[[ .. ]]` syntax, but this is a
bash builtin and some platforms default to `sh` when doing
`$(shell ...)` in Makefiles.

Fix is to make it sh friendly.

Ref: nodejs#24551

PR-URL: nodejs#24841
Refs: nodejs#24551
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Richard Lau <riclau@uk.ibm.com>
BridgeAR pushed a commit that referenced this pull request Dec 5, 2018
PR-URL: #24551
Reviewed-By: Refael Ackermann <refack@gmail.com>
Reviewed-By: Richard Lau <riclau@uk.ibm.com>
BridgeAR pushed a commit that referenced this pull request Dec 5, 2018
5e80a9a introduced check-xz, using `[[ .. ]]` syntax, but this is a
bash builtin and some platforms default to `sh` when doing
`$(shell ...)` in Makefiles.

Fix is to make it sh friendly.

Ref: #24551

PR-URL: #24841
Refs: #24551
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Richard Lau <riclau@uk.ibm.com>
@BridgeAR BridgeAR mentioned this pull request Dec 5, 2018
4 tasks
BridgeAR pushed a commit that referenced this pull request Dec 7, 2018
PR-URL: #24551
Reviewed-By: Refael Ackermann <refack@gmail.com>
Reviewed-By: Richard Lau <riclau@uk.ibm.com>
BridgeAR pushed a commit that referenced this pull request Dec 7, 2018
5e80a9a introduced check-xz, using `[[ .. ]]` syntax, but this is a
bash builtin and some platforms default to `sh` when doing
`$(shell ...)` in Makefiles.

Fix is to make it sh friendly.

Ref: #24551

PR-URL: #24841
Refs: #24551
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Richard Lau <riclau@uk.ibm.com>
BridgeAR pushed a commit that referenced this pull request Dec 7, 2018
PR-URL: #24551
Reviewed-By: Refael Ackermann <refack@gmail.com>
Reviewed-By: Richard Lau <riclau@uk.ibm.com>
BridgeAR pushed a commit that referenced this pull request Dec 7, 2018
5e80a9a introduced check-xz, using `[[ .. ]]` syntax, but this is a
bash builtin and some platforms default to `sh` when doing
`$(shell ...)` in Makefiles.

Fix is to make it sh friendly.

Ref: #24551

PR-URL: #24841
Refs: #24551
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Richard Lau <riclau@uk.ibm.com>
MylesBorins pushed a commit that referenced this pull request Dec 26, 2018
PR-URL: #24575
Refs: #24551
Refs: #12958
Refs: #12957
Refs: #8325
Reviewed-By: Refael Ackermann <refack@gmail.com>
Reviewed-By: Daniel Bevenius <daniel.bevenius@gmail.com>
Reviewed-By: Richard Lau <riclau@uk.ibm.com>
refack pushed a commit to refack/node that referenced this pull request Jan 14, 2019
PR-URL: nodejs#24575
Refs: nodejs#24551
Refs: nodejs#12958
Refs: nodejs#12957
Refs: nodejs#8325
Reviewed-By: Refael Ackermann <refack@gmail.com>
Reviewed-By: Daniel Bevenius <daniel.bevenius@gmail.com>
Reviewed-By: Richard Lau <riclau@uk.ibm.com>
refack pushed a commit to refack/node that referenced this pull request Jan 14, 2019
PR-URL: nodejs#24551
Reviewed-By: Refael Ackermann <refack@gmail.com>
Reviewed-By: Richard Lau <riclau@uk.ibm.com>
refack pushed a commit to refack/node that referenced this pull request Jan 14, 2019
5e80a9a introduced check-xz, using `[[ .. ]]` syntax, but this is a
bash builtin and some platforms default to `sh` when doing
`$(shell ...)` in Makefiles.

Fix is to make it sh friendly.

Ref: nodejs#24551

PR-URL: nodejs#24841
Refs: nodejs#24551
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Richard Lau <riclau@uk.ibm.com>
BethGriggs pushed a commit that referenced this pull request Feb 12, 2019
PR-URL: #24551
Reviewed-By: Refael Ackermann <refack@gmail.com>
Reviewed-By: Richard Lau <riclau@uk.ibm.com>
BethGriggs pushed a commit that referenced this pull request Feb 12, 2019
5e80a9a introduced check-xz, using `[[ .. ]]` syntax, but this is a
bash builtin and some platforms default to `sh` when doing
`$(shell ...)` in Makefiles.

Fix is to make it sh friendly.

Ref: #24551

PR-URL: #24841
Refs: #24551
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Richard Lau <riclau@uk.ibm.com>
@BethGriggs BethGriggs mentioned this pull request Feb 12, 2019
BethGriggs pushed a commit that referenced this pull request Feb 20, 2019
PR-URL: #24551
Reviewed-By: Refael Ackermann <refack@gmail.com>
Reviewed-By: Richard Lau <riclau@uk.ibm.com>
BethGriggs pushed a commit that referenced this pull request Feb 20, 2019
5e80a9a introduced check-xz, using `[[ .. ]]` syntax, but this is a
bash builtin and some platforms default to `sh` when doing
`$(shell ...)` in Makefiles.

Fix is to make it sh friendly.

Ref: #24551

PR-URL: #24841
Refs: #24551
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Richard Lau <riclau@uk.ibm.com>
rvagg added a commit that referenced this pull request Feb 28, 2019
PR-URL: #24551
Reviewed-By: Refael Ackermann <refack@gmail.com>
Reviewed-By: Richard Lau <riclau@uk.ibm.com>
rvagg added a commit that referenced this pull request Feb 28, 2019
5e80a9a introduced check-xz, using `[[ .. ]]` syntax, but this is a
bash builtin and some platforms default to `sh` when doing
`$(shell ...)` in Makefiles.

Fix is to make it sh friendly.

Ref: #24551

PR-URL: #24841
Refs: #24551
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Richard Lau <riclau@uk.ibm.com>
rvagg added a commit that referenced this pull request Feb 28, 2019
PR-URL: #24575
Refs: #24551
Refs: #12958
Refs: #12957
Refs: #8325
Reviewed-By: Refael Ackermann <refack@gmail.com>
Reviewed-By: Daniel Bevenius <daniel.bevenius@gmail.com>
Reviewed-By: Richard Lau <riclau@uk.ibm.com>
BethGriggs pushed a commit that referenced this pull request Mar 7, 2019
PR-URL: #24575
Refs: #24551
Refs: #12958
Refs: #12957
Refs: #8325
Reviewed-By: Refael Ackermann <refack@gmail.com>
Reviewed-By: Daniel Bevenius <daniel.bevenius@gmail.com>
Reviewed-By: Richard Lau <riclau@uk.ibm.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
aix Issues and PRs related to the AIX platform. author ready PRs that have at least one approval, no pending requests for changes, and a CI started. build Issues and PRs related to build files or the CI.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants