Improve Windows support

[twpayne]

chezmoi is currently targeted at macOS/Linux, but there is no reason why it shouldn't also run on Windows. Contributions improving Windows support are very welcome.

[twpayne]

#43 adds CI on Windows, but it is not yet passing due to path issues.

[roelofvkr]

I looked into it a little bit today and the issue lies in using syscall.Stat_t here. This is a direct linux systemcall and is unavailable on windows.

From what I can find the best way to get an ID of a volume on windows would be using GetVolumeInformationW. Which can return a uint32 with the serial number the operating system generates when the volume is formatted.

Another option would be to use the drive letter (or in the case of network shares, the ip address and the share name), but that would require a change of FSMutator.devCache and FSMutator.tempDirCache to use strings instead of uints on windows.

[zb140]

I've managed to get a reasonably usable Windows build going (code here: https://github.com/zb140/chezmoi/tree/windows). I didn't see @roelofvkr 's comment before I started down this path, so my fork isn't based on his, but we came up with very similar solutions using GetVolumeInformation. I also implemented a custom FSMutator.Chmod function for Windows using ACLs to change permissions, and fixed Config.exec to work on Windows.

With these changes (plus a total hack in go-shell to return a value on Windows), all of the commands in the Quickstart guide work as expected, plus a few others. archive and upgrade don't work yet, and I haven't tested chattr, import, remove, update, or any of the secret or script stuff. Everything else seems to work (though my experience with chezmoi is very minimal, so I'm not sure I'd notice if things were subtly broken).

It will take quite a bit of cleanup to be in a mergeable state (apart from anything else, I think it breaks all of the rules in the Contributing doc except the one about commit message formatting 😉 ) but I wanted to go ahead and share what I have. There's also a couple of total hacks in there, plus this is the first or maybe second time I've ever worked on a project in Go, so I'm guessing my code will probably drive a legit Go programmer crazy 😄 But I'm super excited to have something working, and I'm sure everything else can be fixed.

[twpayne]

Thanks @zb140, your branch looks very promising - although, as you say, it does need a some cleanup. With a couple of fixes (some imports are missing in cmd/config_other.go and cmd/update_other.go, and getOwner in cmd/upgrade_other.go needs a typecast to int in the return statement) the tests pass on macOS, which is reassuring. As a nit, don't forget to run gofmt on your code (it looks like your editor is using four spaces for indentation, it should use tabs).

go-shell should already support Windows: https://github.com/twpayne/go-shell/blob/master/shell_windows.go. If this doesn't work, please can you report a bug there.

For a Windows-specific chmod, is it not sufficient to use the github.com/hectane/go-acl.Chmod function? If this function doesn't set DELETE permissions, then it's probably worth submitting a PR upstream.

Note the Windows branch uses github.com/google/renameio, which doesn't seem to support Windows yet, see google/renameio#1 and google/renameio#5.

Again, this is very promising :)

[zb140]

Looks like I had an outdated version of go-shell, which explains why it wasn't working for me. After updating it does indeed work, but since it relies on %ComSpec% it will always return some version of cmd.exe. Since the default in newer versions of Windows is PowerShell and Microsoft has all but deprecated cmd.exe, it'd be nice to be able to use that instead. There is a registry setting that indicates whether a user has switched back to cmd.exe, but I'm not too sure if reading that is necessarily the right way to go. In any case, that's outside the scope of this project.

My intent was always to submit a PR to the go-acl project to handle the DELETE issue; I just hadn't done it yet because I was trying to make progress as quickly as possible. One of the cleanup steps would certainly be to split that piece off and submit a PR upstream.

For renameio, it looks like it's functional on Windows but not correct: in other words, it does actually create/rename files, but it doesn't do so atomically. That's why I didn't notice an issue: it appeared as though it was doing the right thing even though it wasn't really. I'm not too sure what the right resolution here is, other than to try to help get google/renameio#5 merged.

Are there any instructions on how to run the tests? I'd love to give them a try on Windows; it'd be nice to have a clearer picture of exactly where it stands in terms of what isn't working. Also, I don't have a Mac, but I've got a couple of Linux boxes, so I should be able to fix up the build issues (and formatting) pretty easily too.

