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

ifdef VARIATION

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

	ifdef VARIATION
	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? ☝️