Skip to content

Put Unix.SO_REUSEPORT at the end of data variant#10073

Merged
Octachron merged 1 commit intoocaml:trunkfrom
dra27:fix-9869
Dec 9, 2020
Merged

Put Unix.SO_REUSEPORT at the end of data variant#10073
Octachron merged 1 commit intoocaml:trunkfrom
dra27:fix-9869

Conversation

@dra27
Copy link
Copy Markdown
Member

@dra27 dra27 commented Dec 7, 2020

In #9869 (comment) I noted that the new SO_REUSEPORT should ideally have gone at the end of the variant for vague C compatibility reasons.

However, there's another possibly stronger reason and, given that we're still at alpha, I propose we do actually change it. Here's an error message from Lwt, prior to ocsigen/lwt#804 being merged:

File "src/unix/lwt_unix.cppo.mli", lines 1020-1030, characters 0-13:
Error: This variant or record definition does not match that of type
         Unix.socket_bool_option
       Constructors number 4 have different names, SO_REUSEPORT and SO_KEEPALIVE.

which looks much more serious and less obvious than, with this PR:

File "src/unix/lwt_unix.cppo.mli", lines 1020-1030, characters 0-13:
Error: This variant or record definition does not match that of type
         Unix.socket_bool_option
       The constructor SO_REUSEPORT is only present in the original definition.

(cc @kit-ty-kate)

@dra27
Copy link
Copy Markdown
Member Author

dra27 commented Dec 7, 2020

Another option, of course, would be to increase the sophistication of the error message even further (so spot that one set of constructors is a subset of the other)

@Octachron
Copy link
Copy Markdown
Member

Octachron commented Dec 7, 2020

The error message could be amended, but the two messages seems quite comparable to me. Moreover, fixing the error highlighted in the first message will yield the second error message.

gasche
gasche approved these changes Dec 7, 2020
View reviewed changes
Copy link
Copy Markdown
Member

@gasche gasche left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The change looks reasonable (and, in particular, correct) to me, and I think that adding new fields at the end is indeed a good practice.

@dra27
Copy link
Copy Markdown
Member Author

dra27 commented Dec 7, 2020

@Octachron - the present message is of course technically correct, but it doesn't lead towards the likely correct remedy (it's reminding me of this joke!).

@Octachron
Copy link
Copy Markdown
Member

Octachron commented Dec 7, 2020

The current error message does lead towards the correct remedy even if the user follow it blindly? It is the number of steps required that is suboptimal. And I expect most people to simply resynchronize the re-exported definition rather than trying to fix this error by hand.
Note that I am neutral to the change, this particular motivation just feels dubious to me.

@gasche
Copy link
Copy Markdown
Member

gasche commented Dec 7, 2020

I am happy with the change. Should we have it in 4.12? (I think yes.)

@dra27
Copy link
Copy Markdown
Member Author

dra27 commented Dec 7, 2020

This needs to be 4.12 or not at all - I think it would be bad to have 4.12 with SO_REUSEPORT = 3 and 4.13+ with SO_REUSEPORT = 9

@kit-ty-kate
Copy link
Copy Markdown
Member

kit-ty-kate commented Dec 7, 2020

Even though this is breaking change with previous alphas I'm all for this PR.

That I can remember, only batteries and lwt will need to change again after this.
cc @raphael-proust would it be ok to squeeze that before the release?
cc @UnixJunkie would it be ok to release batteries 3.2.1 with this change?

@UnixJunkie
Copy link
Copy Markdown
Contributor

UnixJunkie commented Dec 8, 2020

@kit-ty-kate we can release a bugfixed batteries version if needed. No problem.

@Octachron Octachron merged commit 6bad570 into ocaml:trunk Dec 9, 2020
kit-ty-kate added a commit to kit-ty-kate/lwt that referenced this pull request Dec 9, 2020
@kit-ty-kate
Fix OCaml 4.12 support (port breaking change ocaml/ocaml#10073)
8c0d996
kit-ty-kate added a commit to kit-ty-kate/lwt that referenced this pull request Dec 9, 2020
@kit-ty-kate
Fix OCaml 4.12 support (port breaking change ocaml/ocaml#10073)
371ceaa
@kit-ty-kate kit-ty-kate mentioned this pull request Dec 9, 2020
Merged
kit-ty-kate added a commit to kit-ty-kate/batteries-included that referenced this pull request Dec 9, 2020
@kit-ty-kate
Fix OCaml 4.12 support (port breaking change ocaml/ocaml#10073)
b4d968d
@kit-ty-kate kit-ty-kate mentioned this pull request Dec 9, 2020
Merged
@kit-ty-kate
Copy link
Copy Markdown
Member

kit-ty-kate commented Dec 9, 2020

Ported to lwt (ocsigen/lwt#826), batteries (ocaml-batteries-team/batteries-included#992) and core (janestreet/core#142)

Octachron added a commit that referenced this pull request Dec 9, 2020
@Octachron
Merge pull request #10073 from dra27/fix-9869
12f935e 
Put Unix.SO_REUSEPORT at the end of data variant

(cherry picked from commit 6bad570)
gasche added a commit to ocaml-batteries-team/batteries-included that referenced this pull request Dec 9, 2020
@gasche
Merge pull request #992 from kit-ty-kate/412
b6ba003 
Fix OCaml 4.12 support (port breaking change ocaml/ocaml#10073)
aantron pushed a commit to ocsigen/lwt that referenced this pull request Dec 9, 2020
@kit-ty-kate
Fix OCaml 4.12 support (#826)
c0c921a 
See ocaml/ocaml#10073.
@Octachron Octachron mentioned this pull request Dec 17, 2020
Merged
@djs55 djs55 mentioned this pull request Apr 17, 2021
Merged
@Octachron Octachron mentioned this pull request Apr 21, 2021
Merged
@dra27 dra27 deleted the fix-9869 branch July 6, 2021 13:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@gasche gasche gasche approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@dra27 @Octachron @gasche @kit-ty-kate @UnixJunkie