Skip to content

Conversation

@c-cube
Copy link
Contributor

@c-cube c-cube commented Nov 16, 2025

Pure OCaml compiler for .proto files

CHANGES:
  • better parsing of protobuf options
  • generation of validation code with --pp_options
  • feat: add --encode-only and --decode-only features
  • take presence semantics in account, to be closer to compliance
    with the official protobuf docs and implementation
    • use a bitfield to track presence for scalar fields
    • use options in more places
    • do not serialize non-present fields, to save space
    • always generate make builders, setters, presence checks
  • merge mutable and immutable types, only generate a single record per
    message type
  • feat codegen: use polyvariants for very large sum types
  • fix parser: handle reserved in enums
  • improved tests

Copy link
Member

@jmid jmid left a comment

Choose a reason for hiding this comment

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

Thanks!

There is a problem with runtest on ocaml-protoc.4.0:

#=== ERROR while compiling ocaml-protoc.4.0 ===================================#
# context              2.5.0~alpha1 | linux/x86_64 | ocaml-base-compiler.5.4.0 | pinned(https://github.com/mransan/ocaml-protoc/releases/download/v4.0/ocaml-protoc-4.0.tbz)
# path                 ~/.opam/5.4/.opam-switch/build/ocaml-protoc.4.0
# command              ~/.opam/opam-init/hooks/sandbox.sh build dune build -p ocaml-protoc -j 71 @install @runtest
# exit-code            1
# env-file             ~/.opam/log/ocaml-protoc-7-e22d4d.env
# output-file          ~/.opam/log/ocaml-protoc-7-e22d4d.out
### output ###
# Error: No rule found for src/tests/yojson/yojson.data
# -> required by alias src/tests/yojson/runtest-yojson_unittest_ml in
#    src/tests/yojson/dune:2
# -> required by alias src/tests/yojson/runtest
# (cd _build/default/src/tests/unit-tests && ./parse_field_options.exe)
# Parse Field Options ... Ok
# (cd _build/default/src/tests/unit-tests && ./verify_syntax_invariants.exe)
# Verify syntax invariants ... Ok
# (cd _build/default/src/tests/unit-tests && ./parse_enum.exe)
# Parse Enum ... Ok
# (cd _build/default/src/tests/unit-tests && ./graph_test.exe)
# Graph Test ... Ok
# (cd _build/default/src/tests/unit-tests && ./parse_message.exe)
# Parse Message ... Ok
# Parse Message ... Ok
# (cd _build/default/src/tests/unit-tests && ./parse_file_options.exe)
# Parse File Options ... Ok
# (cd _build/default/src/tests/unit-tests && ./pbtt_compile_p1.exe)
# Pbtt Compile P1 ... Ok
# (cd _build/default/src/tests/unit-tests && ./parse_import.exe)
# Parse Import ... Ok
# (cd _build/default/src/tests/unit-tests && ./pbtt_compile_p2.exe)
# Pbtt Compile P2 ... Ok
# (cd _build/default/src/tests/google_unittest && ./google_unittest.exe)
# Google unittest .... OK

