feat(migration): complete headerkit backend migration and C++ feature parity - #61
Open
elijahr wants to merge 38 commits into
Open
feat(migration): complete headerkit backend migration and C++ feature parity#61elijahr wants to merge 38 commits into
elijahr wants to merge 38 commits into
Conversation
Replace duplicated IR classes, PxdWriter, Cython type registries, and keyword lists with thin shim modules that re-export from headerkit. The shims provide 100% backward-compatible imports so no consuming code needs changes. The ir_writer shim subclasses headerkit's PxdWriter with stub_cimport_prefix="autopxd.stubs" pre-configured, preserving autopxd2's stub cimport behavior. Update PycparserBackend.parse() signature to match headerkit's ParserBackend protocol (accepts keyword-only params it doesn't use). Delete test_ir.py (24 tests) and test_cython_types.py (15 tests) which are now maintained in headerkit. All 359 remaining tests pass.
Summary of ChangesHello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request significantly refactors the Highlights
🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. Changelog
Activity
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
There was a problem hiding this comment.
Code Review
This pull request is a major refactoring that migrates significant parts of the codebase (IR, PxdWriter, type registries) to a new external library, headerkit. This greatly reduces code duplication and centralizes the logic. The changes are well-structured, creating shim modules to maintain backward compatibility.
My review focuses on a few points related to dependency management and reliance on private APIs, which could affect the project's stability and build reproducibility. Overall, this is an excellent cleanup.
| ] | ||
| requires-python = ">=3.10" | ||
| dependencies = ["Click", "pycparser"] | ||
| dependencies = ["Click", "pycparser", "headerkit"] |
There was a problem hiding this comment.
The headerkit dependency is not pinned to the specific development version required by this PR, which is inconsistent with .pre-commit-config.yaml and will likely cause installation issues for anyone trying to build from this branch. To ensure the project is buildable, please pin it to the same git branch/commit.
Suggested change
| dependencies = ["Click", "pycparser", "headerkit"] | |
| dependencies = ["Click", "pycparser", "headerkit @ git+https://github.com/axiomantic/headerkit.git@elijahr/autopxd2-migration"] |
| additional_dependencies: | ||
| - pycparser | ||
| - click | ||
| - headerkit @ git+https://github.com/axiomantic/headerkit.git@v0.6.0 |
There was a problem hiding this comment.
For better stability and reproducibility, it's recommended to pin this dependency to a specific commit hash instead of a branch name. Branches can be updated or deleted, which could break the pre-commit setup for others or in the future.
Comment on lines
+3
to
+12
| from headerkit.writers._cython_types import ( | ||
| CYTHON_STDLIB_HEADERS, | ||
| CYTHON_STDLIB_TYPES, | ||
| HEADERKIT_STUB_TYPES, | ||
| LIBCPP_HEADERS, | ||
| LIBCPP_TYPES, | ||
| get_cython_module_for_type, | ||
| get_libcpp_module_for_type, | ||
| get_stub_module_for_type, | ||
| ) |
There was a problem hiding this comment.
Importing from a private module (_cython_types) can be fragile as its API is not guaranteed to be stable across headerkit versions. It would be more robust to import these from a public API within headerkit if possible. Consider exposing these symbols in headerkit.writers.cython or a similar public module.
Comment on lines
+3
to
+7
| from headerkit.writers._cython_keywords import ( | ||
| C_keywords, | ||
| cython_keywords, | ||
| keywords, | ||
| ) |
There was a problem hiding this comment.
Similar to cython_types.py, this module imports from a private headerkit module (_cython_keywords). This creates a dependency on headerkit's internal implementation, which might change unexpectedly. It's recommended to rely on public APIs. Please consider exposing these keywords through a public module in headerkit.
BREAKING CHANGE: Remove autopxd.cython_types and autopxd.keywords modules (zero consumers after migration to headerkit shims). Remove dead elif/else branches in translate() for backends not supporting full ParserBackend protocol (both backends now do). Remove unused IGNORE_DECLARATIONS and STDINT_DECLARATIONS from declarations.py. Fix stale test reference in contributing docs. Bump to 4.0.0.
BREAKING CHANGE: The pycparser backend has been removed. libclang is now the only parser backend. Install libclang via your system package manager or headerkit's install_libclang tool. Remove pycparser_backend.py, declarations.py, regenerate_stubs.py, and all pycparser fake header stubs (stubs/include/, stubs/darwin-include/). Remove pycparser from runtime dependencies. Simplify resolve_backend() and CLI: --backend only accepts "auto" or "libclang". Remove validate_libclang_options() (moot with single backend). Remove FALLBACK_WARNING (no pycparser fallback). Convert all pycparser-specific tests to use libclang backend. Update all documentation to reflect single-backend architecture.
The expected file was written for the old pycparser-based writer. The headerkit CythonWriter produces a different (correct) ordering: forward declarations and typedefs first, then enums with cdef instead of ctypedef, globals before struct bodies, ctypedef struct for Buffer, and parameter names stripped from function pointer typedefs.
elijahr
marked this pull request as draft
February 28, 2026 23:35
Configure Dependabot for pip and github-actions ecosystems with weekly schedules and grouped minor/patch updates. Add a scheduled workflow to run pre-commit autoupdate weekly and create PRs for hook version bumps.
- Add .github/dependabot.yml for pip and github-actions ecosystems - Add pre-commit autoupdate workflow (weekly, via peter-evans/create-pull-request) - Bump headerkit dependency to >=0.6.1,<1.0.0 - Update pre-commit mypy headerkit pin to v0.6.1
elijahr
marked this pull request as ready for review
September 3, 2026 06:43
Owner
Author
|
/ai-review |
…require-all-libraries option
…d add backend tests
- Update treesitter optional dependencies to include tree-sitter-cpp. - Parametrize C++ header tests across libclang and tree-sitter backends. - Add CLI test for C++ declarations under --backend tree-sitter. - Document tree-sitter backend and installation extra in README.
Owner
Author
|
/ai-review |
Tree-sitter tests were nested inside libclang-marked classes, so the
documented `-m "not libclang"` selector (used by the new Windows CI job)
deselected the backend it exists to exercise. Move the tree-sitter CLI test
into its own treesitter-marked class, demote the class-level libclang markers
in test_realistic_headers to the libclang-only methods, mark the three
test_type_qualifiers classes that call get_backend("libclang") explicitly, and
mark the backend fixture's tree-sitter param so it is selectable with -m.
Replace assertions that could not go red: `len(pxd) > 0` (an extern block with
no declarations compiles cleanly) with per-fixture symbol checks, and
`"No such option" not in result.output` (which passes on exit code 2) with
exit-code and output assertions. --no-recursive and --max-depth have no
observable effect on single-file CLI output; those tests say so rather than
inventing a check. Strengthening --std surfaced that a C++ std without --cpp
fails, now pinned by a test.
Convert the missing-stub skip to an assertion: the stubs now ship in
third-party headerkit under a >= constraint, so a dropped stub must fail.
fmt and doctest carried an unmeasured ">5 minutes" parse claim. Measured on
Apple M4 Pro: fmt/core.h is a 14-line shim parsing in 0.3s, so its skip is
removed; doctest parses in 1.5s but exceeds the 300s timeout in pxd
generation, so its skip stays with the corrected cause and measurement.
Replace the undocumented glob.has_magic with an explicit check, and pin
astral-sh/setup-uv by commit SHA to match the sibling workflow.
The headerkit migration introduced output regressions that were absorbed by editing the expected-output fixtures to match the broken output, while the three gates that should have caught them were each disabled or blind. The underlying defects are fixed in headerkit 0.38.0; this commit repairs the gates, regenerates the fixtures against correct output, and fixes the runtime and documentation defects found alongside them. Test gates: - validate_cython_compiles generated a .pyx containing only `from test cimport *`, so Cython emitted none of the declarations under test -- it compiled the header, not the generated pxd. Cythonizing a golden fixture this way produced 4797 lines of C with zero occurrences of the types it declared. The smoke test is now derived from the pxd's own parse tree, referencing every declared symbol. A deliberately defective pxd now fails where it previously passed. - _SKIP_CYTHON_COMPILATION exempted seven fixtures from the compile gate behind a comment claiming correctness was "still verified" by an assertion that compares against those same fixtures. Deleted; all seven compile. - The golden assertion had been weakened to `actual.strip() + "\n"`, which ignores leading whitespace in a tool whose entire product is whitespace-significant text. Exact equality restored. - Repairing the first gate surfaced three real defects that were shipping silently, now fixed upstream: nested tagged records emitted as incomplete types, a double-pointer function parameter emitting unparseable Cython, and macro extraction emitting valueless and #undef'd macros. Runtime: - --cpp rewrote the emitted header name to input.hpp for any real file named input.h, because the stdin sentinel was detected by name comparison. - --backend auto could never fall back: headerkit assigns its default at registration time, not by availability, so auto always resolved to libclang and exited 1 when it was missing, even with tree-sitter installed. - --std and --clang-arg were accepted and silently discarded on non-libclang backends; they now error. - Whitelist matching compared an absolute pattern against clang's relative location path, so every declaration was filtered out. Both sides are now resolved, glob patterns still work, and the whitelist is forwarded to the backend so included-header declarations are retained rather than dropped before the filter runs. Markers and assertions: - The only CLI tree-sitter test and both tree-sitter fixture tests sat inside libclang-marked classes, so `pytest -m "not libclang"` -- the selector the new Windows job uses -- excluded the backend it exists to exercise. `-m treesitter` selected 0 tests; it now selects 37, and the "not libclang" selection passes. - Eight CLI option classes asserted only that the option parsed. They now assert exit status and observable behavior. - A stub-presence check skipped rather than failed, though the stubs are now a third-party package's contents under an open version constraint. - The fmt skip claimed a >5 minute parse; measured 0.3s against a 14-line deprecation shim, so it is removed. The doctest skip is kept with the measured cause recorded. Packaging, CI and docs: dead package-data and MANIFEST entries for deleted paths, inconsistent headerkit pins, an extra duplicating headerkit's own dependency set, restored Windows and Python 3.10/3.14 coverage, third-party actions pinned by SHA, and documentation claims corrected against measured behavior -- destructors are not emitted, namespaces are supported, and the tree-sitter backend is documented along with its real limitations. Requires headerkit >= 0.38.0. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
headerkit renamed its include filter from `whitelist` to `allowlist` and added `denylist`. The parameter was unreleased, so it carries no alias upstream. - `translate()` and the CLI gain `allowlist`/`--allowlist` and `denylist`/`--denylist`. `whitelist`/`--whitelist` keep working, and `--whitelist` keeps `-w`, but both now emit a DeprecationWarning naming the replacement. Passing an allowlist and a whitelist together is an error rather than a silent precedence guess. `--denylist` takes no short flag so `-w` stays unambiguous. Deny wins over allow where both match, agreeing with headerkit rather than contradicting it. `--quiet` suppresses the CLI deprecation, matching how `--compiler-directive` is already gated. - `_expand_whitelist_for_backend` is deleted. It pre-expanded globs to work around headerkit keeping an included file's declarations only when an entry resolved to a real file; headerkit expands globs itself now, verified end to end before removal. - The post-parse filter is kept and extended to the denylist. Measured rather than assumed: stubbing it to the identity function reddens exactly two tests, both about the root header, because headerkit applies neither list to the declarations of the file it was handed. Passing the allowlist to the backend is also not redundant -- it drives dependency resolution, which the post-filter cannot reproduce. Both layers are load-bearing. - The two tests importing headerkit's removed private `_is_umbrella_header` are deleted. That heuristic gated recursion on a header having at least three non-system includes and fewer than three declarations, so it overruled the explicit `recursive_includes` flag and made a one- or two-include forwarding header produce an empty pxd with no error. Recursion is now controlled by the flag alone, and the docs said otherwise -- they described the threshold as current behavior and are corrected. - test_unused_typedef_not_included asserted an unused typedef from an included header is absent. That minimality still exists, but behind `--no-recursive`/`max_depth=0` rather than by default. Split into two tests so the regression protection is preserved rather than dropped. - Expected-output fixtures regenerated for xnvme_opts, cpp_container, simple_cpp, jansson and zlib. Every changed line was classified against both the pre-migration output and the header source before being written: bitfield width comments, `from libcpp cimport bool`, `const` restored, parameter names on function-pointer typedefs, declarations from included non-system headers, and a reorder that fixes a declare-after-use inversion. No line was accepted merely because the current code produces it. Requires headerkit >= 0.38.0. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
headerkit shipped output-changing behavior and a parameter rename in the 0.38.0 minor release, so an unbounded >= would let a future minor silently change the .pxd files autopxd2 generates. The bound is a range rather than an exact pin because autopxd2 is a library: an exact pin would make it uninstallable alongside other headerkit consumers and block patch fixes. Track uv.lock so the resolution is reproducible and reviewable. The lock is repo-only infrastructure, so it is excluded from the sdist and from check-manifest alongside the other repo-only files.
windows-latest ships LLVM/clang preinstalled, so the libclang parameterizations can run there. Verify clang and that libclang actually loads before the suite, with headerkit.install_libclang as a fallback. Real-header tests still skip on Windows: there is no scripts/install-test-libs-windows.sh, so --require-all-libraries is not passed.
…ortability The header name is embedded in a Cython string literal (cdef extern from "..."), where a backslash starts an escape sequence, so an absolute Windows path was rejected as an invalid \U escape. Test-side Windows fixes: size_t spells as unsigned long long under the LLP64 MSVC target; libclang reports include paths with backslashes; the compile check now uses clang rather than a MinGW cc so its predefined macros match the parse target; and the zlib golden comparison skips with a reason naming zconf.h's _WIN32 branch.
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
headerkit.stubsclang2dependency and replaces with HeaderKit's multi-version clang loaderQuality Gates