Replace cardano-diffusion's ping parser with an identical local parser - #1413
Merged
Merged
Conversation
Jimbo4350
requested review from
carbolymer,
disassembler,
kevinhammond,
newhoggy and
palas
as code owners
August 12, 2026 14:10
Contributor
There was a problem hiding this comment.
Pull request overview
This PR restores cardano-cli ping’s flag-based interface by replacing the dependency on cardano-diffusion:ping’s command-line parser with a local optparse-applicative-fork-based parser, unblocking downstream builds (e.g., CHaP) that cannot flip manual cabal flags in released sdists.
Changes:
- Introduces a local
pingoption parser supporting repeatable endpoints (--host,--unixsock,--srv) plus new flags (--color,--short-hash) and precedence rules (--query-versionsover--tip). - Adds a dedicated
Test.Cli.Pingtest module to lock down endpoint-to-Addressmapping and default option behavior. - Updates golden help output and removes the
cardano-diffusion+optparse-applicative-forkflag override fromcabal.project.
Reviewed changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
cardano-cli/src/Cardano/CLI/EraIndependent/Ping/Option.hs |
Implements the local flag-based ping parser and endpoint-to-address conversion logic. |
cardano-cli/test/cardano-cli-test/Test/Cli/Ping.hs |
Adds parser-focused tests for endpoints, defaults, and failure cases. |
cardano-cli/test/cardano-cli-golden/files/golden/help/ping.cli |
Updates command-specific help text to reflect the restored flag interface. |
cardano-cli/test/cardano-cli-golden/files/golden/help.cli |
Updates aggregated CLI help output for the ping command. |
cardano-cli/cardano-cli.cabal |
Registers the new test module and adds needed test-suite dependencies. |
cabal.project |
Removes the cardano-diffusion manual flag override to ensure default-flag buildability. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Comment on lines
+44
to
+51
| pHost :: Opt.Parser PingEndPoint | ||
| pHost = | ||
| fmap HostEndPoint $ | ||
| Opt.strOption $ | ||
| mconcat | ||
| [ Opt.long "host" | ||
| , Opt.short 'h' | ||
| , Opt.metavar "HOST" |
Comment on lines
+20
to
+24
| parsePingCmd :: [String] -> Maybe PingCmd | ||
| parsePingCmd = | ||
| Opt.getParseResult | ||
| . Opt.execParserPure Opt.defaultPrefs (Opt.info pPing mempty) | ||
|
|
Jimbo4350
force-pushed
the
jordan/ping-parser-drop-network-cmdline-parser
branch
2 times, most recently
from
August 13, 2026 14:43
ff48d1c to
996c65e
Compare
cardano-cli used Ping.cmdlineParser from cardano-diffusion:ping, which only type-checks when cardano-diffusion is built with its manual optparse-applicative-fork cabal flag. That flag was set via cabal.project, which does not ship with the sdist, so the released package was unbuildable with default flags (CHaP CI rejected 11.2.0.0). The parser is replaced with a verbatim local copy written against optparse-applicative-fork: same options, readers, defaults and help text, so the command line interface is unchanged. The cabal.project flag stanza is removed, and Test.Cli.Ping pins the parser's defaults, option readers, address classification and rejected command lines.
Jimbo4350
force-pushed
the
jordan/ping-parser-drop-network-cmdline-parser
branch
from
August 13, 2026 15:48
996c65e to
8d29779
Compare
This was referenced Aug 13, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Changelog
Context
The
cardano-cli-11.2.0.0release cannot be added to CHaP: CHaP PR #1431 failsbuild-new-packages(CI run) becausepPing = uncurry PingCmd <$> Ping.cmdlineParsermixesParsertypes from two different packages: cardano-cli usesoptparse-applicative-fork, whilePing.cmdlineParseris built against plainoptparse-applicativeunless the manual cabal flagoptparse-applicative-forkofcardano-diffusionis set. This repo set that flag incabal.project, butcabal.projectdoes not ship with the sdist and manual flags are never flipped by the solver, so the released package is unbuildable for CHaP CI and for every downstream consumer with default flags.The fix is to stop using
Ping.cmdlineParserand give cardano-cli its own ping parser written againstoptparse-applicative-fork, so no optparse type crosses the package boundary and the flag becomes irrelevant. Thepackage cardano-diffusion / flags: +optparse-applicative-forkstanza is removed fromcabal.projectto prove it: the build now succeeds with cardano-diffusion's default flags, exactly like CHaP CI builds it.The command line interface is unchanged: the local parser is a verbatim copy (modulo qualification) of
Cardano.Network.Ping.cmdlineParser— same options, readers, defaults and help text — socardano-cli pingbehaves exactly as it does on master.Ping/Command.hs,Ping/Run.hsand the golden help files are untouched relative to master; the whole change is confined toPing/Option.hsplus tests.The follow-up release
cardano-cli-11.2.1.0will supersede CHaP PR #1431 (the11.2.0.0sdist cannot be fixed in place). Once released, the network team can drop theoptparse-applicative-forkflag fromcardano-diffusion(added as a stopgap in ouroboros-network#5392).How to trust this PR
cabal.projectno longer sets+optparse-applicative-forkforcardano-diffusion, so CI now buildscardano-diffusion:pingexactly as CHaP CI does — with plainoptparse-applicative— and cardano-cli still compiles.help.cli,help/ping.cli) are byte-identical to master's and the golden tests pass, so the renderedcardano-cli pinghelp output is unchanged.Ping/Option.hsis a line-for-line copy ofpingOptsParser/argParserfromCardano.Network.Ping, so it can be reviewed by diffing the two side by side.Test.Cli.Pingpins the parser's behaviour: the defaults (count, mainnet magic, text output, ping mode,_cardano._tcpSRV prefix, auto color, full hash), every option and its reader (--mode,--coloraccept exactly their three values), address classification (IP-literal-with-port vs domain-with-port vs file path vs SRV name), and rejected command lines — including the pre-11.2 spellings (--host,--unixsock,--port,--magic,-t,-Q), which stay gone.Checklist