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.

Sign up for GitHub

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
Closed

tools: remove bashisms from macOS release scripts #36121

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 4 additions & 4 deletions Makefile
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -979,7 +979,7 @@ $(PKG): release-only
--release-urlbase=$(RELEASE_URLBASE) \
$(CONFIG_FLAGS) $(BUILD_RELEASE_FLAGS)
$(MAKE) install V=$(V) DESTDIR=$(MACOSOUTDIR)/dist/node
SIGN="$(CODESIGN_CERT)" PKGDIR="$(MACOSOUTDIR)/dist/node/usr/local" bash \
SIGN="$(CODESIGN_CERT)" PKGDIR="$(MACOSOUTDIR)/dist/node/usr/local" sh \
tools/osx-codesign.sh
mkdir -p $(MACOSOUTDIR)/dist/npm/usr/local/lib/node_modules
mkdir -p $(MACOSOUTDIR)/pkgs
Expand All @@ -1001,8 +1001,8 @@ $(PKG): release-only
productbuild --distribution $(MACOSOUTDIR)/installer/productbuild/distribution.xml \
--resources $(MACOSOUTDIR)/installer/productbuild/Resources \
--package-path $(MACOSOUTDIR)/pkgs ./$(PKG)
SIGN="$(PRODUCTSIGN_CERT)" PKG="$(PKG)" bash tools/osx-productsign.sh
bash tools/osx-notarize.sh $(FULLVERSION)
SIGN="$(PRODUCTSIGN_CERT)" PKG="$(PKG)" sh tools/osx-productsign.sh
sh tools/osx-notarize.sh $(FULLVERSION)

.PHONY: pkg
# Builds the macOS installer for releases.
Expand Down Expand Up @@ -1120,7 +1120,7 @@ $(BINARYTAR): release-only
cp LICENSE $(BINARYNAME)
cp CHANGELOG.md $(BINARYNAME)
ifeq ($(OSTYPE),darwin)
SIGN="$(CODESIGN_CERT)" PKGDIR="$(BINARYNAME)" bash tools/osx-codesign.sh
SIGN="$(CODESIGN_CERT)" PKGDIR="$(BINARYNAME)" sh tools/osx-codesign.sh
endif
tar -cf $(BINARYNAME).tar $(BINARYNAME)
$(RM) -r $(BINARYNAME)
Expand Down
9 changes: 5 additions & 4 deletions tools/osx-codesign.sh
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,17 +1,18 @@
#!/bin/bash
#!/bin/sh

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.

[ -z "$SIGN" ] && \
echo "No SIGN environment var. Skipping codesign." >&2 && \
exit 0
fi

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

codesign \
--sign "$SIGN" \
--entitlements tools/osx-entitlements.plist \
Expand Down
14 changes: 6 additions & 8 deletions tools/osx-notarize.sh
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
#!/bin/bash
#!/bin/sh

# Uses gon, from https://github.com/mitchellh/gon, to notarize a generated node-<version>.pkg file
# with Apple for installation on macOS Catalina and later as validated by Gatekeeper.
Expand All @@ -8,18 +8,16 @@ set -e
gon_version="0.2.2"
gon_exe="${HOME}/.gon/gon_${gon_version}"

__dirname="$(CDPATH= cd "$(dirname "${BASH_SOURCE[0]}")" && pwd)"
pkgid="$1"

if [ "X${pkgid}" == "X" ]; then
echo "Usage: $0 <pkgid>"
[ -z "$pkgid" ] && \
echo "Usage: $0 <pkgid>" \
exit 1
fi

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?

[ -z "$NOTARIZATION_ID" ] && \
echo "No NOTARIZATION_ID environment var. Skipping notarization." \
exit 0
fi

set -x

Expand Down
10 changes: 6 additions & 4 deletions tools/osx-productsign.sh
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,12 +1,14 @@
#!/bin/bash
#!/bin/sh

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?

[ -z "$SIGN" ] && \
echo "No SIGN environment var. Skipping codesign." >&2 && \
exit 0
fi

# shellcheck disable=SC2154
productsign --sign "$SIGN" "$PKG" "$PKG"-SIGNED
# shellcheck disable=SC2154
Comment on lines +11 to +13
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?

mv "$PKG"-SIGNED "$PKG"