ppxlib icon indicating copy to clipboard operation
ppxlib copied to clipboard

trunk-support does't build with ocaml-trunk

Open hhugo opened this issue 2 years ago • 10 comments

#=== ERROR while compiling ppxlib.0.29.1 ======================================#
# context     2.1.0 | linux/x86_64 |  | pinned(git+https://github.com/ocaml-ppx/ppxlib.git#trunk-support#cc54c517)
# path        ~/.opam/trunk/.opam-switch/build/ppxlib.0.29.1
# command     ~/.opam/opam-init/hooks/sandbox.sh build dune build -p ppxlib -j 7 @install
# exit-code   1
# env-file    ~/.opam/log/ppxlib-149928-37f429.env
# output-file ~/.opam/log/ppxlib-149928-37f429.out
### output ###
# (cd _build/default && /home/hugo/.opam/trunk/bin/ocamlc.opt -w -9 -g -bin-annot -I astlib/.astlib.objs/byte -I /home/hugo/.opam/trunk/lib/ocaml-compiler-libs/common -I /home/hugo/.opam/trunk/lib/ocaml/compiler-libs -no-alias-deps -open Astlib__ -o astlib/.astlib.objs/byte/astlib__Ast_502.cmo -c -impl astlib/ast_502.pp.ml)
# File "astlib/ast_502.ml", lines 113-195, characters 2-33:
# Error: This variant or record definition does not match that of type
#          "Parsetree.core_type_desc"
#        An extra constructor, "Ptyp_open", is provided in the original definition.

hhugo avatar Jul 19 '23 09:07 hhugo

Thanks for the report!

This seems to be due to https://github.com/ocaml/ocaml/pull/12044 being merged in trunk. Usually, we are tagged in PRs that modify the parsetree, so that we can react and update the trunk-support branch, but this time we were not. We'll try to update the branch!

(FTR, there is also https://github.com/ocaml/ocaml/pull/12236 which modifies the parsetree and certainly broke trunk-support).

panglesd avatar Jul 21 '23 21:07 panglesd

Small update: I opened #451 for re-adding trunk support.

The migration is not completely sound yet, but I'll try to improve this before I go in holiday. But it compiles, and depending on what you want to do, it might be "sufficient" (I am not confident in saying this).

panglesd avatar Aug 01 '23 15:08 panglesd

Broken again due to https://github.com/ocaml/ocaml/pull/12639

hhugo avatar Dec 02 '23 08:12 hhugo

Hello @hhugo , thanks for reporting!

Manually maintaining a ppxlib branch to support OCaml trunk is a lot more work than we predicted. In case you want to know why: In the past, most OCaml parsetree changes (in fact, IIRC, all changes since OCaml 4.11.0 till 5.0.0 included) were syntax additions. Syntax additions are very easy to capture in our parsetree migrations. However, both for OCaml 5.1.0 and for OCaml 5.2.0, several parsetree changes are parsing changes, such as the new one you're pointing to here. Those changes need a lot of attention to detail to get the migrations right. So writing those migrations required both a lot of time and a deep context switch from what we usually do.

That's a lot of detail for saying: As you can read in our last meeting notes, we've decided to stop the efforts to manually support OCaml trunk. Instead, we're looking for ways to allocate time to upstream Astlib.

What do you need OCaml trunk support for? Is it for js_of_ocaml? What did you do in that context before we started the "manually support trunk" efforts?

pitag-ha avatar Dec 07 '23 11:12 pitag-ha

I use trunk support to regularly test trunk or open ocaml PRs with jsoo. It is useful to give early feedback. In the past I've been contributing to migrate-parsetree or patching ppxlib myself. It seems to me that dropping manual support for trunk is an overreaction, but well.

It's true that some recent changes have been harder to integrate into ppxlib but I don't imagine this to become the new standard.

I first heard about having Astlib in the compiler back in 2019, but I haven't seen much progress so far. Do you have any timeline for this ?

Meanwhile, I've decided use my own fork again and move forward testing trunk.

hhugo avatar Dec 07 '23 13:12 hhugo

I don't really understand why https://github.com/ocaml-ppx/ppxlib/pull/451 has not been merged already. It's not like it's going to be part of the main branch. It doesn't have to be perfect. For the record, the change needed to build against trunk can be found in
https://github.com/hhugo/ppxlib/commits/jsoo-ocaml-52

hhugo avatar Dec 07 '23 14:12 hhugo

Thanks @hhugo for the ping on the compiler parsetree change, and for the fix!

It's not like it's going to be part of the main branch. It doesn't have to be perfect.

When OCaml 5.2 is released, we need to have the migrations ready. So if the trunk-support branch is of good quality and sufficiently tested, then yes it's going to be part of the main branch! But good quality may mean some work to have the migration round-trip, even wrt locations.

Maybe a solution is to assign a different function to the "trunk-support" branch (at least, different from what I though). The trunk support branch could be a branch for "quick and not perfect" patch, that are useful for testing opam packages against trunk. In this case, it could be community-driver, with quick merges (since it does not have to be perfect!) and at least, the community would do the work (such as your patch) only once!

What do you think @pitag-ha @hhugo ?

panglesd avatar Dec 07 '23 19:12 panglesd

Sounds very good to me. If that sounds good to @hhugo, we can merge https://github.com/hhugo/ppxlib/commits/jsoo-ocaml-52 into our trunk-support branch.

pitag-ha avatar Dec 07 '23 20:12 pitag-ha

Sounds good to me.

hhugo avatar Dec 07 '23 20:12 hhugo

https://github.com/ocaml-ppx/ppxlib/pull/487

hhugo avatar Apr 27 '24 21:04 hhugo

I'm closing this as it has been fixed since then!

NathanReb avatar Jul 09 '24 08:07 NathanReb