-
Notifications
You must be signed in to change notification settings - Fork 24
build: package for apple intel and apple silicon machines alongside universal binaries as fallback #101
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
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #101 +/- ##
==========================================
+ Coverage 63.31% 63.40% +0.09%
==========================================
Files 211 211
Lines 22282 22284 +2
==========================================
+ Hits 14107 14130 +23
+ Misses 7086 7067 -19
+ Partials 1089 1087 -2 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
zimeg
left a comment
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.
💌 For the kind reviewer reviewing, I left a few notes on what's happening in these changes!
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.
🗣️ AFAICT we are not using this file and it is almost identical to the production .goreleaser.yml - the one difference is an included LICENSE in production.
| post: |- | ||
| sh -c ' | ||
| zip ./dist/slack_cli_{{.Env.BUILD_VERSION}}_macOS_64-bit.zip ./dist/slack-macos_darwin_amd64_v1/bin/slack -j | ||
| zip ./dist/slack_cli_{{ .Env.BUILD_VERSION }}_macOS_{{ .Arch }}.zip ./dist/slack-macos_{{ .Target }}/bin/slack -j |
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.
📝 Here we zip the architecture specific files:
./dist/slack_cli_3.4.5_macOS_amd64.zip./dist/slack_cli_3.4.5_macOS_arm64.zip
| universal_binaries: | ||
| - id: slack-macos | ||
| name_template: bin/slack | ||
| replace: false | ||
| hooks: | ||
| post: |- | ||
| sh -c ' | ||
| zip ./dist/slack_cli_{{.Env.BUILD_VERSION}}_macOS_64-bit.zip ./dist/slack-macos_darwin_all/bin/slack -j | ||
| ' |
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.
📚 https://goreleaser.com/customization/universalbinaries/
The name_template is used to match packaging paths in tarballs later!
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.
universal_binaries support is such a great discovery! Thank you @zimeg and GoReleaser!
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.
I must give so much credit to the great folks at @goreleaser 🙏 ✨
| if version_lt "$SLACK_CLI_DEV_VERSION" "3.3.0"; then | ||
| slack_cli_url="https://downloads.slack-edge.com/slack-cli/slack_cli_${SLACK_CLI_DEV_VERSION}_macOS_64-bit.tar.gz" | ||
| else | ||
| case "$(uname -m)" in | ||
| x86_64) | ||
| slack_cli_url="https://downloads.slack-edge.com/slack-cli/slack_cli_${SLACK_CLI_DEV_VERSION}_macOS_amd64.tar.gz" | ||
| ;; | ||
| arm64 | aarch64) | ||
| slack_cli_url="https://downloads.slack-edge.com/slack-cli/slack_cli_${SLACK_CLI_DEV_VERSION}_macOS_arm64.tar.gz" | ||
| ;; | ||
| *) | ||
| slack_cli_url="https://downloads.slack-edge.com/slack-cli/slack_cli_${SLACK_CLI_DEV_VERSION}_macOS_64-bit.tar.gz" | ||
| ;; | ||
| esac | ||
| fi |
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.
📣 We're guarding against specific installations of versions prior to next release here!
This adds some overhead to this script, but I don't think backporting releases can be done without so much effort and potential error otherwise...
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.
Yea, this is a clean way to do it and doesn't add very much overhead. Nice work!
mwbrooks
left a comment
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.
✅ Amazing work! I expected this to be a challenge to be compatible with slack upgrade and your work makes it look like a breeze. 😎
🧪 Manual testing is good on my side. I've tested it on a Macbook Intel (amd64).
Install Script correctly detects amd64 on my Macbook Intel:

Upgrade command correctly detect amd64 on my Macbook Intel:

