| FazBrowse GitHub Viewer | Trending | | Home |
| Tools: [Download Repo ZIP] [Original HTTPS Page] |
|
@aduh95 note that shells are weird (feel free to ignore if you already know all this):
Personally I would avoid bash for pretty much anything and rewrite this code in Node or Python ^^ |
Sorry, something went wrong.
|
@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. |
Sorry, something went wrong.
Sorry, something went wrong.
|
@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. |
Sorry, something went wrong.
There was a problem hiding this comment.
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)
Sorry, something went wrong.
There was a problem hiding this comment.
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"
falseAlso we have to take in consideration the case where the user provides 0 (the numbering starts at 1).
Sorry, something went wrong.
There was a problem hiding this comment.
Then probably just the first two bits can be ((keynum > 0)) ?
Sorry, something went wrong.
There was a problem hiding this comment.
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
Sorry, something went wrong.
Sorry, something went wrong.
Sorry, something went wrong.
Sorry, something went wrong.
PR-URL: nodejs#36123 Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
PR-URL: #36123 Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
|
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 |
Sorry, something went wrong.
|
+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. |
Sorry, something went wrong.
PR-URL: #36123 Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
| Back | FazBrowse Home | New Git URL |
In preparation for #36099.
Checklist