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

Package ppx_deriving_decoders.0.1 #26742

Merged

Conversation

benbellick
Copy link
Contributor

ppx_deriving_decoders.0.1

Deriving Decoders using PPX
Using mattjbray/ocaml-decoders, use a ppx to automatically generate instances of a decoder for a particular type using PPX.



🐫 Pull-request generated by opam-publish v2.4.0

@benbellick benbellick force-pushed the opam-publish-ppx_deriving_decoders.0.1 branch 8 times, most recently from 8b54c0e to 88cc1e9 Compare October 14, 2024 23:26
@shonfeder
Copy link
Collaborator

Hello 👋 Thank you for publishing your package!

Since it looks like this is your first time publishing a package to opam, allow me offer a warm welcome and a bit of an intro :)

We run a CI system to ensure that packages can be installed on all intended systems. You can see the results for your PR at https://opam.ci.ocaml.org/github/ocaml/opam-repository/commit/88cc1e996b0437ff2d34fd37af31645a80408a79 (or by clicking the Details link next to the opam-ci status check).

This shows the results of the builds and tests across our build matrix. Because we build and test on such a large matrix, it is very common for errors to be identified during package publication.

In my following comments and suggestions, I'll try to provide pointers and suggestions to get everything squared away. Please let us know if you have any questions, as we are here to help :)

Copy link
Collaborator

@shonfeder shonfeder left a comment

Choose a reason for hiding this comment

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

Aside from the lower bound error noted below, I see we are also seeing a failure to build decoders on arm32 (https://opam.ci.ocaml.org/github/ocaml/opam-repository/commit/88cc1e996b0437ff2d34fd37af31645a80408a79/variant/extras,arm32-ocaml-5.2,ppx_deriving_decoders.0.1) and x86_32 . In both cases it looks like the following:

#=== ERROR while compiling decoders.1.0.0 =====================================#
# context              2.4.0~alpha1~dev | linux/arm32 | ocaml-base-compiler.5.2.0 | file:///home/opam/opam-repository
# path                 ~/.opam/5.2/.opam-switch/build/decoders.1.0.0
# command              ~/.opam/opam-init/hooks/sandbox.sh build dune build -p decoders -j 15 @install
# exit-code            1
# env-file             ~/.opam/log/decoders-7-b537cc.env
# output-file          ~/.opam/log/decoders-7-b537cc.out
### output ###
# File "src/dune", lines 6-11, characters 0-105:
#  6 | (rule
#  7 |  (targets shims_let_ops_.ml)
#  8 |  (action
#  9 |   (with-stdout-to
# 10 |    %{targets}
# 11 |    (run ./gen/mkshims.exe))))
# Error: No rule found for src/gen/mkshims.exe

This is a problem with the docoder dependency rather than this package. I've reported it upstream in mattjbray/ocaml-decoders#75

depends: [
"ocaml" {>= "4.08.0"}
"dune" {>= "3.11"}
"ppxlib"
Copy link
Collaborator

Choose a reason for hiding this comment

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

We are seeing a syntax error on https://opam.ci.ocaml.org/github/ocaml/opam-repository/commit/88cc1e996b0437ff2d34fd37af31645a80408a79/variant/compilers,4.08,ppx_deriving_decoders.0.1,lower-bounds

-> installed ppxlib.0.7.0
[ERROR] The compilation of ppx_deriving_decoders.0.1 failed at "dune build -p ppx_deriving_decoders -j 39 @install".

#=== ERROR while compiling ppx_deriving_decoders.0.1 ==========================#
# context              2.4.0~alpha1~dev | linux/x86_64 | ocaml-base-compiler.4.08.1 | pinned(https://github.com/benbellick/ppx_deriving_decoders/archive/refs/tags/0.1.tar.gz)
# path                 ~/.opam/4.08/.opam-switch/build/ppx_deriving_decoders.0.1
# command              ~/.opam/opam-init/hooks/sandbox.sh build dune build -p ppx_deriving_decoders -j 39 @install
# exit-code            1
# env-file             ~/.opam/log/ppx_deriving_decoders-7-6cd26d.env
# output-file          ~/.opam/log/ppx_deriving_decoders-7-6cd26d.out
### output ###
# (cd _build/.sandbox/3b8b8817e830e46625b42d49541b2972/default && .ppx/0224ad3443a846e54f1637fccb074e7d/ppx.exe --cookie 'library-name="ppx_deriving_decoders"' -o src/expander.pp.ml --impl src/expander.ml -corrected-suffix .ppx-corrected -diff-cmd - -dump-ast)
# File "src/expander.ml", line 251, characters 13-14:
# 251 |           let* [%p var_pat] = field [%e str] [%e subexpr] in
#                    ^
# Error: Syntax error

The error is pointing to https://github.com/benbellick/ppx_deriving_decoders/blob/bed675eaf4a0fa73cf8a2fd5eab27343bd3a596a/src/expander.ml#L251 and teh test is a lower bounds test, wherein we check that packages can build with the lower bounds of the dependencies they declare.

Since ocaml 4.08 supported let-binding operators already, I'm wondering if we may need to increase the lower bound on ppxlib here to, e.g.,

Suggested change
"ppxlib"
"ppxlib" {>= "0.20.0"}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah addressing this one, but amazing! Thanks for the help 😄

@benbellick benbellick force-pushed the opam-publish-ppx_deriving_decoders.0.1 branch from 88cc1e9 to 861ff8f Compare October 17, 2024 13:21
@shonfeder
Copy link
Collaborator

Great! The lower bounds checks are passing and only the decoder issue persists, and we’ll tackle that up stream. Thanks very much for authoring and sharing this useful package :)

Consider announcing it on discuss.ocaml.org :)

@shonfeder shonfeder merged commit c761ea8 into ocaml:master Oct 17, 2024
2 of 3 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants