Faliures when running arkade update on windows

[Johannestegner]

Expected Behaviour

Clean update without any issues when running arkade update on windows.

Current Behaviour

The replaceExec function fails due to not being able to replace the file (it's in use), I have a potential solution to this.

λ .\bin\arkade.exe update                                                                                                                                                                                                                                                                                                                                                                                                 
Downloading: https://github.com/alexellis/arkade/releases/download/0.10.9/arkade
9.41 MiB / 9.41 MiB [-------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------] 100.00%
Checksum verified..OK.
Replaced: D:\Repositories\arkade testing\bin\arkade.exe..OK.

 🚀 Speed up GitHub Actions/GitLab CI + reduce costs: https://actuated.dev

λ .\bin\arkade.exe version                                                                                                                                                                                                                                                                                                                                                                                                
This version of D:\Repositories\arkade testing\bin\arkade.exe is not compatible with the version of Windows you're running. Check your computer's system information and then contact the software publisher.

Possible Solution

The issue seems to be possible to partially fix by not "defering" the close calls on the stream. Basically just moving the closes down and not defer, as the file have to be closed to be replaced.

	df, err := os.OpenFile(newExec, os.O_CREATE|os.O_WRONLY|os.O_TRUNC, 0755)
	if err != nil {
		return err
	}

	if _, err := io.Copy(df, sf); err != nil {
		return err
	}
	_ = sf.Close()
	_ = df.Close()

	// Replace the current executable file with the new executable file
	if err := os.Rename(newExec, currentExec); err != nil {
		return err
	}

	return nil

Further, it seems like the original file needs to be renamed rather than replaced, and I can't figure out how to remove it after (seeing the process is running and hence locking the binary).

This could be done by just moving it to an .arkade.exe.old file and remove at another stage or next time the binary is updated. OR there is a solution which I have yet to figure out :)

Steps to Reproduce (for bugs)

1. Download or build arkade on windows.
2. Update

Your Environment

Using windows 10, currently cmder/conemu while the same issue exist in git bash.

• What arkade version is this?

0.10.9 (or rather the master branch while testing).

[Johannestegner]

Removed the part about the wrong binary being downloaded. Was testing in conemu instead of git-bash, which works.
The replace part still persist in bash as well.

[alexellis]

Does conemu support uname, which we use for determining OS/arch?

[alexellis]

For Windows, we could probably look at os.Executable() to see if it has an extension of .exe over here:

https://github.com/alexellis/arkade/blob/master/pkg/env/env.go#L17

That would make it work a little better in standard cmd.exe prompt.

Checking runtime.GOOS would probably also work and give back something like "Windows"

[Johannestegner]

While debugging I get the following:

2023/09/29 12:45:01 exec: "uname": executable file not found in %PATH%
2023/09/29 12:45:01 exec: "uname": executable file not found in %PATH%
2023/09/29 12:45:01 exec: "uname": executable file not found in %PATH%
2023/09/29 12:45:01 exec: "uname": executable file not found in %PATH%

Which makes me believe quite strongly that it does not support uname, hehe.
So no, it doesn't, and thats why it downloaded the wrong file for some reason.

---

Checking runtime.GOOS might work to get the OS for sure, but to future proof it I'd as well check the GOARCH to throw an error on aarch64 windows builds (which is not yet common, but hopefully will be in the future) :)

Is the uname part the only reason that you recommend git-bash?

[alexellis]

We talked about this with @JanDeDobbeleer ... and I think GOOS/GOARCH might be fine for Windows. Windows Arm64 isn't in scope for arkade at all at the moment.

#662

Is the uname part the only reason that you recommend git-bash?

arkade is only tested with bash, that's one of the reasons. If the above was sorted out, we'd need to look at other places where we execute binaries and see if cmd.exe could be supported.

[nunix]