I didn't have any time tonight to work on it any further, but thanks for all your comments. I expect to be able to spend some more time working through some of these issues this weekend.

[twpayne]

You can run the tests the usual go way:

go test ./...

There are a few dependencies that are also worth testing on Windows:

go test github.com/twpayne/go-vfs/...
go test github.com/twpayne/go-xdg/...

The XDG stuff helps chezmoi put config files and data in the right place in the user's home directory. This is something that will probably be different on Windows too.

For cmd.exe / PowerShell, chezmoi is likely to be used by "power" users who likely also use PowerShell, so I'm happy to have PowerShell hard coded in on Windows initially.

For renameio, it's OK to proceed with it in its current state (functional but not correct, as you say).

For the formatting, running

make format

will reformat everything. You'll need to install some tools, best to look at the install section of .travis.yml.

Writing this tells me that I should add more documentation to CONTRIBUTING.md.

[twpayne]

Contribution guidelines updated in #309.

[zb140]

Thanks for all the pointers! I ran the go-vfs tests and encountered a few issues that needed fixing; I filed twpayne/go-vfs#25 to track that work. I also tried the go-xdg tests; the only failures there were due to path separators mismatching. Since Windows APIs actually accept / except for in obscure edge cases, those cases actually do the right thing anyway, and I haven't done anything to fix them (at least, not yet).

The tests for chezmoi itself still don't all pass yet either, though many of them are working. I'll be working on that next.

[zb140]

Come to think of it, does it make sense for the Windows-specific Chmod to be pushed up into go-vfs? Not all the stuff to add DELETE permission; that certainly belongs to go-acl (I've filed hectane/go-acl#11 to track moving it there). I just mean something in osfs that calls go-acl.Chmod on Windows.

Here's why I'm asking: some of chezmoi's tests are failing on Windows because they call config.ensureSourceDirectory, which in turn calls fsmutator.Chmod. When working directly on the real filesystem, it works fine, but when using a PathFS (as the tests of course do) it fails because the prefix hasn't been added. I don't see an obvious way to add it from within fsmutator.Chmod (though as I said I'm not very experienced with Go, so maybe there is one that I just couldn't figure out). But even if there were a way to do it, that doesn't really feel like the right solution to me. go-vfs does provide a Chmod, and it would be nice if it more-or-less-transparently does the right thing on Windows too.

[zb140]

Also, I'm having a little trouble understanding why filepath.Abs is used in runAddCmd here. It's causing some test failures on Windows because it adds a volume specifier to paths that start with a separator (which is technically the right thing for it to do, even if it is a little unexpected even for people like me who've been Windows devs for quite some time now). Since I don't really understand why filepath.Abs is needed here, I'm not really sure how to make it do the right thing on Windows. Would you mind explaining a bit?

[twpayne]

Come to think of it, does it make sense for the Windows-specific Chmod to be pushed up into go-vfs?

I'd like go-vfs to be a very thin wrapper around the os package so its behavior can be as transparent as possible. That said, I think that there's a reasonably tidy way to do this. I'll implement this now, it'll break API compatibility, but this is what Go modules are for.

[twpayne]

Also, I'm having a little trouble understanding why filepath.Abs is used in runAddCmd here.

Fair question :) The reason is that the user can run the chezmoi command from any directory, and we need to normalize filenames to check that they're in the target directory. Normalizing early (with filepath.Abs) was the easy way to do this. The work-around might be to also normalize DestDir (probably here).

[twpayne]

Come to think of it, does it make sense for the Windows-specific Chmod to be pushed up into go-vfs?

Added in twpayne/go-vfs#30. Please help improve WindowsOSFS.

[zb140]

Come to think of it, does it make sense for the Windows-specific Chmod to be pushed up into go-vfs?

Added in twpayne/go-vfs#30. Please help improve WindowsOSFS.

Awesome, thanks! I'd be happy to help.

[zb140]

I've been slowly chipping away at the failing tests for the last couple of days now. I've managed to get quite a few of them working, but I'm nearing a point where I'm not really sure how to continue making progress.

I still haven't come up with a good solution to the filepath.Abs issue I mentioned above. Calling Abs to normalize the paths is definitely the right thing to do outside the context of the tests (and after all, there's nothing stopping someone from passing in an absolute path in the first place). But while running the tests, PathFS expects to be able to prepend some arbitrary other path on the front of an ostensibly absolute path, which on Windows will always contain a volume specifier. I just can't come up with a solution that works both during the tests and during regular operation.

