build: update binary-upload to use correct tarball name

[sxa]

The binary-upload target in the top level Makefile uses $(TARNAME)-$(OSTYPE)-$(ARCH).tar as the name to upload whereas it is created by the $(BINARYTAR) target as $(BINARYNAME).tar. Since BINARYNAME includes the optional VARIATION (set to musl) to distinguish it from linux-x64 this is missing during the binary-upload target, for example during a release build for Alpine/musl causing the error described in nodejs/build#4423 (comment)

This changes the binary-upload target to use the same variable for the tarball that is used when the file is created so there is consistency.

[sxa]

Confirmed that it works with the test build at https://ci-release.nodejs.org/job/iojs+release/nodes=alpine-x64-release/12016/console

Noting that there may be further issues if the promotion step has any problems with the filenames including the extra VARIATION section.

[nodejs-github-bot]

CI: https://ci.nodejs.org/job/node-test-pull-request/75837/

[sxa]

Ping @nodejs/releasers for review in case there's anything I might have missed with this change or if there are other regexs etc. which may need to be changed elsewhere to handle this.

Noting that this is likely still a reasonable change even if other things are required as it does not seem to have a negative impact on any existing releases from what I can see as the filenames are as they were before - BINARYNAME is defined at

node/Makefile

Line 1108 in 9e23066

ifdefVARIATION

and the iojs+release job does not use VARIATION anywhere other than the new code added for Alpine.

[nodejs-github-bot]

CI: https://ci.nodejs.org/job/node-test-pull-request/75838/

[nodejs-github-bot]

CI: https://ci.nodejs.org/job/node-test-pull-request/75839/

[richardlau]

(Not a blocking comment).

Noting here that $(BINARYNAME) is defined as

node/Makefile

Lines 1108 to 1112 in 9e23066

	ifdefVARIATION
	BINARYNAME=$(TARNAME)-$(PLATFORM)-$(ARCH)-$(VARIATION)
	else
	BINARYNAME=$(TARNAME)-$(PLATFORM)-$(ARCH)
	endif

whereas what is being replaced is

$(TARNAME)-$(OSTYPE)-$(ARCH)

$(OSTYPE) is defined as

node/Makefile

Lines 13 to 16 in 9e23066

	OSTYPE := $(shell uname -s | tr '[:upper:]' '[:lower:]')
	ifeq ($(findstring os/390,$OSTYPE),os/390)
	OSTYPE ?= os390
	endif

$(PLATFORM) is defined as

node/Makefile

Lines 1001 to 1004 in 9e23066

	PLATFORM=$(shell uname | tr '[:upper:]' '[:lower:]')
	ifeq ($(findstring os/390,$PLATFORM),os/390)
	PLATFORM ?= os390
	endif

i.e. they happen to have the same definition (as uname without parameters behaves as uname -s) which is how this has been working up until now 😅.

Using $(BINARYNAME) for consistency with what was created is much safer than the above, so +1.

[sxa]

As per the comments at nodejs/build#4423 (comment) we will need to get this backported to the release lines ASAP if we want to make this live. Once this is approved and merged in main I would recommend that we bypass the usual two week period for backporting, especially given that this is not a functional change to the runtime.

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 90.31%. Comparing base (91a99c5) to head (f570c90).
⚠️ Report is 3 commits behind head on main.

Additional details and impacted files

@@ Coverage Diff @@## main #65282 +/- ##
==========================================
- Coverage 90.32% 90.31% -0.02% 
==========================================
Files 751 751 Lines 250000 250000 Branches 47231 47230 -1 ==========================================
- Hits 225816 225788 -28 - Misses 15566 15620 +54 + Partials 8618 8592 -26 

see 35 files with indirect coverage changes

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
• 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

[richardlau]

As per the comments at nodejs/build#4423 (comment) we will need to get this backported to the release lines ASAP if we want to make this live. Once this is approved and merged in main I would recommend that we bypass the usual two week period for backporting, especially given that this is not a functional change to the runtime.

@nodejs/releasers thoughts? ☝️

[nodejs-github-bot]

Landed in ad7a5b8

[sxa]

No objectins after a day from the release team to the above comment or in slack so I've backported and verified with Alpine (alpine-x64-release label) enabled in iojs+release as follows:

Branch	Commit	iojs+release test job
v26.x-staging	9dafc78cdf	12028
v24.x-staging	e2e5c4fe88	12027
v22.x-staging	e21045ec51	12025

