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

Updated GetClientArch to use runtime package instead of uname checks. #988

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Updated GetClientArch to use runtime package instead of uname checks. #988

wants to merge 1 commit into from

Conversation

Johannestegner
Copy link
Contributor

@Johannestegner Johannestegner commented Oct 10, 2023 •
edited
Loading

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 executing uname 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 that windows is not supported while on supported platforms, the application is downloaded correctly.

Remarks

The implementation will for now return ming instead of windows, 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

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • [] Breaking change (fix or feature that would cause existing functionality to change)

Checklist:

  • My code follows the code style of this project.
  • I've read the CONTRIBUTION guide
  • I have signed-off my commits with git commit -s
  • I have tested this on arm, or have added code to prevent deployment

@Johannestegner Johannestegner force-pushed the refactoring/goos-instead-of-uname branch from 9a2adf1 to a74e645 Compare October 10, 2023 09:12
Johannestegner
Johannestegner commented Oct 10, 2023
View reviewed changes
pkg/env/env.go Outdated Show resolved Hide resolved
alexellis
alexellis reviewed Oct 20, 2023
View reviewed changes
pkg/env/env.go Outdated
)

// GetClientArch returns a pair of arch and os
func GetClientArch() (arch string, os string) {
task := execute.ExecTask{
Command: "uname",
Copy link
Owner

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

Copy link
Contributor Author

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! :)

@Johannestegner Johannestegner force-pushed the refactoring/goos-instead-of-uname branch from a74e645 to 39db32c Compare October 23, 2023 07:13
@Johannestegner
Copy link
Contributor Author

PR updated and Type changed to Breaking (see "remarks" in description).

Further, I included both go:build and build directives, I'm not 100% sure this is needed, but wanted to make sure I don't break the build (let me know if the old build directive should be removed).

@alexellis
Copy link
Owner

Although this PR allows for usage of other terminals than git-bash/ming in windows, this will make the ming target break, this due to the fact that earlier, the uname command determined if it was windows with ming while any terminal on windows now will return windows.

This will most likely require the ming check to be removed and replaced with a windows check (should this be included in this pull request)?

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.

@Johannestegner
Copy link
Contributor Author

What if MING was returned from the function instead of "windows"?

This could work, as long as the application downloaded doesn't actually require ming.
It could be a decent first step at the least.

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.

@Johannestegner
Created new 'env_windows' file.
284bdd4 
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]>
@Johannestegner Johannestegner force-pushed the refactoring/goos-instead-of-uname branch from 39db32c to 284bdd4 Compare October 24, 2023 07:10
@Johannestegner
Copy link
Contributor Author

Johannestegner commented Oct 24, 2023 •
edited
Loading

Replaced return value with hardcoded ming string, tested and it works fine both in windows terminal as well as git-bash :)

λ .\arkade.exe get yq
Downloading: yq
2023/10/24 09:06:06 Looking up version for yq
2023/10/24 09:06:07 Found: v4.35.2
Downloading: https://github.com/mikefarah/yq/releases/download/v4.35.2/yq_windows_amd64.exe

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 :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@alexellis alexellis alexellis left review comments

@yarons yarons yarons left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@Johannestegner @alexellis @yarons