On top of that, file permissions have started to present some really big challenges. Changing permissions works well thanks to the new Windows-specific Chmod in go-vfs, but of course Stat/Lstat don't know anything about Windows ACLs. That meas anything that tries to check whether a given permission is set will not do the right thing. "So", I figured, "let's just override Stat and Lstat to check the ACLs and return an os.FileInfo that contains the correct mode information. I spent all day today trying to update go-acl to support reading permissions in addition to setting them, only to find out it's basically impossible[1]. Even if I could read back the permissions, I don't know enough Go to be able to replace the mode info inside an os.FileInfo with my own.

Having said all that, I'm not giving up yet. I really want a tool I can use to manage my config files across all the systems I use, and chezmoi is super, super close to exactly what I need. The only thing missing is being able to run on Windows. So I'll keep working on it, but I think progress is going to be slower while I try to work through these issues.

I hope this doesn't sound like I'm complaining; I'm just feeling a little frustrated after trying to work through these issues for most of the day today 😄

---

[1] Windows uses "Security Identifiers" to map sets of permissions to users and groups in an ACL. There are a few well-known SIDs that represent "whoever created this file", "the primary group of whoever created this file", and "everyone" (among others). go-acl uses these well-known IDs when setting permissions. However, when you try to read back the permissions, these well-known IDs have been "converted" to whichever user is appropriate for the file, so it is not possible to query based on the well-known ID. It is possible to query the owner and group for a given file, but on all of the files I've tried it on, the owner and group are set to the same Security Identifier, making it impossible to tell the difference between user and group permissions[2].

[2] For files whose ACLs were set with acl.Chmod in the first place, there's a hacky workaround for this. The way acl.Chmod works, it creates separate entries in the ACL for user and group. Both of these entries are still present when reading the ACL back out, even though they technically point to the same Security Identifier. By assuming the entries are returned in the same order in which they were created (which in practice is true) it is possible to distinguish user and group permissions. But crucially, this will only work for files whose ACL was originally set with acl.Chmod.

[twpayne]

Thanks for your hard work on this! Here are some thoughts.

For the filepath.Abs issue, can we just strip the volume identifier off the second path when appending? PathFS basically just exists to provide cheap sandboxes for running tests. It's not used outside the tests.

For the ACLs, a couple of ideas:

1. chezmoi needs to be idempotent, but it doesn't matter if we do extra work. For example, setting the file permissions every time the user runs chezmoi apply is correct, even if it feels a little ugly and inefficient.

2. chezmoi only distinguishes between public and private (user only) files. Maybe we should just sidestep UNIX-style file permissions on Windows and use ACLs directly. It doesn't matter that we can't determine the equivalent of "group" on Windows as we don't use this anyway.

3. os.FileInfo is an interface, not a struct. So you can return any struct that satisfies the interface from your Chmod function. That said, given (2) above, it might be simpler to use a different permissions setup on Windows above.

[zb140]

I'm happy to help out when I can, and thankfully I don't get discouraged easily 😄

Thanks a lot for the suggestions. For filepath.Abs, yes, if it's ok to make the assumption that PathFS is mostly just for tests, I think that's the best solution. I tried implementing it this way and things seemed to mostly work. One of the dump tests is failing now (because targetPath is now absolute when the test expects it to be relative). I've run out of time for today but I don't anticipate this one being hard to fix.

For the permissions, I like your suggestion of side-stepping UNIX permissions entirely. I had a go at implementing this and the results are looking promising, but it's a relatively big undertaking, so I don't have it working just yet. I think I'm on the right track now though.

The good news is, nearly all the tests are passing now. There are 4 individual failing test cases: 3 to do with private files, plus the dump test I mentioned before. Everything else is passing now, including (somewhat surprisingly, to me anyway) the script tests. I've pushed my changes up to the windows branch in my fork. As usual, it's a work in progress, and there's still a couple of total hacks in there (I'm looking at you, temporary script file names) so if anyone happens to look at it, go a little easy on me -- there's still work left to do 😉

[zb140]

3 down, 1 to go! I've updated my fork with some new fixes that address the permissions issues. At this point, all but one of the tests are passing, and the last one is just the dump test with the absolute vs relative path issue. I'm hoping to get that one taken care of tonight too, so fingers crossed...

Note that the new updates in my fork depend on pending upstream changes in go-acl, and some of the permissions tests won't pass without twpayne/go-vfs#32. I've just noticed that one of the build checks on that PR failed, so I'll look into that too.

[zb140]

🎉 🎉 🎉
Turns out the dump problem was because I was calling filepath.Abs on an empty string, which apparently returns the full path of $PWD (I get why it does that, but I can't say I was expecting it...)

As of now, all the tests are passing on Windows using my windows branch. There's definitely still work to be done (for instance, upgrade doesn't work yet), and more cleanup to do, but I think it's safe to say we're on the home stretch now.

[twpayne]

This is great to hear!

[twpayne]

@zb140 Thank you so much for your work on this so far. I'm thinking of a v2.0.0 release soon. Should this include Windows support?

[zb140]

Hey @twpayne, sorry, I've been busier than normal for the last week or two and somehow I missed this notification. I'd love to get Windows support into v2.0.0 if it's not too late. Work on this kind of stalled out because it depends on hectane/go-acl#14, which hasn't been merged (or I think even reviewed) yet. I'll post over there and ask for an update.

Last time I worked on this, all the tests were passing and everything seemed to work as expected, with the exception of upgrade (which turns out to be difficult because Windows won't let you overwrite a running executable). I also did some general cleanup of the code to make it ready for review. This weekend I'll rebase onto the latest master and make sure things are still working, and hopefully there will be some movement on go-acl by then.

[twpayne]

Thanks very much for the update. There's no real rush for a 2.0.0 release - I'm thinking maybe sometime this summer.

If hectance/go-acl#14 doesn't advance, I'm happy to maintain a fork for chezmoi so we don't get blocked on this.

Let's just disable the update command on Windows for now.

[zb140]

I've updated my branch to the latest from master and fixed up a couple of test issues on both Windows and Linux. As of now, all tests are passing on both platforms, assuming a build with the changes from hectane/go-acl#14. I also had to make a minor fix to the new Glob functionality in go-vfs, for which I submitted twpayne/go-vfs#35. I have not yet done anything to disable update on Windows, but I'll probably get to that tomorrow.

[zb140]

I've rebased my branch onto your latest changes from yesterday, and I think it's just about ready for a PR. The one last problem is that after the Glob refactor in go-vfs, the remove tests no longer pass. This is because targetstate.Apply tries to relativize the paths returned from vfs.Glob (here: https://github.com/zb140/chezmoi/blob/windows/lib/chezmoi/targetstate.go#L189) but when using a PathFS those paths always contain slashes instead of backslashes (see https://github.com/twpayne/go-vfs/blob/master/windows.go#L41)

This can be fixed one of two ways: either change targetstate.Apply to convert the matched paths to the host-appropriate path separator before relativizing, or change the trimPrefix called by PathFS.Glob to return host-separated paths and update the tests accordingly (prototype here: zb140/go-vfs@991072e)

I lean towards the second solution, but what do you think? If you agree, I'll submit a PR for that.

Once that's wrapped up, anyone will be able to check out my branch and expect the all the tests to pass on Windows and Linux. I expect they'll work on Mac as well but I don't have one so I can't verify that. At that point, I can create a PR for the Windows branch to kick off the review process.

[twpayne]

I lean towards the second solution, but what do you think? If you agree, I'll submit a PR for that.

I agree. Please do submit a PR.

[twpayne]

Thanks to @zb140's work, there are currently no known issues on Windows, so I'm going to mark this issue fixed. Thanks @zb140!

[zb140]

Woohoo! I'm glad I could help. The benefits of chezmoi over my old, ad-hoc git repo have already made it worth the work 😀

Thanks for building such a great tool (and one of the few, if not the only, that could even feasibly support Windows in the first place!)