[sxa]

The binary-upload target in the top level Makefile uses $(TARNAME)-$(OSTYPE)-$(ARCH).tar as the name to upload whereas it is created by the $(BINARYTAR) target as $(BINARYNAME).tar. Since BINARYNAME includes the optional VARIATION (set to musl) to distinguish it from linux-x64 this is missing during the binary-upload target, for example during a release build for Alpine/musl causing the error described in nodejs/build#4423 (comment)

This changes the binary-upload target to use the same variable for the tarball that is used when the file is created so there is consistency.

[sxa]

Confirmed that it works with the test build at https://ci-release.nodejs.org/job/iojs+release/nodes=alpine-x64-release/12016/console

Noting that there may be further issues if the promotion step has any problems with the filenames including the extra VARIATION section.

[nodejs-github-bot]

CI: https://ci.nodejs.org/job/node-test-pull-request/75837/

[sxa]

Ping @nodejs/releasers for review in case there's anything I might have missed with this change or if there are other regexs etc. which may need to be changed elsewhere to handle this.

Noting that this is likely still a reasonable change even if other things are required as it does not seem to have a negative impact on any existing releases from what I can see as the filenames are as they were before - BINARYNAME is defined at

node/Makefile

Line 1108 in 9e23066

ifdefVARIATION

and the iojs+release job does not use VARIATION anywhere other than the new code added for Alpine.

[nodejs-github-bot]

CI: https://ci.nodejs.org/job/node-test-pull-request/75838/

[nodejs-github-bot]

CI: https://ci.nodejs.org/job/node-test-pull-request/75839/

[richardlau]

(Not a blocking comment).

Noting here that $(BINARYNAME) is defined as

node/Makefile

Lines 1108 to 1112 in 9e23066

	ifdefVARIATION
	BINARYNAME=$(TARNAME)-$(PLATFORM)-$(ARCH)-$(VARIATION)
	else
	BINARYNAME=$(TARNAME)-$(PLATFORM)-$(ARCH)
	endif

whereas what is being replaced is

$(TARNAME)-$(OSTYPE)-$(ARCH)

$(OSTYPE) is defined as

node/Makefile

Lines 13 to 16 in 9e23066

	OSTYPE := $(shell uname -s | tr '[:upper:]' '[:lower:]')
	ifeq ($(findstring os/390,$OSTYPE),os/390)
	OSTYPE ?= os390
	endif

$(PLATFORM) is defined as

node/Makefile

Lines 1001 to 1004 in 9e23066

	PLATFORM=$(shell uname | tr '[:upper:]' '[:lower:]')
	ifeq ($(findstring os/390,$PLATFORM),os/390)
	PLATFORM ?= os390
	endif

i.e. they happen to have the same definition (as uname without parameters behaves as uname -s) which is how this has been working up until now 😅.

Using $(BINARYNAME) for consistency with what was created is much safer than the above, so +1.

[sxa]

As per the comments at nodejs/build#4423 (comment) we will need to get this backported to the release lines ASAP if we want to make this live. Once this is approved and merged in main I would recommend that we bypass the usual two week period for backporting, especially given that this is not a functional change to the runtime.

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 90.31%. Comparing base (91a99c5) to head (f570c90).
⚠️ Report is 3 commits behind head on main.

Additional details and impacted files

@@ Coverage Diff @@## main #65282 +/- ##
==========================================
- Coverage 90.32% 90.31% -0.02% 
==========================================
Files 751 751 Lines 250000 250000 Branches 47231 47230 -1 ==========================================
- Hits 225816 225788 -28 - Misses 15566 15620 +54 + Partials 8618 8592 -26 

see 35 files with indirect coverage changes

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
• 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

[richardlau]

As per the comments at nodejs/build#4423 (comment) we will need to get this backported to the release lines ASAP if we want to make this live. Once this is approved and merged in main I would recommend that we bypass the usual two week period for backporting, especially given that this is not a functional change to the runtime.

@nodejs/releasers thoughts? ☝️

[nodejs-github-bot]

Landed in ad7a5b8

[sxa]

No objectins after a day from the release team to the above comment or in slack so I've backported and verified with Alpine (alpine-x64-release label) enabled in iojs+release as follows:

Branch	Commit	iojs+release test job
v26.x-staging	9dafc78cdf	12028
v24.x-staging	e2e5c4fe88	12027
v22.x-staging	e21045ec51	12025

