Closed leostera closed 5 months ago
Got here too late: My branch is here https://github.com/DiningPhilosophersCo/utop/tree/prometheansacrifice%40ocaml-5-2 Atleast I think I can lend a hand with a code review. Will leave a comment if necessary. This was fun!
Thanks for this PR. I think that this is going in the right direction but the main thing that is left to address is compatibility. This version of utop needs to build and run on OCaml 4.11 to 5.2.0 (that seems large but bear in mind that I've already axed much of the compatibility so it's a lot easier than it used to be). To have a system where it is easy to test compat code, I recommend the following set up:
dune-workspace.multi
(do not commit this file to the repository)
(lang dune 1.0)
(context (opam (switch utop-411)))
(context (opam (switch utop-412)))
(context (opam (switch utop-413)))
(context (opam (switch utop-414)))
(context (opam (switch utop-500)))
(context (opam (switch utop-510)))
(context (opam (switch utop-520)))
$ opam sw create utop-411 ocaml-base-compiler.4.11.1
$ opam install --switch utop-411 utop --deps-only --with-test
...
$ opam sw create utop-510 ocaml-base-compiler.5.1.0
$ opam install --switch utop-510 utop --deps-only --with-test
$ opam sw create utop-520 ocaml-variants.5.2.0+trunk
$ opam install --switch utop-520 utop --deps-only --with-test
(note the --switch ...
flags passed to install
, and the fact we used ocaml-base-compiler
everywhere but ocaml-variants
in the 5.2.0
case)
dune build
, use dune build --workspace dune-workspace.multi
. This is going to make a dune build that builds using all the different switches. Error messages will carry the name of the opam switch that failed, so you can use that to know what went wrong.This is the main trick and the most important part to get a green build on all supported versions.
Now, regarding architecture, utop used to have many #if
s sprinkled all around the code base. I moved most of them to uTop_compat.ml
. Please aim to put new compat code in there but I appreciate that it's not always possible.
I'll leave the rest of my comments as a review. Thanks again.
@emillon thanks for the guidance! 🙏🏼 made most of the changes (except the add_cmi_hook
, but left you a question there). Let me know how you want to proceed 🙌🏼
(I haven't forgotten, I'll get back to you about this during week 1)
@leostera I rebased this on top of master
, added a changelog crediting all coauthors.
This is good to go for a release supporting 5.2 but there will probably be things to fix regarding hidden paths, and we'll have to decide if we want to provide a precise API for the load path.
Thanks everyone. that was the first time I was looking for external contributors and this was very succesful, despite the delay in merging!
Would it be possible to get a new release?
@kit-ty-kate You don't have access to do that on your own? We could give it to you.
as I said previously, I'm waiting for 5.2 in ocaml-ci to cut the release to make sure it works there. this should happen after the first alpha, right?
alpha1 is already available but ocaml-ci hasn't been updated yet. This is tracked at https://github.com/ocurrent/ocaml-ci/issues/917
In the meantime, is there any chance you could spawn an opam switch locally instead? It should take less than 3 minutes
Hi folks! I saw Sabine's tweet and I figured I'd give this a shot. There's a few things that I'd like to upstream to the compiler (like Ast_helpers to create function params), but otherwise this seems to be working.
Happy to amend anything here, just figured I'd open the PR early to get feedback / directions.
Ideally would close #466