ocaml / camlp-streams

The Stream and Genlex libraries for use with Camlp4 and Camlp5
Other
15 stars 6 forks source link

Restore compatibility with Standard Library modules on OCaml 4.x #5

Closed dra27 closed 2 years ago

dra27 commented 2 years ago

Use the Standard Library modules for OCaml 4.x, but override 4.14's to eliminate the deprecation warning.

Octachron commented 2 years ago

A potential problem with the special casing of 4.14 is that it would be the only version where Stdlib.Stream.t and Stream.t are valid but incompatible types.

dra27 commented 2 years ago

Very good point! I’ll push a revision shortly

dra27 commented 2 years ago

Not the prettiest workaround, but I've amended the dune so that:

xavierleroy commented 2 years ago

Apologies for a late and basic question, but: what's the motivation? what problem is this PR addressing?

dra27 commented 2 years ago

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.

xavierleroy commented 2 years ago

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.

dra27 commented 2 years ago

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

dra27 commented 2 years ago

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 Stream.t from camlp-streams isn't equal to Stdlib.Stream.t, which has also been coming up.

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.

dra27 commented 2 years ago

(obviously the test is irrelevant for 5.x as there's no Stdlib.Stream to test against)

dra27 commented 2 years ago

Argh - this won't work on MSVC (for 4.x) - fix incoming!

dra27 commented 2 years ago

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 Stdlib module) do the same as 4.14 - they create a Genlex and Stream which are compatible with the Standard Library's. In many respects, that's more consistent.

I have a version which can do this with a static dune file, but it's not pretty - I have a solution for 4.02-4.06 for MSVC which works for installation, but not for Dune vendoring. On the basis that we do need a release, I'd propose this goes in now and the MSVC issue gets fixed in the future.

dra27 commented 2 years ago

@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"?

dra27 commented 2 years ago

(I'm also happy to do the release, obviously)

xavierleroy commented 2 years ago

Feel free to do a release when you think it's ready. Thanks!