[sxa]

The binary-upload target in the top level Makefile uses $(TARNAME)-$(OSTYPE)-$(ARCH).tar as the name to upload whereas it is created by the $(BINARYTAR) target as $(BINARYNAME).tar. Since BINARYNAME includes the optional VARIATION (set to musl) to distinguish it from linux-x64 this is missing during the binary-upload target, for example during a release build for Alpine/musl causing the error described in nodejs/build#4423 (comment)

This changes the binary-upload target to use the same variable for the tarball that is used when the file is created so there is consistency.

[sxa]

Confirmed that it works with the test build at https://ci-release.nodejs.org/job/iojs+release/nodes=alpine-x64-release/12016/console

Noting that there may be further issues if the promotion step has any problems with the filenames including the extra VARIATION section.

[nodejs-github-bot]

CI: https://ci.nodejs.org/job/node-test-pull-request/75837/

[sxa]

Ping @nodejs/releasers for review in case there's anything I might have missed with this change or if there are other regexs etc. which may need to be changed elsewhere to handle this.

Noting that this is likely still a reasonable change even if other things are required as it does not seem to have a negative impact on any existing releases from what I can see as the filenames are as they were before - BINARYNAME is defined at

node/Makefile

Line 1108 in 9e23066

ifdefVARIATION

and the iojs+release job does not use VARIATION anywhere other than the new code added for Alpine.

[nodejs-github-bot]

CI: https://ci.nodejs.org/job/node-test-pull-request/75838/

[nodejs-github-bot]

CI: https://ci.nodejs.org/job/node-test-pull-request/75839/

[richardlau]

(Not a blocking comment).

Noting here that $(BINARYNAME) is defined as

node/Makefile

Lines 1108 to 1112 in 9e23066

	ifdefVARIATION
	BINARYNAME=$(TARNAME)-$(PLATFORM)-$(ARCH)-$(VARIATION)
	else
	BINARYNAME=$(TARNAME)-$(PLATFORM)-$(ARCH)
	endif

whereas what is being replaced is

$(TARNAME)-$(OSTYPE)-$(ARCH)

$(OSTYPE) is defined as

node/Makefile

Lines 13 to 16 in 9e23066

	OSTYPE := $(shell uname -s | tr '[:upper:]' '[:lower:]')
	ifeq ($(findstring os/390,$OSTYPE),os/390)
	OSTYPE ?= os390
	endif

$(PLATFORM) is defined as

node/Makefile

Lines 1001 to 1004 in 9e23066

	PLATFORM=$(shell uname | tr '[:upper:]' '[:lower:]')
	ifeq ($(findstring os/390,$PLATFORM),os/390)
	PLATFORM ?= os390
	endif

i.e. they happen to have the same definition (as uname without parameters behaves as uname -s) which is how this has been working up until now 😅.

Using $(BINARYNAME) for consistency with what was created is much safer than the above, so +1.

[sxa]

As per the comments at nodejs/build#4423 (comment) we will need to get this backported to the release lines ASAP if we want to make this live. Once this is approved and merged in main I would recommend that we bypass the usual two week period for backporting, especially given that this is not a functional change to the runtime.

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 90.31%. Comparing base (91a99c5) to head (f570c90).
⚠️ Report is 3 commits behind head on main.

Additional details and impacted files

@@ Coverage Diff @@## main #65282 +/- ##
==========================================
- Coverage 90.32% 90.31% -0.02% 
==========================================
Files 751 751 Lines 250000 250000 Branches 47231 47230 -1 ==========================================
- Hits 225816 225788 -28 - Misses 15566 15620 +54 + Partials 8618 8592 -26 

see 35 files with indirect coverage changes

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
• 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

[richardlau]

As per the comments at nodejs/build#4423 (comment) we will need to get this backported to the release lines ASAP if we want to make this live. Once this is approved and merged in main I would recommend that we bypass the usual two week period for backporting, especially given that this is not a functional change to the runtime.

@nodejs/releasers thoughts? ☝️

[nodejs-github-bot]

Landed in ad7a5b8

[sxa]

No objectins after a day from the release team to the above comment or in slack so I've backported and verified with Alpine (alpine-x64-release label) enabled in iojs+release as follows:

Branch	Commit	iojs+release test job
v26.x-staging	9dafc78cdf	12028
v24.x-staging	e2e5c4fe88	12027
v22.x-staging	e21045ec51	12025

[sxa]

The binary-upload target in the top level Makefile uses $(TARNAME)-$(OSTYPE)-$(ARCH).tar as the name to upload whereas it is created by the $(BINARYTAR) target as $(BINARYNAME).tar. Since BINARYNAME includes the optional VARIATION (set to musl) to distinguish it from linux-x64 this is missing during the binary-upload target, for example during a release build for Alpine/musl causing the error described in nodejs/build#4423 (comment)

This changes the binary-upload target to use the same variable for the tarball that is used when the file is created so there is consistency.

[sxa]

Confirmed that it works with the test build at https://ci-release.nodejs.org/job/iojs+release/nodes=alpine-x64-release/12016/console

Noting that there may be further issues if the promotion step has any problems with the filenames including the extra VARIATION section.

[nodejs-github-bot]

CI: https://ci.nodejs.org/job/node-test-pull-request/75837/

[sxa]

Ping @nodejs/releasers for review in case there's anything I might have missed with this change or if there are other regexs etc. which may need to be changed elsewhere to handle this.

Noting that this is likely still a reasonable change even if other things are required as it does not seem to have a negative impact on any existing releases from what I can see as the filenames are as they were before - BINARYNAME is defined at

node/Makefile

Line 1108 in 9e23066

ifdefVARIATION

and the iojs+release job does not use VARIATION anywhere other than the new code added for Alpine.

[nodejs-github-bot]

CI: https://ci.nodejs.org/job/node-test-pull-request/75838/

[nodejs-github-bot]

CI: https://ci.nodejs.org/job/node-test-pull-request/75839/

[richardlau]

(Not a blocking comment).

Noting here that $(BINARYNAME) is defined as

node/Makefile

Lines 1108 to 1112 in 9e23066

	ifdefVARIATION
	BINARYNAME=$(TARNAME)-$(PLATFORM)-$(ARCH)-$(VARIATION)
	else
	BINARYNAME=$(TARNAME)-$(PLATFORM)-$(ARCH)
	endif

whereas what is being replaced is

$(TARNAME)-$(OSTYPE)-$(ARCH)

$(OSTYPE) is defined as

node/Makefile

Lines 13 to 16 in 9e23066

	OSTYPE := $(shell uname -s | tr '[:upper:]' '[:lower:]')
	ifeq ($(findstring os/390,$OSTYPE),os/390)
	OSTYPE ?= os390
	endif

$(PLATFORM) is defined as

node/Makefile

Lines 1001 to 1004 in 9e23066

	PLATFORM=$(shell uname | tr '[:upper:]' '[:lower:]')
	ifeq ($(findstring os/390,$PLATFORM),os/390)
	PLATFORM ?= os390
	endif

i.e. they happen to have the same definition (as uname without parameters behaves as uname -s) which is how this has been working up until now 😅.

Using $(BINARYNAME) for consistency with what was created is much safer than the above, so +1.

[sxa]

As per the comments at nodejs/build#4423 (comment) we will need to get this backported to the release lines ASAP if we want to make this live. Once this is approved and merged in main I would recommend that we bypass the usual two week period for backporting, especially given that this is not a functional change to the runtime.

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 90.31%. Comparing base (91a99c5) to head (f570c90).
⚠️ Report is 3 commits behind head on main.

Additional details and impacted files

@@ Coverage Diff @@## main #65282 +/- ##
==========================================
- Coverage 90.32% 90.31% -0.02% 
==========================================
Files 751 751 Lines 250000 250000 Branches 47231 47230 -1 ==========================================
- Hits 225816 225788 -28 - Misses 15566 15620 +54 + Partials 8618 8592 -26 

see 35 files with indirect coverage changes

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
• 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

[richardlau]

As per the comments at nodejs/build#4423 (comment) we will need to get this backported to the release lines ASAP if we want to make this live. Once this is approved and merged in main I would recommend that we bypass the usual two week period for backporting, especially given that this is not a functional change to the runtime.

@nodejs/releasers thoughts? ☝️

[nodejs-github-bot]

Landed in ad7a5b8

[sxa]

No objectins after a day from the release team to the above comment or in slack so I've backported and verified with Alpine (alpine-x64-release label) enabled in iojs+release as follows:

Branch	Commit	iojs+release test job
v26.x-staging	9dafc78cdf	12028
v24.x-staging	e2e5c4fe88	12027
v22.x-staging	e21045ec51	12025

[sxa]

The binary-upload target in the top level Makefile uses $(TARNAME)-$(OSTYPE)-$(ARCH).tar as the name to upload whereas it is created by the $(BINARYTAR) target as $(BINARYNAME).tar. Since BINARYNAME includes the optional VARIATION (set to musl) to distinguish it from linux-x64 this is missing during the binary-upload target, for example during a release build for Alpine/musl causing the error described in nodejs/build#4423 (comment)

This changes the binary-upload target to use the same variable for the tarball that is used when the file is created so there is consistency.

[sxa]

Confirmed that it works with the test build at https://ci-release.nodejs.org/job/iojs+release/nodes=alpine-x64-release/12016/console

Noting that there may be further issues if the promotion step has any problems with the filenames including the extra VARIATION section.

[nodejs-github-bot]

CI: https://ci.nodejs.org/job/node-test-pull-request/75837/

[sxa]

Ping @nodejs/releasers for review in case there's anything I might have missed with this change or if there are other regexs etc. which may need to be changed elsewhere to handle this.

Noting that this is likely still a reasonable change even if other things are required as it does not seem to have a negative impact on any existing releases from what I can see as the filenames are as they were before - BINARYNAME is defined at

node/Makefile

Line 1108 in 9e23066

ifdefVARIATION

and the iojs+release job does not use VARIATION anywhere other than the new code added for Alpine.

[nodejs-github-bot]

CI: https://ci.nodejs.org/job/node-test-pull-request/75838/

[nodejs-github-bot]

CI: https://ci.nodejs.org/job/node-test-pull-request/75839/

[richardlau]

(Not a blocking comment).

Noting here that $(BINARYNAME) is defined as

node/Makefile

Lines 1108 to 1112 in 9e23066

	ifdefVARIATION
	BINARYNAME=$(TARNAME)-$(PLATFORM)-$(ARCH)-$(VARIATION)
	else
	BINARYNAME=$(TARNAME)-$(PLATFORM)-$(ARCH)
	endif

whereas what is being replaced is

$(TARNAME)-$(OSTYPE)-$(ARCH)

$(OSTYPE) is defined as

node/Makefile

Lines 13 to 16 in 9e23066

	OSTYPE := $(shell uname -s | tr '[:upper:]' '[:lower:]')
	ifeq ($(findstring os/390,$OSTYPE),os/390)
	OSTYPE ?= os390
	endif

$(PLATFORM) is defined as

node/Makefile

Lines 1001 to 1004 in 9e23066

	PLATFORM=$(shell uname | tr '[:upper:]' '[:lower:]')
	ifeq ($(findstring os/390,$PLATFORM),os/390)
	PLATFORM ?= os390
	endif

i.e. they happen to have the same definition (as uname without parameters behaves as uname -s) which is how this has been working up until now 😅.

Using $(BINARYNAME) for consistency with what was created is much safer than the above, so +1.

[sxa]

As per the comments at nodejs/build#4423 (comment) we will need to get this backported to the release lines ASAP if we want to make this live. Once this is approved and merged in main I would recommend that we bypass the usual two week period for backporting, especially given that this is not a functional change to the runtime.

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 90.31%. Comparing base (91a99c5) to head (f570c90).
⚠️ Report is 3 commits behind head on main.

Additional details and impacted files

@@ Coverage Diff @@## main #65282 +/- ##
==========================================
- Coverage 90.32% 90.31% -0.02% 
==========================================
Files 751 751 Lines 250000 250000 Branches 47231 47230 -1 ==========================================
- Hits 225816 225788 -28 - Misses 15566 15620 +54 + Partials 8618 8592 -26 

see 35 files with indirect coverage changes

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
• 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

[richardlau]

As per the comments at nodejs/build#4423 (comment) we will need to get this backported to the release lines ASAP if we want to make this live. Once this is approved and merged in main I would recommend that we bypass the usual two week period for backporting, especially given that this is not a functional change to the runtime.

@nodejs/releasers thoughts? ☝️

[nodejs-github-bot]

Landed in ad7a5b8

[sxa]

No objectins after a day from the release team to the above comment or in slack so I've backported and verified with Alpine (alpine-x64-release label) enabled in iojs+release as follows:

Branch	Commit	iojs+release test job
v26.x-staging	9dafc78cdf	12028
v24.x-staging	e2e5c4fe88	12027
v22.x-staging	e21045ec51	12025

