Improve Windows support. #381

zb140 · 2019-07-02T03:30:08Z

Fix handling of paths and permissions to work on Windows
Make sure all tests are passing

zb140 · 2019-07-02T03:32:12Z

lib/chezmoi/fsmutator_windows.go

+	"github.com/twpayne/go-vfs/vfst"
+)
+
+func (a *FSMutator) getCorrectedPath(file string) string {


I don't know if there's a better way to deal with the fact that during the tests a path will get passed in that needs to have the test sandbox dir prepended to it. This feels like a hack but it's probably better than pushing IsPrivate up into go-vfs.

I think the right way to handle this is for go-vfs to include a function that returns the path to the file on the underlying filesystem. twpayne/go-vfs#40

twpayne · 2019-07-02T12:39:07Z

This is really impressive work @zb140 :)

I'll only have time to review it fully at the weekend, but there a few changes that I would like to see:

IsPrivate should not be a method on Mutator. Instead, I think it should be a top-level function in the lib/chezmoi package.
I really don't like if runtime.GOOS == statements. I'd rather factor out such code into separate POSIX and Windows source files and use build tags to control which gets built.

zb140 · 2019-07-03T03:08:55Z

cmd/config.go

@@ -369,8 +364,9 @@ func getDefaultData(fs vfs.FS) (map[string]interface{}, error) {
 	group, err := user.LookupGroupId(currentUser.Gid)
 	if err == nil {
 		data["group"] = group.Name
-	} else if cgoEnabled {
-		// Only return an error if CGO is enabled.
+	} else if cgoEnabled && runtime.GOOS != "windows" {


I've removed nearly all of the uses of runtime.GOOS that I added. I left this one in because I really think it makes more sense to just add this boolean check rather than try to refactor this into separate files.

Yup, makes sense.

zb140 · 2019-07-03T04:58:07Z

Thanks! It's quite a big change, so I definitely understand it'll take a while to review.

In addition to removing the usages of runtime.GOOS, I had a go at making IsPrivate a top-level function. Unfortunately that turned out to be a lot more complicated, again because of TestFS. During the tests, the paths passed into IsPrivate do not include the test sandbox, but I need a full absolute path to pass into acl.GetEffectiveAccessMode. I just don't see any way to get that path without some kind of involvement from FSMutator.

Next I revisited the idea discussed in #23 to override Mutator.Stat and return a Windows-specific FileInfo that understands ACLs. So far I haven't been able to get that going either; I managed to get it to build but for some reason I haven't figured out yet, it's still not using my Mode() override.

I'm not sure right now how best to proceed. If you have any thoughts or suggestions, I'd love to hear them.

twpayne

This looks really good.

I think that with twpayne/go-vfs#40 you can make IsPrivate a top-level function in lib/chezmoi.

Instead of using the _other suffix for non-Windows systems, please use _posix instead. This just requires renaming a few files.

This PR is probably a couple of commits away from being merged :)

twpayne · 2019-07-06T17:22:19Z

cmd/config.go

@@ -369,8 +364,9 @@ func getDefaultData(fs vfs.FS) (map[string]interface{}, error) {
 	group, err := user.LookupGroupId(currentUser.Gid)
 	if err == nil {
 		data["group"] = group.Name
-	} else if cgoEnabled {
-		// Only return an error if CGO is enabled.
+	} else if cgoEnabled && runtime.GOOS != "windows" {


Yup, makes sense.

twpayne · 2019-07-06T17:24:26Z

lib/chezmoi/fsmutator_windows.go

+	"github.com/twpayne/go-vfs/vfst"
+)
+
+func (a *FSMutator) getCorrectedPath(file string) string {


I think the right way to handle this is for go-vfs to include a function that returns the path to the file on the underlying filesystem. twpayne/go-vfs#40

lib/chezmoi/fsmutator_windows.go

twpayne · 2019-07-06T18:03:54Z

lib/chezmoi/fsmutator_windows.go

+
+// WriteFile implements Mutator.WriteFile.
+func (a *FSMutator) WriteFile(name string, data []byte, perm os.FileMode, currData []byte) error {
+	// Special case: if writing to the real filesystem, use github.com/google/renameio


After a private chat with @stapelberg last week (author of github.com/google/renameio) I understood that atomic file replacement is not possible on Windows, so we can remove the use of renameio here. I've asked for confirmation.

I see. I've left it in for now but once he confirms I'll go ahead and remove it.

lib/chezmoi/fsmutator_windows.go

twpayne · 2019-07-06T18:05:13Z

lib/chezmoi/script.go

@@ -121,10 +121,14 @@ func (s *Script) Apply(fs vfs.FS, mutator Mutator, applyOptions *ApplyOptions) e
 	}

 	// Write the temporary script file.
-	f, err := ioutil.TempFile("", filepath.Base(s.targetName))
+	pathBase := filepath.Base(s.targetName)
+	pathBase = strings.Replace(pathBase, "#", "*", 1)


Could you add a comment explaining why this replacement is necessary?

Turns out I massively overcomplicated this, which I realized once I started explaining the replacement. My only defense is that some of this code was written pretty late at night 😉

(For the record, the point of doing this was to preserve the file extension, which Windows uses to decide if it knows how to execute a file or not. Because of that, TempFile's randomness couldn't just go on the end of the name, which is the default, so I needed a * to indicate where it should go. But * is not a legal character in a Windows filename, thus the indirection. Of course, the much simpler solution is... just put it on the front of whatever the real filename is.)

…ks. Some other stuff works, but some is still broken

…indows.

… failure is due to absolute vs relative paths in one of the dump tests.

* that will return the absolute path of $PWD, which is almost certainly not the right thing to do

…o add back some code that got lost in the merge.

* Make sure to close the persistent store before trying to delete the directory it lives in. (Aside from causing the tests to fail, surprisingly this also causes them to take a very long time to run *before* they fail) * The ability to run a script on Windows is mainly controlled by the file extension, so all the script names must end with `.bat`.

…s disabled on Windows for now. Also, build tags for `noupgrade.go` should be ORed rather than ANDed.

creating a constant to compare against, but that doesn't seem like it really solves any problems, nor does it appear to be common practice in other projects.

…build tags instead.

…w things to better match conventions.

… vfs.Stater but the Windows one can't.

* Newlines on Windows have a CR before them, which `prompt` didn't allow for.

zb140 · 2019-07-07T03:35:52Z

I think I've addressed everything you mentioned (except removing renamio, for now). Thanks a lot for the RawPath addition -- it really made things easier. I've also included a bonus fix for prompting; I ran into this bug this evening while migrating some files from my old repo.

… tests. Also, for tests that launch Powershell, do so in NonInteractive mode.

…emporary script file names.

twpayne · 2019-07-07T08:09:52Z

Brilliant! Thank you so much! I've merged this, will do a quick round of minor clean-ups, and tag a new version.

zb140 commented Jul 2, 2019

View reviewed changes

zb140 commented Jul 3, 2019

View reviewed changes

twpayne reviewed Jul 6, 2019

View reviewed changes

zb140 added 15 commits July 6, 2019 17:51

Improve Windows compatibility. Everything in the quickstart guide wor…

e11a0ff

…ks. Some other stuff works, but some is still broken

Fix some tests to run on Windows.

23614d4

Fix several more tests on Windows. All but 4 test cases pass now on W…

a1876d5

…indows.

Fix the permissions related problems in the tests. The only remaining…

8fa3dae

… failure is due to absolute vs relative paths in one of the dump tests.

Don't turn DestDir into an absolute path if it's an empty string.

f2ff742

* that will return the absolute path of $PWD, which is almost certainly not the right thing to do

Implement an actual privateness check in root.go

0586ab4

Reformat files to meet standards

6d1eee7

Fix incorrect boolean check

eef846a

Fix Windows tests for symlink following and remove functionality. Als…

c094eba

…o add back some code that got lost in the merge.

Fix tests on non-Windows platforms.

2e23916

Remove unneeded getOwner abstraction in upgrade since the command i…

aaccef5

…s disabled on Windows for now. Also, build tags for `noupgrade.go` should be ORed rather than ANDed.

Disable lint checking for runtime.GOOS == "windows" It suggests

8c6bca4

creating a constant to compare against, but that doesn't seem like it really solves any problems, nor does it appear to be common practice in other projects.

Remove (nearly) all new uses of runtime.GOOS by refactoring to use …

a703fad

…build tags instead.

Fix build on non-Windows platforms.

a56f6b9

zb140 force-pushed the windows branch from 7faff96 to a56f6b9 Compare July 7, 2019 00:56

zb140 added 3 commits July 6, 2019 19:41

Move IsPrivate out of Mutator to a top-level method. Also rename a fe…

4bdbb4a

…w things to better match conventions.

Fix lint -- the posix implementation of IsPrivate can get away with a…

fa6b866

… vfs.Stater but the Windows one can't.

Fix prompting on Windows

d974593

* Newlines on Windows have a CR before them, which `prompt` didn't allow for.

zb140 added 2 commits July 6, 2019 20:52

Remove redundant hash characters from script filenames in the Windows…

10171d8

… tests. Also, for tests that launch Powershell, do so in NonInteractive mode.

Restore an accidentally-removed filepath.Base call when computing t…

1d3d743

…emporary script file names.

twpayne merged commit cbfea11 into twpayne:master Jul 7, 2019

twpayne mentioned this pull request Jul 7, 2019

Fix IsPrivate #386

Merged

twpayne mentioned this pull request Jul 7, 2019

add --recursive with private dirs is broken on Windows #387

Closed

github-actions bot locked as resolved and limited conversation to collaborators Mar 14, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Improve Windows support. #381

Improve Windows support. #381

zb140 commented Jul 2, 2019

zb140 Jul 2, 2019

twpayne Jul 6, 2019

twpayne commented Jul 2, 2019

zb140 Jul 3, 2019

twpayne Jul 6, 2019

zb140 commented Jul 3, 2019

twpayne left a comment

twpayne Jul 6, 2019

twpayne Jul 6, 2019

twpayne Jul 6, 2019

zb140 Jul 7, 2019

twpayne Jul 6, 2019

zb140 Jul 7, 2019

zb140 commented Jul 7, 2019

twpayne commented Jul 7, 2019

Improve Windows support. #381

Improve Windows support. #381

Conversation

zb140 commented Jul 2, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

twpayne commented Jul 2, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zb140 commented Jul 3, 2019

twpayne left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zb140 commented Jul 7, 2019

twpayne commented Jul 7, 2019