Restore lib-unix/common/cloexec.ml test on Windows#13879
Conversation
Windows supports socketpair, so it can be tested in the same way.
Test refactored to use fds both on Unix and on Windows, and consequently to use Unix.execv instead of Unix.create_process to call the status checker.
The windows / not-windows tests catch Cygwin as well (since Cygwin runs on Windows). not-target-windows passes as long the compiler targets neither MSVC nor mingw-w64.
Ensures that both Unix.create_process (using posix_spawn, when available) and Unix.execve are correctly doing cloexec. At present, test disabled on Windows, because Unix.create_process needs reimplementing in terms of MSVC's spawn primitive (in order to inherit fds correctly).
Enables a whole series of Cygwin tests which should never have been disabled.
libunix / libwin32unix are now in place for their actual purpose
Use it to eliminate all remaining uses of the libunix and libwin32unix actions.
gasche
left a comment
There was a problem hiding this comment.
This looks well-done and well-justified. I didn't look at the tweaks in details, but I am confident given the explanations and the fact that the testsuite passes.
This motivates me to push for not <predicate>; in ocamltest, which I implemented as part of #13315 but remains unmerged. That PR implements something like conditional tests as well, and requires more work, but I should extract the negation part and submit it on its own as it is already useful.
|
Thanks, @gasche! I was thinking of your negation PR as I added the second of the predicates, indeed… |
|
The problem is that working on ocamltest is not very pleasant right now: I believe that some refactoring would help making this sort of language-extensions easier (we maintain both the old tree-style and new ast-style ASTs in parallel, and this makes adding negation and conditionals cumbersome), but no one is willing to review trivial changes like #13317, so my appetite for making larger changes that will take years to get merged is limited. I put this as an item for discussion for our upcoming maintainer meeting. |
This test was added in #650, and tests the inheritance of
Unix.file_descrs across process creation. This is done in the main test by setting up various different "kinds" offile_descrand then exec'ing a small auxiliary program which tests the state of each fd.As part of the switch to ocamltest, this test was disabled on Windows, for reasons covered in fd05a75.
The issue at play is that while the OCaml runtime doesn't leave any CRT fds open at the end of startup, the namespace for
HANDLEvalues on Windows is shared with more than just files, so there was a strong chance that aHANDLEwhich was marked as not inheritable (i.e. a close-on-exec fd) would end up being opened in the exec'd checker.I wish to use a further-tweaked version of this test for a second purpose as a regression test for fixing a different bug (which - as may not be a surprise by now - is a fix related to Relocatable OCaml).
The first commit removes special-cases for Windows which haven't been necessary since #10192 (though the test was of course disabled at that point).
The second commit is the major part of the change. On Windows, rather than manipulating
HANDLEvalues, we actually manipulate CRT fd values instead. This has the benefit of removing most of the Windows-specific code from the C portion of the test - we simply use the required C primitive. However, at this point the test on Windows would hit a technical deficiency of the implementation ofUnix.create_process, as it's implemented over the Windows API, rather than the CRT, so the test is switched to useUnix.execvinstead.Partly as a todo item, and partly because testing both
posix_spawnandexecvw.r.t.cloexecis valuable, the next two commits then run the test using the originalUnix.create_processmechanism - but this will fail on the mingw-w64 and MSVC ports so a newnot-target-windowsprimitive is added to ocamltest. At some point, it would be worth re-implementingUnix.create_processon Windows in terms of the CRT'sspawn, but this will need care w.r.t. all thecloexecwork, but it's not urgent.Having added this
not-target-windowsprimitive, I was reminded of #12844 - and the remaining commits use thisnot-target-windowsand then the dualtarget-windowsfinally to clarify and tidy uplibunix/libwin32unixwhich are now no longer used.