FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

tools: remove bashisms from macOS release scripts by aduh95 · Pull Request #36121 · nodejs/node · GitHub

/ node Public

tools: remove bashisms from macOS release scripts - #36121

Closed
aduh95 wants to merge 1 commit into
nodejs:masterfrom
aduh95:macos-sh-bashisms
ConversationCommits (1)ChecksFiles changed
Closed

tools: remove bashisms from macOS release scripts#36121
aduh95 wants to merge 1 commit into
nodejs:masterfrom
aduh95:macos-sh-bashisms

Conversation

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

nodejs-github-bot added build Issues and PRs related to build files or the CI. macos Issues and PRs related to the macOS platform / OSX. tools Issues and PRs related to the tools directory. labels Nov 14, 2020
aduh95 mentioned this pull request Nov 14, 2020
Closed
8 tasks

targos commented Nov 15, 2020

Copy link
Copy Markdown
Member

What's the reason for banning bash?

aduh95 commented Nov 15, 2020

Copy link
Copy Markdown
Contributor Author

What's the reason for banning bash?

TBH that because it makes #36099 a bit simpler. I don't think we need bash (we should prefer JS or Python when possible), and I agree with this StackOverflow answer on why sh should be use instead of bash:

  • it is standardized
  • it is much simpler and easier to learn
  • it is portable across POSIX systems — even if they happen not to have bash, they are required to have sh

aduh95 commented Nov 29, 2020

Copy link
Copy Markdown
Contributor Author

@nodejs/releasers can you review this please? All changes here looks relatively safe, and this is blocking #36099.

MylesBorins commented Nov 29, 2020

Copy link
Copy Markdown
Contributor

/cc @nodejs/build

targos commented Nov 29, 2020

Copy link
Copy Markdown
Member

I don't know shell scripting languages good enough to review this, sorry.

Trott reviewed Dec 1, 2020
View reviewed changes
Comment thread tools/osx-codesign.sh

if [ "X$SIGN" == "X" ]; then
echo "No SIGN environment var. Skipping codesign." >&2
# shellcheck disable=SC2154

Trott Dec 1, 2020

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

I don't think this disable directive is necessary?

aduh95 Dec 1, 2020

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

I'm getting this warning when I remove this line:

$ shellcheck --shell=sh --severity=info --enable=all tools/osx-codesign.sh

In tools/osx-codesign.sh line 6:
[ -z "$SIGN" ] && \
      ^---^ SC2154: SIGN is referenced but not assigned.

aduh95 Dec 1, 2020

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

I'm using ShellCheck 0.7.1 btw, that may be a version-specific behavior.

Trott reviewed Dec 1, 2020
View reviewed changes
Comment thread tools/osx-codesign.sh
# hardened runtime enabled.
# See https://github.com/nodejs/node/pull/31459

# shellcheck disable=SC2154

Trott Dec 1, 2020

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Rather than this disable directive, would it be better to add a -z "$PKGDIR" check similar to that on line 7 for $SIGN? Both are supplied by the Makefile and guaranteed to be not-empty. So it would seem to me that we should either check for both or assume non-empty for both. But we seem to check for only one. Check for both?

Or is that a modification outside the scope of this PR and should be done later?

Trott reviewed Dec 1, 2020
View reviewed changes
Comment thread tools/osx-notarize.sh

if [ "X$NOTARIZATION_ID" == "X" ]; then
echo "No NOTARIZATION_ID environment var. Skipping notarization."
# shellcheck disable=SC2154

Trott Dec 1, 2020

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

I don't think this directive is necessary?

Trott reviewed Dec 1, 2020
View reviewed changes
Comment thread tools/osx-productsign.sh

if [ "X$SIGN" == "X" ]; then
echo "No SIGN environment var. Skipping codesign." >&2
# shellcheck disable=SC2154

Trott Dec 1, 2020

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

I don't think this directive is necessary?

Trott reviewed Dec 1, 2020
View reviewed changes
Comment thread tools/osx-productsign.sh
Comment on lines +11 to +13
# shellcheck disable=SC2154
productsign --sign "$SIGN" "$PKG" "$PKG"-SIGNED
# shellcheck disable=SC2154

Trott Dec 1, 2020

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Instead of these two disable directives, would it make sense to add a -z "$PKG" check similar to the one for $SIGN on line 7? Both are guaranteed by the Makefile so it would seem that we should either check for both or assume both, but we are checking for just one and assuming the other. Or is that outside the scope of this PR?

Trott approved these changes Dec 1, 2020
View reviewed changes

Trott left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

LGTM with or without my comments addressed.

