mransan / ocaml-protoc

A Protobuf Compiler for OCaml
https://mransan.github.io/ocaml-protoc/
MIT License
179 stars 33 forks source link

Proposal for automated generation of dune diff rules in examples/ #231

Closed mbarbin closed 10 months ago

mbarbin commented 10 months ago

Hello team,

I'd like to propose a change related to the diff rules used under the examples/ directory, which were recently added in #230.

The idea is to automate the generation of these diff rules using a helper program. The generated rules would be written to the dune.inc file, which is then included in the main dune file. If there are any changes, running dune promote would update the dune.inc file accordingly.

I understand that this introduces a bit more complexity into the dune files, and it might be a matter of personal preference. Therefore, I thought it would be best to propose this change as a separate PR, so you can decide whether the benefits outweigh the added complexity.

In the future, this setup could potentially be replaced by generic dune rules, once they are supported. Until then, using gen-dune type helper programs is not an uncommon workaround that I've found to be quite effective.

I look forward to hearing your thoughts on this proposal.

mbarbin commented 10 months ago

From my understanding, the (include {file}) construct in dune requires the file to be present in the tree. It doesn't seem to work with files dynamically generated by a rule. I don't think a file can both be generated by a rule, and be checked in at the same time.

This forces to check in the file dune.inc. To keep this file in sync, we generate another file and use the standard dune runtest + dune promote workflow to synchronize the file in the tree.

This process can be likened to a fixpoint operation. If a new rule is added to the file, a runtest+promote step should be executed first to update the dune file, followed by another runtest+promote step to validate the new test. Running with the -w option makes this process seamless.

While I acknowledge my familiarity with this pattern might influence my perspective, I also believe there's value in having the generated rules visible in the tree. It provides quick access for understanding the rules, serving as an 'expect test' for the code generating the rules. Even if we could include a dynamic file, there's a chance I'd still lean towards this approach for its clarity and accessibility. YMMV.

mbarbin commented 10 months ago

Just one more thing to mention:

serving as an 'expect test' for the code generating the rules

I put this to good use while I was developing the code that generates the rule. I could visualize the incremental changes to the dune file until I was satisfied with the result (dune promote).

A word of caution though: once you experience the efficiency of the expect-test workflow, it's hard to go back. Consider yourself warned! :smiley:

c-cube commented 10 months ago

Oh that's great, thank you for the explanation. Then it's perfect, I'm merging this. That makes me think that maybe ocaml-protoc should also ship with a small binary (ocaml-protoc-genrules?) that takes a list of .proto files as arguments as well as arguments for ocaml-protoc itself, and spits out a dune.inc file that contains the rules for these particular modules? Like ocaml-protoc-genrules foo.proto bar.proto --pp --binary -I dir produces a file with:

; dune.inc

(rule
  (targets foo.ml foo.mli)
  (deps (:file foo.proto))
  (action ocaml-protoc %{file} --pp --binary --ml_dir=.))

; … same for bar

What do you think?

I like expect tests, I just don't use them enough :-).