GV

GeezerVison

The World Wide Web, restored to 800 pixels of glory.

Home|About|Gaming cabinet|Archives|How it works|Starter sites

On this site

  • Sign in
  • ActionsCI/CD and automation
  • CodespacesCloud development environments
  • All Features
  • Documentation
  • Training
  • Certifications
  • Build with Copilot
  • Customize Copilot
  • Integrations
  • Trending repos
  • GitHub Sponsors
  • Developer articles
  • Enterprises

This view

  • Original page
  • New address

GeezerVison is showing github.com — tools: remove bashisms from release script by aduh95 · Pull Request #36123 · nodejs/node

Conversation

@aduh95

@aduh95 aduh95 commented Nov 14, 2020

Copy link
Copy Markdown
Contributor

In preparation for #36099.

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

bricss reacted with eyes emoji
All reactions
  • 👀 1 reaction
@aduh95
aduh95 force-pushed the release-script-bashisms branch from b14fde0 to 2f9d621 Compare November 14, 2020 18:52
@nodejs-github-bot nodejs-github-bot added the tools Issues and PRs related to the tools directory. label Nov 14, 2020
@aduh95 aduh95 mentioned this pull request Nov 14, 2020
tools: lint shell scripts #36099
Closed
8 tasks done
ExE-Boss
ExE-Boss reviewed Nov 15, 2020
View reviewed changes
Comment thread tools/release.sh Resolved

Uh oh!

There was an error while loading. Please reload this page.

benjamingr
benjamingr reviewed Nov 15, 2020
View reviewed changes
Comment thread tools/release.sh Resolved

Uh oh!

There was an error while loading. Please reload this page.

benjamingr
benjamingr reviewed Nov 15, 2020
View reviewed changes
Comment thread tools/release.sh Resolved

Uh oh!

There was an error while loading. Please reload this page.

benjamingr
benjamingr reviewed Nov 15, 2020
View reviewed changes
Comment thread tools/release.sh Resolved

Uh oh!

There was an error while loading. Please reload this page.

benjamingr
benjamingr reviewed Nov 15, 2020
View reviewed changes
Comment thread tools/release.sh Resolved

Uh oh!

There was an error while loading. Please reload this page.

@benjamingr

benjamingr commented Nov 15, 2020

Copy link
Copy Markdown
Member

@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 ^^

ExE-Boss and aduh95 reacted with thumbs up emoji
All reactions
  • 👍 2 reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@aduh95

aduh95 commented Nov 19, 2020

Copy link
Copy Markdown
Contributor Author

@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.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@aduh95
aduh95 requested a review from benjamingr December 2, 2020 18:31
benjamingr
benjamingr approved these changes Dec 2, 2020
View reviewed changes
@aduh95 aduh95 added author ready PRs with CI started, the required approvals, and no outstanding review comments. request-ci Add this label to start a Jenkins CI on a PR. Only starts once the PR has an approving review. review wanted PRs that need review. labels Dec 2, 2020
@github-actions github-actions Bot removed the request-ci Add this label to start a Jenkins CI on a PR. Only starts once the PR has an approving review. label Dec 2, 2020
@nodejs-github-bot

nodejs-github-bot commented Dec 2, 2020

Copy link
Copy Markdown
Collaborator

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

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@aduh95

aduh95 commented Dec 2, 2020

Copy link
Copy Markdown
Contributor Author

@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.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

benjamingr
benjamingr reviewed Dec 2, 2020
View reviewed changes
Comment thread tools/release.sh Resolved

Uh oh!

There was an error while loading. Please reload this page.

aduh95
aduh95 commented Dec 2, 2020
View reviewed changes
Comment thread tools/release.sh Resolved

Uh oh!

There was an error while loading. Please reload this page.

benjamingr
benjamingr reviewed Dec 2, 2020
View reviewed changes
Comment thread tools/release.sh Outdated

@benjamingr benjamingr Dec 2, 2020

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

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.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

@aduh95 aduh95 Dec 2, 2020 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Contributor Author

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

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).

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

@benjamingr benjamingr Dec 3, 2020

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

@aduh95 aduh95 Dec 3, 2020 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Contributor Author

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

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.

Uh oh!

There was an error while loading. Please reload this page.

benjamingr reacted with thumbs up emoji
All reactions
  • 👍 1 reaction
aduh95
aduh95 commented Dec 3, 2020
View reviewed changes
Comment thread tools/release.sh Resolved

Uh oh!

There was an error while loading. Please reload this page.

