fix: Don't use replace-executable for WinGet installations

[bradenhilton]

Fixes #2736

Very basic, not ready for merge yet.

[twpayne]

This looks very reasonable. What are the remaining steps needed to make this ready to merge?

[bradenhilton]

At a minimum, I think we need to wait for go1.21 (expected August) for the necessary fix to be released.

I'd also like to figure out a nicer way to test, as I was doing everything manually when writing the code.

[bradenhilton]

Some general thoughts on how this can be improved:

WinGet has configurable root directories for portable apps, for both user and machine scopes:

• Portable Package User Root, defaults to %LOCALAPPDATA%\Microsoft\WinGet\Packages\¹²
• Portable Package Machine Root, defaults to %PROGRAMFILES%\WinGet\Packages\¹²
• Default install root (unsure at the moment if this affects portable apps)

These paths can be set in WinGet's settings.json file, the location of which differs depending on how WinGet was installed:

• %LOCALAPPDATA%\Packages\Microsoft.DesktopAppInstaller_8wekyb3d8bbwe\LocalState\settings.json if packaged, most users will likely have this
• %LOCALAPPDATA%\Microsoft\WinGet\Settings\settings.json if non-packaged or built from source

The absolute executable path reported by chezmoi is likely to be a symlink when installed via WinGet (~\AppData\Local\Microsoft\WinGet\Links\chezmoi.exe³)

A more robust solution would employ an approach like this:

• Resolve the executable symlink to its target path (typically ~\AppData\Local\Microsoft\WinGet\Packages\twpayne.chezmoi_Microsoft.Winget.Source_8wekyb3d8bbwe\chezmoi.exe³ with default settings), if necessary
• Read and unmarshal WinGet's settings.json, if found
• Check if the real executable path contains any of the paths present in settings.json, or their default values if not specified

The current solution in this PR only really handles the default path(s), which is probably acceptable for a first pass. That said, one could run install.ps1 to install chezmoi to ~\WinGet\³ (which is admittedly a little silly, but definitely possible)⁴, and this would definitely confuse chezmoi with the current approach, so it should be improved at some point to support as many users as is feasible.

I think I'm right at my limit when it comes to Go, so if it gets any more complicated than this, I might not be able to work on it without some more learning.

Footnotes

1. The defaults only apply if no corresponding value is specified, they are not set by default in the settings.json file. ↩ ↩²

2. The default values must be expanded, non-default values must be absolute paths so expanding is not necessary. ↩ ↩²

3. ~ used for shorthand, not to imply it is a valid absolute path. ↩ ↩² ↩³

4. Granted, one could also use install.ps1 to install chezmoi to the actual WinGet package directory, without installing chezmoi via WinGet. I don't think there exists a perfect solution to this. ↩

[bradenhilton]

784ec9a is my current snapshot. It's super rough and definitely needs more refinement, but the inability to test properly without go1.21 makes it rather tedious at the moment.

[twpayne]

Thanks very much for the updates. Happy to wait for Go 1.21 for this.

[bradenhilton]

Just FYI, if/when anyone feels like it, I'd absolutely welcome a review on the file unmarshalling, logic, flow, and error handling, as I'm not particularly well versed in these at the moment.

I attempted to map it out as:

• The default path values are used unless the settings.json file both exists and contains the appropriate value(s).
• The settings.json file existing but being somehow unreadable at the time of unmarshal means we should probably exit early, as we can't determine if the file contains the value(s) or not.
• If the settings.json file and value(s) exist, but we can't find a chezmoi binary there, we should probably also exit.

I'm reasonably confident that the command invocation works, or at least it did when I built go from source to test it a few months ago.

I think treating WinGet errors as chezmoi errors should be fine for now. We can absolutely handle them in a follow up PR if needed.

[bradenhilton]

Preface: In my working copy I have split the Windows-specific upgrade behavior into upgradecmd_windows.go, and doing this may have introduced bugs I am not aware of. Regardless of what I'm about say, I will still commit this code in a snapshot.

I don't think this is possible.

Firstly, this is just awful to test. I'm open to any ideas anyone might have to make it less painful.

I created a build from my branch with version 2.37.100 and dropped it into the WinGet install directory, this causes chezmoi upgrade to error out via WinGet:

❯ chezmoi upgrade
Found chezmoi [twpayne.chezmoi] Version 2.38.0
This application is licensed to you by its owner.
Microsoft is not responsible for, nor does it grant any licenses to, third-party packages.
Successfully verified installer hash
Extracting archive...
Successfully extracted archive
Starting package uninstall...
Unable to remove Portable package as it has been modified; to override this check use --force
chezmoi: exit status 0x8a150057

