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 archive platform suffix resolution #46

Merged

Conversation

mjburling
Copy link
Contributor

@mjburling mjburling commented Oct 15, 2023

Closes #45.

This change set extends get_target_platform() functionality to include the separate get_arch() function. This now includes an extended map of (std::env::consts::OS, std::env::consts::ARCH) pairs that map to Go equivalents formatted vaguely as the following: ~"${runtime.GOOS}_${runtime.GOARCH}".

Pardon my Rust, but this seems to work and remains pretty consistent with your solution so far.

Feedback welcome! Thanks for your consideration!

Copy link
Owner

@ASleepyCat ASleepyCat left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for reporting the issue and creating a PR for it.

The only issues I have is that this drops support for other OSes like OpenBSD and Solaris, and that I'd rather return an error than panic.

It's fine if you don't address the first issue, since those other OSes aren't fully supported by Rust anyway. I prefer errors since that won't produce a stack trace to the user that probably won't mean anything to them.

Also the unit tests will need to be updated since you deleted get_arch().

@mjburling
Copy link
Contributor Author

Thanks for reporting the issue and creating a PR for it.

The only issues I have is that this drops support for other OSes like OpenBSD and Solaris, and that I'd rather return an error than panic.

It's fine if you don't address the first issue, since those other OSes aren't fully supported by Rust anyway. I prefer errors since that won't produce a stack trace to the user that probably won't mean anything to them.

Whoops! I didn't mean to leave any users out in the cold! I've re-added support for freebsd, openbsd, and solaris–those are supported by tofu/terraform–there are 14 distinct supported combinations as of tofu/terraform ~> 1.6.

I see what you mean on the panic bit. Strictly speaking, there shouldn't be anything here from stopping users from running this application on say a combination of ("linux", "powerpc64"), but... maybe that is a meaningful place to panic? There's no upstream support for this platform, thus no archive (terraform|tofu)_x.y.z_linux_powerpc64.zip.

Also the unit tests will need to be updated since you deleted get_arch().

Very sloppy of me! I thought the build would fail by default if there were any unresolved/unreachable symbols. Seems the test scope gets slightly different treatment. I've elected to remove these instead... there's less to test and even less value in attempting to test it now.

Copy link
Owner

@ASleepyCat ASleepyCat left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see what you mean on the panic bit. Strictly speaking, there shouldn't be anything here from stopping users from running this application on say a combination of ("linux", "powerpc64"), but... maybe that is a meaningful place to panic? There's no upstream support for this platform, thus no archive (terraform|tofu)_x.y.z_linux_powerpc64.zip.

That's fair, I came to the same realisation with all the other platforms being put back in now.

Very sloppy of me! I thought the build would fail by default if there were any unresolved/unreachable symbols. Seems the test scope gets slightly different treatment. I've elected to remove these instead... there's less to test and even less value in attempting to test it now.

Yeah, the test module isn't part of compilation unless you run cargo t. I'm fine with not having a bunch of unit tests for the function.

LGTM.

@ASleepyCat ASleepyCat merged commit 7d53fb5 into ASleepyCat:main Oct 16, 2023
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.

Errorneous Platform Resolution for macos/darwin
2 participants