ocaml-ppx / ppx_import

Less redundancy in type declarations and signatures
MIT License
89 stars 28 forks source link

Remove a warning in OCaml 4.11 #49

Closed kit-ty-kate closed 4 years ago

kit-ty-kate commented 4 years ago

Currently ppx_import does not support OCaml 4.11. This PR fixes the issue

ejgallego commented 4 years ago

Thanks a lot @kit-ty-kate , it seems you need to add the beta opam repos to the travis file?

ejgallego commented 4 years ago

Will merge ASAP, this need a changes.md entry [I can add it myself, or feel free to do it yourself]

ejgallego commented 4 years ago

@kit-ty-kate I see you did not update the opam file, should that also wait?

ejgallego commented 4 years ago

Umm, CI fails now, how bizarre, I think it worked before?

kit-ty-kate commented 4 years ago

I think it did not work before, the CI wasn't triggered it seems. I'll fix it, sorry for that.

ejgallego commented 4 years ago

@kit-ty-kate I see you did not update the opam file, should that also wait?

Yup that made CI fail, what should we do?

kit-ty-kate commented 4 years ago

Oops, sorry. Fixed.

Allowing to compile with later compiler should be ok as this package uses ocaml-migrate-parsetree. We'll constrain the package if necessary in opam-repository.

As for ppxlib, I couldn't find any use of it in the source code. The dependency was apparently added in https://github.com/ocaml-ppx/ppx_import/pull/27 but never used.

ejgallego commented 4 years ago

Allowing to compile with later compiler should be ok as this package uses ocaml-migrate-parsetree. We'll constrain the package if necessary in opam-repository.

Actually ppx_import uses some parts of the typed tree thus unfortunately ocaml-migrate-parsetree doesn't fully capture the version-specific dependencies. Thus the < 4.12 constrain has to be kept.

See also https://github.com/ocaml-ppx/ocaml-migrate-parsetree/issues/52

kit-ty-kate commented 4 years ago

Sure, but I'm not sure a potential breakage implies that every compiler has to be checked by hand. For instance if ppx_import hadn't had the constraint I would've not have to do this PR (this PR only fixes a warning). Technically the previous version of ppx_import works with 4.11 if only the constraint was removed.

ejgallego commented 4 years ago

Given the hard dependency ppx_import has on OCaml internals I'm not sure I'd feel comfortable not using an upper bound. I prefer the dependency set to be consistent over more general but potentially broken.

ejgallego commented 4 years ago

@kit-ty-kate , I did squash with the intention of merging, but it seems the opam solver cannot satisfy the dependencies in 4.11 ? Having a look locally.

ejgallego commented 4 years ago
The following dependencies couldn't be met:
  - ppx_import → ppx_deriving >= 4.2.1 → ppx_tools >= 4.02.3 → ocaml < 4.11
      base of this switch (use `--unlock-base' to force)
  - ppx_import → ppx_deriving >= 4.2.1 → ocaml < 4.11.0
      base of this switch (use `--unlock-base' to force)
No solution found, exiting
The command "opam install --deps-only -d -t ppx_import" exited with 20.
ejgallego commented 4 years ago

Yup, ppx_deriving cannot be installed in 4.11, thus no way to run the test suite; I guess we'll have to wait until this is fixed.

kit-ty-kate commented 4 years ago

ah right, this requires https://github.com/ocaml-ppx/ppx_tools/pull/82. Hopefully I'll make a new release by next week. If you want to test it locally you can use https://github.com/kit-ty-kate/opam-alpha-repository. There should be everything you need with that.