Add an upper ppxlib bound

[pitag-ha]

Hi there,

Thanks for writing this nice PPXs, it looks very interesting!

I'm following up on https://discuss.ocaml.org/t/why-is-building-ocaml-projects-still-so-hard/11812: The PPX is incompatible with ppxlib's AST bump to OCaml 4.14/5.0 in 0.26.0. A better solution than mine here would be to make it compatible and have a >= 0.26.0 constraint instead.

[davesnx]

That's a great addition @pitag-ha while we could add support for 4.14/5.

[jchavarri]

Thanks for the patch and explanation @pitag-ha !

A better solution than mine here would be to make it compatible and have a >= 0.26.0 constraint instead.

If we did so, don't we risk future issues like the one that triggered this PR? Is there a way for us to we keep the ppxlib dependency constraint "broad" enough to play nice with other ppxs that users might install, while at the same time not risking a future breakage if ppxlib happens to change things in some version >= 0.26.0?

[pitag-ha]

Is there a way for us to we keep the ppxlib dependency constraint "broad" enough to play nice with other ppxs that users might install, while at the same time not risking a future breakage if ppxlib happens to change things in some version >= 0.26.0?

Yes, you're right, that would be best! I've just had a look at your code to see where you use Pext_decl and can't find it. Do you use it at all? If not, the problem is different than I thought.

[davesnx]

Wops, I got it wrong as well. Re-reading the post on the forum seems like the problem with ppxlib bound fails under https://github.com/little-arhat/ppx_jsobject_conv/ since they use Pext_decl not under jsoo-react (that it supports both 4.14 and 5.0) since it was added support for jsoo 5.1 here: https://github.com/ml-in-barcelona/jsoo-react/pull/177/files

[pitag-ha]

Oh, I've just had another look at the discuss conversation and the problem seems to come from ppx_jsobject_conv. ppx_jsobject_conv was patched correctly! This is strange. I'll look into it next week. Do you use ppx_jsobject_conv?

[pitag-ha]

Hadn't seen your answer when I wrote mine, @davesnx. Do I understand correctly that you've already found out what the problem is?

[jchavarri]

I think the problem was that the example repo was pinning ppx_jsobject_conv to an older version (that included a fix I had contributed to that repo, but the maintainer had not had the time to publish in opam at the time):

https://github.com/jchavarri/jsoo-react-realworld-example-app/blob/main/jsoo-react-realworld-example.opam#L39

And that version of ppx_jsobject_conv didn't have the upper constraint yet:

https://github.com/little-arhat/ppx_jsobject_conv/blob/b1c43be251d8c7d39ee32ddc9968fdc3d0042c0b/ppx_jsobject_conv.opam#L20

[jchavarri]

~So I understand another way to fix the problem from the point of view of the user that complained would have been to update the example to use an unpinned version of ppx_jsobject_conv, which would have included the upper constraint that had already been added in ppx_jsobject_conv.~ Edit: no, this would still break because of jsoo-react.

[davesnx]

The problem is in the example repo: https://github.com/jchavarri/jsoo-react-realworld-example-app/blob/main/jsoo-react-realworld-example.opam that depends on ppx_jsobject_conv which has ppxlib >= 26 and uses Pext_decl which it's what breaks in newer versions of ppxlib (from 0.22 to 0.26).

[jchavarri]

that depends on ppx_jsobject_conv which has ppxlib >= 26

ppx_jsobject_conv in its latest version constraints ppxlib >= 26, but the version used by the example repo was an old one. That's the reason why the error happened when building ppx_jsobject_conv, rather than jsoo-react.

[pitag-ha]

Thanks for looking into this and explaining the situation I have a couple of different questions/thoughts following up.

Is it still necessary for you in the example repo to pin ppx_jsobject_conv? From what I understand, the change you depend on is now released in ppx_jsobject_conv. Is that right?

The case of a pinned PPX is indeed a problem in our current PPX compatibility workflow. Thanks for pointing it out! I'll see if there's something we can do to trigger a better error message in case of failure due to a pinned PPX.

And about the upper ppxlib bound: So, clearly we don't want a ppxlib < "0.26.0" bound here. It still might be a good idea to add a < "0.30.0" bound, though (the current last ppxlib version is 0.29). I have a question to figure out if we want that or not: If a ppxlib release in the future breaks jsoo-react, will your CI notice that, and will you react to that quickly - either adding the new upper ppxlib bound or patching the breakage? If not, I'd probably add the bound.

[jchavarri]

Thanks for following up on this @pitag-ha !

Is it still necessary for you in the example repo to pin ppx_jsobject_conv? From what I understand, the change you depend on is now released in ppx_jsobject_conv. Is that right?

You are right, it would not be necessary to pin anymore, that was just a temporary workaround while I was prototyping / experimenting. I think doing what you suggest —changing that pin to the released version— would have solved the original issue. But ultimately, I decided to archive that example repo because time is limited and I had no plans to maintain it after I finished the experiment that it was created for.

I'll see if there's something we can do to trigger a better error message in case of failure due to a pinned PPX.

This would be really cool. 💙

I have a question to figure out if we want that or not: If a ppxlib release in the future breaks jsoo-react, will your CI notice that, and will you react to that quickly - either adding the new upper ppxlib bound or patching the breakage? If not, I'd probably add the bound.

I guess if the repo goes through some changes —so that CI runs— it would show the breakage. But unfortunately we don't run scheduled builds, so unless someone is changing the repository, we won't notice it, and if there are no updates for a while and the breaking change happens, the error could explode on some project consuming jsoo-react downstream.

As a side note, one can see how CI has picked up the latest version of ppxlib 0.29.1 lately: https://github.com/ml-in-barcelona/jsoo-react/actions/runs/4553001260/jobs/8029036344#step:5:51

---

My conclusion from the OCaml forum thread and this one is that we should not maintain an OCaml library that relies on ppxlib without publishing it in the opam repository. For this reason, I am inclined to either:

• go ahead and publish jsoo-react in the opam repository
• or rather archive the library.

I feel inclined for the latter, as I don't use jsoo-react at the moment and my focus is elsewhere. @davesnx what do you think?

[davesnx]

I'm happy to push it to opam, even under 0.0.1. It's better to keep things working.

[jchavarri]

After discussing with @davesnx, we are publishing a version of jsoo-react in opam: https://github.com/ocaml/opam-repository/pull/23646

I think this will ease maintenance for everyone involved. @pitag-ha Do you think we would still need to add an upper bound on ppxlib after the package is published in the opam repository?

[pitag-ha]

After discussing with @davesnx, we are publishing a version of jsoo-react in opam: https://github.com/ocaml/opam-repository/pull/23646

Cool!

@pitag-ha Do you think we would still need to add an upper bound on ppxlib after the package is published in the opam repository?

No, if jsoo-react is on opam repository, it's better not to add the upper ppxlib bound I was mentioning. If we ever break jsoo-react, we'll notice and only then add the upper bound.

Thanks for taking care also of this potential problem even though the real problem was somewhere else!

[jchavarri]

Cool, I'm gonna close this PR then, as there's no need to set that upper constraint anymore. Thanks again for your help while we figured this out!