-
-
Notifications
You must be signed in to change notification settings - Fork 289
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
Updated GetClientArch to use runtime package instead of uname checks. #988
base: master
Are you sure you want to change the base?
Updated GetClientArch to use runtime package instead of uname checks. #988
Conversation
9a2adf1
to
a74e645
Compare
pkg/env/env.go
Outdated
) | ||
|
||
// GetClientArch returns a pair of arch and os | ||
func GetClientArch() (arch string, os string) { | ||
task := execute.ExecTask{ | ||
Command: "uname", |
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'm sorry but this is not what I was asking for.
There should be a separate version of GetClientArch() which is combined only for Windows which looks at runtime.GOOS and returns the correct value. It could even be hard-coded given that it will only build in for Windows.
Then leave the existing code as it is for "unix"
Here's an example of how it works in Go: https://dave.cheney.net/2013/10/12/how-to-use-conditional-compilation-with-the-go-build-tool
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.
Ahh, I must have misunderstood, will fix asap! :)
a74e645
to
39db32c
Compare
PR updated and Further, I included both |
What if MING was returned from the function instead of "windows"? Otherwise, it'd require a lot of search and replace and testing to make sure we'd covered all the various templates and their unit tests. |
This could work, as long as the application downloaded doesn't actually require ming. From looking at the list of applications supported on windows in arkade, it seems like most runs fine in the standard terminal as well. And worst case, someone will report a bug. This could also possibly remove the "breaking change" part, as it would work just as before for ming users. |
Conditionally compiled on windows targets and makes use of the runtime.GOARCH and runtime.GOOS constants instead of executing uname to get the arch and os. Signed-off-by: Johannes Tegnér <[email protected]>
39db32c
to
284bdd4
Compare
Replaced return value with hardcoded
Kinda want to leave a comment about it being used like that to keep application installations working as they are... If you want me to add that, please let me know :) |
Description
This pull request adds a
env_windows.go
file which will only be included when built for windows targets (+build and go:build directives added in both files).The new file makes use of the
runtime
lib to fetch architecture and OS instead of relying on executinguname
and parsing the result.Motivation and Context
The primary motivation for this PR is the discussion in #977, and the fact that using goos/goarch will allow for the values resolving fine on windows without the use of the
git bash
tool.How Has This Been Tested?
This has been tested by building (
make dist
) the binaries, then transferring them to the following platforms:darwin-arm64
,darwin-amd64
,linux-amd64
,linux-armv7l
,linux-arm64
.Running
arkade get --help
to see the resulting architecture and OS resolved as the default variables.Running
arkade get <application>
and see thatwindows
is not supported while on supported platforms, the application is downloaded correctly.Remarks
The implementation will for now return
ming
instead ofwindows
, this to keep the currently created applications installable on windows computers.Change will hence have no breaking change, while it might be an idea to, in the future, update applications for windows to target
windows
and keep ming separated for applications that actually requires ming.Types of changes
Checklist:
git commit -s