Use conditional compilation for Lwt_unix + OCaml 4.05 compat (O_KEEPEXEC) #322

mfp · 2017-02-28T18:42:59Z

Compatibility with OCaml 4.05, broken with the introduction of the O_KEEPEXEC constructor in Unix.open_flag, see #321.

Tested on 4.02.3 and 4.05+beta2.
I'm getting these apparently benign warnings, which I have not been able to prevent (I didn't find the proper way to use InterfacePatterns/ImplementationPatterns apparently):

W: Cannot find source file matching module 'Lwt_unix' in library lwt-unix.
W: Use InterfacePatterns or ImplementationPatterns to define this file with feature "source_patterns".
W: No exported module defined for library lwt-simple-top
W: No exported module defined for library lwt-syntax-options
W: Cannot find source file matching module 'Lwt_unix' in library lwt-unix.
W: Use InterfacePatterns or ImplementationPatterns to define this file with feature "source_patterns".
W: No exported module defined for library lwt-simple-top
W: No exported module defined for library lwt-syntax-options
W: Cannot find source file matching module 'Lwt_unix' in library lwt-unix.
W: Use InterfacePatterns or ImplementationPatterns to define this file with feature "source_patterns".
W: Cannot find source file matching module 'Lwt_unix' in library lwt-unix.
W: Use InterfacePatterns or ImplementationPatterns to define this file with feature "source_patterns".

…XEC).

mfp · 2017-02-28T18:49:23Z

hmm no CI? Are all the bots sleeping, or is CI skipped if the branch is created directly on this repos?

vasilisp · 2017-02-28T19:07:30Z

lwt.opam

@@ -35,6 +35,7 @@ depends: [
  "ocamlfind" {build & >= "1.5.0"}
  "ocamlbuild" {build}
  "result"
+  "cppo"


You can add the {build} predicate here.

Thank you, pushed.

aantron · 2017-02-28T19:42:00Z

XOCamlbuildPluginTags was broken in OASIS 0.4.7 and fixed in 0.4.8 (ocaml/oasis#84, changelog), so should also change the oasis constraint in opam to >= 0.4.8. Looks good modulo this and @vasilisp's comment.

Not sure what just happened with the AppVeyor CI. I don't have time to debug it at the moment. I restarted the builds. If you keep having problems, you're welcome to comment out build rows/disable AppVeyor, and I will look later.

aantron · 2017-02-28T19:44:19Z

And

hmm no CI? Are all the bots sleeping, or is CI skipped if the branch is created directly on this repos?

Maybe it was just taking a while to be submitted by GitHub. CI is enabled for branches created directly on the repo. In fact, I'd like to disable double building of PRs from branches on the repo, but neither AppVeyor nor Travis supported this as of two months ago.

Refer to #322 (comment)

mfp · 2017-02-28T20:16:16Z

Thank you both for the extra eyeballs :)

Indeed CI eventually launched after a while (around an hour an a half or so) -- I was used to Github doing its thing faster.

Will sit on this for a few days (4.05 is not released yet) before merging just in case we/somebody find a reason not to.

aantron · 2017-02-28T20:40:54Z

@mfp I pushed into this branch a commit to enable testing on 4.05+beta2 in Travis. Hopefully, that will go well.

Also, I think it will be necessary to add < 4.05 compiler constraints to all the existing lwt packages in OPAM. If you don't want to do this tedious work, I will do it some time before the 3.0.0 release :)

mfp · 2017-02-28T21:26:15Z

4.05 CI: was doing the same in a (now deleted) branch :)

Most lwt versions had more conservative constraints (4.02 or 4.03), so there weren't that many in need of a change. ocaml/opam-repository#8578

Surprisingly, 2.4.5 builds on 4.04 as per http://opam.ocamlpro.com/builder/html/report-full.html#lwt

aantron · 2017-03-02T00:37:26Z

Looks like we need to wait on Camlp4 for 4.05 to be in OPAM for the CI to work. Fortunately, there is a PR: ocaml/opam-repository#8583. I'll restart the build when that's in.

Thanks for the constraints.

Surprisingly, 2.4.5 builds on 4.04

2.4.5 builds on 4.04 because that was the last old release before the Lwt PPX:

$ git tag --contains `git log --reverse --format='%H' -- ppx | head -n 1`                                                                      141
2.4.6
2.4.7
2.4.8
2.5.0
2.5.1
2.5.2
2.6.0
2.7.0

There were some incompatibilities in the PPX when 4.03 came out, fixed in Lwt 2.5.2. I guess the PR was #227.

See comment included in the diff for details.

[skip appveyor]

aantron · 2017-03-03T19:51:50Z

Should be ready for merge now.

fdopen · 2017-03-06T12:43:44Z