After learning this, I created two more chezmoi builds (as chezmoi-local to avoid conflicts) with versions 2.37.100 and 2.38.0. Then I created custom local WinGet manifests for them which download the zips from my Dropbox (this gives several anti-virus false positives which I had to manually allow in order to proceed).

Here is chezmoi-local doctor with version 2.37.100:

❯ chezmoi-local doctor
RESULT    CHECK                MESSAGE
ok        version              v2.37.100, commit 5682362fb54c388a58ee7af9384dd8f19983a355-dirty, built at 2023-08-23T12:49:15Z
warning   latest-version       v2.38.0
ok        os-arch              windows/amd64
ok        systeminfo           Microsoft Windows 10 Pro (10.0.19045 N/A Build 19045)
ok        go-version           go1.21.0 (gc)
ok        executable           ~/AppData/Local/Microsoft/WinGet/Links/chezmoi-local.exe
ok        upgrade-method       winget-upgrade
...

And here is chezmoi-local upgrade, again, version 2.37.100, in which I hard coded the path to my local manifest for 2.38.0:

❯ chezmoi-local upgrade
Found chezmoilocal [bradenhilton.chezmoilocal] Version 2.38.0
This application is licensed to you by its owner.
Microsoft is not responsible for, nor does it grant any licenses to, third-party packages.
Downloading https://<dropbox-url>
  ██████████████████████████████  20.0 MB / 20.0 MB
Successfully verified installer hash
Extracting archive...
Successfully extracted archive
Starting package uninstall...
An unexpected error occurred while executing the command:
remove: unknown error: "~\AppData\Local\Microsoft\WinGet\Packages\bradenhilton.chezmoilocal__DefaultSource\chezmoi-local.exe"
Uninstall failed with exit code: 0x8a150003 : Executing command failed

I'm not certain, but I believe this error is given because chezmoi is running at the time of upgrade/uninstall.

[twpayne]

Thank you very much for working on this!

I'm not certain, but I believe this error is given because chezmoi is running at the time of upgrade/uninstall.

Previously I've worked around this by renaming (but not deleting) the current executable. See #1605 (comment).

[bradenhilton]

Previously I've worked around this by renaming (but not deleting) the current executable. See #1605 (comment).

I don't know how to incorporate this into the upgrade flow for WinGet.

I haven't checked yet, but I'm pretty sure chezmoi.exe needs to exist at time of removal, and it needs to be the exact same chezmoi.exe that WinGet installed.

[twpayne]

Thanks again for all your work here. It might well be the case that this self-upgrade through winget functionality is simply not possible to achieve on Windows. Feel free to close this and #2736 if needed.

[bradenhilton]

I'm inclined to agree, but I still think we need to do something, as chezmoi currently thinks it can use replace-executable with WinGet installs, which will break them.

Should I change c.winGetUpgrade() to print a simple message informing the user that chezmoi upgrade is not currently supported for WinGet, along with the command to upgrade chezmoi via WinGet itself? We could also do this for Scoop and Chocolatey in follow up PRs if needed.

I'm thinking the commit message can be changed to fix: Don't use replace-executable for WinGet installations or similar.

We could also consider releasing new Windows binaries specifically for use with package managers which are built with noupgrade, but it seems a bit excessive.

[twpayne]

Should I change c.winGetUpgrade() to print a simple message informing the user that chezmoi upgrade is not currently supported for WinGet, along with the command to upgrade chezmoi via WinGet itself? We could also do this for Scoop and Chocolatey in follow up PRs if needed.

Yes, this sounds perfect.

We could also consider releasing new Windows binaries specifically for use with package managers which are built with noupgrade, but it seems a bit excessive.

Agreed that this would be excessive at the moment.

[bradenhilton]

FYI, if we aren't invoking winget.exe (which is currently the case), I don't think we require the changes that were released in go1.21.

If we needed to invoke winget.exe, that could technically mean we need to wait for go1.21 to be our minimum supported version, which would mean waiting for go1.22.

[twpayne]

OK. We can go with this as the "official" versions of chezmoi are built with Go 1.21. Anyone building chezmoi with Go 1.20 will not need the upgrade command.

I'll merge this now and just do a little tidy-up to reduce the code duplication between upgradecmd.go and upgradecmd_windows.go.