-
-
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
Better Windows Support (msys + powershell/CMD) #662
Comments
Hi @JanDeDobbeleer thanks for logging this. When I put together arkade originally, I left this note in the README file:
What I am wondering, is whether arkade would support CMD.exe/PS with the patches above, or whether we'd run into issues elsewhere? The following may also not work as expected, since Git Bash may report Windows as an OS to Go:
Do you see Windows users not adopting WSL, or Git Bash and needing to run arkade from cmd.exe? So I have an interest in supporting Windows users better, but wonder what other hidden gotchas may exist. All the templates we maintain use "MING" as their string, so I wonder how you'd suggest working around that when the value is MSYS instead? We fork the Alex |
With this change, every shell should be supported as it only needs to understand where to store the binaries. The binaries are compatible. I was only focussing on installing tools, there might be something I didn't see yet for other functionality. I would rewrite the logic to identify the user's home := os.Getenv("HOME")
if len(home) > 0 {
return home
}
// fallback to older implementations on Windows
home = os.Getenv("HOMEDRIVE") + os.Getenv("HOMEPATH")
if home == "" {
home = os.Getenv("USERPROFILE")
}
return home
More from PowerShell, but yes.
In my opinion, there's no need to use The fact that I have |
@alexellis I did a "small" rework based on the comments above as indeed, you'd want the Windows executable to work in all shells and that implies we can't force the user to do any work other than download and run arkade. I did some digging and we can get the architecture from Windows itself, this will work anyways, regardless of where you run it as we can use a There are two commits to look at:
|
What about 64-bit ARM Windows? |
Couple of comments - > JanDeDobbeleer@0c48c24 |
@alexellis that should return the same, oh-my-posh uses the same logic. |
Hey @JanDeDobbeleer,
|
|
I'm not much of a windows user myself, but reading about this, it seems similar to calling gnu binaries on a unix system, so it should be fine as you said so. If you willing to create a PR with the mingw version as discussed above, I don't see any reasons why we shouldn't add this. |
As a side-note, I'd consider working with cmd.exe a non-goal at this time. It may change in the future, at which point we now know what's required. |
@JanDeDobbeleer when do Windows users need to use Msys2 over Git bash? |
@alexellis this was me running this inside PowerShell if I'm not mistaken. But it's been a while, I'll have a proper look in the course of next week to address the comments too. |
@alexellis just had a look and made sure to make the most minimal changes to support this correctly.
This is not about cmd.exe but any shell outside of git bash (and truth be told, we need to get rid of that monster). You can find the slimmed down version here. |
Thanks for continuing the discussion @JanDeDobbeleer 👍 What I like: It's got a windows-only build tag: What I'm unsure about:
Does this require CGO? Am I over-simplifying this? If all we're doing is finding out whether we're on Windows or not, can't we get that by the .exe extension of the binary or the GOOS variable? |
What do you get with this?
|
@alexellis it doesn't require cgo, all it does is call the native windows API (which, I understand, looks very alien of you've never been there before). Additionally, you can run amd64 on Windows ARM and 386 on amd64 so you'll want to know the actual architecture to offer the best compatible experience. runtime.GOOS is a hardcoded "windows" string, it never tells you the architecture. |
Do we need an architecture? Only x86_64 is supported by arkade for Windows downloads. I can't see a need to ever support 386 (32-bit Windows). Perhaps one day ARM64 will be popular because Windows devs are flocking to the Apple M1/M2? Then can't that also be derived from the binary too?
|
@alexellis those two are whatever you built for. It doesn't necessarily reflect the actual OS's architecture (see my previous remark). For oh-my-posh this is required as we do have that support, and this way it's also available to anyone going forward. Windows ARM will become the norm, just like Apple also moved to it for obvious reasons. |
That’s OK with me for where we are today. There is no support in any of our app definitions for Windows ARM. So the only platform is AMD64 - I.e. if you’re using a .exe we know the platform exactly If we added ARM64, the built flag would be different and so input the correct architecture. Or are you telling me that someone could install the arm binary on amd64, then execute it? |
The other way around, you can run a amd64 binary on ARM and nothing would work. So you need this. |
Spell out the scenario for me please? |
@alexellis can we take a step back before I do that as it's important to understand why I did this. All it does is mimic the same functionality that's in As for your question, I can install and run the Windows amd64 arkade binary on a Windows arm64 system which will then also install amd64 binaries and not arm64 binaries when we only look at |
Expected Behaviour
It works on all Windows flavors and shells
Current Behaviour
It only works for mingw and git bash (which is hardly a native experience)
Are you a GitHub Sponsor (Yes/No?)
Check at https://github.com/sponsors/alexellis
Possible Solution
There are two issue
Steps to Reproduce (for bugs)
Supporting msys
Error: operating system "MSYS_NT-10.0-22000" is not supported. Available prefixes: linux, darwin, ming.
pops up.Proposed fix
Applied fix result
Supporting additional shells
Error: env-var HOME, not set
pops up. 4.Proposed fix
To apply the result, I also had to ensure oh-my-posh can resolve on msys:
Applied fix result
This fix has also been validate on macOS to ensure no breaking change occurs:
I would additionally propose to create a helper function to correctly resolve Windows in the templates instead of relying on the
ming
string in all tools. That I haven't done yet due to time constraints.Context
I want to allow people to use arkade anywhere, without friction
The text was updated successfully, but these errors were encountered: