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
os: consider syscall.EEXIST in os.IsExist #29295
Comments
It should not be defined on Windows. As far as I remember (I could be wrong), when first Windows version of Go appeared, we copied modified UNIX version of syscall package to make all other packages work. And only later we started writing Windows code in packages other than syscall.
Maybe we can remove these symbols in Go 2. Whatever Go 2 is.
I don't see syscall package is platform specific. You cannot assume that Alex |
I linked to a widely used example in my issue report: |
Fair enough. I take my word about "anyone uses syscall.EEXIST on Windows". But my point still stands - you assumed things about Alex |
Let me be clear: This is not my code. I fixed some other person's code. And the repository I linked to is owned by a person that happens to also be on the Go team at Google -- which may indicate that this is a mistake that is a little bit too easy to make. |
Everyone makes mistakes. Only people who don't write code do not make mistakes. The person who wrote the code, should not have used Alex |
@bep you could propose a documentation change to warn readers about this... |
This sounds to me like a documentation change needs to be made.
I'm the one who wrote the offending code originally. If there were better
docs, I wouldn't have.
…On Mon, Dec 17, 2018 at 2:24 PM Liam ***@***.***> wrote:
@bep <https://github.com/bep> you could propose a documentation change to
warn readers about this...
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#29295 (comment)>, or mute
the thread
<https://github.com/notifications/unsubscribe-auth/AAKlZO8lrArEd93EwRBb4Im7poXyO_Jaks5u5--BgaJpZM4ZVMKy>
.
|
It doesn't seem so bad to me to have |
Any documentation sounds good to me. I just do not know what to say -
I do not see reason for What would the code that uses Alex |
The code that uses // Special purpose file system layer available on both Unix and Windows.
func (mfs *MyFileSystem) Open(path string) (*MyFile, error) {
if mfs.noSuchPath(path) {
return nil, syscall.EEXIST // this error is available on both Unix and WIndows
}
...
} |
Sorry my message is long. It is mistake (in my opinion) to import syscall package into the source file that is compiled for both Windows and Linux (I assumed your code builds on both *_linux.go and *_windows.go). I know source files like that happens here and there, but I find they always causes problems for me. For example, for #28477 (comment) I tried to make $GOROOT/os/pipe_test.go to work on Windows. So I removed Also, please show what mfs.noSuchPath does. If mfs.noSuchPath uses os package to implement its logic, then mfs.noSuchPath should just return whatever error it receives from os package. If mfs.noSuchPath does not uses os package, then you should not be using os.IsExist to test returned errors. Unless you write tests that show it is OK to use os.IsExist. Also not many people will be writing code like yours. But os.IsExist code is read by everyone. If we are to make code change, we would need some good comment there explaining the problem and our reasoning. Also, if we start worrying about people using syscall.EEXIST incorrectly only because EEXIST name exists on both Windows and Linux, we should worry about many other errors in syscall package: ENODEV, ENOEXEC and many others. Should we also try and make syscall.Syscall "safer" to use for people who just build their Linux code on Windows? Alex |
Note that I'm not very categorical in my issue title, but this debugging of a subtle Afero file system bug has given me enough gray hairs to at least deserve an issue/discussion.
The program below runs fine on *nix but fails on Windows (it compiles fine):
I assume that
syscall.EEXIST
will never happen on Windows, and then it should probably also not be defined for Windows. I guess that ship has sailed, but you should then consider to expandos.IsExist
on Windows to also include this error. Because there may be other people creating file system abstractions that can be bitten by this./cc @spf13
The text was updated successfully, but these errors were encountered: