[mypyc] Fix Extension.depends empty for fully-cached groups in incremental builds#6
Merged
Merged
Conversation
…ental builds In separate=True mode, when generate_c returns empty cfiles for a group (the fully-cached path — typical of pip's second setup.py invocation), per_cfile_deps was never populated for that group. Extension.depends therefore stayed empty, so cross-group export-table header changes caused by inserting a new class that shifts struct offsets never triggered a recompile of the cached consumer's .o. The stale .o then baked in the old struct offsets, silently resolving them to wrong classes at runtime. Fix: when the on-disk .c file exists for a cached group, read it before calling get_header_deps so the dep resolver can walk the transitive header chain and include cross-group headers in Extension.depends. Also fixes an inconsistent errors="replace" in resolve_cfile_deps (now plain encoding="utf-8" throughout) and adds a test that directly demonstrates the before/after behavior.
There was a problem hiding this comment.
Pull request overview
This PR fixes mypyc incremental build dependency tracking for fully cached compilation groups by reading the existing generated .c file so header dependencies can still populate Extension.depends.
Changes:
- Reads cached on-disk
.cfiles inmypyc_buildwhen no generated C files are returned for a group. - Uses strict UTF-8 decoding consistently in header dependency resolution.
- Adds a regression-oriented test for cached group dependency discovery.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
mypyc/build.py |
Populates dependency metadata from existing cached C files and standardizes file decoding. |
mypyc/test/test_misc.py |
Adds a test scenario for resolving cross-group header dependencies from cached C output. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+197
to
+199
| per_cfile_deps = [ | ||
| (consumer_c, get_header_deps([(os.path.basename(consumer_c), existing_text)])) | ||
| ] |
VaggelisD
added a commit
that referenced
this pull request
May 18, 2026
Fixes mypy var-annotated errors introduced in #6 where iterating over a bare empty list left cfile_full/dep_names untyped.
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.
Summary
In
separate=Truemode, whengenerate_creturns emptycfilesfor a group (the fully-cached path — typical of pip's secondsetup.pyinvocation for the wheel-build phase),per_cfile_depswas never populated for that group.Extension.dependstherefore stayed empty, so cross-group export-table header changes went undetected by setuptools.The concrete failure mode: inserting a new class earlier in a compiled module shifts subsequent members'
export_table_<group>struct offsets. If the consumer group's.ois not recompiled, it bakes in the old offsets, silently resolving them to wrong classes at runtime (e.g.TypeError: Unexpected keyword 'is_string' for UniformSamplewhen constructingUuid).Fix
When the fully-cached path fires and the on-disk
.cfile exists, read it before callingget_header_depssoresolve_cfile_depscan walk the transitive header chain and include cross-group headers inExtension.depends. Setuptools then sees the changed header and recompiles the consumer's.o.Also fixes an inconsistent
errors="replace"inresolve_cfile_deps(now plainencoding="utf-8"throughout, matching the rest of the file).Test
test_cached_group_deps_populated_from_disk_cfiledirectly demonstrates the before/after:get_header_deps([(cfile, "")])returns no includes →Extension.dependsis empty → cross-group header changes undetected.cis read → transitive resolver finds cross-group header → it appears inExtension.depends