Don't forget to update the c stub, it already happend in the past at this point: #170

The sizes of https://github.com/ocaml/ocaml/blob/dd0d93f0f1b2c9e9c91713ae3aac0accbbf82310/otherlibs/unix/open.c#L41 and

lwt/src/unix/lwt_unix_unix.c

Line 1113 in 0f37f8e

static int open_flag_table[] = {

must be kept in sync. Otherwise caml_convert_flag_list could return garbage, if you pass O_KEEPEXEC to it.

mfp · 2017-03-06T21:55:53Z

On Mon, Mar 06, 2017 at 04:43:44AM -0800, Andreas Hauptmann wrote: Don't forget to update the c stub, it already happend in the past at this point: #170 The sizes of https://github.com/ocaml/ocaml/blob/dd0d93f0f1b2c9e9c91713ae3aac0accbbf82310/otherlibs/unix/open.c#L41 and https://github.com/ocsigen/lwt/blob/0f37f8e8c970cec9b22588ee9b2034e141bbbb17/src/unix/lwt_unix_unix.c#L1113 must be kept in sync. Otherwise `caml_convert_flag_list` could return garbage, if you pass `O_KEEPEXEC` to it.

Great catch, thanks a lot! I'm updating the PR.

…

-- Mauricio Fernández

avsm · 2017-03-22T17:15:54Z

We're now using ocaml-migrate-parsetree in cstruct, so the bulk build train for 4.05 is now blocked on Lwt again :-) mirage/ocaml-cstruct#127

Would be great to get this built into a release so that the 4.05 betas work.

aantron · 2017-03-22T18:05:25Z

This is going into a 2.x.x release within the next two weeks or so, to be released concurrently with 3.0.0 (where 3.0.0 will contain only the planned breaking changes).

mfp · 2017-03-22T21:26:39Z

On Wed, Mar 22, 2017 at 10:15:54AM -0700, Anil Madhavapeddy wrote: We're now using ocaml-migrate-parsetree in cstruct, so the bulk build train for 4.05 is now blocked on Lwt again :-) mirage/ocaml-cstruct#127 Would be great to get this built into a release so that the 4.05 betas work.

Sorry about the delay. I'm planning to revisit the patch this weekend.

…

-- Mauricio Fernández

…EXEC.

The default close-on-exec behavior for Unix.openfile might change in future OCaml releases, refer to ocaml/ocaml#650 This patch ensures Lwt_unix.openfile's default behavior will match Unix.openfile (and also keep consistency w.r.t. it between Win32 and other platforms).

mfp · 2017-03-26T12:15:13Z

I tried to write some tests for O_CLOEXEC/O_KEEPEXEC but frankly it was a PITA: pre-processor needed to test 4.05's O_KEEPEXEC, platform-dependent code to test Lwt_unix.fork + exec on Unix-like and CreateProcess on win32 separately, etc.

Edit added separate issue for ?cloexec #327

aantron · 2017-03-26T17:53:57Z

I think writing a private one-off program for testing, and exercising the various combinations once locally, is fine for now.

aantron · 2017-03-26T18:33:19Z

src/unix/lwt_unix_unix.c

+    cloexec = 0;
+#endif
+
+#if !defined(NEED_CLOEXEC_EMULATION)


As I understand it, with this diff, NEED_CLOEXEC_EMULATION will no longer ever be defined.

Indeed. I lost the line that defined NEED_CLOEXEC_EMULATION if O_CLOEXEC wasn't defined in the last-second patch cleanup, and missed it since Linux + glibc does have O_CLOEXEC.
Should be fixed now.

mfp · 2017-04-03T21:20:53Z

I bit the bullet and tested both O_CLOEXEC and O_KEEPEXEC (when available) on non-Windows.
On win32, Lwt_unix.openfile uses Unix.openfile, so there's not that much to gain in going out of our way to test it, anyway.

aantron · 2017-04-03T21:46:48Z

tests/unix/test_lwt_unix.cppo.ml

+          Lwt_unix.close fd >>= fun () ->
+          Lwt_unix.waitpid [] n >>= function
+            | _, Unix.WEXITED 0 -> Lwt.return_true
+            | _ -> Lwt.return_false


It looks like this is failing the automated builds with warning 4, because it does not list the cases for the Unix.process_status explicitly, and we have lots of warnings enabled, and build with warnings as errors in CI. I think in this case, it's best to just list the two remaining possible cases explicitly, so we fail out in the extremely unlikely event Unix.process_status changes one day.

aantron · 2017-04-07T15:22:25Z

Much appreciated!

Lifted from https://github.com/ocaml/ocaml/blob/469eb86d12f3c17f18d406d7797b28d96ceb2ec1/otherlibs/unix/open.c#L44 Follow-on to #322. [skip ci]

Use conditional compilation for Lwt_unix + OCaml 4.05 compat (O_KEEPE…

2f10fdb

…XEC).

vasilisp reviewed Feb 28, 2017

View reviewed changes

mfp added 2 commits February 28, 2017 21:01

Fix cppo dependency (build predicate).

b5ea8db

Fix oasis build dependency: >= 0.4.8 required.

b3edc4d

Refer to #322 (comment)

aantron added 2 commits March 2, 2017 19:06

Suppress deprecation warning on Array1.map_file

cafaf8e

See comment included in the diff for details.

Travis: test on OCaml 4.05+beta2

361fcaa

[skip appveyor]

aantron force-pushed the lwt_unix+4.05-compat branch from a37cf94 to 361fcaa Compare March 3, 2017 01:07

mfp added 2 commits March 25, 2017 23:46

Ensure Lwt_unix.openfile's flag tables have defined values for O_KEEP…

522e166

…EXEC.

aantron reviewed Mar 26, 2017

View reviewed changes

mfp added 2 commits April 3, 2017 23:02

Unit test Lwt_unix.openfile behavior wrt. O_CLOEXEC and O_KEEPEXEC.

70889cf

Fix O_CLOEXEC emulation in Lwt_unix.openfile.

44ca04b

aantron reviewed Apr 3, 2017

View reviewed changes

Prevent warning 4 in Lwt_unix.openfile tests.

c7bc813

mfp merged commit 6ec1298 into master Apr 7, 2017

aantron mentioned this pull request Apr 7, 2017

Compilation against 4.05 #321

Closed

aantron added a commit that referenced this pull request Apr 8, 2017

Internal docs: minor comment in lwt_unix_unix.c

e7535b7

Lifted from https://github.com/ocaml/ocaml/blob/469eb86d12f3c17f18d406d7797b28d96ceb2ec1/otherlibs/unix/open.c#L44 Follow-on to #322. [skip ci]

aantron deleted the lwt_unix+4.05-compat branch April 8, 2017 17:47

aantron mentioned this pull request Apr 8, 2017

4.02 Jenkins appears to be broken #330

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Use conditional compilation for Lwt_unix + OCaml 4.05 compat (O_KEEPEXEC) #322

Use conditional compilation for Lwt_unix + OCaml 4.05 compat (O_KEEPEXEC) #322

mfp commented Feb 28, 2017

mfp commented Feb 28, 2017

vasilisp Feb 28, 2017

mfp Feb 28, 2017

aantron commented Feb 28, 2017

aantron commented Feb 28, 2017

mfp commented Feb 28, 2017

aantron commented Feb 28, 2017

mfp commented Feb 28, 2017 •

edited

Loading

aantron commented Mar 2, 2017

aantron commented Mar 3, 2017

fdopen commented Mar 6, 2017

mfp commented Mar 6, 2017 via email

avsm commented Mar 22, 2017

aantron commented Mar 22, 2017

mfp commented Mar 22, 2017 via email

mfp commented Mar 26, 2017 •

edited

Loading

aantron commented Mar 26, 2017

aantron Mar 26, 2017

mfp Apr 3, 2017 •

edited

Loading

mfp commented Apr 3, 2017

aantron Apr 3, 2017

aantron commented Apr 7, 2017

Use conditional compilation for Lwt_unix + OCaml 4.05 compat (O_KEEPEXEC) #322

Use conditional compilation for Lwt_unix + OCaml 4.05 compat (O_KEEPEXEC) #322

Conversation

mfp commented Feb 28, 2017

mfp commented Feb 28, 2017

vasilisp Feb 28, 2017

Choose a reason for hiding this comment

mfp Feb 28, 2017

Choose a reason for hiding this comment

aantron commented Feb 28, 2017

aantron commented Feb 28, 2017

mfp commented Feb 28, 2017

aantron commented Feb 28, 2017

mfp commented Feb 28, 2017 • edited Loading

aantron commented Mar 2, 2017

aantron commented Mar 3, 2017

fdopen commented Mar 6, 2017

mfp commented Mar 6, 2017 via email

avsm commented Mar 22, 2017

aantron commented Mar 22, 2017

mfp commented Mar 22, 2017 via email

mfp commented Mar 26, 2017 • edited Loading

aantron commented Mar 26, 2017

aantron Mar 26, 2017

Choose a reason for hiding this comment

mfp Apr 3, 2017 • edited Loading

Choose a reason for hiding this comment

mfp commented Apr 3, 2017

aantron Apr 3, 2017

Choose a reason for hiding this comment

aantron commented Apr 7, 2017

mfp commented Feb 28, 2017 •

edited

Loading

mfp commented Mar 26, 2017 •

edited

Loading

mfp Apr 3, 2017 •

edited

Loading