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

Jump to bottom

Fix darwin arch selection logic #388

Closed
wants to merge 3 commits into from
Closed

Fix darwin arch selection logic #388

wants to merge 3 commits into from

Conversation

alex-shafer-1002
Copy link

@alex-shafer-1002 alex-shafer-1002 commented Mar 30, 2023 •
edited
Loading

This corrects the regex to choose amd64 for versions < 1.0.2

Tested as follows:

  1. Created a script to test the logic in isolation:
#!/usr/bin/env bash

while read version; do
    if [[ "${version}" =~ ^0\. || "${version}" =~ ^1\.0\.[01]$ ]]; then
      echo "amd64  $version"
    else
      echo "ARM64  $version"
    fi;
done
  1. Piped tfenv list-remote to this script to test all current versions:
tfenv list-remote | ~/version-logic.sh
  1. Verified that the output switched from amd64 to arm64 at the expected version (truncated for brevity):
ARM64  1.0.4
ARM64  1.0.3
ARM64  1.0.2
amd64  1.0.1
amd64  1.0.0
amd64  0.15.5
  1. Temporarily patched my local installed libexec/tfenv-install with no TFENV_ARCH set in my environment and tested some relevant cases (note the arch in the download URL):
% tfenv install 1.0.1
Installing Terraform v1.0.1
Downloading release tarball from https://releases.hashicorp.com/terraform/1.0.1/terraform_1.0.1_darwin_amd64.zip
...
Installation of terraform v1.0.1 successful. To make this your default version, run 'tfenv use 1.0.1'

% tfenv install 1.0.2
Installing Terraform v1.0.2
Downloading release tarball from https://releases.hashicorp.com/terraform/1.0.2/terraform_1.0.2_darwin_arm64.zip
...
Installation of terraform v1.0.2 successful. To make this your default version, run 'tfenv use 1.0.2'

% tfenv install 1.0.10
Installing Terraform v1.0.10
Downloading release tarball from https://releases.hashicorp.com/terraform/1.0.10/terraform_1.0.10_darwin_arm64.zip
...
Installation of terraform v1.0.10 successful. To make this your default version, run 'tfenv use 1.0.10'

% tfenv install 1.0.11
Installing Terraform v1.0.11
Downloading release tarball from https://releases.hashicorp.com/terraform/1.0.11/terraform_1.0.11_darwin_arm64.zip
...
Installation of terraform v1.0.11 successful. To make this your default version, run 'tfenv use 1.0.11'

@denizgenc
Copy link
Contributor

Your script produces the exact same output when run against tfutils:master. There isn't anything wrong with the line you're changing; you're probably comparing against the latest released binary on brew, which is based on the 3.0.0 label, where tfenv-install looked like this.

(sorry if I'm being a bit defensive, I wrote the line that this PR replaces 😅)

@alex-shafer-1002
Copy link
Author

Got it, yes this was against the current brew version, we just really need a new deploy I guess.

@bryanhiestand
Copy link
Contributor

Your script produces the exact same output when run against tfutils:master. There isn't anything wrong with the line you're changing; you're probably comparing against the latest released binary on brew, which is based on the 3.0.0 label, where tfenv-install looked like this.

(sorry if I'm being a bit defensive, I wrote the line that this PR replaces 😅)

I'm having trouble following you or understanding how these two lines could produce the same output. Can you point me at the code you're talking about if it's not this?

tfenv/libexec/tfenv-install

Line 127 in 1ccfddb

if [[ "${version}" =~ 0\..+$ || "${version}" =~ 1\.0\.0|1$

I just opened #403 to try to fix the bugs in these regexes, but it looks like this PR fixes the behavior as well.

Current master (but not release 3.0.0) fixes the issue where arm64 is chosen instead of amd64 for versions < 1.0.2. But with the code I linked (and this PR changes), some versions > 1.0.2 use amd64 instead of arm64.

@denizgenc
Copy link
Contributor

You're definitely right, I didn't use the start of line anchors and should have been stricter about what I was matching with the regexes. Apologies to Alex

@Zordrak
Copy link
Collaborator

Zordrak commented Dec 19, 2023

Superseded by #403

@Zordrak Zordrak closed this Dec 19, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

No arm64 builds for terraform < 1.0.2 404 during specified package install
4 participants
@alex-shafer-1002 @denizgenc @bryanhiestand @Zordrak