Did a first test and if I ignore the uname errors (as written above, there's no impact in the download of the right file), I'm facing the issue about the file being used (as expected on Windows).

After the failed update, the file is still there.

To my understanding (and please note I can be terribly wrong here), the other Windows apps always use a 2nd binary (i.e. arkade-updater.exe) to create the illusion of an auto-upgrade.

Here's a high level process idea:

• Run arkade update
• Check if it's the latest version
• If not latest, downloads the latest arkade.exe and the helper binary arkade-updater.exe
• Launch arkade-updater.exe which kills the arkade.exe process and performs the copy of the new file
• Run arkade version

To bring a sort of comparison, this is also why there's currently no winget update winget as it ends with the same error.

Hope this makes sense.

[Johannestegner]

From my testing it is possible to rename, but not remove, the exe, so could name it .arkade.exe.old or similar and remove it at startup or with another command, that way, Arkade could execute the new binary and close, where the new binary does the cleanup.
Worst case, seeing Arkade is quite tiny, it could be done during the start of the update command and just leave the "old" file after until next update.

[alexellis]

@Johannestegner I like both of those ideas.

Can you send a PR for the simpler variant for GOOS=windows?

[Johannestegner]

I might have some issues creating the PR today, so if someone want's to do it quicker it's totally okay, else I'll try to get it done tomorrow (contract work + birthday will take up this day).

[alexellis]

Thanks for working on this @Johannestegner. I'll keep an eye out for a PR.

[Johannestegner]

Seeing uname -m/-s returns quite different values than runtime.GOOS/GOARCH and the current GetClientArch function is used quite a bit over the application, I think that the best way would be to make the GetClientArch function map to the expected output from uname -m and uname -s. Does this sound like a good initial solution?

I feel that this is quite a vital function for the application, so considering to first write a test for it. But, testing the runtime package is kind of a mess from what I can see in questions about it online.

Would it be preferable if I made two internal functions, say:

func getOs() string {
  return runtime.GOOS
}

Which are then used in GetClientArch, and write tests which mocks those functions?

There are very few test cases in the application, so I kind of wonder if it's even something that is wanted or if I rather should just test the binary on multiple platforms (I have linux amd/arm64/armhf, windows and mac intel/aarch64 available for testing if that is the case).

[alexellis]

There are very few test cases in the application

"Very few" - there are 156 unit tests 😢

Just go ahead and write the code how you think it should work, don't over think it and I'll see if it makes sense to add any tests during the review - once it's working on Windows, macOS and Linux. GetClientArch doesn't sound like something should be tested.

[Johannestegner]

Not sure why I wrote "very few in the application", that was bad of me, there are actually more than I initially saw :P

The reason I want tests is due to the fact that uname -s and uname -m returns a tad different values than the runtime package reports, eg x86_64 vs amd64 and such. I think that the -s one with goos should be fine, although I think one of them returns the value with first letter upper case. Just want to make sure I don't mess the depending functionality up in ways I was unsure about ;)

[alexellis]

Hope you had a good birthday.

Send a PR for how you think it should work, and I'll advise on whether I think additional automated tests are required.

How does that sound?

[Johannestegner]

Sorry for taking some time to do this, been really busy with ongoing contract.

So, by using the runtime package, it seems like most ARCHs and OS:es should work right out of the box with the current implementation. That is, just updating the GetClientArch() function with the following:

func GetClientArch() (arch string, os string) {
	return runtime.GOARCH, runtime.GOOS
}

But, the issue that I have is for arm32 platforms.

With the above code, the GOARCH would for a pi-zero return arm, which is kinda fine I guess, but the expected output of uname -m is not just arm but rather armv7l (zero2) or armv6l (zero1).
Looking at the code that makes use of the GetClientArch method, it looks like both of those would resolve to armhf for binary downloads (which I think is quite alright and correct). Would something like this be sufficient for the function?

func GetClientArch() (arch string, os string) {
	arch = runtime.GOARCH
	if arch == "arm" {
		arch = "armhf"
	}

	return arch, runtime.GOOS
}

If so, I can create a PR with that change right away, but I fear that it might be a bit to naïve, hehe.

[Johannestegner]

Tested with above code on:

Darwin (intel)
Darwin (m1)
ARM64 (rpi4) (fails)
ARMv7 (rpi-zero2) (fails)

It seems like the armhf idea is wrong, especially when looking at the MakeTools function, which rather checks for armv7l/armv6l/aarch64 where as the new method would just return armhf and arm64, so those two should probably be mapped to others (except for darwin, where result is expected to be arm64).

[Johannestegner]

The following half-ugly snippet does work though:

func GetClientArch() (arch string, os string) {
	arch = runtime.GOARCH
	os = strings.ToLower(runtime.GOOS)

	if arch == "arm" {
		arch = "armv7l"
	}

	if arch == "arm64" && os != "darwin" {
		arch = "aarch64"
	}

	return arch, runtime.GOOS
}

Tested on:

Darwin (intel)
Darwin (m1)
ARM64 (rpi4)
ARMv7 (zero2)
AMD64 (ubuntu, WSL2)

Windows currently returns "Error: update is not supported on Windows at this time", which is expected, but it also show that windows is resolved correctly in the application, which is positive!

Now, I'm not super happy with the code, the if parts are quite ugly, but you can decide if it's okay or not in the PR :)