[sxa]

The binary-upload target in the top level Makefile uses $(TARNAME)-$(OSTYPE)-$(ARCH).tar as the name to upload whereas it is created by the $(BINARYTAR) target as $(BINARYNAME).tar. Since BINARYNAME includes the optional VARIATION (set to musl) to distinguish it from linux-x64 this is missing during the binary-upload target, for example during a release build for Alpine/musl causing the error described in nodejs/build#4423 (comment)

This changes the binary-upload target to use the same variable for the tarball that is used when the file is created so there is consistency.

[sxa]

Confirmed that it works with the test build at https://ci-release.nodejs.org/job/iojs+release/nodes=alpine-x64-release/12016/console

Noting that there may be further issues if the promotion step has any problems with the filenames including the extra VARIATION section.

[nodejs-github-bot]

CI: https://ci.nodejs.org/job/node-test-pull-request/75837/

[sxa]

Ping @nodejs/releasers for review in case there's anything I might have missed with this change or if there are other regexs etc. which may need to be changed elsewhere to handle this.

Noting that this is likely still a reasonable change even if other things are required as it does not seem to have a negative impact on any existing releases from what I can see as the filenames are as they were before - BINARYNAME is defined at

node/Makefile

Line 1108 in 9e23066

ifdefVARIATION

and the iojs+release job does not use VARIATION anywhere other than the new code added for Alpine.

[nodejs-github-bot]

CI: https://ci.nodejs.org/job/node-test-pull-request/75838/

[nodejs-github-bot]

CI: https://ci.nodejs.org/job/node-test-pull-request/75839/

[richardlau]

(Not a blocking comment).

Noting here that $(BINARYNAME) is defined as

node/Makefile

Lines 1108 to 1112 in 9e23066

	ifdefVARIATION
	BINARYNAME=$(TARNAME)-$(PLATFORM)-$(ARCH)-$(VARIATION)
	else
	BINARYNAME=$(TARNAME)-$(PLATFORM)-$(ARCH)
	endif

whereas what is being replaced is

$(TARNAME)-$(OSTYPE)-$(ARCH)

$(OSTYPE) is defined as

node/Makefile

Lines 13 to 16 in 9e23066

	OSTYPE := $(shell uname -s | tr '[:upper:]' '[:lower:]')
	ifeq ($(findstring os/390,$OSTYPE),os/390)
	OSTYPE ?= os390
	endif

$(PLATFORM) is defined as

node/Makefile

Lines 1001 to 1004 in 9e23066

	PLATFORM=$(shell uname | tr '[:upper:]' '[:lower:]')
	ifeq ($(findstring os/390,$PLATFORM),os/390)
	PLATFORM ?= os390
	endif

i.e. they happen to have the same definition (as uname without parameters behaves as uname -s) which is how this has been working up until now 😅.

Using $(BINARYNAME) for consistency with what was created is much safer than the above, so +1.

[sxa]

As per the comments at nodejs/build#4423 (comment) we will need to get this backported to the release lines ASAP if we want to make this live. Once this is approved and merged in main I would recommend that we bypass the usual two week period for backporting, especially given that this is not a functional change to the runtime.

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 90.31%. Comparing base (91a99c5) to head (f570c90).
⚠️ Report is 3 commits behind head on main.

Additional details and impacted files

@@ Coverage Diff @@## main #65282 +/- ##
==========================================
- Coverage 90.32% 90.31% -0.02% 
==========================================
Files 751 751 Lines 250000 250000 Branches 47231 47230 -1 ==========================================
- Hits 225816 225788 -28 - Misses 15566 15620 +54 + Partials 8618 8592 -26 

see 35 files with indirect coverage changes

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
• 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

[richardlau]

As per the comments at nodejs/build#4423 (comment) we will need to get this backported to the release lines ASAP if we want to make this live. Once this is approved and merged in main I would recommend that we bypass the usual two week period for backporting, especially given that this is not a functional change to the runtime.

@nodejs/releasers thoughts? ☝️

[nodejs-github-bot]

Landed in ad7a5b8

[sxa]

No objectins after a day from the release team to the above comment or in slack so I've backported and verified with Alpine (alpine-x64-release label) enabled in iojs+release as follows:

Branch	Commit	iojs+release test job
v26.x-staging	9dafc78cdf	12028
v24.x-staging	e2e5c4fe88	12027
v22.x-staging	e21045ec51	12025

[sxa]

The binary-upload target in the top level Makefile uses $(TARNAME)-$(OSTYPE)-$(ARCH).tar as the name to upload whereas it is created by the $(BINARYTAR) target as $(BINARYNAME).tar. Since BINARYNAME includes the optional VARIATION (set to musl) to distinguish it from linux-x64 this is missing during the binary-upload target, for example during a release build for Alpine/musl causing the error described in nodejs/build#4423 (comment)

This changes the binary-upload target to use the same variable for the tarball that is used when the file is created so there is consistency.

[sxa]

Confirmed that it works with the test build at https://ci-release.nodejs.org/job/iojs+release/nodes=alpine-x64-release/12016/console

Noting that there may be further issues if the promotion step has any problems with the filenames including the extra VARIATION section.

[nodejs-github-bot]

CI: https://ci.nodejs.org/job/node-test-pull-request/75837/

[sxa]

Ping @nodejs/releasers for review in case there's anything I might have missed with this change or if there are other regexs etc. which may need to be changed elsewhere to handle this.

Noting that this is likely still a reasonable change even if other things are required as it does not seem to have a negative impact on any existing releases from what I can see as the filenames are as they were before - BINARYNAME is defined at

node/Makefile

Line 1108 in 9e23066

ifdefVARIATION

and the iojs+release job does not use VARIATION anywhere other than the new code added for Alpine.

[nodejs-github-bot]

CI: https://ci.nodejs.org/job/node-test-pull-request/75838/

[nodejs-github-bot]

CI: https://ci.nodejs.org/job/node-test-pull-request/75839/

[richardlau]

(Not a blocking comment).

Noting here that $(BINARYNAME) is defined as

node/Makefile

Lines 1108 to 1112 in 9e23066

	ifdefVARIATION
	BINARYNAME=$(TARNAME)-$(PLATFORM)-$(ARCH)-$(VARIATION)
	else
	BINARYNAME=$(TARNAME)-$(PLATFORM)-$(ARCH)
	endif

whereas what is being replaced is

$(TARNAME)-$(OSTYPE)-$(ARCH)

$(OSTYPE) is defined as

node/Makefile

Lines 13 to 16 in 9e23066

	OSTYPE := $(shell uname -s | tr '[:upper:]' '[:lower:]')
	ifeq ($(findstring os/390,$OSTYPE),os/390)
	OSTYPE ?= os390
	endif

$(PLATFORM) is defined as

node/Makefile

Lines 1001 to 1004 in 9e23066

	PLATFORM=$(shell uname | tr '[:upper:]' '[:lower:]')
	ifeq ($(findstring os/390,$PLATFORM),os/390)
	PLATFORM ?= os390
	endif

i.e. they happen to have the same definition (as uname without parameters behaves as uname -s) which is how this has been working up until now 😅.

Using $(BINARYNAME) for consistency with what was created is much safer than the above, so +1.

[sxa]

As per the comments at nodejs/build#4423 (comment) we will need to get this backported to the release lines ASAP if we want to make this live. Once this is approved and merged in main I would recommend that we bypass the usual two week period for backporting, especially given that this is not a functional change to the runtime.

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 90.31%. Comparing base (91a99c5) to head (f570c90).
⚠️ Report is 3 commits behind head on main.

Additional details and impacted files

@@ Coverage Diff @@## main #65282 +/- ##
==========================================
- Coverage 90.32% 90.31% -0.02% 
==========================================
Files 751 751 Lines 250000 250000 Branches 47231 47230 -1 ==========================================
- Hits 225816 225788 -28 - Misses 15566 15620 +54 + Partials 8618 8592 -26 

see 35 files with indirect coverage changes

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
• 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

[richardlau]

As per the comments at nodejs/build#4423 (comment) we will need to get this backported to the release lines ASAP if we want to make this live. Once this is approved and merged in main I would recommend that we bypass the usual two week period for backporting, especially given that this is not a functional change to the runtime.

@nodejs/releasers thoughts? ☝️

[nodejs-github-bot]

Landed in ad7a5b8

[sxa]

No objectins after a day from the release team to the above comment or in slack so I've backported and verified with Alpine (alpine-x64-release label) enabled in iojs+release as follows:

Branch	Commit	iojs+release test job
v26.x-staging	9dafc78cdf	12028
v24.x-staging	e2e5c4fe88	12027
v22.x-staging	e21045ec51	12025

