-
Notifications
You must be signed in to change notification settings - Fork 1.2k
Release ppxlib and ppxlib-tools 0.36.2 #28610
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
Conversation
There's this error in
I'm not quite sure where that comes from, it seems dune's expanding ppxlib's version to the empty string somehow. |
I added upper bounds on the |
d405516
to
98ee139
Compare
Looking at the CI it seems the test were broken on ppx_factory and ppx_deriving_jsonschema before this release! |
98ee139
to
307bd6a
Compare
The failure on ppx_show is unrelated to this release! |
ping @ocaml/opam-repository! I'd like to get this in before a final release of the 5.4 compatible 0.37.0 release! I don't think any of the remaining errors are tied to these changes but I might have missed something! |
are there any reasons why this is still a draft PR? |
Because I forgot to undraft it... My bad |
Can you check that the generated META file that is installed mentions the version? |
307bd6a
to
a4bb466
Compare
Indeed, when I run Should we explicitly set the version in the dune-project when releasing? That's probably something we should fix in dune-release somehow as manually setting it isn't ideal. |
Indeed that seems to fix the META files! It does result in adding the version field to the opam files as well which I thought was discouraged now, am I correct? |
CHANGES: - Make Ast_builder's default `value_binding` constructor generate the proper `pvb_constraint` from the pattern and expression arguments. (ocaml-ppx/ppxlib#589, @NathanReb) - Fix pprintast to output correct syntax from `Ppat_constraint (pat, Ptyp_poly ...)` nodes until they are completely dropped. (ocaml-ppx/ppxlib#588, @NathanReb)
a4bb466
to
b75aaf2
Compare
dune-release does strip the version field from the opam files for the release so that's all good. |
|
Nice to see the version issue is fixed! |
Hi there, after this upgrade building package fstar on OCaml 4.14.2 fails with:
See here. It seems like 4.14.2 does not have this syntax for the mod operator. Should the OCaml version constraint here be tighter? (I'm not familiar with this syntax, not sure when it was introduced.) |
I'll take a look and see if I can reproduce this and track down the cause but from the look of it, I'm not sure that strictly ppxlib's doing. I need to know a bit more about fstar first! |
@smorimoto to circle back on those errors, The other two packages though seem to have had those test failures before ppxlib.0.36.2, I assume their test suite have been unmaintained for a little while! I can take a look and send PRs upstream if that helps with opam's revdeps CI in the future. |
@NathanReb fstar.2025.08.07 and fstar.2025.09.04 fails on ocaml.4.14.2, ocaml.5.0.0, ocaml.5.1.1 after the new release of ppxlib |
F*'s build is a bit complicated, but I think what's happening here is that we used to print |
Can you open an issue on ppxlib please? That'll give us a more suited discussion channel for this! |
Should we add a conflict at least to fstar here on the repo in the meantime? |
@mtzguido wrote:
This |
When was this change introduced in ppxlib? |
We didn't properly update our copy of Note that this should not impact regular ppx users as we don't use the driver's source output but instead marshall out the AST. From a quick look it seems fstar uses ppxlib's library for some Ocaml code generation which is leading to this. One possible fix for this in fstar would be to pretty print using the compiler's pprintast, i.e. migrate the AST to the installed compiler version and use its Pprintast to make sure the syntax is compatible. Note that this bit you with the identifier compat feature but it could also for other syntax changes. |
This is the raw literal notation added in 5.2 to make it possible to use an old library using a new keyword (aka |
In the meantime, in ppxlib, we could prevent this syntax from being used pre 5.2 which would fix the issue for fstar. |
I've made a patch here: NathanReb/ppxlib@230ffe1 just in case but I'm not quite sold on what's the best approach from ppxlib's perspective here. |
See also ocaml/ocaml#14279 for the pretty-printer patch that makes sure to print |
As per usual, starting with a draft to see how revdeps look like.
This should be an improvement over the failed attempt at releasing 0.36.1 as users are less susceptible of producing nodes that will be incorrectly printed.
It is still possible to manually construct them but
Ast_builder
users should not have that problem anymore.Changes
value_binding
constructor generate the properpvb_constraint
from the pattern and expression arguments.(Fix Ast_builder's
value_binding
to generate the correctpvb_constraint
ocaml-ppx/ppxlib#589, @NathanReb)Ppat_constraint (pat, Ptyp_poly ...)
nodes until they are completely dropped. (Fix pprintast with ppat_constraint in let bindings ocaml-ppx/ppxlib#588, @NathanReb)