Switch stanc.exe to use Cmdliner for cli #1478
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #1478 +/- ##
==========================================
+ Coverage 89.72% 90.05% +0.33%
==========================================
Files 65 66 +1
Lines 10693 10751 +58
==========================================
+ Hits 9594 9682 +88
+ Misses 1099 1069 -30
|
WardBrian
force-pushed
the
cmdliner
branch
2 times, most recently
from
75b386e to
68ff259
Compare
|
Okay, I think I finally excised the ghosts in the multiarch jenkins build for this... |
|
Note for reviewers: The review is probably easiest to do commit-by-commit, with the actual code changes all happening in the first commit. The second is some documentation, which can be reviewed, and the third is the result of a lot of jenkins tweaking to keep the build happy, which can probably be skipped if you're willing to trust the fact that it is building. |
src/stanc/CLI.ml
Outdated
| ; print_lir | ||
| ; debug_generate_data | ||
| ; debug_generate_inits | ||
| ; debug_data_json= Option.map ~f:In_channel.read_all debug_data_file } |
There was a problem hiding this comment.
This crashes if the provided --debug-data-file cannot be read. Do we have an existing test for that? Looks like test/integration/cli-args/debug-generate-data.t only has error cases where either no data file is provided or the file is well-formed JSON that does not contain the expected data.
There was a problem hiding this comment.
Fixed this, and a separate issue where it would crash if the file existed, but could not be opened
| MULTIARCH_DOCKER_TAG = 'multiarch-ocaml-4.14-v2' | ||
| MULTIARCH_DOCKER_TAG = 'multiarch-ocaml-4.14-v2-and-cmdliner' |
There was a problem hiding this comment.
I don't know anything about Jenkins, but what's the reason for the tag change?
There was a problem hiding this comment.
I needed to re-build the multiarch images to add cmdliner. Because they don't install the developer-only dependencies like ocamlformat, I guess they didn't have it already.
This was a headache, and it's definitely non-obvious how the multiarch build works, see #1480
I mentioned this in #1477. The use of imperative style argument parsing in
stanc.exeled to some weird code patterns and didn't give us type-checking guarantees about each setting actually being available (or, conversely, being used).This PR switches to using the
cmdlinerlibrary: https://erratique.ch/software/cmdliner. This is by the same author as thefmtlibrary we already use, and is used byopamitself. I think it might already be installed on most of our switches as a result, but if not it's a very minimal addition.Submission Checklist
Release notes
Improved the command-line argument parser for
stanc. Existing flags are still supported, but new aliases may be available. See the new and improvedstanc --helpfor details.Copyright and Licensing
By submitting this pull request, the copyright holder is agreeing to
license the submitted work under the BSD 3-clause license (https://opensource.org/licenses/BSD-3-Clause)