Open JanDeDobbeleer opened 2 years ago
Hi @JanDeDobbeleer thanks for logging this.
When I put together arkade originally, I left this note in the README file:
Windows users: arkade requires bash to be available, therefore Windows users should install and use Git Bash
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:
+ if runtime.GOOS != "windows" {
+ return os.Getenv("HOME")
+ }
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 uname -a
command to get this data in the first place, so how does your cmd.exe system have that available, or is it coming from somewhere else in your patch?
Alex
What I am wondering, is whether arkade would support CMD.exe/PS with the patches above, or whether we'd run into issues elsewhere?
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
folder the same way we do for oh-my-posh as that has been proven to work cross platform for quite a while already (mainly removing theruntime.GOOS != "windows"
check above):
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
Do you see Windows users not adopting WSL, or Git Bash and needing to run arkade from cmd.exe?
More from PowerShell, but yes.
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?
In my opinion, there's no need to use uname -a
on Windows as with the change above you can simply populate the OS
variable with windows
when runtime.GOOS == "windows"
and everything will work, regardless of the shell being used In that case it's a simple find and replace if HasPrefix .OS "ming"
-> if eq .OS "windows"
. That's the cleanest fix, but I tried to stay compliant with the current functionality.
The fact that I have uname
in my path is because I added the git's bin
folder to my path. That one contains the executable (so this is the things that needs to be highlighted in the README for Windows.
@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 win32
call. I have a branch with the changes and adjustments to the tests/tools and all validations pass. I also validated this to work on Windows and macOS.
There are two commits to look at:
uname
for windows (https://github.com/JanDeDobbeleer/arkade/commit/0c48c246d1232bbbc04a19b0b67c55f9610bc8ea)In my opinion, there's no need to use uname -a on Windows as with the change above you can simply populate the OS variable with windows when runtime.GOOS == "windows" and everything will work, regardless of the shell being used In that case it's a simple find and replace if HasPrefix .OS "ming" -> if eq .OS "windows" . That's the cleanest fix, but I tried to stay compliant with the current functionality.
What about 64-bit ARM Windows?
Couple of comments - > https://github.com/JanDeDobbeleer/arkade/commit/0c48c246d1232bbbc04a19b0b67c55f9610bc8ea
What about 64-bit ARM Windows?
@alexellis that should return the same, oh-my-posh uses the same logic.
Hey @JanDeDobbeleer, I have several questions about this implementation:
Is it possible to implement this in a way, that it maps msys to ming, and ming stays compatible with mingw aswell?
See, if we were to implement this 1:1 we would have to rewirte every single template that currently uses ming as the OS flag in the template conditions, then we need to change the tests as well, then we need to change the end2end tests too cause they only run on linux amd64
...I think you can see the never ending to-do-list here, it is a really huge leap tho'.
Isn't it easier to map the "windows terminal emulators" to mingw?
Is it possible to avoid calling dll files?
To keep arkade
as a selfcontained, statically linked binary is a design choice that I don't think we need to change and also really good thing. (but correct me if I'm wrong here, @alexellis)
@Shikachuu
@Shikachuu
sure, that could be done I guess.
no, and there's no harm in calling win32 on Windows, golang does the same itself btw. They just abstract it away for you 😄
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.
It only works for mingw and git bash (which is hardly a native experience)
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.
Error Error: operating system "MSYS_NT-10.0-22000" is not supported. Available prefixes: linux, darwin, ming. pops up.
@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.
I'd consider working with cmd.exe a non-goal at this time
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: //go:build windows
What I'm unsure about:
import (
"fmt"
"unsafe"
"golang.org/x/sys/windows"
)
var (
dllKernel = windows.NewLazyDLL("kernel32.dll")
procGetNativeSystemInfo = dllKernel.NewProc("GetNativeSystemInfo")
)
_, _, _ = procGetNativeSystemInfo.Call(uintptr(unsafe.Pointer(&systemInfo)))
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?
GOOS=windows go build -o jan.exe
./jan.exe
package main
import (
"fmt"
"runtime"
)
func main() {
fmt.Printf("The current operating system is: %s\n", runtime.GOOS)
}
@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?
GOARCH
GOARM
@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?
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 GetClientArch
for unix and mac. That one uses uname -m
to identify the architecture and calling GetNativeSystemInfo
is the equivalent on Windows. Yes, we could resort to GOOS
and GOARCH
but the same argument goes for unix and mac as well. There's no difference. The only "downside" is that you always have to install the correct arkade binary.
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 GOOS
and GOARCH
.
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