Manifest (11): bin_node, bin_client, bin_codec, bin_proxy_server
This is a small subset of !3366 (merged), which aims at generating dune and .opam files from a manifest.ml file. See !3366 (merged) for a broad view of the end goal.
Because !3366 (merged) is too large to review in one go, the plan is to make several MRs targetting subparts of the dune and .opam files, without the generator itself. The goal is to check all changes in dune and .opam files made by !3366 (merged). Even if we do not merge !3366 (merged) ever, we'll fix some inconsistencies between dune and .opam files this way. After all changes to dune and .opam files have been reviewed and merged, we can merge the generator itself and easily see that it generates the exact same files.
This MR contains the subset of !3366 (merged) for:
src/bin_nodesrc/bin_clientsrc/bin_codecsrc/bin_proxy_server
Tricky Parts
This MR is more tricky to review than other satellites of !3366 (merged) because the four executables abuse the (select) stanza.
- Any typo in those
(select)stanzas does NOT result in an error, the package is just not linked. So we need to be very, very rigorous when reviewing them. In particular, please note that up to Carthage, registration packages are named with a dot (e.g.tezos-client-006-PsCARTHA-commands.registration), but from Delphi, they are named with a dash (e.g.tezos-client-007-PsDELPH1-commands-registration). - The
void_for_linkingfilenames have changed, because the generator puts the full package name instead of a human-chosen shorter version. - I tried to keep the order of packages mostly the same, but there are small differences, such as
alphawhich now always come last.
Moreover, those dune and opam files are modified by scripts/link_protocol.sh, which must still work correctly. It has some very specific requirements:
- each
selectstanza must be exactly on 3 lines, not less, not more; - two
selectstanzas related to the same protocol cannot be put next to each other, otherwise it counts as 6 lines, not 3 (!); - we can't have more closing parentheses at the very end of the stanza as
link_protocol.shwould copy those extra parentheses.
I had to tweak the generator because of these, but my testing shows that it works fine. Note that in the final version of the generator, link_protocol.sh updates the manifest instead of opam and dune files directly, significantly simplifying the process and making it more robust at the same time. But we're not merging the generator yet, so we need link_protocol.sh to work.
Dependency Inconsistencies
When a dependency was present in the dune file but not in the .opam file, it was added to the .opam file. So if you see a new dependency in the .opam file, just check whether it also used by the dune file (if not, tell me!).
Conversely, when a dependency was not present in the dune file but was present in the .opam file, it was removed from the .opam file.
All dependencies that appear in (select) stanzas are now in the depopts section of the corresponding .opam files. Indeed, the goal of the (select) stanza is to make these dependencies optional. If the .opam file does not actually declare those dependencies as optional, they are not actually optional!
General Review Tips
patdiff helps a lot, as well as the -w option of git diff. You can use patdiff with git diff as follows:
opam install patdiff
export GIT_EXTERNAL_DIFF=`pwd`/_opam/bin/patdiff-git-wrapper
If you read the diff in GitLab, the -w option of git diff can be activated by opening preferences (the gear icon while reviewing) and unchecking "show whitespace changes".
Apparently, VS Code is also very nice to review whitespace diffs.
Manually testing the MR
Compile everything (or let the CI do it for you).
Test link_protocol.sh with:
./scripts/link_protocol.sh src/proto_011_PtHangzH
Checklist
-
Select suitable reviewers using the Reviewersfield below. -
Select as Assigneethe next person who should take action on that MR