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

Upgrade ocaml-migrate-parsetree to support latest ocaml #275

Merged
merged 4 commits into from
Jul 10, 2020
Merged

Upgrade ocaml-migrate-parsetree to support latest ocaml #275

merged 4 commits into from
Jul 10, 2020

Conversation

bikallem
Copy link
Contributor

@bikallem bikallem commented Jun 7, 2020

This PR upgrades the ocaml-migrate-parsetree to the latest version supporting ocaml up to version 4.11.

The PR also upgrades dune to v2.0 to address some dune warnings.

All tests pass.

Copy link
Member

@avsm avsm left a comment

Choose a reason for hiding this comment

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

Really nice work, thank you for this! Just some minor comments in the review, and then I'm happy to merge and release it.

@@ -1,2 +1,5 @@
(lang dune 1.0)
(lang dune 2.0)
Copy link
Member

Choose a reason for hiding this comment

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

This needs the ppx_cstruct.opam to have a corresponding dune {>= "2.0.0"} entry.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

Copy link
Member

Choose a reason for hiding this comment

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

Sorry, just one last change needed here before merge. Since the dune-project affects all the associated projects, every opam package needs a corresponding dune >2 entry. e.g. the Drone CI is failing as its trying to install cstruct with dune 1.11 and that no longer works.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I have now added dune {>= "2.0.0"} constraint to all .opam files in repo.

dune-project Show resolved Hide resolved
@@ -10,5 +10,5 @@ type foo64 =
[@@uint64_t]
]

let%lwt foo = Lwt.return ()
let foo = Lwt_main.run (Lwt.return ())
Copy link
Member

Choose a reason for hiding this comment

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

Why change this? That use of let%lwt was there in the test suite was there to check that ppx_cstruct and ppx_lwt work together.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The recent version of ppx_lwt gives the warning below,

File "ppx_test/with-lwt/ppx_cstruct_and_lwt.ml", line 13, characters 0-27:
Error (warning 22): let%lwt should not be used at the module item level.
Replace let%lwt x = e by let x = Lwt_main.run (e)

I have now modified the test to use let%lwt inside rather than at the module toplevel to get around the warning.

Copy link
Member

Choose a reason for hiding this comment

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

Good call; there should be no interop problems with modern ppx, but there were in the past, so its good to keep the test around.

@bikallem
Copy link
Contributor Author

bikallem commented Jun 9, 2020

@avsm all checks are green. I think it is ready to merge.

@avsm avsm merged commit 31e19be into mirage:master Jul 10, 2020
@avsm
Copy link
Member

avsm commented Jul 10, 2020

Thanks! Releasing shortly.

avsm added a commit to avsm/opam-repository that referenced this pull request Jul 15, 2020
…uct-sexp and cstruct-lwt (5.2.0)

CHANGES:

Upgrade the `ppx_cstruct` library to use the OCaml 4.11
AST rather than than OCaml 4.04, which in turn should
make it easier to port to ppxlib in the future and
improve interoperability with other PPXs (@bikallem mirage/ocaml-cstruct#275).

Also upgrades build files to use dune 2.0 (@bikallem mirage/ocaml-cstruct#275)
and fixes the GitHub Actions versions (@smorimoto mirage/ocaml-cstruct#273)
and also test OCaml 4.10.0 (@avsm).
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.

2 participants