[sxa]

The binary-upload target in the top level Makefile uses $(TARNAME)-$(OSTYPE)-$(ARCH).tar as the name to upload whereas it is created by the $(BINARYTAR) target as $(BINARYNAME).tar. Since BINARYNAME includes the optional VARIATION (set to musl) to distinguish it from linux-x64 this is missing during the binary-upload target, for example during a release build for Alpine/musl causing the error described in nodejs/build#4423 (comment)

This changes the binary-upload target to use the same variable for the tarball that is used when the file is created so there is consistency.

[sxa]

Confirmed that it works with the test build at https://ci-release.nodejs.org/job/iojs+release/nodes=alpine-x64-release/12016/console

Noting that there may be further issues if the promotion step has any problems with the filenames including the extra VARIATION section.

[nodejs-github-bot]

CI: https://ci.nodejs.org/job/node-test-pull-request/75837/

[sxa]

Ping @nodejs/releasers for review in case there's anything I might have missed with this change or if there are other regexs etc. which may need to be changed elsewhere to handle this.

Noting that this is likely still a reasonable change even if other things are required as it does not seem to have a negative impact on any existing releases from what I can see as the filenames are as they were before - BINARYNAME is defined at

node/Makefile

Line 1108 in 9e23066

ifdefVARIATION

and the iojs+release job does not use VARIATION anywhere other than the new code added for Alpine.

[nodejs-github-bot]

CI: https://ci.nodejs.org/job/node-test-pull-request/75838/

[nodejs-github-bot]

CI: https://ci.nodejs.org/job/node-test-pull-request/75839/

[richardlau]

(Not a blocking comment).

Noting here that $(BINARYNAME) is defined as

node/Makefile

Lines 1108 to 1112 in 9e23066

	ifdefVARIATION
	BINARYNAME=$(TARNAME)-$(PLATFORM)-$(ARCH)-$(VARIATION)
	else
	BINARYNAME=$(TARNAME)-$(PLATFORM)-$(ARCH)
	endif

whereas what is being replaced is

$(TARNAME)-$(OSTYPE)-$(ARCH)

$(OSTYPE) is defined as

node/Makefile

Lines 13 to 16 in 9e23066

	OSTYPE := $(shell uname -s | tr '[:upper:]' '[:lower:]')
	ifeq ($(findstring os/390,$OSTYPE),os/390)
	OSTYPE ?= os390
	endif

$(PLATFORM) is defined as

node/Makefile

Lines 1001 to 1004 in 9e23066

	PLATFORM=$(shell uname | tr '[:upper:]' '[:lower:]')
	ifeq ($(findstring os/390,$PLATFORM),os/390)
	PLATFORM ?= os390
	endif

i.e. they happen to have the same definition (as uname without parameters behaves as uname -s) which is how this has been working up until now 😅.

Using $(BINARYNAME) for consistency with what was created is much safer than the above, so +1.

[sxa]

As per the comments at nodejs/build#4423 (comment) we will need to get this backported to the release lines ASAP if we want to make this live. Once this is approved and merged in main I would recommend that we bypass the usual two week period for backporting, especially given that this is not a functional change to the runtime.

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 90.31%. Comparing base (91a99c5) to head (f570c90).
⚠️ Report is 3 commits behind head on main.

Additional details and impacted files

@@ Coverage Diff @@## main #65282 +/- ##
==========================================
- Coverage 90.32% 90.31% -0.02% 
==========================================
Files 751 751 Lines 250000 250000 Branches 47231 47230 -1 ==========================================
- Hits 225816 225788 -28 - Misses 15566 15620 +54 + Partials 8618 8592 -26 

see 35 files with indirect coverage changes

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
• 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

[richardlau]

As per the comments at nodejs/build#4423 (comment) we will need to get this backported to the release lines ASAP if we want to make this live. Once this is approved and merged in main I would recommend that we bypass the usual two week period for backporting, especially given that this is not a functional change to the runtime.

@nodejs/releasers thoughts? ☝️

[nodejs-github-bot]

Landed in ad7a5b8

[sxa]

No objectins after a day from the release team to the above comment or in slack so I've backported and verified with Alpine (alpine-x64-release label) enabled in iojs+release as follows:

Branch	Commit	iojs+release test job
v26.x-staging	9dafc78cdf	12028
v24.x-staging	e2e5c4fe88	12027
v22.x-staging	e21045ec51	12025