(it is not triggered on the Windows workflows since these don't runtest)

Finally, would you consider adding x-maintenance-intent entries?
https://github.com/ocaml/opam-repository/blob/master/governance/policies/archiving.md

@c-cube c-cube force-pushed the release-ocaml-protoc-v4.0 branch from a2f3d0f to 5f4d12f Compare November 17, 2025 01:11
@c-cube c-cube requested a review from jmid November 17, 2025 16:51
Copy link
Member

@jmid jmid left a comment

Choose a reason for hiding this comment

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

Looking good! 😃

There are a couple of opam-2.0 failures (which can be ignored).

There is some warnings from opam linters which I include proposals to fix:

Warning in pbrt_yojson.4.0: Dubious use of 'dune subst'. 'dune subst' should always only be called with {dev} (i.e. ["dune" "subst"] {dev}) If your opam file has been autogenerated by dune, you need to upgrade your dune-project to at least (lang dune 2.7).
Warning in pbrt_services.4.0: Dubious use of 'dune subst'. 'dune subst' should always only be called with {dev} (i.e. ["dune" "subst"] {dev}) If your opam file has been autogenerated by dune, you need to upgrade your dune-project to at least (lang dune 2.7).
Warning in pbrt.4.0: Dubious use of 'dune subst'. 'dune subst' should always only be called with {dev} (i.e. ["dune" "subst"] {dev}) If your opam file has been autogenerated by dune, you need to upgrade your dune-project to at least (lang dune 2.7).
Warning in ocaml-protoc.4.0: Dubious use of 'dune subst'. 'dune subst' should always only be called with {dev} (i.e. ["dune" "subst"] {dev}) If your opam file has been autogenerated by dune, you need to upgrade your dune-project to at least (lang dune 2.7).

Perhaps this would be an opportunity to upgrade (lang dune ...)?
https://github.com/mransan/ocaml-protoc/blob/v4.0/dune-project#L1

@c-cube
Copy link
Contributor Author

c-cube commented Nov 17, 2025

It's always the same linter :)

I don't like the idea of bumping a lower bound just because the linter is unhappy (especially because dune subst is not actually in use).

@jmid
Copy link
Member

jmid commented Nov 17, 2025

Fair enough! Perhaps just remove the dune subst lines, which will eliminate the problem?

@c-cube
Copy link
Contributor Author

c-cube commented Nov 17, 2025

they're autogenerated… why is this linter so important?

@c-cube c-cube requested a review from jmid November 17, 2025 19:50
Copy link
Member

@jmid jmid left a comment

Choose a reason for hiding this comment

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

LGTM, thanks!

Would you consider adding x-maintenance-intent entries for the packages?
https://github.com/ocaml/opam-repository/blob/master/governance/policies/archiving.md

@c-cube
Copy link
Contributor Author

c-cube commented Nov 17, 2025

I've never done that before, does it require an opam template when used in conjunction with dune?

@jmid
Copy link
Member

jmid commented Nov 17, 2025

For older dune versions it needs to be done through templating.
Example PR: ocaml-multicore/multicoretests#527

Since dune.3.18 it supports generating x-maintenance-intent entries for the opam files:
https://github.com/ocaml/dune/blob/main/CHANGES.md#3180-2025-04-03
Example PR: ocaml-multicore/multicoretests#570

@c-cube c-cube force-pushed the release-ocaml-protoc-v4.0 branch from c984680 to adc5db1 Compare November 19, 2025 19:31
@c-cube c-cube requested a review from jmid November 19, 2025 20:55
Copy link
Member

@jmid jmid left a comment

Choose a reason for hiding this comment

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

Thanks for adding x-maintenance-intent entries! 🙏

The linter spotted a small remaining dune subst nit. Otherwise, this is good to go! 👍

@c-cube c-cube force-pushed the release-ocaml-protoc-v4.0 branch from adc5db1 to de08b32 Compare November 20, 2025 02:29
@c-cube c-cube requested a review from jmid November 20, 2025 02:46
CHANGES:

- better parsing of protobuf options
- generation of validation code with `--pp_options`
- feat: add `--encode-only` and `--decode-only` features
- take presence semantics in account, to be closer to compliance
    with the official protobuf docs and implementation
  * use a bitfield to track presence for scalar fields
  * use options in more places
  * do not serialize non-present fields, to save space
  * always generate `make` builders, setters, presence checks
- merge mutable and immutable types, only generate a single record per
    message type
- feat codegen: use polyvariants for very large sum types
- fix parser: handle `reserved` in enums
- improved tests
@c-cube c-cube force-pushed the release-ocaml-protoc-v4.0 branch from de08b32 to e5fd09c Compare November 20, 2025 03:12
Copy link
Member

@jmid jmid left a comment

Choose a reason for hiding this comment

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

Thanks a bunch, LGTM!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants