-
Notifications
You must be signed in to change notification settings - Fork 1.1k
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
Release v0.17 of Core and dependencies #25818
Conversation
Seems like many packages have errors due to the deprecated |
I see there are still a bunch of errors related to |
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.
Hello @dkalinichenko-js,
I wanted to mention to you in case this is useful information, as I was trying different combinations of core and ocaml versions I noticed that core.v0.17.0
doesn't build with ocaml.5.2.0
.
File "core/src/gc.ml", line 581, characters 6-8:
581 | | () ->
^^
Error: This pattern matches values of type unit/2
but a pattern was expected which matches values of type
Stdlib.Gc.Memprof.t
File "_none_", line 1:
Definition of type unit/2
Looks like it is due to an Experimental API change in ocaml/ocaml#12381 the function Stdlib.Gc.Memprof.start
now returns a t
rather than unit
.
As it stands, it seems you would need to constraint core.v0.17.0
to 5.1
only.
It's possible there's a quick fix that makes the pattern permit the type t
(perhaps a version dependent preprocess or something), but looking into the function measure_and_log_allocation_local
as a whole, it seems there might be a more subtle story with respect to multicore runtime. I'll defer to you on that.
I'm interested to know if you plan for having a core.v0.17.* that's compatible with 5.2
or, if you think it'll likely to await v0.18.*
.
Thanks!
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.
Hi! We definitely want v0.17 of our libraries to be available with OCaml 5.2. I'll look into the problem after finishing releasing all packages and will submit Core v0.17.1 if there's a fix.
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.
Great! Thanks for letting me know, and good luck with the release.
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.
Should we add an upper bound on ocaml5.2 to core v0.17?
This PR is huge. @dkalinichenko-js would it be possible to move the upper bounds on third party packages to a separate PR? I can merge that straight away, then we can add the bounds to core here and have an easier time with the review
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.
Done, see #25888. I also added an upper bound to Core.
Based on the previous CI run, I think all remaining failures are unrelated to this release.
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.
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.
Done, though one commit adding version bounds still got left out of the previous 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.
Thanks for spotting it!
Besides the CI issues, there are a few failures I would like to look at before merging. I’m traveling and have bad internet these days, but I should be able to do it on Thursday |
Is this failure in qucheck-core 0.20--0.21.3 related to the new release?
Is this also related to this release?
|
The rest seems fine (or unrelated and fixed in a separate PR that I am submitting now) |
As far as I can tell, the first failure you linked is caused by |
|
The first part yes, it was this my doubt:
|
Just checked, |
Thanks for checking! |
And for the release |
This PR contains the latest release of Jane Street's Core and necessary dependencies.