[inherit-extend] refactor: validate inherit rules eagerly, more standard naming - #2967
Open
henryiii wants to merge 2 commits into
Open
[inherit-extend] refactor: validate inherit rules eagerly, more standard naming#2967henryiii wants to merge 2 commits into
henryiii wants to merge 2 commits into
Conversation
- Validate inherit rule keys against the known option names, with did-you-mean suggestions; a missing colon or a typo in CIBW_INHERIT is now an error instead of a silent no-op. - Require a single token per rule; convert shlex ValueError and non-string TOML values to OptionsReaderError instead of tracebacks. - Parse overrides and CIBW_INHERIT eagerly in OptionsReader.__init__ so malformed config fails fast on every code path. - Convert OptionFormat.NotSupported to OptionsReaderError when an append/prepend rule hits a non-mergeable option. - Filter the xbuild-tools NUL sentinel out of merged lists. - Scope platform env rules with CIBW_INHERIT_<PLATFORM> instead of a -<platform> suffix inside CIBW_INHERIT keys; CIBW_INHERIT rules also apply to CIBW_<OPTION>_<PLATFORM> unless overridden. - Fold parse_arbitrary_key_value_string into parse_key_value_string. - Derive the schema's inherit keys from the option list and give each platform/overrides section its own restricted inherit table. Assisted-by: ClaudeCode:claude-fable-5
henryiii
changed the title
henryiii
changed the title
Assisted-by: ClaudeCode:claude-fable-5 Signed-off-by: Henry Schreiner <henryfs@princeton.edu>
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.
This is based on an AI review of #2928. @joerick, I can trim out any changes you don't like. The simplification pass was based on comparisons with main, so some parts of the diff simplify the total change more than the diff in this PR itself.
CIBW_INHERIT_<PLATFORM>does seem to be more natural, do you like it better?The failing CI is because the target branch needs rebasing.
🤖 AI text below 🤖
Follow-ups from a review of #2928, targeting
inherit-extend.Validation.
parse_inheritnow validates rule keys against the known option names (with "Perhaps you meant ...?" suggestions), requires a single token per rule, and converts shlexValueError/ non-string TOML values intoOptionsReaderError. Overrides andCIBW_INHERITare parsed eagerly inOptionsReader.__init__again, so malformed config fails fast even on--print-build-identifiers. An append/prepend rule on a non-mergeable option (e.g.container-engine) now raises a clean error instead of a bareOptionFormat.NotSupportedtraceback, and an inherit rule onxbuild-toolsno longer leaks the"\u0000"sentinel default into the tool list.Interface change (worth a look). Platform-scoped env rules now use
CIBW_INHERIT_<PLATFORM>(matching theCIBW_<OPTION>_<PLATFORM>convention) instead of a-<platform>suffix insideCIBW_INHERITkeys — with key validation, the suffix form would have collided with option-name checking, andCIBW_INHERIT_LINUXis what users will try first anyway.CIBW_INHERITrules also apply to the platform variables unless a platform rule overrides them. Docs updated.Schema.
generate_schema.pyderives theinheritkeys from the option list instead of a hand-copied block (it was missingenable,archs, etc.), and each platform/overrides section gets an inherit table restricted to the options that section can set.Cleanup.
parse_arbitrary_key_value_stringis now a thin wrapper overparse_key_value_stringrather than a near-copy of it.Regression tests were added for each fix and confirmed to fail beforehand.
One open question: the runtime also accepts the string form (
inherit = "before-all: append") in TOML, but the docs and schema only describe the table form there. Left as-is; happy to restrict or document it either way.