Restore compatibility with Standard Library modules on OCaml 4.x #5
Conversation
Use the Standard Library modules for OCaml 4.x, but override 4.14's to eliminate the deprecation warning.
|
A potential problem with the special casing of 4.14 is that it would be the only version where |
|
Very good point! I’ll push a revision shortly |
|
Not the prettiest workaround, but I've amended the
|
| (target %s.mli) | ||
| (action (with-stdout-to %%{target} | ||
| (echo "[@@@ocaml.warning \"-3\"] include module type of %s"))) | ||
| (enabled_if (< %%{ocaml_version} 5.0))) |
There was a problem hiding this comment.
Do we need this .mli? I am always a bit suspicious of module type of...
There was a problem hiding this comment.
Argh - I think you're right... it was supposed to have been include module type of struct include %s end, although it is working in this case. @Octachron - is there any problem with just having the header generated from include Stream in the .ml file?
There was a problem hiding this comment.
Including Stream creates a slight documentation issue due to the presence of hidden functions in the interface of Stream. I am not sure if there is alternative (outside of conditionally adding the type equation for 4.14 in the compatibility mli file?)
|
Apologies for a late and basic question, but: what's the motivation? what problem is this PR addressing? |
|
#4 - at present camlp-streams doesn’t work properly on 4.04-4.06. I just need to push a revised rule for the .mli file. |
|
Ah, thanks for the reminder. We do need to make the package empty for OCaml < 4.07, indeed. For 4.07-4.13, I'm not sure what the best course of action is. |
|
I think it’s best to do nothing all the way up to 4.13 - 4.04-4.06 have an immediately-visible linking problem, but there’s a type equality problem for 4.07 onwards too, IIRC |
|
I added two tests: one explicitly tests linking an executable which camlp-streams and a library which doesn't use camlp-streams (that's the original issue reported in #4) and another which fails of The tests fail as expected (the link test fails on 4.02-4.06; the equality test fails on 4.07-4.14) without this PR. |
|
(obviously the test is irrelevant for 5.x as there's no |
There was a problem hiding this comment.
This looks right to me now. The only thing that become a problem is the use of Jbuild_plugin for the dune file, which is something of a slow spot for incremental dune builds and generally not recommended. However, this is something we could fix up in a future iteration of camlp-streams rather than delay this PR (since it is needed to unblock odoc-parser and a plethora of OCaml 5.0 opam packages)
|
Argh - this won't work on MSVC (for 4.x) - fix incoming! |
dra27
changed the title
|
We hit the same problem as stdlib-shims that we MSVC doesn't support empty archives until OCaml 4.11. Having battled with various things, I think the best thing for now is to accept that this library doesn't work properly on OCaml 4.02-4.06 for MSVC only. I've changed it so that OCaml 4.07-4.14 (i.e. all the versions of OCaml with the I have a version which can do this with a static |
4.02-4.06 remain broken.
We hit the issue corrected in PR#2041 in OCaml 4.08 - even in OCaml 4.07 with its prefixed Stdlib, we can't override modules opened from Stdlib.
|
@xavierleroy - please could we have a new release? I expect 5.0.1 is OK as a release number - we've not changed any "features"? |
|
(I'm also happy to do the release, obviously) |
|
Feel free to do a release when you think it's ready. Thanks! |
Use the Standard Library modules for OCaml 4.x, but override 4.14's to eliminate the deprecation warning.