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

Fix the behavior of Unix.read on Windows + Makefile changes #797

Closed
wants to merge 7 commits into
base: trunk
from

Conversation

Projects
None yet
5 participants
@msprotz
Contributor

msprotz commented Sep 7, 2016

msprotz and others added some commits Sep 7, 2016

@gasche

This comment has been minimized.

Show comment
Hide comment
@gasche

gasche Sep 7, 2016

Member

Could we have a test in the testsuite that would fail (on Windows, I guess) with present-day code and works after your patch?

Member

gasche commented Sep 7, 2016

Could we have a test in the testsuite that would fail (on Windows, I guess) with present-day code and works after your patch?

@gasche

This comment has been minimized.

Show comment
Hide comment
@gasche

gasche Sep 7, 2016

Member

Is 725b80c meant to be part of the PR?

Member

gasche commented Sep 7, 2016

Is 725b80c meant to be part of the PR?

@msprotz

This comment has been minimized.

Show comment
Hide comment
@msprotz

msprotz Sep 7, 2016

Contributor

Yes I've fixed the runtop target because I needed it to test my fix, so I've submitted these changes along with the pull request.

Contributor

msprotz commented Sep 7, 2016

Yes I've fixed the runtop target because I needed it to test my fix, so I've submitted these changes along with the pull request.

@msprotz

This comment has been minimized.

Show comment
Hide comment
@msprotz

msprotz Sep 7, 2016

Contributor

