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

[ci] [opam] Bump OCaml upper bound to 4.12.0 #53

Merged
merged 1 commit into from
Mar 1, 2021

Conversation

ejgallego
Copy link
Collaborator

ppx_import seems to work in 4.12 without further changes.

We may want to update the AST at a later point.

A question is if we should do a new release for this change in the
Opam file, but IMHO it would be more practical just to update
opam-repos?

What do you think @gasche @kit-ty-kate ?

@ejgallego
Copy link
Collaborator Author

ejgallego commented Feb 26, 2021

Actually there is a subtle problem in opam-repository, ppx_import 1.7.0 doesn't declare an upper bound, so it is installable under 4.12.0, however 1.8.0 is not. 1.7.0 seems to work fine too.

@kit-ty-kate
Copy link
Contributor

yeah I had already removed the restriction before the previous release. I missed the new unnecessary upper-bound constraint in the latest release two weeks ago. I'll remove the constraint from opam-repo in 5 minutes

@kit-ty-kate
Copy link
Contributor

@ejgallego
Copy link
Collaborator Author

@kit-ty-kate fantastic thanks; do you suggest then we remove the constraint in the .opam file too?

@kit-ty-kate
Copy link
Contributor

yeah I think it would be better to remove it. In my opinion upper-bound constraints on packages make sense for packages that explicitly require a range of versions (OMP, ppxlib, camlp4, camlp5, reason, merlin, …) or if you know in advance something will/does break for sure. For pretty much everything else, it doesn't make much sense to have it in the dev repository

ppx_import seems to work in 4.12 without further changes.

We may want to update the AST at a later point.

Following advice from the Opam maintainers we remove the upper bound
for OCaml.
@ejgallego
Copy link
Collaborator Author

yeah I think it would be better to remove it. In my opinion upper-bound constraints on packages make sense for packages that explicitly require a range of versions (OMP, ppxlib, camlp4, camlp5, reason, merlin, …) or if you know in advance something will/does break for sure. For pretty much everything else, it doesn't make much sense to have it in the dev repository

Thanks for the feedback, adjusted!

@gasche
Copy link
Contributor

gasche commented Feb 27, 2021

If whatever variant of ppx_tools we use has a 4.12 release, we should also try to upgrade to that, so that 4.12-specific AST constructs are handled properly by ppx-import.

@ejgallego
Copy link
Collaborator Author

If whatever variant of ppx_tools we use has a 4.12 release, we should also try to upgrade to that, so that 4.12-specific AST constructs are handled properly by ppx-import.

Indeed that sounds good, would require a new release; I will merge this tweak for now and we can try to bump the AST version when we get a few free cycles.

@ejgallego ejgallego merged commit 3373bf5 into ocaml-ppx:master Mar 1, 2021
@ejgallego ejgallego deleted the ocaml+412 branch March 1, 2021 18:01
@ejgallego
Copy link
Collaborator Author

I'm not sure if ppx_tools_versioned will support 4.12 soon, so if the plan for them is not to do so, we may need to solve #44 first.

kit-ty-kate pushed a commit to ocaml/opam-repository that referenced this pull request Jan 13, 2022
CHANGES:

  * Migrate to ppxlib ocaml-ppx/ppx_import#54 , closes ocaml-ppx/ppx_import#44 (tatchi)

  * Drop support for OCaml `4.04.2`. Minimal supported version is now `4.05.0` ocaml-ppx/ppx_import#54 (tatchi)

  * Bump minimum dune version to 1.11 ocaml-ppx/ppx_import#56 (tatchi)

  * Update CI to test OCaml 4.12.0, no changes required
    (ocaml-ppx/ppx_import#53, Emilio J. Gallego Arias)

  * Remove the OCaml upper bound in the opam file
    (ocaml-ppx/ppx_import#53, Emilio J. Gallego Arias, kit-ty-kate)
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.

3 participants