Trott added the request-ci Add this label to start a Jenkins CI on a PR. label Dec 1, 2020
github-actions Bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Dec 1, 2020

nodejs-github-bot commented Dec 1, 2020

Copy link
Copy Markdown
Collaborator

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

AshCripps commented Dec 1, 2020

Copy link
Copy Markdown
Member

Has this been tested to ensure the signing still works?

rvagg commented Dec 2, 2020

Copy link
Copy Markdown
Member 
$ /bin/sh --version
GNU bash, version 3.2.57(1)-release (x86_64-apple-darwin19)
Copyright (C) 2007 Free Software Foundation, Inc.
$ /bin/bash --version
GNU bash, version 3.2.57(1)-release (x86_64-apple-darwin19)
Copyright (C) 2007 Free Software Foundation, Inc.

is this really necessary? seems like needless churn to me

rvagg commented Dec 2, 2020

Copy link
Copy Markdown
Member

test build running, should come out @ https://nodejs.org/download/test/v16.0.0-test202012024dc74c4fbb/ shortly, someone can test that if you really want to merge this and need it verified. The .pkg should get full coverage of these changes I think - signing, notarization, etc.

rvagg commented Dec 2, 2020

Copy link
Copy Markdown
Member

https://nodejs.org/download/test/v16.0.0-test202012024dc74c4fbb/node-v16.0.0-test202012024dc74c4fbb.pkg there's yer pkg to test, it got built without error at least.

aduh95 commented Dec 2, 2020

Copy link
Copy Markdown
Contributor Author

is this really necessary? seems like needless churn to me

On my macOS Catalina machine, I got the same output as you for --version. But that doesn't mean the two executables are the same. If you take a look at the file size, you can see they are quite different:

$ ls -lh /bin/bash /bin/sh
-r-xr-xr-x  1 root  wheel   609K Sep 22 02:30 /bin/bash
-rwxr-xr-x  1 root  wheel    31K Sep 22 02:30 /bin/sh

there's yer pkg to test, it got built without error at least.

I tried to install it, and the installation went smoothly. Can I take the notorization works?

nodejs-github-bot commented Dec 2, 2020

Copy link
Copy Markdown
Collaborator

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

aduh95 added the author ready PRs that have at least one approval, no outstanding review comments, and a CI started. label Dec 2, 2020

rvagg commented Dec 3, 2020

Copy link
Copy Markdown
Member

If you take a look at the file size, you can see they are quite different

I believe because on macOS sh is a simple executable redirect to bash. This isn't a Dash vs Bash situation, it's both the same Bash.

I tried to install it, and the installation went smoothly. Can I take the notorization works?

yep, and I can see a notarization email for it too:

Your Mac software has been notarized. You can now export this software and distribute it directly to users.

Bundle Identifier: org.nodejs.pkg.v16.0.0-test202012024dc74c4fbb
Request Identifier: d3ff1b52-4705-4b14-824a-32bf16def8c8

aduh95 added the commit-queue Add this label to land a pull request using GitHub Actions. label Dec 4, 2020
github-actions Bot removed the commit-queue Add this label to land a pull request using GitHub Actions. label Dec 4, 2020

github-actions Bot commented Dec 4, 2020

Copy link
Copy Markdown
Contributor

Landed in 1729ba7...8973075

nodejs-github-bot pushed a commit that referenced this pull request Dec 4, 2020
tools: remove bashisms from macOS release scripts
8973075 
PR-URL: #36121
Reviewed-By: Rich Trott <rtrott@gmail.com>
github-actions Bot closed this Dec 4, 2020
aduh95 deleted the macos-sh-bashisms branch December 4, 2020 22:57
danielleadams pushed a commit that referenced this pull request Dec 7, 2020
tools: remove bashisms from macOS release scripts
754b7a7 
PR-URL: #36121
Reviewed-By: Rich Trott <rtrott@gmail.com>
danielleadams mentioned this pull request Dec 7, 2020
Merged
targos pushed a commit that referenced this pull request May 1, 2021
tools: remove bashisms from macOS release scripts
a033910 
PR-URL: #36121
Reviewed-By: Rich Trott <rtrott@gmail.com>
danielleadams mentioned this pull request May 3, 2021
Merged
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
Show hidden characters
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

Trott Trott approved these changes

Assignees

No one assigned

Labels

author ready PRs that have at least one approval, no outstanding review comments, and a CI started. build Issues and PRs related to build files or the CI. macos Issues and PRs related to the macOS platform / OSX. 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.

7 participants

Back | FazBrowse Home | New Git URL