I meant: I added the runtop target to Makefile.nt (it wasn't there)

Contributor

msprotz commented Sep 7, 2016

I meant: I added the runtop target to Makefile.nt (it wasn't there)

@msprotz

This comment has been minimized.

Show comment
Hide comment
@msprotz

msprotz Sep 7, 2016

Contributor

Your request for a test is legitimate. I shall point out, however, that:

  • there is no testsuite/tests/lib-unix (no comment);
  • I just tried writing a test, and of course, the tests are not capable of adding the right PATH (not LD_LIBRARY_PATH) like I did for runtop on Windows.

This is definitely not fun.

Contributor

msprotz commented Sep 7, 2016

Your request for a test is legitimate. I shall point out, however, that:

  • there is no testsuite/tests/lib-unix (no comment);
  • I just tried writing a test, and of course, the tests are not capable of adding the right PATH (not LD_LIBRARY_PATH) like I did for runtop on Windows.

This is definitely not fun.

@msprotz

This comment has been minimized.

Show comment
Hide comment
@msprotz

msprotz Sep 7, 2016

Contributor

So the tests are run in bytecode and native; the bytecode version needs an extra -I ../../../otherlibs/win32unix after path/to/ocamlrun.exe, but the Makefile doesn't have a way of differentiating the native and bytecode targets...

Contributor

msprotz commented Sep 7, 2016

So the tests are run in bytecode and native; the bytecode version needs an extra -I ../../../otherlibs/win32unix after path/to/ocamlrun.exe, but the Makefile doesn't have a way of differentiating the native and bytecode targets...

msprotz added some commits Sep 7, 2016

@msprotz

This comment has been minimized.

Show comment
Hide comment
@msprotz

msprotz Sep 7, 2016

Contributor

I couldn't find out how to add a new directory to the test suite, but looking at the Appveyor logs it was correctly picked up (and the test was successful it seems).

Contributor

msprotz commented Sep 7, 2016

I couldn't find out how to add a new directory to the test suite, but looking at the Appveyor logs it was correctly picked up (and the test was successful it seems).

@fdopen

This comment has been minimized.

Show comment
Hide comment
@fdopen

fdopen Sep 7, 2016

Contributor

This change will break third party code that re-use Unix.file_descr for named pipes (e.g. https://github.com/djs55/ocaml-named-pipe/ ). So it should be documented properly.
(In the case of named pipes you have to distinguish between 0 and EPIPE. 0 means that WriteFile was called with 0, EPIPE that the pipe was closed. Your change conflates both, so Unix.read can't be used any longer for named pipes)

Contributor

fdopen commented Sep 7, 2016

This change will break third party code that re-use Unix.file_descr for named pipes (e.g. https://github.com/djs55/ocaml-named-pipe/ ). So it should be documented properly.
(In the case of named pipes you have to distinguish between 0 and EPIPE. 0 means that WriteFile was called with 0, EPIPE that the pipe was closed. Your change conflates both, so Unix.read can't be used any longer for named pipes)

Fix
@msprotz

This comment has been minimized.

Show comment
Hide comment
@msprotz

msprotz Sep 7, 2016

Contributor

Any suggestion on how to fix this cleanly?

Contributor

msprotz commented Sep 7, 2016

Any suggestion on how to fix this cleanly?

@msprotz

This comment has been minimized.

Show comment
Hide comment
@msprotz

msprotz Sep 8, 2016

Contributor

Also, are you saying that WriteFile with 0 is ok on a named piped but not on an anonymous pipe?

Contributor

msprotz commented Sep 8, 2016

Also, are you saying that WriteFile with 0 is ok on a named piped but not on an anonymous pipe?

@msprotz

This comment has been minimized.

Show comment
Hide comment
@msprotz

msprotz Sep 8, 2016

Contributor

I am told that there's no way to distinguish a named pipe from an anonymous pipe using the Windows API.

Contributor

msprotz commented Sep 8, 2016

I am told that there's no way to distinguish a named pipe from an anonymous pipe using the Windows API.

@msprotz

This comment has been minimized.

Show comment
Hide comment
@msprotz

msprotz Sep 20, 2016

Contributor

Had a quick chat with Damien.

  • Rename the existing function read_raw (or something similar)
  • Write a read wrapper that catches the EPIPE exception and turns it into a zero-read
  • Document in the description of Unix.read that advanced users who wish to tell apart EPIPE and 0 because they're on Windows should bind read_raw using an external declaration.

Thoughts?

Contributor

msprotz commented Sep 20, 2016

Had a quick chat with Damien.

  • Rename the existing function read_raw (or something similar)
  • Write a read wrapper that catches the EPIPE exception and turns it into a zero-read
  • Document in the description of Unix.read that advanced users who wish to tell apart EPIPE and 0 because they're on Windows should bind read_raw using an external declaration.

Thoughts?

@xavierleroy

This comment has been minimized.

Show comment
Hide comment
@xavierleroy

xavierleroy Dec 4, 2016

Contributor

The proposed code is an improvement over what we have today and which makes pipes completely nonportable between Unix and Windows. So, I'm in favor of merging this PR as is.

The problem with named pipes and zero-length writes is weird. I'm positive that under Unix a write of length 0 has no effect. I'm not sure we want to go out of our way to preserve this strange Win32 idiom.

Contributor

xavierleroy commented Dec 4, 2016

The proposed code is an improvement over what we have today and which makes pipes completely nonportable between Unix and Windows. So, I'm in favor of merging this PR as is.

The problem with named pipes and zero-length writes is weird. I'm positive that under Unix a write of length 0 has no effect. I'm not sure we want to go out of our way to preserve this strange Win32 idiom.

@xavierleroy xavierleroy added the approved label Dec 4, 2016

@protz

This comment has been minimized.

Show comment
Hide comment
@protz

protz Dec 16, 2016

Apparently ocaml-named-pipes is aware of the issue and certainly will adapt their code if needed... Who should I see to actually perform the merge? (My personal account, i.e. this one, doesn't have commit rights on GitHub.)

protz commented Dec 16, 2016

Apparently ocaml-named-pipes is aware of the issue and certainly will adapt their code if needed... Who should I see to actually perform the merge? (My personal account, i.e. this one, doesn't have commit rights on GitHub.)

gasche added a commit that referenced this pull request Dec 16, 2016

@gasche

This comment has been minimized.

Show comment
Hide comment
@gasche

gasche Dec 16, 2016

Member

I merged manually to squash as appropriate.

Member

gasche commented Dec 16, 2016

I merged manually to squash as appropriate.

@gasche gasche closed this Dec 16, 2016

camlspotter pushed a commit to camlspotter/ocaml that referenced this pull request Oct 17, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment