Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Improve Windows support #25

Closed
zb140 opened this issue May 20, 2019 · 2 comments · Fixed by #26
Closed

Improve Windows support #25

zb140 opened this issue May 20, 2019 · 2 comments · Fixed by #26

Comments

@zb140
Copy link
Contributor

zb140 commented May 20, 2019

While working on twpayne/chezmoi#23 I ran the go-vfs tests on Windows and encountered a couple of failures. I've put together some fixes (https://github.com/zb140/go-vfs/tree/windows) after which all of the tests pass on both Windows and Linux (I don't have a Mac so I haven't tested there).

There were a couple of changes necessary that I think are worth calling special attention to:

  • On Windows, symlinks should not be created before the things they point to exist (because links to directories are different from links to files, and the difference must be explicitly specified when calling CreateSymbolicLink). Since the filesystems in vfst are declared as a map and you can't rely on iteration order, tests that were creating symlinks to directories were often creating them before the directories existed, causing them not to work. I solved this by adding support to Builder.build to be able to take a list of any of the types it previously supported, then recursively call them in order. See https://github.com/zb140/go-vfs/blob/cd3b3edf4f462bb963afdab8900eb7e6836fff33/vfst/vfst.go#L83
  • When Windows is asked to make a directory containing a path component that exists but is not a directory, the Windows APIs return (the equivalent of) ENOENT rather than ENOTDIR. This causes MkdirAll to attempt to recursively create the path, which will fail because it already exists. This is detected correctly but MkdirAll was swallowing the error, causing the test to detect no error when one was expected. I fixed it by returning the error rather than ignoring it. See https://github.com/zb140/go-vfs/blob/cd3b3edf4f462bb963afdab8900eb7e6836fff33/mkdirall.go#L49
  • PathFS.join expects incoming path names to be absolute before the internal prefix is added. But on Windows, a path is never considered absolute if it doesn't start with a volume specifier, which means the check was always failing. To fix this, I coerce the incoming paths to Unix-style using filepath.ToSlash, then check for an absolute path using path.IsAbs instead of filepath.IsAbs. See https://github.com/zb140/go-vfs/blob/cd3b3edf4f462bb963afdab8900eb7e6836fff33/path_fs.go#L224
@twpayne
Copy link
Owner

twpayne commented May 20, 2019

Thanks, this all looks very good. Please do create a PR for these when you feel the changes are ready. I can test on macOS if needed, but if it works on Linux it probably works on macOS too.

@twpayne
Copy link
Owner

twpayne commented May 20, 2019

Since the filesystems in vfst are declared as a map and you can't rely on iteration order, tests that were creating symlinks to directories were often creating them before the directories existed, causing them not to work.

Note that undefined map iteration order does not help you here. Even if map iteration order was defined (e.g. sorted) you can still create the symlink before the target. Consider the case where a and c are both symlinks to b.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants