Skip to content

Replace cardano-diffusion's ping parser with a local flag-based parser - #1413

Open
Jimbo4350 wants to merge 3 commits into
masterfrom
jordan/ping-parser-drop-network-cmdline-parser
Open

Replace cardano-diffusion's ping parser with a local flag-based parser#1413
Jimbo4350 wants to merge 3 commits into
masterfrom
jordan/ping-parser-drop-network-cmdline-parser

Conversation

@Jimbo4350

@Jimbo4350 Jimbo4350 commented Aug 12, 2026

Copy link
Copy Markdown
Contributor

Changelog

- description: |
    Restore the pre-11.2 `cardano-cli ping` interface (`--host`, `--unixsock`, `--port`, `--magic`, `--json`, `--quiet`, `--query-versions`, `--tip`) and stop depending on the command line parser of `cardano-diffusion:ping`, which made the released package unbuildable with default cabal flags.
  type:
    # - feature        # introduces a new feature
    # - breaking     # the API has changed in a breaking way
    # - compatible   # the API has changed but is non-breaking
    # - optimisation # measurable performance improvements
    # - refactoring  # QoL changes
    - bugfix         # fixes a defect
    # - test         # fixes/modifies tests
    # - maintenance  # not directly related to the code
    # - release      # related to a new release preparation
    # - documentation # change in code docs, haddocks...

Context

The cardano-cli-11.2.0.0 release cannot be added to CHaP: CHaP PR #1431 fails build-new-packages (CI run) because pPing = uncurry PingCmd <$> Ping.cmdlineParser mixes Parser types from two different packages: cardano-cli uses optparse-applicative-fork, while Ping.cmdlineParser is built against plain optparse-applicative unless the manual cabal flag optparse-applicative-fork of cardano-diffusion is set. This repo set that flag in cabal.project, but cabal.project does 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.cmdlineParser and give cardano-cli its own ping parser written against optparse-applicative-fork, so no optparse type crosses the package boundary and the flag becomes irrelevant. The package cardano-diffusion / flags: +optparse-applicative-fork stanza is removed from cabal.project to prove it: the build now succeeds with cardano-diffusion's default flags, exactly like CHaP CI builds it.

The command line interface is restored to exactly what was last on master before the migration (Slack discussion) — the generated help output is byte-identical to the pre-#1384 one (modulo one line of trailing whitespace from the newer optparse renderer) — keeping breaking changes to cardano-cli ping under this repo's control. This replaces the positional-argument interface that #1384 introduced via cardano-diffusion:ping (positional ADDRS, --mode, --network-magic, --srv-prefix, --color, --short-hash), which never reached CHaP. In particular -h means --host again, -m/--magic keeps its old spelling, and -Q/-t map onto the ping library's PingMode (-Q takes precedence, matching the old cardano-ping behaviour). The new ping library options are pinned internally to their old-behaviour values (ColorNever, FullHash); SRV lookups are not reachable from the CLI. All of these can be exposed later as non-breaking additions if wanted.

The pre-11.2 misconfiguration guard is also restored: cardano-cli ping --unixsock ... without --tip/--query-versions fails with "Unix sockets only support queries for available versions or a tip." (the ping library silently no-ops in that case: (NodeToClient, PingMode) -> pure ()).

Known behavioural deltas vs the pre-#1384 implementation, either pre-existing consequences of the library migration or deliberate tightening:

  • --port accepts only numbers now (the old implementation passed a string to getAddrInfo, so /etc/services names technically worked);
  • hostnames are resolved via DNS queries (the dns package) instead of getAddrInfo, so /etc/hosts-only names may not resolve on machines without an NSS-aware stub resolver; IP literals are unaffected (they bypass DNS entirely);
  • --host values containing a port (host:3001) are rejected at parse time with a pointer to --port (they never worked — the old implementation failed on them at resolution time with an obscure getAddrInfo error).

The follow-up release cardano-cli-11.2.1.0 will supersede CHaP PR #1431 (the 11.2.0.0 sdist cannot be fixed in place). Once released, the network team can drop the optparse-applicative-fork flag from cardano-diffusion (added as a stopgap in ouroboros-network#5392).

How to trust this PR

  • The cabal.project no longer sets +optparse-applicative-fork for cardano-diffusion, so CI now builds cardano-diffusion:ping exactly as CHaP CI does — with plain optparse-applicative — and cardano-cli still compiles.
  • The regenerated golden help file help/ping.cli is byte-identical to the one last on master before Integration for Node release 11.1 #1384 (82a19b757^) apart from a trailing-whitespace line emitted by the newer optparse usage renderer. (That help, and the whole ping implementation, is in turn identical between 82a19b757^ and the cardano-cli-11.1.0.0 tag, so the restored interface also matches the last release consumers could actually build.)
  • Test.Cli.Ping locks down the endpoint-to-Address mapping (IP literals bypass DNS via the IP constructor, bare unix socket names are not mistaken for domain names by mkAddress's heuristic), the option defaults, the pinned library options, and the restored unix-socket misconfiguration guard.

Checklist

  • Commit sequence broadly makes sense and commits have useful messages
  • New tests are added if needed and existing tests are updated
  • Self-reviewed the diff

The released cardano-cli cannot be built with default cabal flags:
Ping.cmdlineParser returns a Parser from plain optparse-applicative
unless cardano-diffusion's manual optparse-applicative-fork flag is set,
and the cabal.project stanza setting that flag does not ship with the
sdist. Parse the ping options locally with optparse-applicative-fork so
that no optparse type crosses the package boundary, and drop the flag
stanza from cabal.project.

This restores the old flag-based ping interface (--host, --unixsock,
--port, --magic, --json, --quiet, --query-versions, --tip), allows
repeating the endpoint flags to ping multiple endpoints, and exposes the
new ping library features via --srv, --srv-prefix, --color and
--short-hash.
Copilot AI lite review requested due to automatic review settings August 12, 2026 14:10

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 ping option parser supporting repeatable endpoints (--host, --unixsock, --srv) plus new flags (--color, --short-hash) and precedence rules (--query-versions over --tip).
  • Adds a dedicated Test.Cli.Ping test module to lock down endpoint-to-Address mapping and default option behavior.
  • Updates golden help output and removes the cardano-diffusion +optparse-applicative-fork flag override from cabal.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)

Per review: drop --srv, --srv-prefix, --color and --short-hash, and the
repeatable/inline-port endpoint extensions. The interface is now exactly
the pre-11.2 one; the generated help output is byte-identical to
11.1.0.0's apart from a trailing-whitespace line from the newer optparse
renderer. The unexposed ping library options are pinned to their
old-behaviour values (ColorNever, FullHash).

Also restore the pre-11.2 misconfiguration guard: --unixsock without
--tip/--query-versions fails again with "Unix sockets only support
queries for available versions or a tip." instead of the ping library's
silent no-op, and --host values containing a port are rejected at parse
time with a pointer to --port.
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