Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

tools: remove bashisms from macOS release scripts #36121

Closed
wants to merge 1 commit into from

Conversation

aduh95
Copy link
Contributor

@aduh95 aduh95 commented Nov 14, 2020

In preparation for #36099.

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

@nodejs-github-bot 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 aduh95 mentioned this pull request Nov 14, 2020
8 tasks
@targos
Copy link
Member

targos commented Nov 15, 2020

What's the reason for banning bash?

@aduh95
Copy link
Contributor Author

aduh95 commented Nov 15, 2020

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
Copy link
Contributor Author

aduh95 commented Nov 29, 2020

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

@MylesBorins
Copy link
Contributor

/cc @nodejs/build

@targos
Copy link
Member

targos commented Nov 29, 2020

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


set -x
set -e

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

Choose a reason for hiding this comment

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

I don't think this disable directive is necessary?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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


# All macOS executable binaries in the bundle must be codesigned with the
# hardened runtime enabled.
# See https://github.com/nodejs/node/pull/31459

# shellcheck disable=SC2154
Copy link
Member

Choose a reason for hiding this comment

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

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?


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

Choose a reason for hiding this comment

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

I don't think this directive is necessary?


set -x
set -e

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

Choose a reason for hiding this comment

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

I don't think this directive is necessary?

Comment on lines +11 to +13
# shellcheck disable=SC2154
productsign --sign "$SIGN" "$PKG" "$PKG"-SIGNED
# shellcheck disable=SC2154
Copy link
Member

Choose a reason for hiding this comment

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

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?

Copy link
Member

@Trott Trott left a comment

Choose a reason for hiding this comment

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

LGTM with or without my comments addressed.

@Trott Trott added the request-ci Add this label to start a Jenkins CI on a PR. label Dec 1, 2020
@github-actions 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
Copy link
Collaborator

@AshCripps
Copy link
Member

Has this been tested to ensure the signing still works?

@rvagg
Copy link
Member

rvagg commented Dec 2, 2020

$ /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
Copy link
Member

rvagg commented Dec 2, 2020

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
Copy link
Member

rvagg commented Dec 2, 2020

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
Copy link
Contributor Author

aduh95 commented Dec 2, 2020

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
Copy link
Collaborator

@aduh95 aduh95 added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Dec 2, 2020
@rvagg
Copy link
Member

rvagg commented Dec 3, 2020

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 aduh95 added the commit-queue Add this label to land a pull request using GitHub Actions. label Dec 4, 2020
@github-actions github-actions bot removed the commit-queue Add this label to land a pull request using GitHub Actions. label Dec 4, 2020
@github-actions
Copy link
Contributor

github-actions bot commented Dec 4, 2020

Landed in 1729ba7...8973075

nodejs-github-bot pushed a commit that referenced this pull request Dec 4, 2020
PR-URL: #36121
Reviewed-By: Rich Trott <rtrott@gmail.com>
@github-actions github-actions bot closed this Dec 4, 2020
@aduh95 aduh95 deleted the macos-sh-bashisms branch December 4, 2020 22:57
danielleadams pushed a commit that referenced this pull request Dec 7, 2020
PR-URL: #36121
Reviewed-By: Rich Trott <rtrott@gmail.com>
@danielleadams danielleadams mentioned this pull request Dec 7, 2020
cjihrig pushed a commit to cjihrig/node that referenced this pull request Dec 8, 2020
PR-URL: nodejs#36121
Reviewed-By: Rich Trott <rtrott@gmail.com>
targos pushed a commit that referenced this pull request May 1, 2021
PR-URL: #36121
Reviewed-By: Rich Trott <rtrott@gmail.com>
@danielleadams danielleadams mentioned this pull request May 3, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
author ready PRs that have at least one approval, no pending requests for changes, 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
Development

Successfully merging this pull request may close these issues.

7 participants