Update to OCaml 5.1.1, jsoo 5.8.2 #602
Conversation
AltGr
force-pushed
the
ocamlup
branch
3 times, most recently
from
6296e2b to
9da4406
Compare
|
Build on OSX fails, but actually at the OCaml compilation stage ; I have no idea about the state of the OCaml 5.1 support, maybe it's been discontinued for x86_64 ? These issues are mostly unrelated to the current PR though. |
|
All other checks pass ; some more manual testing is still warranted (I checked the printing, running and grading only on a few ones) |
|
Hi @AltGr, thanks a lot for this meticulous work!
Yes I just saw this failure. Given the
FWIW I am a macOS user, not a macOS expert but I could test the macOS artifacts anyway!
Ah OK, great!
Yes. Except the macOS static binaries I'd say, as it will block continuous deployment and release. What do you think of just putting |
on second thought: given master already failed this way because of the arm image, I'll push a separate PR with this change. Then you'll be able to rebase on this small PR (and maybe add |
| (compilation_mode whole_program) | ||
| (flags --no-source-map --opt=2 --enable=use-js-string --target-env=browser))) | ||
|
|
||
| (dev (flags (:standard -safe-string -w -32 -warn-error -a+8)) |
There was a problem hiding this comment.
Two questions:
-
adding this
devprofile, should I change my usual learn-ocaml programming inner loop?Namely, coding then building learn-ocaml with a bash alias that runs:
cd $learn_ocaml && eval $(opam env) && opam install . --deps-only --locked && make && make opaminstall && learn-ocaml build --repo=demo-repository serve -
I had the impression that the dev profile had less warnings enabled than in the release profile ? but probably I'm mistaken
There was a problem hiding this comment.
Historically it was impossible to build in dev mode because (i) dune didn't allow to customise the jsoo flags manually, it was tied to the built-in profiles, and (ii) source-map was blowing on us and the compilation wouldn't terminate. Thankfully both of these are now fixed :)
In the new setting, the jsoo --opt=2 flag makes release compilation quite a bit slower so I'd advise to use the dev mode when, well, developping 🤯 . The Makefile still defaults to release though, I'll add a PROFILE ?= release variable.
Then you could just run make testrun PROFILE=dev
- hm it's quite possible, I haven't taken much attention to the specific warning flags ; I've copied them from another of my projects mainly to disable warn-error in dev mode which I find very hard to work with ("yes I know that this argument is unused, I just commented out the code, will you go on anyways ?" ;) )
|
Last question: in the end, would you like that we squash-merge your PR ? or keep all the commits in the master's history. In the latter case, I'd suggest you To make this easier, below are some suggestions for the 31 commits of the PR.
|
|
I'd prefer it not to be squashed as this loses a lot of information. In particular, if one wanted a build working with 4.14, it can easily be extracted now by selecting the correct intermediate commit, and that would be completely lost if squashed. I don't believe, however, that most of these commit belong to the changelog: saying that we updated jsoo and OCaml versions is the gist of it (plus a few aside improvements, like the |
|
Thanks a lot for reviewing, this last patch should address your concerns :) |
|
This has been long enough, I think this should be merged even with the failure which is limited to the macos scripts; |
|
Dear @AltGr,
Apologies for the ridiculously late review for this PR... |
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
erikmd
force-pushed
the
ocamlup
branch
3 times, most recently
from
c3dc438 to
6be6e54
Compare
|
Hi @AltGr, good news! the branch is ready to merge from my point of view. Thanks again for this important PR! I just let you take a quick look at my patches before that if you wish (otherwise, feel free to merge the PR now). A few comments meanwhile:
See you soon. |
|
@AltGr Last question for you! → what is the status of these two dependencies? Lines 86 to 95 in 50d5182 learn-ocaml/learn-ocaml-client.opam Lines 59 to 64 in 50d5182 |
|
Thanks @erikmd, for the last review and fixes ! Indeed that zstd linking issue warranted digging deeper than I'd have expected 😅 About 4., I think so, but without certainty... I am not sure about expliciting Yes, js_of_ocaml is a bit slow, but I don't think there is much we can do about it, we are asking a lot of work from it! Maybe we can try fiddling with the options and see if there is no unneeded debug flags left (I remember that, at some point, not disabling source-maps would lead to compilation that wouldn't even terminate -- within my patience timeframe anyway); we should check the weight of the js artifacts as well. Dependencies:
|
it's a bit annoying to have the client depend on asak just because of a datatype that's not used, but fixing that would require some reorganisation.
(so that it's already in the Docker images)
Fix CI's FTBFS on (macOS, opam/--locked)
href: mirage/ocaml-conduit@f5facd8 TODO-Later: Remove `<` bound, Bump cohttp-lwt-unix's minimal version, Fix build
- hopefully -
Avoid the following error at ld time:
```
ld: Undefined symbols:
_ZSTD_compressStream2, referenced from:
_caml_zstd_compress in ocamlcommon.a[114](zstd.npic.o)
_ZSTD_createCCtx, referenced from:
_caml_zstd_compress in ocamlcommon.a[114](zstd.npic.o)
_ZSTD_decompress, referenced from:
_caml_zstd_decompress in ocamlcommon.a[114](zstd.npic.o)
_ZSTD_freeCCtx, referenced from:
_caml_zstd_compress in ocamlcommon.a[114](zstd.npic.o)
clang: error: linker command failed with exit code 1 (use -v to see invocation)
File "caml_startup", line 1:
Error: Error during linking (exit code 1)
ocamlc src/main/.learnocaml_main.eobjs/byte/dune__exe__Learnocaml_main.{cmi,cmti}
ocamlc src/repo/.learnocaml_process_repository_lib.objs/byte/learnocaml_process_exercise_repository.{cmo,cmt}
ocamlc src/repo/.learnocaml_process_repository_lib.objs/byte/learnocaml_process_tutorial_repository.{cmo,cmt}
```
as for some reason a duplicate flag was passed, causing failure: href: https://github.com/ocaml-sf/learn-ocaml/actions/runs/12846419515/job/35821578447?pr=602 ``` #26 15.79 - File "src/app/dune", line 86, characters 7-30: #26 15.79 - 86 | (name learnocaml_student_view) #26 15.79 - ^^^^^^^^^^^^^^^^^^^^^^^ #26 15.79 - (cd _build/default/src/app && /home/opam/.opam/5.1/bin/js_of_ocaml --no-source-map --no-source-map --opt=2 --enable=use-js-string --target-env=browser -o learnocaml_student_view.bc.js /home/opam/.opam/5.1/lib/cstruct/cstruct.js /home/opam/.opam/5.1/lib/bigstringaf/runtime.js ../ace-lib/ace_bindings.js learnocaml_student_view.bc-for-jsoo) #26 15.79 - js_of_ocaml: option '--no-source-map' cannot be repeated #26 15.79 - Usage: js_of_ocaml [COMMAND] … #26 15.79 - Try 'js_of_ocaml --help' for more information. ``` using js_of_ocaml 5.9.1
now that asak 0.5 and ocp-indent-nlfork 1.5.5 are published.
|
Rebased and removed the pin-depends ; note that ocp-indent-nlfork is still pending publication (ocaml/opam-repository#27305) so CI is expected to fail until then |
|
Thanks a lot @AltGr for your feedback and for the rebase! I propose to:
Fine with you? After the release, feel free to make the announcement on Discourse OCaml and So feature-wise, this release will be twofold:
|
|
Regarding the last failing CI job, it seems that the image Other minor questions (follow-up of the upcoming merge of this PR):
|
(reopen of #601, closed by mistake)
This mainly required time debugging and finding which parts of the code where silently failing with the newer version. Once I found the way to make our version of ppx_metaquot work with the newer ppxlib (very small code change in the end, but it took quite a bit of digging the ppxlib api), most of the effort was actually in updating jsoo rather than OCaml.
We stick at 5.1 for now because ppx_tools (of which we vendor a patched bit) could easily work with it, but isn't ported to 5.2 yet. There has been lots of improvements in jsoo in the 5.x branch, and I expect things to be faster although I haven't run benches.
I expect this to be part of a 1.1 release and bumped the version number in prevision