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

Improve TFENV_ARCH initialisation for Apple Silicon Macs #351

Merged
merged 3 commits into from
Oct 1, 2022

Conversation

denizgenc
Copy link
Contributor

@denizgenc denizgenc commented Jul 19, 2022

Fixes #350. I've changed the order of the uname -s and uname -m steps in order to make this work. As such, I've created a new variable, kernel, that is set during the uname -s case statement. kernel is then used when setting TFENV_ARCH when aarch-64 | arm64 is found, and also later when setting the os 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):

$ tfenv install 0.11.15
Installing Terraform v0.11.15
Downloading release tarball from https://releases.hashicorp.com/terraform/0.11.15/terraform_0.11.15_darwin_arm64.zip
curl: (22) The requested URL returned error: 404                                                   

Tarball download failed

$ tfenv-deniz install 0.11.15
Installing Terraform v0.11.15
Downloading release tarball from https://releases.hashicorp.com/terraform/0.11.15/terraform_0.11.15_darwin_amd64.zip
############################################################################################# 100.0%
...

@aleclerc-sonrai
Copy link

This looks great! Anything holding up approvals?

@Zordrak Zordrak mentioned this pull request Oct 1, 2022
@Zordrak Zordrak merged commit 34c744b into tfutils:master Oct 1, 2022
Comment on lines +111 to +118
# 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]$
Copy link

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.

Suggested change
# 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]$

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'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.

Copy link
Contributor Author

@denizgenc denizgenc Feb 25, 2023

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.

Copy link

@n3ph n3ph Feb 25, 2023

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 😉

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

arm64 availability is different across Linux and Mac
4 participants