Skip to content
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

[WIP] Dunify with variants #384

Closed
wants to merge 3 commits into from
Closed

Conversation

avsm
Copy link
Member

@avsm avsm commented Dec 27, 2018

Prompted by @copy's pure OCaml checksum stubs, I've begun the port of tcpip to use the new Dune variants feature. This feature in Dune promotes the so-called "linking hack" (where a cmi is shared among multiple implementations) to a first-class supported entity.

We use virtual libraries to break out the checksum handling into a separate tcpip-checksum library that has unix, xen and ocaml variants that can be selected at executable link time. Since each of the implementations is compiled separately, we also have good CFLAGS discipline.

  • where on earth is the freestanding version of the checksum stubs at present? Needs to be added here.
  • document the renaming of the tcpip ocamlfind library to tcpip-checksum and the use of virtual libraries.
  • remove the duplication of the tcpip_checksum implementations via a copy_file rule.
  • reduce the number of modules that are compiled with ppx_cstruct to the ones that really need it (usually just the wire libraries).
  • compile the pure OCaml version with jsoo so we can ship a JS tcpip stack. @copy, did you have any usecase for the JavaScript version that we could include as a testcase to verify that it works?
  • dune 1.7 needs to be released before this can be merged.

@copy
Copy link

copy commented Dec 29, 2018

@copy, did you have any usecase for the JavaScript version that we could include as a testcase to verify that it works?

Nothing open-source at the moment (and not really embeddable either), sorry.

@hannesm
Copy link
Member

hannesm commented Jan 2, 2019

the solo5 checksum stubs are part of mirage-solo5 https://github.com/mirage/mirage-solo5/blob/master/lib/bindings/checksum_stubs.c

I agree with @samoht #378 (comment) and would appreciate to move the checksum code to checkseum and depend on checkseum. this would remove all c stubs from this library :D
for a smooth upgrade path:

  • add crc to checkseum using some checkseum_ prefix, release checkseum
  • depend on released checkseum, get rid of stubs, release tcpip
  • get rid of checksum stubs in mirage-solo5 (release that and conflict with earlier tcpip)

@dinosaure
Copy link
Member

I will be happy to improve checkseum and provide something for mirage-tcpip. However, checkseum is on top of optint which did not have a strong test suite and use a hack about integer representation of native integer in the OCaml world.

One, and the biggest, issue will be to test checkseum on a 32-bits arch and on a 64-bits.

@samoht
Copy link
Member

samoht commented Jan 4, 2019

That'd be a good occasion to fix checkseum :-)

@avsm
Copy link
Member Author

avsm commented Jan 6, 2019

I don't think we should switch to checkseum just yet -- the two checksumming methods seem entirely orthogonal to each other, and TCP 1s-complement checksumming isn't used very often elsewhere.

This PR maintains compat and introduces a new tcp-checksum package using Dune variants, so I'd prefer to release this first (once Dune 1.7 is out) and then consider moving the stubs out.

@avsm
Copy link
Member Author

avsm commented Feb 3, 2019

This depends on dune 1.7 being released, so deferring it for now in favour of #391. Will rebase later

avsm added 3 commits February 12, 2019 07:21
This changes the external ocamlfind interface to break out the
virtual checksum stubs library into a separate `tcpip-checksum`
library.  This library is then depended on by the rest of
the tcpip stack.
@avsm avsm force-pushed the dunify-with-variants branch from 39976f9 to 11490b7 Compare February 12, 2019 15:33
@avsm
Copy link
Member Author

avsm commented Feb 12, 2019

Now that dune 1.7 is out I've pushed a rebase of this PR that shifts to virtual_modules. See also mirage/mirage#969

@yomimono
Copy link
Contributor

yomimono commented Mar 8, 2019

Any chance for another rebase?

@hannesm
Copy link
Member

hannesm commented Apr 11, 2019

now that dune 1.9.0 is out, can we get a rebased PR? :) /cc @TheLortex

@avsm
Copy link
Member Author

avsm commented Jul 8, 2019

Holding off on rebasing this until ocaml/dune#2322 is fixed

@hannesm
Copy link
Member

hannesm commented Nov 25, 2020

thanks for your effort -- closing now that we have a slightly revised (xen) stack, where the checksum stubs are provided by mirage-xen themselves.

@hannesm hannesm closed this Nov 25, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants