Remove Windows support

[twpayne]

This is an alternative "fix" to #1 and #17. It drops any pretense of Windows support by not providing any functionality on Windows, i.e. when compiled on Windows, the package is empty.

The reasons for this are:

• On Windows, it is not possible to atomically replace a file, so this library cannot serve its intended purpose.
• Windows does not support UNIX-like file permissions, and pretending to support them (as the Go standard library does) only causes surprises and unexpected behavior (as @andhe describes).

By providing an empty package on Windows, users will have to work around these limitations in their application code when building on Windows.

[twpayne]

@profclems this is probably relevant to you. Also see this comment.

Please note that I am a contributor and not a maintainer. I do not decide if this PR should be merged.

[stapelberg]

Can you add a section to the README that references golang/go#22397 (comment) and issue #17 please?

[twpayne]

README updated. I also removed the GitHub Actions commit - I'll create a separate PR for that.

[profclems]

@profclems this is probably relevant to you. Also see this comment.

Please note that I am a contributor and not a maintainer. I do not decide if this PR should be merged.

Since windows support is dropped for this package, we've created a helper function in a separate file that uses renameio
but exempts it from windows build, then hook in a windows-specific implementation of that function which uses ioutil.WriteFile in profclems/glab#212

[stapelberg]

but exempts it from windows build, then hook in a windows-specific implementation of that function which uses ioutil.WriteFile in profclems/glab#212

I wonder if such a package should be added as an additional helper package to the renameio module? Sounds like others might need to do the same thing. Would you be up for sending a PR?

[profclems]

but exempts it from windows build, then hook in a windows-specific implementation of that function which uses ioutil.WriteFile in profclems/glab#212

I wonder if such a package should be added as an additional helper package to the renameio module?

Yeah... adding it as a helper package to renameio module sounds a great idea. I'll be sending a PR

[twpayne]

Apologies for taking a different view but the helper package will contain one four-line function:

func WriteFile(name string, data []byte, perm os.FileMode) error {
     if runtime.GOOS == "windows" {
        return ioutil.WriteFile(name, data, perm)
    }
    return renameio.WriteFile(name, data, perm)
}

I would suggest that the helper package be named github.com/google/renameio/maybe so you can write

    maybe.WriteFile(name, data, perm) // on Windows it's not atomic and perm is not respected

I realize I'm being really snarky here, but I really think the assumptions about atomic updates and respecting permissions need to be visible to application authors, and we shouldn't try to hide them.

[profclems]

I'm thinking the need for importing/linking runtime could be avoided by creating a writefile_windows.go file which is Windows-specific (+build windows) and implements WrifeFile using ioutil.WriteFile.
The readme should be transparent to application authors on this implementation.

[twpayne]

Put perhaps more constructively, I understand the good intention to make it easier for people to use the excellent functionality in this library. One of the hardest problems to debug is when a library silently does the wrong thing.

The library says that the atomic file write was successful, and your program continues under that assumption. At some later point, you read back the file only to find that it's corrupt. You have no idea how this corruption occurred because the cause of the problem and its effect are so far removed from each other.

It's much easier to debug when things fail early and loudly. With this PR, the early fail is pretty much as early and loud as it can be: if you expect renameio.WriteFile to work on Windows then you will get an error when you compile your program for Windows. You can then adjust your strategy.

[twpayne]

I'm thinking the need for importing/linking runtime could be avoided by creating a writefile_windows.go file which is Windows-specific (+build windows) and implements WrifeFile using ioutil.WriteFile.

The Go compiler is sufficiently clever to recognize that importing runtime in this case does not have any cost. See this thread on golang-nuts for a definitive answer from Ian Lance Taylor.

The readme should be transparent to application authors on this implementation.

I also wish that users read documentation.

[profclems]

Put perhaps more constructively, I understand the good intention to make it easier for people to use the excellent functionality in this library. One of the hardest problems to debug is when a library silently does the wrong thing.

The library says that the atomic file write was successful, and your program continues under that assumption. At some later point, you read back the file only to find that it's corrupt. You have no idea how this corruption occurred because the cause of the problem and its effect are so far removed from each other.

It's much easier to debug when things fail early and loudly. With this PR, the early fail is pretty much as early and loud as it can be: if you expect renameio.WriteFile to work on Windows then you will get an error when you compile your program for Windows. You can then adjust your strategy.

This sounds good to me... things would be easier if

... users read documentation.

[stapelberg]

I realize I'm being really snarky here, but I really think the assumptions about atomic updates and respecting permissions need to be visible to application authors, and we shouldn't try to hide them.

Yes, a clear name will be key for this package.

[benitogf]

Why wasn't the retry approach used here https://go-review.googlesource.com/c/go/+/181541/7/src/cmd/go/internal/robustio/robustio_windows.go ported to renameio instead of removing windows support?

[twpayne]

@benitogf why didn't you suggest this before?

[benitogf]

@twpayne sorry I wasn't following until my build broke on windows 😅

[twpayne]

Note that "retrying until it works" is not the same as "atomic rename". Consider two processes trying to write the same file at the same time. With atomic rename you are guaranteed a winner. With retries the processes might end up racing each other and flipping the file between different states for as long as the retries occur.

[benitogf]

Still think that no thread safety support is better than no support

[twpayne]

If you can't do what you promised to do, should you tell people that you can't do it or should you do something different and pretend that you did what you promised?

I would argue that, for such a fundamental package as this, it's better to be honest.

[benitogf]

My use case is a single writer and single reader on different threads, think that the retry should cover it? there could be a lock in the application side to make multi writers work as well?

As long as the documentation explains that there's no thread safety on rename for windows and provides the "best possible" solution should be ok imo

[stapelberg]

robustio is different than renameio, hence it also has a different package name.

If you want robustio semantics, I suggest making/using a different package.

[profclems]

I think I agree with @benitogf. If the documentation clearly and boldly explains that there's no thread-safety support for rename on windows, the user should be convinced enough and there shouldn't be a problem.

A clear description could be added to the package documentation header, and the Writefile function documented to notify the package user that the Writefile function falls back to ioutil.Writefile on windows since the package provides no support for windows.
At least to avoid breaking their build on windows. Some may not test their packages on windows

[stapelberg]

If people are concerned with build breakage, they should use the https://pkg.go.dev/github.com/google/renameio@v1.0.0/maybe package, which is a no-op on windows (but builds).

[benitogf]

@stapelberg I'm actually using 'TempFile' not 'WriteFile' so maybe a fork will do for now, thanks!