-
Notifications
You must be signed in to change notification settings - Fork 368
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
add support for darwin/arm64 (Apple Silicon) #693
Conversation
Signed-off-by: Joe Lanford <[email protected]>
@@ -22,9 +22,11 @@ Krew self-hosts). | |||
```sh | |||
( | |||
set -x; cd "$(mktemp -d)" && | |||
OS="$(uname | tr '[:upper:]' '[:lower:]')" && | |||
ARCH=`case "${OS}" in "darwin") uname -m | sed -e 's/x86_64/amd64/' ;; *) uname -m | sed -e 's/x86_64/amd64/' -e 's/arm.*$/arm/' -e 's/aarch64$/arm64/' ;; esac` && |
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.
this part is a little suspicious: why did you need case "${OS}" in "darwin")
?
it seems that uname -m
already returns arm64 in Apple Silicon chip, so it would work as is?
if you did this because of -e 's/arm.*$/arm/'
, that's understandable as it could return values like armv7l
, armv8
in other platforms such as Linux. in that case, we can rewrite the regexp pattern in a way that does not replace amd64
(coming from M1).
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.
Yeah, this is the part that I mentioned in the PR description.
The install scripts are a bit more complicated now because uname -m on Apple Silicon returns arm64, so the sed -e 's/arm.*$/arm/' substitution resulted in darwin_arm, rather than darwin_arm64.
I fixed that as best I could with a case statement, but perhaps there's a simpler approach. I looked into a pure sed solution, but it doesn't seem like sed supports the necessary lookahead features that would enable search/replace for arm + anything other than 64.
Any better ideas?
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.
Pushed a new update with a better sed-only replacement that seems to work. WDYT?
Signed-off-by: Joe Lanford <[email protected]>
Looks good. Thanks. |
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: ahmetb, joelanford The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
This PR adds support for Apple Silicon by adding
darwin/arm64
to the build matrix.The install scripts are a bit more complicated now because
uname -m
on Apple Silicon returnsarm64
, so thesed -e 's/arm.*$/arm/'
substitution resulted indarwin_arm
, rather thandarwin_arm64
.I fixed that as best I could with a case statement, but perhaps there's a simpler approach. I looked into a pure sed solution, but it doesn't seem like sed supports the necessary lookahead features that would enable search/replace for
arm
+ anything other than64
.Fixes #686