Update to OCaml 5.1.1, jsoo 5.8.2

[AltGr]

trafficstars

(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

[AltGr]

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 ? The CI should probably be updated but we need someone with familiarity with these systems to do so. Note also that with the new support of opam for Windows, it probably wouldn't be too difficult to propose native Windows builds, as well.

These issues are mostly unrelated to the current PR though.

[AltGr]

All other checks pass ; some more manual testing is still warranted (I checked the printing, running and grading only on a few ones)

[erikmd]

Hi @AltGr, thanks a lot for this meticulous work!

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 ?

Yes I just saw this failure. I think it is not an OCaml error, but the fact the macos-latest GHA image now only provide arm support: https://github.com/actions/runner-images/issues/9741

Given the static-bin-macos CI job manually installs opam*x86_64-macos, I see two approaches:

• either we use the macos-13 image,
• or we replace the script with installing an arm version of opam/ocaml (this shouldn't induce much change I guess… but maybe reduce the compatibility for old macOS users, won't it?)

The CI should probably be updated but we need someone with familiarity with these systems to do so.

FWIW I am a macOS user, not a macOS expert but I could test the macOS artifacts anyway!

Note also that with the new support of opam for Windows, it probably wouldn't be too difficult to propose native Windows builds, as well.

Ah OK, great! anyway in the meantime, users can rely on WSL.

These issues are mostly unrelated to the current PR though.

Yes. Except the macOS static binaries I'd say, as it will block continuous deployment and release.

What do you think of just putting macos-13 here? https://github.com/ocaml-sf/learn-ocaml/blob/f318b958dae945d7f7e7a7b8fe091f51eb22263e/.github/workflows/static-builds.yml#L75

[erikmd]

What do you think of just putting macos-13 here?

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 feat:/fix: prefixes to more commits messages in your PR - those that look important to you)

[erikmd]

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 rebase -i with rewording some commits messages just to add some prefix (you know, this is important because they will automatically populate the release changelog only if there's some prefix).

To make this easier, below are some suggestions for the 31 commits of the PR.

• 21f51fdd déjà OK feat(build): Migrate ocplib-i18n to ppxlib
• c29d4ae0 Suggestion=> deps(opam): Use pin-depends until release of newer ocp-indent-nlfork
• 7e744ff8 Suggestion=> build(dune): Update stdlib file names for 4.13
• f69fb6bf déjà OK fix(build): Fix/disable some warnings
• 82c1ca79 Suggestion+Typo=> fix: Update genlifter.ml from upstream ppx_tools
• 6a4e595f Suggestion=> deps(opam): Remove dependency on ocaml-migrate-parsetree
• e7651067 Suggestion=> feat: Upgrade to OCaml 4.13
• c40c8807 Suggestion+Typo=> feat: Update to OCaml 4.14.2
• 3b5835a9 Suggestion=> refactor: Disable Asak for now (it's not compatible with OCaml 5.0 at the moment)
• 28139442 Suggestion=> build(dune): Update dune-project
• 97d97836 Suggestion=> deps(opam): ocplib-json-typed is now json-data-encoding
• f5fd701b Suggestion=> deps(opam): Update opam files for OCaml 5.0
• 6b4289e5 Suggestion=> build(dune): ppx_ocplib_i18n doesn't cope well with dune sandboxing
• 2546d479 Suggestion=> refactor: Disable partitioning functionality for now (asak isn't compatible with 5.0)
• 3f1d191e Suggestion=> build(duine): Update stdlib cmi list
• 64b49681 OK Now compiles (but is broken) with 5.0.0
• b9dc09c2 Suggestion=> fix: a bunch of deprecation warnings & errors
• 6032da4c Suggestion=> fix(dune): Adjust jsoo compilation flags
• 5d555bd3 Suggestion=> deps(opam): Working with OCaml 5.0.0 / jsoo 5.0.1
• 0217355f Suggestion=> deps(opam): Update jsoo to 5.1.1
• 378c951f Suggestion=> fix: Many fixes and update for jsoo 5
• 3641979e Suggestion=> fix: Actually bind the --dry-run flag
• 993e286e Suggestion=> refactor: Improve web-worker debug messages
• b6c9cc0c Suggestion=> feat: Adjust to OCaml 5.1.1
• ebd00919 déjà OK feat: complete porting to OCaml 5.1 and jsoo 5.8
• e87ad942 déjà OK feat: bump version to 1.1.0
• 787440da déjà OK feat: Reenable asak (using pin-depends for compat patch with OCaml 5)
• fc6fafe0 déjà OK fix(CI): fix Dockerfile and client dependencies
• c10038b8 déjà OK fix(CI): use older dependency releases in lockfile
• 5c5a1332 déjà OK fix(CI): fix static builds and some specific deps
• 73423d10 déjà OK fix(CI): fix base Alpine version for final Docker images

[AltGr]

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 --dry-run flag fix). For example, it doesn't hold much meaning to have a changelog stating that Asak was disabled and then re-enabled.

[AltGr]

Thanks a lot for reviewing, this last patch should address your concerns :)

[AltGr]

This has been long enough, I think this should be merged even with the failure which is limited to the macos scripts; Unless there are comments within the next days ?

[erikmd]

Dear @AltGr, Best wishes for the new year!

This has been long enough, I think this should be merged even with the failure which is limited to the macos scripts; Unless there are comments within the next days ?

Apologies for the ridiculously late review for this PR... Also, thank you Louis for posting this follow-up comment, without forcibly merging the PR without notice. So I'm working on the remaining issue tonight.

[erikmd]

I tried rebuilding the branch on a fresh opam switch with --deps-only --locked on a macOS version 14.5.

The bad news is that the compilation stalls with something like:

ld: warning: ignoring duplicate libraries: '-lunixbyt'
Done: 98% (1439/1462, 23 left) (jobs: 8)

with no progress despite waiting a dozen of minutes.

I don't know if this issue is the same as the issue you got with the CI. Anyway, I'll push a temporary debugging commit in your branch to make the macOS build more verbose in the CI.

[erikmd]

FYI, the macOS log now says

[NOTE] You can retry with '--assume-depexts' to skip this check, or run 'opam option depext=false' to permanently disable handling of system packages.

       Running the system package manager non-interactively requires '--confirm-level=unsafe-yes'.

The following system packages will first need to be installed:
    pkg-config

<><> Handling external dependencies <><><><><><><><><><><><><><><><><><><><>  🐫 

opam believes some required external dependencies are missing. opam can:
> 1. Run brew to install them (may need root/sudo access)
  2. Display the recommended brew command and wait while you run it manually (e.g. in another terminal)
  3. Continue anyway, and, upon success, permanently register that this external dependency is present, but not detectable
  4. Abort the installation

[1/2/3/4] 4

Switch initialisation failed: clean up? ('n' will leave the switch partially installed) [y/n] y
Error: Process completed with exit code 10.

while the CI script already contains: https://github.com/ocaml-sf/learn-ocaml/blob/5780d3ab58961b0d386d89bf608c8fa8798ae1b1/.ci-macosx.sh#L39

so I guess I faced this known issue: https://discuss.ocaml.org/t/system-package-pkg-config-can-no-longer-be-found-opam-homebrew/15649/

@AltGr, what is the best fix for this issue from your viewpoint?

[erikmd]

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:

1. my tests confirmed that the implementation of a previously-discussed idea regarding CI and opam dependencies (namely, having jobs that test-build learn-ocaml (1) with the --locked dependencies & (2) with the latest compatible dependencies) was a great idea, and made it possible to detect several issues
2. I tried to keep the guideline of atomic commits, while in particular the static-build-linking-flags fix is split in two commits on-purpose, as I suspect the commit https://github.com/ocaml-sf/learn-ocaml/pull/602/commits/6be6e54e39c156784bfa86409dfb42f10a4db1d5 might be reverted later somehow if -lzstd can be dropped when migrating to ocaml 5.2, see also the issue https://github.com/ocaml/ocaml/issues/12562
3. I added https://github.com/ocaml-sf/learn-ocaml/pull/602/commits/de6c0def3e11f06ee0bada49706fdfb988ac7847 just for a debug purpose at first, but I propose to keep it as is, given this verbosity flag is a minimal one, and it may be puzzling to have no output at all with a ~10' build time!
4. quick question for you: with dune build --display=short, is the displayed step run after it is displayed, or before? (I believe it is "after", but I'd like to be sure ;)
5. finally regarding performance, I noticed the last js_of_ocaml build steps are a bit long, but given for example that a full build (% make clean ; time make) on Apple M1 + macOS 14.5 with 8 cores takes ~8', I think it's fine anyway!

   % time make
   (...)
   make  952,03s user 682,79s system 344% cpu 7:54,05 total
   

See you soon.

[erikmd]

@AltGr Last question for you! → what is the status of these two dependencies?

https://github.com/ocaml-sf/learn-ocaml/blob/50d5182db2c0cca9d41e2f11090a0228f418e0e3/learn-ocaml.opam#L86-L95

https://github.com/ocaml-sf/learn-ocaml/blob/50d5182db2c0cca9d41e2f11090a0228f418e0e3/learn-ocaml-client.opam#L59-L64

[AltGr]

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 --display=short as it may override user preferences in ~/.config/dune/config ; but anyway that's the correct setting so I'm fine with it ;)

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:

• asak ⇒ the PR has been merged upstream and asak 0.5 released so we should switch to that
• ocp-indent ⇒ I'm afraid we are stuck with the nlfork version, which changed the processing model quite a bit, but which couldn't make it into ocp-indent master -- changing would mean a painful rewrite of the ace interface code (which would probably hit the same difficulties that led to the creation of that fork in the first place). Seeing if more recent changes to ocp-indent can be backported to this branch would be nice though. EDIT: nevermind, I had ocp-indent-nlfork published on opam already, so that just needed a new version (https://github.com/ocaml/opam-repository/pull/27305) ; I'll guess we can wait for publication before merging this so that the pin-depends can be cleaned up.

[AltGr]

Rebased and removed the pin-depends ; note that ocp-indent-nlfork is still pending publication (https://github.com/ocaml/opam-repository/pull/27305) so CI is expected to fail until then

[erikmd]

Thanks a lot @AltGr for your feedback and for the rebase!

I propose to:

• merge your PR as soon as the CI is green (thanks for handing the point about the dependencies)
• check that the docker image for master builds successfully, then release 1.1.0 myself

Fine with you?

After the release, feel free to make the announcement on Discourse OCaml and [caml-list] if you wish (given you're the main contributor for 1.1.0 !) or let me know if you prefer that I look after this - there's an old template here.

So feature-wise, this release will be twofold:

• support OCaml 5.1 and Jsoo 5.8
• provide the CLI feature build serve --serve-during-build I had introduced (to complement your build serve --replace feature)

[erikmd]

Regarding the last failing CI job, it seems that the image ocamlpro/ocaml:5.1 would need a rebuild or so.

Other minor questions (follow-up of the upcoming merge of this PR):

• regarding the js_of_ocaml performance and your remark "Maybe we can try fiddling with the options and see if there is no unneeded debug flags left":
  would you be OK that we open a side issue, just to remember checking this later? (even if I'm not worried at all about this: I believe the current build time is pretty good);

• just to understand better the issue I workarounded in https://github.com/ocaml-sf/learn-ocaml/pull/602/commits/6f6be0f6c80fa27a002c0268d087fe0f37965f2f:
  do you have an idea why the final js_of_ocaml command includes a duplicate flag --no-source-map for almost each .js compilation? cf. (cd _build/default/src/app && /home/opam/.opam/5.1/bin/js_of_ocaml --no-source-map --no-source-map --opt=2 … in the dune log; although I don't see the flag duplicated in the dune file…

[erikmd]

Hi again, as the ocamlpro/ocaml image was rebuilt this afternoon, I restarted the CI which was successful! So, merging now. Still, I'll wait for you green-light (notably, if you have feedback w.r.t. my previous 2 comments) before I make the release.

[AltGr]

Thanks a lot!

would you be OK that we open a side issue, just to remember checking this later? (even if I'm not worried at all about this: I believe the current build time is pretty good);

Yes, let's keep track of this

do you have an idea why the final js_of_ocaml command includes a duplicate flag --no-source-map for almost each .js compilation?

Not sure, but my guess would be that dune could have changed its default flags ? Historically jsoo flags couldn't really be customised and I had to force out of the dev profile to disable source maps (that would blow up the compiler) ; from the doc sourcemaps are still disabled by default in release mode.