tools: remove bashisms from release script

[aduh95]

In preparation for #36099.

Checklist

• make -j4 test (UNIX), or vcbuild test (Windows) passes
• commit message follows commit guidelines

[benjamingr]

@aduh95 note that shells are weird (feel free to ignore if you already know all this):

• Shells decide how many arguments a command gets based on spaces - shells split the command based on spaces (assuming "IFS" is not defined) and then pass them as arguments. The reason stuff is usually put in double quotes is to prevent this. This is called word splitting.
• Usually $foo and ${foo} mean the exact same thing. The reason you do "$foo" is to avoid the word-splitting mentioned above (so that it doesn't become several arguments if it contains a space). See parameter expansion. There are cases you need to do ${foo} e.g. arrays or if you want to do substitutions.
• test and []s are just aliases and are called the classic test command, an if just looks at exit code so when you have a command returning an exit code it can be avoided. In general conditional expressions are better.

Personally I would avoid bash for pretty much anything and rewrite this code in Node or Python ^^

[aduh95]

@codebytere may I ask you to use next week release as a test for those changes? I mean, if the code does look good to you of course.

[nodejs-github-bot]

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

[aduh95]

@nodejs/build just a heads up if anyone wants to review or block this before it lands, otherwise I'm planning to merge this later this week.

[benjamingr]

Are integer expressions posix?

If they are (I think so) - this [ -z "${keynum##*[!0-9]*}" ] || [ "$keynum" -le 0 ] || [ "$keynum" -gt "$keycount" ] can probably be ((keynum > keycount)) (since keycount is known to be an int)

[aduh95]

Hum are you sure? If keynum is not a number, it won't be true:

$ keycount = 3
$ keynum="test"
$ (( keynum > keycount )) && echo "true"
$ (( keynum > keycount )) || echo "false"
false

Also we have to take in consideration the case where the user provides 0 (the numbering starts at 1).

[benjamingr]

Then probably just the first two bits can be ((keynum > 0)) ?

[aduh95]

The issue is that the behaviour is kind of surprising if keynum is not digits-only:

$ sh -c "keynum=;((keynum > 0)) || echo 'false'" # false, as expected
false
$ sh -c "keynum=1;((keynum > 0)) || echo 'false'" # true, as expected
$ sh -c "keynum="12d";((keynum > 0)) || echo 'false'" # false, but prints a warning to stderr
sh: ((: 12d: value too great for base (error token is "12d")
false
$ sh -c "keynum="0xa";((keynum > 0)) || echo 'false'" # true, expected false

[nodejs-github-bot]

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

[nodejs-github-bot]

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

[nodejs-github-bot]

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

[aduh95]

Landed in 1729ba7

[BethGriggs]

I think this PR has introduced some problems with the tools/release.sh script. I was unable to promote #36476 with this version of the script.

I run the script with ./tools/release.sh -i <key>, and I get the following output after selecting the key:

Select a key: 1

gpg: error reading key: No public key
<!--lint disable no-literal-urls-->
<p align="center">
  <a href="https://nodejs.org/">
    <img
      alt="Node.js"
      src="https://nodejs.org/static/images/logo-light.svg"
      width="400"
    />
  </a>
</p>

Node.js is an open-source, cross-platform, JavaScript runtime environment. It
executes JavaScript code outside of a browser. For more information on using
Node.js, see the [Node.js Website][].

... # Node.js README output continued

# Checking for releases ...
Warning: Identity file  <key1> not accessible: No such file or directory.
no such identity: <key2>: No such file or directory
dist@direct.nodejs.org's password: 

I'll possibly open a revert PR prior to the next releases going out, but will hold off in case there's a quick/easy fix.

/cc @nodejs/releasers

[rvagg]

+1 to revert. I've tried to look over 1729ba7 for obvious problems but it's far too heavy-handed for the brainpower I have available for this. This whole "remove bashisms" is silliness IMO and is primarily an exercise in introducing churn and risk (stability, security, and other) because the nature of Bash is to be obscure when terse, which you can see on display in 1729ba7.

[MylesBorins]

@rvagg we have an open PR that seems to fix the problems. I think it might be reasonable to land that change, see if it fixes things, and then move to revert if we find other regressions

Thoughts?