-
Notifications
You must be signed in to change notification settings - Fork 455
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
Improve TFENV_ARCH
initialisation for Apple Silicon Macs
#351
Conversation
This looks great! Anything holding up approvals? |
Not just M1 Macs are affected by this change, of course.
494e2dd
to
94f0d2c
Compare
# There is no arm64 support for versions: | ||
# < 0.11.15 | ||
# >= 0.12.0, < 0.12.30 | ||
# >= 0.13.0, < 0.13.5 | ||
if [[ "${version}" =~ 0\.(([0-9]|10))\.\d* || | ||
"${version}" =~ 0\.11\.(([0-9]|1[0-4]))$ || | ||
"${version}" =~ 0\.12\.(([0-9]|[1-2][0-9]))$ || | ||
"${version}" =~ 0\.13\.[0-4]$ |
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.
@Zordrak @denizgenc Unfortunately, this is still causing trouble:
➜ tfenv use min-required
No installed versions of terraform matched '0.12.31:^0.12.31$'. Trying to install a matching version since TFENV_AUTO_INSTALL=true
Installing Terraform v0.12.31
Downloading release tarball from https://releases.hashicorp.com/terraform/0.12.31/terraform_0.12.31_darwin_arm64.zip
curl: (22) The requested URL returned error: 404
Tarball download failed
Installing a matching version failed
As pointed out here, 1.0.2
was the very first Darwin arm64 release.
# There is no arm64 support for versions: | |
# < 0.11.15 | |
# >= 0.12.0, < 0.12.30 | |
# >= 0.13.0, < 0.13.5 | |
if [[ "${version}" =~ 0\.(([0-9]|10))\.\d* || | |
"${version}" =~ 0\.11\.(([0-9]|1[0-4]))$ || | |
"${version}" =~ 0\.12\.(([0-9]|[1-2][0-9]))$ || | |
"${version}" =~ 0\.13\.[0-4]$ | |
# There is no arm64 support for all versions below v1.0.2: | |
# https://github.com/hashicorp/terraform/issues/27257#issuecomment-875834420 | |
if [[ | |
"${version}" =~ 0\.(([0-9]|1[0-5]))\.\d* || | |
"${version}" == 1\.0\.[0-1]$ |
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've never used tfenv use min-required
, but I'm sure the problem must be in another file.
The part of the code you're looking at is dealing with Linux, for which the earliest arm64 builds match that regex. For an example, see the releases page for version 0.12.31: https://releases.hashicorp.com/terraform/0.12.31/ - there is a terraform_0.12.31_linux_arm64.zip
, but no corresponding terraform_0.12.31_darwin_arm64.zip
build. Hence Linux should be allowed to download arm64
releases for that version, but not Macs. This change would prevent that occuring.
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.
Wait, when you're running tfenv
, have you built from source or are you using the latest release? A new release hasn't been cut since last July so the latest version of tfenv
available on package managers won't incorporate this PR, and so the architecture issues will still occur on Apple Silicon. If you need the architecture set correctly you can either set TFENV_ARCH
manually or build from source as I mentioned.
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.
Mea culpa, now I see darwin is handled in the next case. 🙈
Just remembered that this PR existed and was wondering why I am still seeing this bug. 😓
Indeed, I am using the homebrew release:
~
➜ brew info tfenv
==> tfenv: stable 3.0.0 (bottled), HEAD
Terraform version manager inspired by rbenv
https://github.com/tfutils/tfenv
Conflicts with:
terraform (because tfenv symlinks terraform binaries)
/opt/homebrew/Cellar/tfenv/3.0.0 (32 files, 213.1MB) *
Poured from bottle using the formulae.brew.sh API on 2023-02-21 at 19:29:31
From: https://github.com/Homebrew/homebrew-core/blob/HEAD/Formula/tfenv.rb
License: MIT
==> Dependencies
Required: grep ✔
==> Options
--HEAD
Install HEAD version
==> Analytics
install: 7,803 (30 days), 24,090 (90 days), 129,443 (365 days)
install-on-request: 7,780 (30 days), 24,021 (90 days), 128,983 (365 days)
build-error: 0 (30 days)
~ took 1s
➜
A new release would be great 😉
Fixes #350. I've changed the order of the
uname -s
anduname -m
steps in order to make this work. As such, I've created a new variable,kernel
, that is set during theuname -s
case
statement.kernel
is then used when settingTFENV_ARCH
whenaarch-64 | arm64
is found, and also later when setting theos
variable. Hope it makes sense.I've checked this against the current
tfenv
installed on my machine (version 3.0.0, installed via brew), and it works where the current version fails (tfenv
== 3.0.0,tfenv-deniz
== this branch):