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

trunk-support does't build with ocaml-trunk #449

Closed
hhugo opened this issue Jul 19, 2023 · 11 comments
Closed

trunk-support does't build with ocaml-trunk #449

hhugo opened this issue Jul 19, 2023 · 11 comments

Comments

@hhugo
Copy link
Collaborator

hhugo commented Jul 19, 2023

#=== 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.
@panglesd
Copy link
Collaborator

Thanks for the report!

This seems to be due to ocaml/ocaml#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 ocaml/ocaml#12236 which modifies the parsetree and certainly broke trunk-support).

@panglesd
Copy link
Collaborator

panglesd commented Aug 1, 2023

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).

@hhugo
Copy link
Collaborator Author

hhugo commented Dec 2, 2023

Broken again due to ocaml/ocaml#12639

@pitag-ha
Copy link
Member

pitag-ha commented Dec 7, 2023

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?

@hhugo
Copy link
Collaborator Author

hhugo commented Dec 7, 2023

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
Copy link
Collaborator Author

hhugo commented Dec 7, 2023

I don't really understand why #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

@panglesd
Copy link
Collaborator

panglesd commented Dec 7, 2023

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 ?

@pitag-ha
Copy link
Member

pitag-ha commented Dec 7, 2023

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.

@hhugo
Copy link
Collaborator Author

hhugo commented Dec 7, 2023

Sounds good to me.

@hhugo
Copy link
Collaborator Author

hhugo commented Apr 27, 2024

#487

@NathanReb
Copy link
Collaborator

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

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

No branches or pull requests

4 participants