❓ Do we need to update our .circleci/config.yml to upload the new binaries to be Code Signed and uploaded to the GitHub Release and S3 Bucket?
❓ In terms of release rollout, my understanding is that we will wait until the future release (after tomorrow's release). We need the CI/CD to build and upload the new binaries before we can upload the new install scripts (although, your check for <= v3.3.0 may allow us to safely upload it sooner).
| updates in the following files on occasion: | ||
|
|
||
| - `.circleci/config.yml` | ||
| - `.goreleaser-dev.yml` |
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.
👋🏻 Bye!
| universal_binaries: | ||
| - id: slack-macos | ||
| name_template: bin/slack | ||
| replace: false | ||
| hooks: | ||
| post: |- | ||
| sh -c ' | ||
| zip ./dist/slack_cli_{{.Env.BUILD_VERSION}}_macOS_64-bit.zip ./dist/slack-macos_darwin_all/bin/slack -j | ||
| ' |
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.
universal_binaries support is such a great discovery! Thank you @zimeg and GoReleaser!
| switch architecture { | ||
| case "amd64": | ||
| filename = fmt.Sprintf("slack_cli_%s_macOS_amd64.zip", version) | ||
| case "arm64": | ||
| filename = fmt.Sprintf("slack_cli_%s_macOS_arm64.zip", version) | ||
| default: | ||
| filename = fmt.Sprintf("slack_cli_%s_macOS_64-bit.zip", version) | ||
| } |
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.
note: so nice! 👌🏻
| if version_lt "$SLACK_CLI_DEV_VERSION" "3.3.0"; then | ||
| slack_cli_url="https://downloads.slack-edge.com/slack-cli/slack_cli_${SLACK_CLI_DEV_VERSION}_macOS_64-bit.tar.gz" | ||
| else | ||
| case "$(uname -m)" in | ||
| x86_64) | ||
| slack_cli_url="https://downloads.slack-edge.com/slack-cli/slack_cli_${SLACK_CLI_DEV_VERSION}_macOS_amd64.tar.gz" | ||
| ;; | ||
| arm64 | aarch64) | ||
| slack_cli_url="https://downloads.slack-edge.com/slack-cli/slack_cli_${SLACK_CLI_DEV_VERSION}_macOS_arm64.tar.gz" | ||
| ;; | ||
| *) | ||
| slack_cli_url="https://downloads.slack-edge.com/slack-cli/slack_cli_${SLACK_CLI_DEV_VERSION}_macOS_64-bit.tar.gz" | ||
| ;; | ||
| esac | ||
| fi |
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.
Yea, this is a clean way to do it and doesn't add very much overhead. Nice work!
|
@zimeg I think we should add a |
zimeg
left a comment
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.
@mwbrooks Thank you for the fast review and testing! It's exciting to soon ship this 🍎 ✨
Do we need to update our
.circleci/config.ymlto upload the new binaries to be Code Signed and uploaded to the GitHub Release and S3 Bucket?
Thanks a ton for calling this out! I found the release on GitHub upload as expected with a perhaps deceiving wildcard:
slack-cli/.circleci/config.yml
Lines 672 to 679 in 8698f51
| - s3-upload: | |
| <<: *filters-tag-triggered-workflow-job | |
| name: upload-to-s3-for-autoupdate-zip | |
| s3-target-path: cli | |
| file-name: "slack_cli_*.zip" | |
| requires: | |
| - create-github-release-and-artifacts | |
| context: slack-cli-release |
But I am not certain that this carries to the S3 uploads - I'll follow up with a the changes we might need for this soon 🔍
We need the CI/CD to build and upload the new binaries before we can upload the new install scripts (although, your check for <= v3.3.0 may allow us to safely upload it sooner).
I believe the installation script can be uploaded whenever too, but we will want to make sure binaries are uploaded before updating the upstream metadata.json with the latest version. Otherwise those darned 403 might appear.
FWIW I'm thinking we might want to attempt a release candidate before tagging an official release, but I'm also hoping the changes soon to CircleCI bring us enough confidence otherwise..
I think we should add a
changeloglabel and maybe update the title to something more straight forward
Thanks for keeping the kind developers in mind as always - will do! 👾
| universal_binaries: | ||
| - id: slack-macos | ||
| name_template: bin/slack | ||
| replace: false | ||
| hooks: | ||
| post: |- | ||
| sh -c ' | ||
| zip ./dist/slack_cli_{{.Env.BUILD_VERSION}}_macOS_64-bit.zip ./dist/slack-macos_darwin_all/bin/slack -j | ||
| ' |
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.
I must give so much credit to the great folks at @goreleaser 🙏 ✨
That's awesome about GitHub. I saw that same wildcard and (to me) it looks like it will upload to both GitHub Releases and S3. |
|
@mwbrooks Lots of awesomeness happening in these changes I felt! Thanks for thinking ahead to the next production release in this review 🤖 ✨ Before merging I will list the now expected files in a release for future reference:
A |

Summary
This PR builds
darwinuniversal binaries along with specific builds for bothamd64and nowarm64! 🍎 ✨Reviewers
Check the installation scripts installs:
Confirm automatic upgrades update:
$ go build -ldflags="-s -w -X 'github.com/slackapi/slack-cli/internal/pkg/version.Version=v3.0.0'" -o bin/slack $ lack version Using lack v3.0.0 $ lack upgrade ... Architecture: arm64 OS: darwin Downloading slack_cli_3.1.0_macOS_arm64.zip Downloading cli from https://downloads.slack-edge.com/slack-cli/slack_cli_3.1.0_macOS_arm64.zipPlease note "arm64" might be "amd64" for some setups. An apple computer is required too to test this 👾
Notes
macOS_64-bitfor backwards compatibilities with improved support. Rosetta is no long required.macOS_64-bitinstallation is moved tomacOS_amd64.macOS_arm64!Requirements