@aduh95 aduh95 added the request-ci Add this label to start a Jenkins CI on a PR. Only starts once the PR has an approving review. label Dec 3, 2020
@github-actions github-actions Bot removed the request-ci Add this label to start a Jenkins CI on a PR. Only starts once the PR has an approving review. label Dec 3, 2020
@nodejs-github-bot

This comment has been minimized.

Sign in to view
@nodejs-github-bot

This comment has been minimized.

Sign in to view
@nodejs-github-bot

nodejs-github-bot commented Dec 4, 2020

Copy link
Copy Markdown
Collaborator

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

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@nodejs-github-bot

nodejs-github-bot commented Dec 4, 2020

Copy link
Copy Markdown
Collaborator

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

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@nodejs-github-bot

nodejs-github-bot commented Dec 4, 2020

Copy link
Copy Markdown
Collaborator

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

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

aduh95 added a commit to aduh95/node that referenced this pull request Dec 4, 2020
@aduh95
tools: remove bashisms from release script
1729ba7
PR-URL: nodejs#36123
Reviewed-By: Benjamin Gruenbaum <[email protected]>
@aduh95 aduh95 closed this Dec 4, 2020
@aduh95
aduh95 force-pushed the release-script-bashisms branch from defaf0d to 1729ba7 Compare December 4, 2020 22:55
@aduh95

aduh95 commented Dec 4, 2020

Copy link
Copy Markdown
Contributor Author

Landed in 1729ba7

benjamingr reacted with hooray emoji
All reactions
  • 🎉 1 reaction

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@aduh95
aduh95 deleted the release-script-bashisms branch December 4, 2020 22:56
danielleadams pushed a commit that referenced this pull request Dec 7, 2020
@aduh95 @danielleadams
tools: remove bashisms from release script
2868ffb
PR-URL: #36123
Reviewed-By: Benjamin Gruenbaum <[email protected]>
@danielleadams danielleadams mentioned this pull request Dec 7, 2020
v15.4.0 proposal #36435
Merged
@BethGriggs

BethGriggs commented Dec 16, 2020

Copy link
Copy Markdown
Member

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
[email protected]'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

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@aduh95 aduh95 mentioned this pull request Dec 16, 2020
tools: fix release script #36540
Closed
2 tasks done
@rvagg

rvagg commented Dec 17, 2020

Copy link
Copy Markdown
Member

+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.

ExE-Boss reacted with thumbs down emoji
All reactions
  • 👎 1 reaction

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@MylesBorins

MylesBorins commented Dec 17, 2020

Copy link
Copy Markdown
Contributor

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

BethGriggs and ExE-Boss reacted with thumbs up emoji
All reactions
  • 👍 2 reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

nodejs-github-bot pushed a commit that referenced this pull request Dec 17, 2020
@aduh95 @nodejs-github-bot
tools: fix release script
f28715d
PR-URL: #36540
Refs: #36123
Reviewed-By: Richard Lau <[email protected]>
Reviewed-By: Beth Griggs <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
Reviewed-By: Myles Borins <[email protected]>
targos pushed a commit that referenced this pull request Dec 21, 2020
@aduh95 @targos
tools: fix release script
27260c7
PR-URL: #36540
Refs: #36123
Reviewed-By: Richard Lau <[email protected]>
Reviewed-By: Beth Griggs <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
Reviewed-By: Myles Borins <[email protected]>
targos pushed a commit that referenced this pull request May 1, 2021
@aduh95 @targos
tools: remove bashisms from release script
9434bb3
PR-URL: #36123
Reviewed-By: Benjamin Gruenbaum <[email protected]>
targos pushed a commit that referenced this pull request May 1, 2021
@aduh95 @targos
tools: fix release script
471da7d
PR-URL: #36540
Refs: #36123
Reviewed-By: Richard Lau <[email protected]>
Reviewed-By: Beth Griggs <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
Reviewed-By: Myles Borins <[email protected]>
@danielleadams danielleadams mentioned this pull request May 3, 2021
v14.17.0 proposal #38507
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@benjamingr benjamingr benjamingr approved these changes
+1 more reviewer
@ExE-Boss ExE-Boss ExE-Boss left review comments
Reviewers whose approvals may not affect merge requirements

Uh oh!

There was an error while loading. Please reload this page.

Assignees

No one assigned

Labels

author ready PRs with CI started, the required approvals, and no outstanding review comments. review wanted PRs that need review. tools Issues and PRs related to the tools directory.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Uh oh!

There was an error while loading. Please reload this page.

7 participants

@aduh95 @benjamingr @nodejs-github-bot @BethGriggs @rvagg @MylesBorins @ExE-Boss
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
You are visitor 01402503 — GeezerVison does not store the pages you look up.
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.