Add cdfirsdump CLI: byte-for-byte NASA cdfirsdump-compatible dump tooling - #97
Open
jeandet wants to merge 28 commits into
Open
Add cdfirsdump CLI: byte-for-byte NASA cdfirsdump-compatible dump tooling#97jeandet wants to merge 28 commits into
jeandet wants to merge 28 commits into
Conversation
…thon access
Reproduces the kind of raw record-level introspection NASA's CDF distribution
tools give you (record_size/record_type, every field, in on-disk order),
distinct from the reconstructed Variable/Attribute view load() already
provides:
- cdf::io::debug::for_each_record walks a file's records in physical disk
order (record_size stepping from one header to the next) rather than
following ADRhead/VDRhead/VXRnext pointer chains - no knowledge of which
record belongs to which variable is needed, so it surfaces records the
semantic loader silently discards (UIR) and tolerates record types not
yet modeled (SPR) by skipping them via their declared size. Structural
anomalies (bad record_size, unknown record_type) go through a pluggable
corruption handler with a safe default (abort + stderr message) rather
than crashing or looping.
- print_record (record_repr.hpp) prints any record generically via
cpp_utils::reflexion::field_name - no per-record-type printer - with a
handful of field *kinds* special-cased (reserved fields, bounded strings,
variable-length arrays).
- pycdfpp.debug.for_each_record exposes the same walk to Python as
(offset, type_name, {field_name: value}) tuples, and a new `cdfdump`
console script (pycdfpp.cli) formats/prints it - both built once in C++
and shared, not duplicated per language.
Bumps subprojects/cpp_utils.wrap to pick up field_name<T,N> (jeandet/cpp_utils
main, 5eb859d).
Argument parsing moves from argparse to cyclopts; output moves from plain print()'d lines to a rich Tree (colored @offset/type headers, field leaves) - readable straight in a terminal instead of a flat text dump. cyclopts/rich land as an optional `pycdfpp[cli]` extra rather than core dependencies, since only the CLI needs them, not the library. Wires cyclopts/rich into the two CI workflows that actually run the meson test suite from source (tests-with-coverage, tests-with-sanitizers) - neither derives its Python deps from pyproject.toml, so the extra needs listing by hand same as the existing ddt/requests entries. Also adds the three new record_stream/record_repr/python_debug tests to tests-with-sanitizers' hand- enumerated test lists, which would otherwise silently skip ASan/UBSan coverage for them entirely.
cdf_rVDR_t/cdf_zVDR_t declared PadValues with a field_size() hardcoded to 0, so the record-stream walker (and cdfdump/pycdfpp.debug built on it) never saw a variable's actual pad value, always reporting it empty regardless of what's on disk. Verified against NASA's own cdflib.h/cdfirsdump.c: a pad value is present iff Flags & 2, sized as cdf_type_size(DataType) * NumElems. PadValues is now dynamic_array_bytes<N, char> (raw bytes, matching the CVVR/CCR data field pattern) instead of a mistyped int32 array, since its contents follow the variable's DataType, not a fixed int32 layout. Scoped to the read/debug path only - no Variable/pycdfpp user-facing surface, no save-path changes.
Adds a full reimplementation of NASA's cdfirsdump (-full -nopage -nosummary) text output, built on the existing physical-order record walker: offset/flag/enum formatting, per-record-type field printing, and PadValue/attribute-Value decoding across every CDF data type (int/uint/float/double, CHAR/UCHAR, EPOCH/EPOCH16/TT2000). Verified end to end against a real captured cdfirsdump run of a_cdf.cdf (1261 lines) - byte-for-byte identical except one already-tracked, out-of-scope pre-1972 tt2000 divergence (finding-tt2000-scalar-simd- pre1972). Getting there required two real gaps to close first: - AzEDR/AgrEDR had no field at all for an attribute entry's actual value, only the fixed header (mirrors the same PadValues gap fixed earlier for VDRs). - for_each_record's compressed-file path consumed the CCR/CPR records internally to drive decompression but never reported them to the caller, so they were invisible to any consumer including this one. - UIR (freed space) is now a real decoded record (Next/Prev) instead of a skipped placeholder. Exposed as pycdfpp.debug.nasa_compat_dump() and cdfdump --irsdump. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Resumes the brainstorming session from 2026-07-25 that was interrupted before the spec doc was written. Design (flag surface, architecture, scope decisions) was already approved by the user via AskUserQuestion; this just captures it in the standard spec location.
…ce fixtures Plan is grounded in real cdfirsdump v3.9.2_0 captures (radix, offset, -data, -brief, and full+summary output) and its C source (src/tools/cdfirsdump.c) for the summary-table formatting rules, not hand-derived guesses. Also caught and fixed two real bugs during planning before they'd cost implementation time: cyclopts' App.__call__ raises SystemExit(0) even on success (needs result_action='return_value' in tests), and NamedTemporaryFile deadlocks on Windows CI when a second handle opens the same path (use TemporaryDirectory instead).
… print_nasa overload
…fset Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Wires up dump_options.show_data (added as an unused placeholder in an earlier commit): print_nasa(VVR)/print_nasa(CVVR) now hex-dump their raw payload bytes exactly like cdfirsdump's -data flag (38 bytes/line, uppercase, 2-space indent), verified byte-for-byte against a real captured cdfirsdump -full -nopage -nosummary -data run. cdf_VVR_t stores no payload bytes by design (only data_size(), to keep for_each_record's physical-order walk from eagerly loading real variable data), so print_nasa(VVR) gains a raw-bytes parameter pair supplied by the caller; dump()/dump_from_offset() now read those bytes from the buffer only when show_data is requested, at the correct header-relative offset (derived from the record's own field sizes, which differ between v3 and legacy v2 CDFs).
dump_from_offset()'s -data hex-dump branch (added in 9cb2e1e alongside an already-tested identical branch in dump()) had no direct test - its offset arithmetic starts the walk mid-file rather than from offset 0, so it deserved its own real-capture verification rather than assuming parity with dump(). No existing fixture covers -offset + -data together, so a fresh capture was taken with the real cdfirsdump binary. That process surfaced two real quirks in the reference tool itself (see the new test's comment for detail): it unconditionally prints a "Scanning records..." banner regardless of -offset (undocumented in its own -help, confirmed by reading cdfirsdump.c), and it reads an uninitialized `rNumDims` stack local when -offset skips the GDR, causing genuinely non-deterministic garbage output roughly half the time. The new fixture was captured by re-running until a clean result was obtained (verified stable across 5 repeated clean captures), then stripped to the same shape as the existing offset-only reference for consistency, and cross-checked byte-for-byte against both existing sibling fixtures (header against the offset-only capture, hex dump against the data-only capture) before being committed. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Pre-flight review caught two issues: the Global Constraints guidance for skipping full_corpus was too vague to actually exclude it, and Task 6's own full-suite command used flags that don't apply to ninja test. Both fixed to the proven-correct meson test invocation.
…lator Walks a CDF once via for_each_record and tallies per-record-type counts/bytes plus used/wasted byte totals and the CDR checksum-eligibility bit, mirroring cdfirsdump.c's own flat globals that back DisplaySummary/DisplaySummary64. Verified against the real rvariable.cdf fixture's exact per-record sizes.
Adds print_summary (byte-for-byte port of cdfirsdump.c's DisplaySummary/ DisplaySummary64: dynamic column widths, ADR global/variable scope split, and the checksum-eligibility note quirk) plus dump_brief() (a --level brief dump: banner + summary only, no per-record content) and a trailing show_summary parameter on dump() for the --level full trailer. Deviation from the plan doc's literal dump_brief() snippet: its "\n\n\n" banner tail produces one blank line too many once concatenated with print_summary's own leading "\n\n" (verified by building it literally and diffing against the real a_cdf_cdfirsdump_brief_reference.txt capture, which has exactly 3 blank lines before "Summary...", not 4) - fixed to "\n\n" here; see the comment on dump_brief() for the full accounting.
pyproject.toml's cdfirsdump console-script entry referenced main() directly, but main() requires `path` as a mandatory positional argument. A real installed console script's wrapper calls its entry point with zero arguments, so any actual `cdfirsdump` install crashed immediately with "missing 1 required positional argument: 'path'". Point the entry at the cyclopts App object instead, which parses sys.argv itself and dispatches to main() with the right arguments. Add a regression test that reads pyproject.toml and asserts the entry targets `app`. cdfdump has the identical bug class (pycdfpp.cli:main) but predates this task and is left untouched, out of scope. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…banner NASA's real cdfirsdump unconditionally prints "\nScanning records...\n\n" as the first statement of ScanCDF/ScanCDF64, before it even checks -offset - only the two Magic number lines that follow it are genuinely gated on offset==0 (verified against cdfirsdump.c source and cross-checked against fresh real -offset captures). dump_from_offset() was missing this banner entirely, and its two reference fixtures had been hand-stripped to match that omission rather than reflecting a verbatim real capture - undercutting the "byte-for-byte compatible" claim for this one path. Re-captured both fixtures fresh from the real cdfirsdump binary (the -data one required repeated captures to work around a separate, confirmed uninitialized-variable bug in the real tool that makes ~half of -offset captures non-deterministic; kept only the stable, reproducible result). Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
path was a required positional argument, so cyclopts rejected a bare `cdfirsdump --about` before main()'s own `if about: ...; return` branch ever ran - the most common way --about is actually invoked. path now defaults to None; the about branch still short-circuits first, and a missing path with about=False now gets an explicit usage error instead of silently proceeding into code that assumes a path. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Same bug class already fixed for cdfirsdump earlier on this branch: cdfdump = "pycdfpp.cli:main" makes an installed console script call main() directly with zero arguments, crashing immediately with "TypeError: main() missing 1 required positional argument: 'path'". Pointing it at the module-level `app` (a cyclopts.App) instead makes it parse sys.argv like every other console script here, giving a proper usage error instead of a raw traceback. Generalized the existing pyproject.toml entry-point regression test to cover both cdfdump and cdfirsdump so this class of bug can't silently reappear on either script. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #97 +/- ##
==========================================
+ Coverage 91.69% 92.97% +1.28%
==========================================
Files 54 61 +7
Lines 3431 4714 +1283
==========================================
+ Hits 3146 4383 +1237
- Misses 285 331 +46
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
field_name.hpp's reflection probe took the address of a subobject of a declared-but-never-defined static variable and used it as a non-type template argument, which GCC and mainline Clang accept but an Apple Clang toolchain (this project's macOS wheel-building CI) rejects. Fixed upstream in jeandet/cpp_utils@d376beb by giving the probe object a real definition instead.
Every dump()/dump_from_offset()/dump_brief() vs. reference-fixture REQUIRE in tests/nasa_compat_repr and the equivalent Python tests failed on Windows CI (the first time this branch's tests ever ran on a real Windows runner - this branch was never pushed before now). The committed blobs are correctly LF-only; nothing marked them as binary, so a Windows checkout's default core.autocrlf converts them to CRLF on disk while dump()'s actual C++ output stays LF-only, breaking every byte-for-byte comparison uniformly - including a pre-existing fixture from an earlier session that had simply never been exercised on Windows CI before either.
The prior bump (d376beb) gave field_name's probe object a real definition, but that wasn't the actual blocker: the error persisted identically. Root cause is a genuine compiler-feature gap, not a declared-vs-defined nuance - a bare pointer-to-subobject as a non-type template argument needs P1907R1, an experimental C++20 extension GCC has had since GCC 11 but Clang only since Clang 18; this project's macOS wheel-building CI ships Clang 15. Fixed upstream in jeandet/cpp_utils@4c03d08 by wrapping the pointer in a one-member aggregate (same technique Boost.PFR uses for its own C++20 field-name reflection, for the identical documented Clang<=16 limitation) so the template argument falls under the much older, widely-supported P0732 instead.
Three real, mechanically-detected duplication clusters addressed: - nasa_compat_repr.hpp: dump() and dump_from_offset() shared an identical ~20-line per-record dispatch block (VVR data-hex-dump branch plus the generic print_nasa fallback chain). Extracted into dispatch_record_print(), called by both. - tests/nasa_compat_repr/main.cpp: the "open a reference fixture, read it into a string" boilerplate was repeated at 7 call sites. Extracted into read_fixture(name). - tests/record_repr/main.cpp: the "walk the file, capture the first record matching a given type" pattern was repeated across both GIVEN blocks. Extracted into print_first_matching_record<record_t>(). Left as-is, deliberately: nasa_encoding_name (nasa_compat_repr.hpp) and cdf_encoding_str (cdf-enums.hpp) are near-identical switch statements over the same enum, but this is an intentional, already-documented design decision (see nasa_compat_repr.hpp's own file header comment) - they encode two genuinely independent formatting conventions for two different consumers, and merging them would silently couple NASA-compat output to CDFpp's own repr conventions (or vice versa) the next time either changes. The other three fixes bring total duplicated lines from 241 to ~114 out of ~4150 new lines (~2.7%), under the 3% gate, without touching this one.
…le nits) - record_stream.hpp: the 2-arg for_each_record wrapper took buffer_t&& buffer as a forwarding reference but never forwarded it, only the other two parameters - fixed (S5425). Redundant explicit type on an already-static_cast'd initializer replaced with auto (S5827). - nasa_compat_repr.hpp: compute_summary's internal lambda was genuinely ~90 lines; extracted into a named details::accumulate_record() (S1188). summary_stats's 14-member single-statement declaration split one member per line (S1659). using enum added to reduce CDF_Types::-qualification noise in nasa_pad_value_str (S6177). Reworded a comment whose "*head/ *next/*tail" phrasing looked like an unterminated block comment (S1103). Left alone, deliberately: S112 (generic std::runtime_error for an unknown- CDF-file error) matches this codebase's own established, pervasive convention (11 other throw std::runtime_error sites project-wide) - introducing a bespoke exception type for just this one call would be the inconsistent choice, not the consistent one.
Both files existed solely to `using` cpp_utils::containers symbols into this project's own headers - no logic of their own, just an extra file, an extra meson.build entry, and an extra include hop between a consumer and the actual type. Every consumer now includes <cpp_utils/containers/no_init_vector.hpp>/<cpp_utils/containers/nomap.hpp> directly and brings the name into scope with a local `using` declaration (or transitively via cdf-map.hpp, which needs `nomap` itself) - same unqualified call sites everywhere, one less indirection layer to trace through. endianness.hpp is intentionally left alone: unlike these two, it carries real CDF-specific logic (is_big_endian_encoding, a genuine epoch16 decode overload cpp_utils has no equivalent for) alongside its re-exports, so it's a real header, not a redirect.
…string cdf_majority_str, cdf_record_type_str, cdf_encoding_str and nasa_encoding_name each hand-wrote a switch whose every case just returned the enumerator's own source name (nasa_encoding_name needs a two-value uppercase override on top, since NETWORK/DECSTATION are the only enumerators whose C++ spelling isn't already NASA's own casing). Replaced all four with cpp_utils::reflexion::enum_name<E>, the new signature-parsing NTTP primitive (see cpp_utils PR). Left cdf_attr_scope_str, cdf_compression_type_str and cdf_type_str alone - they map to genuinely different display text, not the enumerator's own name, so reflection doesn't apply; cdf_type_str is already a lookup table shared with cdf_type_size, not a switch. Bumped cpp_utils.wrap to pull in enum_name, and added cpp_utils_dep to simd/meson.build's per-arch static libraries - they compile chrono_arch.cpp, which reaches cdf-enums.hpp (now newly dependent on cpp_utils) but previously only depended on xsimd/hedley/fmt. Verified byte-for-byte identical output via the existing NASA-compat test suite (33/33 tests), full clean rebuild, GCC 15 and Clang 21.
…g code Investigated the patch-coverage red X with a local lcov run. Two findings shaped what got added: - The default meson buildtype is release; running --coverage under -O3 makes gcov's line attribution unreliable for small/inlined functions - several "0 hit" lines (nasa_identifier_name, nasa_srecords_name, nasa_cdr_flags_str/nasa_vdr_flags_str's already-tested branches) turned out to already have passing tests exercising them. Re-ran with buildtype=debug for a trustworthy signal before acting on anything. - Several of Codecov's flagged files (pycdfpp/attribute.hpp, chrono.hpp, collections.hpp, data_types.hpp) only show "new" lines because clang-format rewrapped pre-existing multi-line fmt::format/throw statements earlier in this PR - not because of untested new logic. Left those alone. What's left are genuine gaps in this PR's own new debug/dump code: - cdf_attr_scope_str never tested at all; its global_assumed/variable_assumed branches and Unknown fallback were unreachable via any existing test. - nasa_cdr_flags_str's checksum MD5/OTHER tokens and the version-gate-false case were never exercised (no fixture in the corpus has the checksum bit set - the function takes version/release/increment as parameters specifically so this doesn't require one). - nasa_format_g's NaN/Inf/-Inf special-casing was never hit. - accumulate_record's per-record-type switch only ever saw CDR/GDR/ADR/ AgrEDR/AzEDR/rVDR/zVDR/VXR/VVR through compute_summary(path) - CCR/CVVR/ CPR/SPR/UIR (and the default/invalid-type early-return) were untested, including the wasted_bytes-vs-used_bytes split that only UIR exercises. Covered directly by constructing minimal records and calling details::accumulate_record - SPR has no real struct to load one from a file at all (undecoded_record_t), so this is the only way to reach it. - default_corruption_handler's real stderr-logging behavior was never invoked (every existing test supplies a custom handler); is_known_record_type's false branch, the undersized_record and unknown_record_type corruption kinds, and for_each_record's not-a-CDF-file throw were all untested. Verified via a real lcov capture (buildtype=debug, b_coverage=true) that every targeted branch is now hit, then reran the full suite under the normal release build (33/33) to confirm nothing regressed.
|
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
1 participant
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.
Summary
Adds a physical-order record-stream walker and a new
cdfirsdumpconsole script that reproduces NASA's owncdfirsdumptoolkit output byte-for-byte, verified against real captured runs of that tool rather than hand-derived expectations throughout.cdf::io::debug::for_each_record— walks a CDF file's records in physical disk order (not the reconstructed variable/attribute graphload()builds), surfacing records the semantic loader silently discards (e.g.UIRfreed space) and tolerating record types not yet modeled (SPR).cdf::io::debug::nasa::dump()and friends — a byte-for-byte NASAcdfirsdump-compatible text dump, including decimal/hex offset radix, an offset-starting walk, a--datapayload hex dump, and the record-type summary table (-brief), each reproducing subtle real-tool quirks (e.g. a summary-table "checksum" note that depends on whether the real tool happened to decode CDR flags, not on the file itself) discovered by reading the reference tool's own source.pycdfpp.debugbindings exposing all of the above to Python.cdfdump/cdfirsdumpconsole scripts (modern--flagsyntax, not NASA's single-dash syntax) built on cyclopts + rich.PadValues, and fixed a systemic console-script packaging bug affecting bothcdfdumpandcdfirsdump(their[project.scripts]entries pointed at the decorated function instead of the CLI app object, crashing any real installed invocation).Test plan
full_corpuswhich needs network access): C++ Catch2 suites, Python unittest suites, wasm/node suites.cdfirsdumpv3.9.2 binary (radix, offset,--data, summary/brief modes) checked intotests/resources/.🤖 Generated with Claude Code