-
Notifications
You must be signed in to change notification settings - Fork 33
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
Ocaml 5.3.0 #340
Ocaml 5.3.0 #340
Conversation
That looks amazing. I usually rebuild the CI images manually for this. Do you have a docker hub user? Happy to give you access. No patches? That's amazing. Have you checked the output of |
I'm
Well no patch in upcoming 5.4.0! In 5.3.0, we actually rely on the backports of the patches merged in the dev branch (hiding in Good point but yes 🎉
|
Wow nice. |
Invite sent! |
Gentle ping to ask if there's anything I can do to help get this merged |
Sorry didn't realize my review had not been submitted.. |
I'm not sure if this is relevant to this PR or something we'll need to patch elsewhere in the repository after it is merged, but I wanted to flag that something weird is happening with The |
@pirbo would you like any assistance getting this over the finish line? Thanks! |
Sorry I was offline last week and I'll still won't be available to tackle that today and tomorrow but I promise it's the 1st item on my todo list for wednesday. |
.github/workflows/ci.yml
Outdated
@@ -70,7 +72,7 @@ jobs: | |||
echo "Revdeps to build: ${REVDEPS_TO_BUILD}" | |||
echo "revdeps=[${REVDEPS_TO_BUILD}]" >> "${GITHUB_OUTPUT}" | |||
- name: Write matrix output | |||
if: matrix.arch == 'x64' && matrix.ocaml_version == '5.1.1' | |||
if: matrix.arch == 'x64' && matrix.ocaml_version == '5.3.0' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should remove arch x86 now. This can be done after this PR is merged.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Right. I can try that in an other PR
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actually, partly done here in the CI file (I didn't renamed the docker image)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks good to me now.
It's building fine?
We should rebuild the CI images before merging.
I've pushed new base docker images. |
Thank you both! |
Awesome work! |
Implements #339