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

Update to OCaml 4.14 #1366

Merged
merged 5 commits into from
Oct 23, 2023
Merged

Update to OCaml 4.14 #1366

merged 5 commits into from
Oct 23, 2023

Conversation

WardBrian
Copy link
Member

This updates to OCaml 4.14, the last update of the 4.x release line.
It is source-compatible with OCaml 5.0 if someone wishes to compile with that instead.

It also updates the Jane Street libraries to v0.16, which included migrating from Core_kernel to Core.

Finally, it also bumped some other dependencies:

  • fmt: 0.8.8 -> 0.9.0
  • menhir: 20210929 -> 20230608
  • yojson: 1.7.0 -> 2.1.0
  • ocamlformat: 0.19.0 -> 0.26.1

This final change required some re-formatting, but with proper configuration it was not too bad.

Existing developers should install and use a new opam switch after this is merged: cd scripts/; ./setup_dev_env.sh

The required updates for building on Windows were done in: ocaml-cross/opam-cross-windows#277 ocaml-cross/opam-cross-windows#278 ocaml-cross/opam-cross-windows#279 ocaml-cross/opam-cross-windows#280 ocaml-cross/opam-cross-windows#286

@serban-nicusor-toptal: this will need new versions of all of our docker images, and a new opam environment on the Jenkins mac. Let me know if you need any help!

Submission Checklist

  • Run unit tests
  • Documentation
    • If a user-facing facing change was made, the documentation PR is here:
    • OR, no user-facing changes were made

Release notes

Updated the compiler to use OCaml 4.14 and newer versions of its dependencies.

Copyright and Licensing

By submitting this pull request, the copyright holder is agreeing to
license the submitted work under the BSD 3-clause license (https://opensource.org/licenses/BSD-3-Clause)

@serban-nicusor-toptal
Copy link
Contributor

Hey Brian, find the docker images at:

  • stanorg/stanc3:debian-ocaml-4.14
  • stanorg/stanc3:debian-windows-ocaml-4.14
  • stanorg/stanc3:static-ocaml-4.14

For the multiarch image, one of the deps seems to not be compatible with arm32 & 32bit

 > [linux/arm/v7 12/13] RUN curl https://raw.githubusercontent.com/stan-dev/stanc3/update/4.14/scripts/install_build_deps.sh | bash:
100   217  100   217    0     0    837      0 --:--:-- --:--:-- --:--:--   951
#75 1.167 [WARNING] Running as root is not recommended
#75 1.431 [WARNING] Running as root is not recommended
#75 3.667 core is now pinned to version v0.16.1
#75 3.800 [WARNING] Running as root is not recommended
#75 11.46 [ERROR] core = v0.16.1 unmet availability conditions: arch != "arm32" & arch != "x86_32"

Our target platforms for multi arch build are: --platform linux/arm/v6,linux/arm/v7,linux/arm64,linux/ppc64le,linux/mips64le,linux/s390x
You can find the ci-scripts changes here: stan-dev/ci-scripts#29

Is there a way we can fix that or do we ditch arm support ?

@WardBrian
Copy link
Member Author

Can you try core 0.16.0 instead of 0.16.1? It doesn’t look like it has the same restriction in the metadata, but that doesn’t mean it will build necessarily.

@WardBrian
Copy link
Member Author

See: ocaml/opam-repository#23918

0.16.1 had this restriction added back, but it should be fine if those platforms use a different minor version

@serban-nicusor-toptal
Copy link
Contributor

Nice! I'll rebuild, it might take a while ...

@serban-nicusor-toptal
Copy link
Contributor

Looks like I got timeout for downloading so many packages haha, I'll try again in the morning.
Do we update core v0.16.0 in the stanc3 PR too or leave this image with core v0.16.0 and the others core v0.16.1 ?

@WardBrian
Copy link
Member Author

I think the main Linux image and the developers should still be 16.1

@serban-nicusor-toptal
Copy link
Contributor

serban-nicusor-toptal commented Oct 5, 2023

Hmmh, looks like it might not be in the repositories v0.16.0 ?
I'm running into:

 > [linux/mips64le 13/19] RUN eval $(opam env) && opam install -y core.v0.16.0:
#81 0.469 [WARNING] Running as root is not recommended
#81 0.564 [WARNING] Running as root is not recommended
#81 194.8 Sorry, no solution found: there seems to be a problem with your request.
#81 194.8
#81 194.8 No solution found, exiting
------

Initially I thought it's a timeout but looks like it it's a missing package in the repo I think.
Edit: I found it in the packages here https://opam.ocaml.org/packages/core/core.v0.16.0/
Edit: I think it might have something to do with the platform mips64le ? I've tried only ARM build and that worked fine.

@WardBrian
Copy link
Member Author

Two things: was there an opam update before that, and is that colon at the end part of the command or just part of the display? (Hopefully the latter)

@serban-nicusor-toptal
Copy link
Contributor

serban-nicusor-toptal commented Oct 5, 2023

I've already tried with RUN eval $(opam env) && opam update && opam upgrade before the install package command.
The colon is just a printing artifact, not in the command.
If it matters, I managed to build all arch EXCEPT linux/mips64le which fails with the above.
Edit: Pushed here stanorg/stanc3:multiarch-ocaml-4.14

@WardBrian
Copy link
Member Author

While I'm debugging the mips64le build I decided to check, according to https://tooomm.github.io/github-release-stats/?username=stan-dev&repository=cmdstan, that architecture has only been downloaded 45 times total (across all versions) since we started offering it in 2.28

(for the record, the version we build for google collab has been downloaded 44 times for just 2.33.1, and the base cmdstan 2.33.1 ~15 thousand times)

If we need to disable builds on that platform for a little while I think that is OK

@WardBrian
Copy link
Member Author

@andrjohns - you originally added the different arch builds for stanc. Was there a specific reason for the selection of archs? mips64el and s390x both seem relatively obscure

@codecov
Copy link

codecov bot commented Oct 5, 2023

Codecov Report

Merging #1366 (6b62412) into master (4b8753a) will increase coverage by 0.48%.
Report is 4 commits behind head on master.
The diff coverage is 87.70%.

❗ Current head 6b62412 differs from pull request most recent head 5e3f6c7. Consider uploading reports for the commit 5e3f6c7 to get more accurate results

@@            Coverage Diff             @@
##           master    #1366      +/-   ##
==========================================
+ Coverage   89.45%   89.93%   +0.48%     
==========================================
  Files          65       63       -2     
  Lines       10774    10704      -70     
==========================================
- Hits         9638     9627      -11     
+ Misses       1136     1077      -59     
Files Coverage Δ
src/analysis_and_optimization/Dataflow_types.ml 0.00% <ø> (-8.70%) ⬇️
src/analysis_and_optimization/Dataflow_utils.ml 100.00% <100.00%> (ø)
...analysis_and_optimization/Debug_data_generation.ml 81.70% <100.00%> (-0.08%) ⬇️
...c/analysis_and_optimization/Dependence_analysis.ml 100.00% <100.00%> (ø)
src/common/FatalError.ml 0.00% <ø> (-66.67%) ⬇️
src/common/Fixed.ml 21.73% <ø> (-6.27%) ⬇️
src/common/Foldable.ml 11.76% <ø> (-9.29%) ⬇️
src/common/Gensym.ml 100.00% <100.00%> (ø)
src/common/Specialized.ml 75.00% <ø> (+8.33%) ⬆️
src/frontend/Ast.ml 68.69% <100.00%> (+11.61%) ⬆️
... and 50 more

... and 5 files with indirect coverage changes

@andrjohns
Copy link
Contributor

@andrjohns - you originally added the different arch builds for stanc. Was there a specific reason for the selection of archs? mips64el and s390x both seem relatively obscure

Not particularly, it was just all archs supported by Debian. I figured that would cover the majority of use-cases

@WardBrian
Copy link
Member Author

This is now building with mips64el disabled. @serban-nicusor-toptal is there a good way to test build the other binaries we normally only build on master?

@serban-nicusor-toptal
Copy link
Contributor

One fast way would be to comment out the test stages, the whens for each binary build and run the pipeline. ( this can also be done in the Jenkins UI after hitting Replay on a pipeline without the need to commit here ).
I can trigger it after this one finishes just so we test the binary builds.

@WardBrian
Copy link
Member Author

Everything built fine, thanks @serban-nicusor-toptal for the tip

@WardBrian
Copy link
Member Author

Thanks to some help from some Opam maintainers we determined that the issue with the mips64el architecture comes from the num library. The fix has already been merged into num, so it's just a matter of re-enabling after it gets released.

Copy link
Contributor

@SteveBronder SteveBronder left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The 'mips64el' port is a 64-bit little endian port using the N64 ABI, hardware floating point and the MIPS64R2 ISA

I'm not sure the math library is compatible with the nintendo 64 so I think we can remove this jenkins path (unless??)

@WardBrian
Copy link
Member Author

We actually just fixed mips64el! I just have to uncomment the Jenkins and test to make sure, but it should be the same as before now

@WardBrian WardBrian merged commit b609679 into master Oct 23, 2023
1 check passed
@WardBrian
Copy link
Member Author

Existing developers should install and use a new opam switch after this merge: git pull; cd scripts/; ./setup_dev_env.sh

Tagging those that come to mind: @SteveBronder @rok-cesnovar @andrjohns @spinkney

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants