Skip to content

Conversation

andersfugmann
Copy link
Contributor

Plugin for protoc protobuf compiler to generate ocaml definitions from a .proto file

CHANGES:
  • Fix potential nameclash for messages defining extensions
  • Resolve compilation warning on deprecated fields enclosed in a oneof
  • Improve how comments are copied to generated code
  • Add flag 'singleton_oneof_as_option' to map single field
    onofs to option type (default on). Set to 'false' to keep old
    behaviour.
  • Improve copying of comments from .proto files into ocaml code using
    omd to parse markdown
  • Fix problem with deserializing large singed integers (OCaml Protoc Plugin incorretly parses Int64.min_int in sint64 field andersfugmann/ocaml-protoc-plugin#45)

CHANGES:

- Fix potential nameclash for messages defining extensions
- Resolve compilation warning on deprecated fields enclosed in a oneof
- Improve how comments are copied to generated code
- Add flag 'singleton\_oneof\_as\_option' to map single field
  onofs to option type (default on). Set to 'false' to keep old
  behaviour.
- Improve copying of comments from .proto files into ocaml code using
  omd to parse markdown
- Fix problem with deserializing large singed integers (andersfugmann/ocaml-protoc-plugin#45)
@andersfugmann
Copy link
Contributor Author

The tests are only expected to run on Linux/Mac platforms where the protoc command can be found in path. Is there a way to instruct CI not to run tests on windows?

@jmid
Copy link
Contributor

jmid commented Aug 28, 2025

Thanks!

I'm puzzled by these new conf-protoc* package versions.

  • First off, they are specifying src and url without using the given src tar-ball.
  • Secondly, they don't seem to add any features to the current latest versions (so why publish new ones? 🤔). Rather they miss some of the updates that have since been made to the existing ones on the repo: 😬

opam-repository$ git grlog packages/conf-protoc/conf-protoc.4.4.0/opam

  • 7f0b734 - (4 months ago) Switch from os-distribution = "fedora" to os-family = "fedora"
  • 4d05149 - (1 year, 6 months ago) Skip ci on oraclelinux-9 for conf-protoc since the package is not available by default
  • 348a169 - (2 years, 1 month ago) Add support for OpenSUSE Tumbleweed
  • e111c6f - (2 years, 6 months ago) Packages conf-protoc.4.4.0 and ocaml-protoc-plugin.4.4.0

opam-repository$ git grlog packages/conf-protoc-dev/conf-protoc-dev.1.0.0/opam

  • 7f0b734 - (4 months ago) Switch from os-distribution = "fedora" to os-family = "fedora"
  • 1421b09 - (1 year, 4 months ago) Remove source reference from conf-protoc-dev and reset to version 1.0.0

Finally, you can disable a package on Windows with available: os != "win32".
If it is just runtest that needs disabling, I believe it can be guarded with { os != "win32 }.

@andersfugmann
Copy link
Contributor Author

My bad. I did not want to publish new versions of the ‘conf-protoc’ package. Ill remove these and inhibit tests on windows.

@andersfugmann
Copy link
Contributor Author

andersfugmann commented Aug 29, 2025

Hmm. Adding & os != "win32" to the test command in opam file did not seem to do the trick.

@jmid
Copy link
Contributor

jmid commented Aug 29, 2025

Hmm. Adding & os != "win32" to the test command in opam file did not seem to do the trick.

The failure comes when attempting to run protoc under the conf-protoc package installation. Currently it doesn't have a depext and hence fails executing protoc (after having installed no external package):

 #=== ERROR while compiling conf-protoc.4.4.0 ==================================#
  "protoc": command not found.

This is thus to be expected.

If we want to extend conf-protoc (probably in a separate PR) I can see Cygwin packages here, but no MinGW packages for protobuf: https://cygwin.com/packages/package_list.html
MSys2 on the other hand has a (64-bit) MinGW package: https://packages.msys2.org/packages/mingw-w64-x86_64-protobuf

As for this PR, I can see lower bounds failures for the omd dependency, which would be good to address 🙂

Edit - I forgot: Thanks for adding x-maintenance-intent 🙏

@andersfugmann
Copy link
Contributor Author

I've addressed the lower bound for omd. I'll see if I can find a way to test the conf-protoc packages on windows later, but I dont think it should block merging of the packages here. wdyt?

@jmid
Copy link
Contributor

jmid commented Sep 1, 2025

I agree, conf-protoc's Windows failure shouldn't block this one.

CI summary:

  • When running --with-test (and exploring lower bounds) this fails on alpine-3.21-ocaml-5.3, archlinux-ocaml-5.3, opensuse-15.6-ocaml-5.3, opensuse-tumbleweed-ocaml-5.3 with #error "C++ versions less than C++14 are not supported." and #error "C++ versions less than C++17 are not supported."
  • uunf.16.0.0 fails to build on 4.14 under flambda, which is a known issue 4.14.2+flambda: stack overflow while compiling uunf 16.0.0 ocaml#14166
  • two FreeBSD workflow time out
  • Windows fails installing conf-protoc as it isn't currently supported

I'm unsure whether anything should be done about the former, failing C++ compiler version check during --with-test - and what is causing it. Older Debian/Ubuntu is using older gcc's, yet pass the test and so does Fedora-42 which IIUC has moved to gcc-15 🤔

@andersfugmann
Copy link
Contributor Author

The errors come from compiling protobuf test stubs, related to json tests.

# /usr/include/absl/base/policy_checks.h:79:2: error: #error "C++ versions less than C++14 are not supported."
2025-08-30 22:50.31: >>> 
# /usr/include/google/protobuf/port_def.inc:210:15: error: static assertion failed: Protobuf only supports C++14 and newer.

Why the package systems does not ensure availability of a usable compiler is beyond me, and I dont think we should fix this. We can add additional x-ci-accept-failures (that already exclude homebrew, because it also does not provide C++14.

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