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

Support ocaml 4.08 and 4.09 #46

Merged
merged 8 commits into from
Feb 21, 2020
Merged

Conversation

emillon
Copy link
Contributor

@emillon emillon commented Feb 18, 2020

Hi!

This PR adds support to OCaml 4.08 (#41). There are several aspects to this:

  • some helpers can be made compatible with more versions of the compiler "for free" by using features of compiler-libs (see the first commits)
  • the main bit of work is being able to deconstruct Types.module_type and Types.signature_item from different versions of the compiler. To do so, I created a Utypes module that contains just what we need, and some shallow conversion functions are provided in a compat module that is selected by dune.

This is not perfect but I think it's a nice solution until OMP or a similar library supports Types.

Some additional remarks:

  • I think that the first commits make sense to merge even if the rest is not
  • 4.09 compatibility is fairly easy to add (part of the init code needs to be put in compat), but this either adds duplication (we'd need a compat file for <= 4.07, one for 4.08, and one for >= 4.09) or complexity (by having one compat module by versioned feature).

Let me know what you think.

Thanks

@emillon emillon requested a review from ejgallego February 18, 2020 15:56
@ejgallego
Copy link
Collaborator

Great, thanks a lot @emillon ! I'll try to review this ASAP.

Main question I have tho is if we should just add support for this in OMP directly as requested in ocaml-ppx/ocaml-migrate-parsetree#52 ?

I'm unsure how much extra work that'd be, just mentioning so this PR is linked to the above issue.

@emillon
Copy link
Contributor Author

emillon commented Feb 18, 2020

The support here is really ad hoc to what's used in ppx_import. Assuming there's support in OMP, this bit can be removed, but it seems that it would add a lot of code to OMP.

@ejgallego
Copy link
Collaborator

The support here is really ad hoc to what's used in ppx_import. Assuming there's support in OMP, this bit can be removed, but it seems that it would add a lot of code to OMP.

Indeed I dunno what OMP maintainers may think; I suggest we ping them. Meanwhile I don't see a problem adding this to ppx_import and removing it later if we migrate the code there.

@ejgallego ejgallego self-assigned this Feb 18, 2020
src/dune Outdated Show resolved Hide resolved
Copy link
Collaborator

@ejgallego ejgallego left a comment

Choose a reason for hiding this comment

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

LGTM, tested on my own projects on 4.07.1 and 4.08.1 and so far seems to work! A couple of comments:

  • see the note about using %{ocaml_version} and avoiding the gen.ml wrapper altoghether
  • I wonder if we want to introduce the UTypes changed constructors, maybe we could indeed just use aliases to the real constructor names as in OMP?

@emillon
Copy link
Contributor Author

emillon commented Feb 19, 2020

OK! I switched it to use the types from the 4.07 Types module (I replaced one unused argument by unit in order not to pull an unused type) and this seems to be working. Let me know if that works for you, in which case I'll squash that.

As for %{ocaml_version}, I don't think we can do that. It will expand to 4.08.1, but we need a way to map from these versions strings to an actual file like ge_408.ml. I don't think we have access to a useful comparison from dune file, so it seems to me that a tiny generator is required.

@ejgallego
Copy link
Collaborator

Sounds good @emillon , thanks! I think this is ready to merge, and do a release.

WDYT folks @gasche @whitequark ?

@ejgallego
Copy link
Collaborator

[I also wonder if you want to squash the last commit that undoes the introduction of Utypes as to get a slightly better git history]

@emillon
Copy link
Contributor Author

emillon commented Feb 19, 2020

Done 👍

@ejgallego
Copy link
Collaborator

ejgallego commented Feb 19, 2020

@emillon I also realized while testing the release that you should add an entry in CHANGES.md

@emillon
Copy link
Contributor Author

emillon commented Feb 20, 2020

Sure, will do! Should I try to add 4.09 support while I'm at it? with the new way of reusing 4.07 types it might not be too bad.

@ejgallego
Copy link
Collaborator

Sure, will do! Should I try to add 4.09 support while I'm at it? with the new way of reusing 4.07 types it might not be too bad.

As you want @emillon ; we can always do that in a separate PR.

I propose to merge tomorrow afternoon if no futher comments arise [but can wait if you want]

@emillon
Copy link
Contributor Author

emillon commented Feb 20, 2020

  • Added 4.09 support. This turns gen.ml into a cppo-like program that includes different fragments.
  • I also noticed that metaquot was listed in (libraries) which is not necessary (and potentially harmful).
  • I updated the changelog!
  • I did not squash in order to make the review easier but I'll do that if you're fine with the changes.

And that should be ready.

Thanks!

@ejgallego ejgallego changed the title Support ocaml 4.08 Support ocaml 4.08 and 4.09 Feb 20, 2020
@ejgallego
Copy link
Collaborator

Thanks a lot @emillon

  • Added 4.09 support. This turns gen.ml into a cppo-like program that includes different fragments.

Great!

  • I also noticed that metaquot was listed in (libraries) which is not necessary (and potentially harmful).

This is likely some legacy code from the initial port to dune.

  • I updated the changelog!
  • I did not squash in order to make the review easier but I'll do that if you're fine with the changes.

Changes look fine to me; I'll leave some time for comments, then merge + release to opam.

We should update the .travis.yml file tho; I have pushed the fix myself.

@emillon
Copy link
Contributor Author

emillon commented Feb 21, 2020

I cleaned the history a bit, should now be all good.

For Types, values are shallowly migrated to 4.07 versions of
`Types.module_type` and `Types.signature_item`.
The code to convert for `Types` to 4.07 versions is selected by dune
according to the compiler version.

In 4.09, the argument passed to init_path is changed, so the right piece
of code is selected.
@ejgallego ejgallego merged commit dc680ca into ocaml-ppx:master Feb 21, 2020
@emillon emillon deleted the ocaml-408 branch February 21, 2020 19:47
kit-ty-kate pushed a commit to ocaml/opam-repository that referenced this pull request Feb 22, 2020
CHANGES:

* OCaml 4.08 and 4.09 support
    ocaml-ppx/ppx_import#46
    (Etienne Millon)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants