feat: add inherit to override

[henryiii]

This implements my follow-up suggestion in #1716, modified. Example:

[tool.cibuildwheel]
environment = {FOO="BAR", "HAM"="EGGS"}
test-command = ["pyproject"]

[[tool.cibuildwheel.overrides]]
select = "cp311*"
inherit.test-command = "append"
inherit.environment = "append"

test-command = ["pyproject-override"]
environment = {FOO="BAZ", "PYTHON"="MONTY"}

This example will provide the command ["pyproject", "pyproject-override"] on
Python 3.11, and will have environment = {FOO="BAZ", "PYTHON"="MONTY", "HAM"="EGGS"}. Both "append" and "prepend" are supported ("none" is the default). You can even chain them to both append and prepend in matching override blocks.

[joerick]

This is cool! I'll look at the implementation in a bit, but on the syntax I have a thought, apologies I didn't manage to respond in the discussion.

Thinking about that syntax, I feel there's maybe something a little weird aesthetically about passing option names as quoted values, how about this?

[tool.cibuildwheel]
environment = {FOO="BAR", "HAM"="EGGS"}
test-command = ["pyproject"]

[[tool.cibuildwheel.overrides]]
select = "cp311*"
inherit.test-command = true
test-command = ["pyproject-append"]
inherit.environment = true
environment = {FOO="BAZ", "PYTHON"="MONTY"}

That avoids the quotes, but functionally it's still very similar. What do you think?

---

(I suppose according to the TOML rules you could also do

[tool.cibuildwheel]
environment = {FOO="BAR", "HAM"="EGGS"}
test-command = ["pyproject"]

[[tool.cibuildwheel.overrides]]
select = "cp311*"
inherit = {test-command=true, environment=true}
test-command = ["pyproject-append"]
environment = {FOO="BAZ", "PYTHON"="MONTY"}

)

[henryiii]

If we are setting it equal to something, then what about = "append"? That would open the possibility in the future of having different modes (like "prepend").

[henryiii]

(If this looks reasonable from a design perspective, I'll start implementing it in scikit-build-core. It goes pretty well with the way if. and if.any. are implemented there, too.)

[joerick]

Yeah, I like your idea of doing the append / prepend thing too! I'm +1 on this.

The only slight question mark in my mind is the word 'inherit' in the context of 'append'/'prepend'.

Thinking out loud-

inherit.test-command = "append"
merge.test-command = "prepend"
extend.test-command = "append"
combine.test-command = "prepend"

I think I still land on inherit actually. Curious to what you think.

[henryiii]

I already considered merge. Let me bring those four options up at our scikit-build developers meeting tomorrow and see if there are any thoughts there. I'm still slightly in favor of inherit, but let's see.

[thewtex]

2 cents 💭 inherit suggests object-oriented programming to me, and there could be a side effect other than setting the value. I do associate extend with changing values in a list-like variable.

[henryiii]

(@thewtex was in our scikit-build meeting mentioned above). I'd say I'm even on inherit and extend. extend sounds a bit better for a list and a bit worse for a table. @joerick happy to have you decide.

[joerick]

Thanks. I feel that inherit makes more sense. extend.test-command = "append" doesn't really say anything about where we're extending from, whereas inherit.test-command = "append" implies more strongly that the 'other' value is coming from a previous configuration.

[joerick]

Nice one henryiii, I think this approach is good, I have some thoughts on how to simplify the implementation though. I'm working on a few changes now...

[joerick]

I'm guessing one or the other of these is right?

[henryiii]

Suggested change

	schema["$id"] = "https://json.schemastore.org/partial-cibuildwheel.json"
	schema["$id"] = "http://json-schema.org/draft-07/schema#"
	schema["$id"] = "https://json.schemastore.org/partial-cibuildwheel.json"
	schema["$schema"] = "http://json-schema.org/draft-07/schema#"

😳

[joerick]

Hi Henry, apologies for barging in on your PR, I hope you don't mind. I've been playing around with different options here as you can see.

Main changes from how you had it:

• refactored the code to make the option cascade more explicit
• changed the logic around option stringification to make it possible to inherit from a string value specified elsewhere
• added a few tests, changed a few internal APIs to make things clearer

I think I'm nearly happy with it. Curious to hear your thoughts.

My only remaining question is whether it would be useful to specify inherit at the top level too?

The one reason I can see somebody wanting this would be something like:

[tool.cibuildwheel]
inherit.repair-wheel-command = "prepend"
repair-wheel-command = "pipx run abi3audit --strict --report {wheel}"

That inherits the default value that we set, but also includes the user value. (we actually include an example of this usage in the docs, but it duplicates the default config on each platform!)

[henryiii]

Sounds good. Inheriting top level is probably something I won't add to scikit-build-core, but I could see it making a bit of sense here. Would it make sense for anything other than repair-wheel-command? I think it's the only one with a default. But I could see it being useful there, yes.

[joerick]

Would it make sense for anything other than repair-wheel-command? I think it's the only one with a default.

The only other one I can see being useful, is if we ever change the default CIBW_BUILD to exclude certain platforms e.g. EOL pythons - one could then inherit the default but then add some extras.

The other thing I think we should add is the ability to inherit from [tool.cibuildwheel] into [tool.cibuildwheel.macos/linux/windows]. I can see this being useful for config-settings, environment, etc.

[joerick]

And of course, we need to document it.

One thing that's bugging me, I feel that when we're documenting, the line inherit.test-command = true is clearer in what it means (enabling inheritance at this level) than inherit.test-command = "append", which, while offering more functionality, is forcing the user to think about two concepts at the same time. I think it grates on me because inheritance and 'appending' are not concepts that naturally fit together, it's not clear what appending means in the context of inheritance.

As such I'd be in favour of allowing true (an alias for 'append'), documenting that as the 'normal' way, and making the append/prepend keywords more of a power-feature for ultimate customisability.

Let me know what you think. I know you're trying to keep things in parity between this and scikit-build, so things might be more complicated on your side!

[henryiii]

I'm not very fond of allowing multiple variations like that. It makes it much harder for users to discover "prepend", and it doesn't clarify how this is merging with the previous lists/tables. It's also messier from the schema standpoint - IDE's would offer "append", "prepend", and "none" makes more sense then having it also offer true/false. People are just going to be copying examples; let's not introduce confusion about the differences between five different options when we really only are offering three. If we started with true/false, then added the three strings, we'd be stuck with it, but let's do the three strings only now.

Maybe the problem is the name? "merge" and "extend" maybe have less of a class with "append"? Does either extend.test-command = "append" or merge.test-command = "append" read better?

Or are "append/prepend" the problem? I think we've only looked at those two, but "before"/"after" I suppose are also options.

[joerick]

People are just going to be copying examples; let's not introduce confusion about the differences between five different options when we really only are offering three.

This is a strong argument. I've been all round the houses on the names again... This was an interesting one that Gemini came up with...

---

Modify

Key: modify.test-command
Values: append, prepend, replace (default)

e.g.

[tool.mytool]
test-command = ["bin/mytests", "bin/unittests"]

[[tool.mytool.overrides]]
select = "*-macos"
modify.test-command = "append"
test-command = ["bin/mactests"]

---

But I still feel that 'inherit' just pips it for me due to the clear link to the previous configuration, and 'append' and 'prepend' are the clearest, so we should probably just leave those as-is. (Apologies for the back-and-forth).

[henryiii]

Should the inherit for platform options be a followup? I'm thinking we can rework the platform options as overrides (as they are really just a special case of overrides anyway).

[joerick]

I think the other inherits should fit into this current structure pretty well, I can take a look tomorrow if you like.

Did you have any thoughts about how to document it? I guess a little extension to Options / Configuration overrides?

[henryiii]

I'm working on docs now. I'm not sure it makes sense to add inheritance to the other areas, though; once key feature of inheritance is that you can use it twice and both append and prepend. But that's only true if you have an array; overrides is an array.

So, for example, right now you can already both append and prepend to cibuildwheel's defaults:

[[tool.cibuildwheel.overrides]]
select = "*"
inherit.repair-wheel-command = "prepend"
repair-wheel-command = "echo 'Before repair'"

[[tool.cibuildwheel.overrides]]
select = "*"
inherit.repair-wheel-command = "append"
repair-wheel-command = "echo 'After repair'"

You can also modify the commands per-OS since OS-specific settings are a subset of overrides. I think extending the classic settings would get into confusing areas, like if you specify "append" for a platform-specific option, but then you use an environment variable for that platform specific option - does that now permanently append? Since you can't specify overrides via environment variable, you get to skip this currently. (IMO, the main reason the platform specific options are still useful post overrides is due to the environment variables).

[henryiii]

Added some basic docs. I think we should stick with it just for overrides, at least for now, and see how it does and if people need or expect more. We can always add later, but we can't take away later.

[joerick]

Good points regarding non-override inherits. I hadn't considered the use of a select = "*" for extending a default. I also think it makes it easier to document, if it's just a feature of overrides.

Okay, agreed, let's run without more inherits for now. I'll take a look at the docs tomorrow.

[joerick]

Great! this looks good-to-go to me!

[joerick]

🚀🚀

[henryiii]

changed the logic around option stringification to make it possible to inherit from a string value specified elsewhere

This broke table entries (see #1803). Tables now append or prepend their values together into a new "multi-key" table, rather than their items. This breaks config-settings, which changes the type from a string to a list if you have multiple matching keys. -Ck=a -Ck=b is the same as the toml k = ["a", "b"]. I think the environment table is unaffected, since setting an environment value twice ends up just taking the last one. But it breaks config-settings.

Any ideas on a fix? The new structure makes it hard to do correctly. An environment variable with "k=A k=B" for a list was supported before, and probably still should be. Longer term I think going scikit-build-core's way of making a declarative data class-based config is much cleaner and statically nicer. But that's a big enough project I've avoided it so far.

[joerick]

Thanks for the ping. Yes, I see the issue. I think I was too focused on environment when making the stringification decision. It feels like perhaps we need a different option merge strategy for config-settings versus environment.

My initial idea for a fix would be to modify _merge_values:

cibuildwheel/cibuildwheel/options.py

Lines 218 to 240 in 291921a

	def _merge_values(before: str | None, after: str, rule: InheritRule, merge_sep: str | None) -> str:
	if rule == InheritRule.NONE:
	return after
	if not before:
	# if before is None, we can just return after
	# if before is an empty string, we shouldn't add any separator
	return after
	if not after:
	# if after is an empty string, we shouldn't add any separator
	return before
	if not merge_sep:
	msg = f"Don't know how to merge {before!r} and {after!r} with {rule}"
	raise ConfigOptionError(msg)
	if rule == InheritRule.APPEND:
	return f"{before}{merge_sep}{after}"
	elif rule == InheritRule.PREPEND:
	return f"{after}{merge_sep}{before}"
	else:
	assert_never(rule)

So that it knows if it's doing a simple merge (current behaviour), or if it needs to eliminate overridden keys.

However, I think that you're right, it's not a small fix because the intermediate representation is always strings. Two routes here, I suppose:

1. go back to a mixed intermediate representation - i.e. setting values in the cascade can be str, or list, or dict, not just str. Then we can perform merges before stringification, if the values are compatible (although I think I'd rather keep environment behavour as-is, so maybe there's a mode to always stringify for that option)
2. figure out a way to get the _merge_values behavour we want while the values are still strings. I suspect we'd have to add some fields to TableFmt to make the stringification bidirectional.

I'd probably lean towards (1), but it's hard to judge without seeing both solutions in code and seeing the ramifications.

I'd be happy to take the work on (it's quite an interesting job to me) but I'm not sure when I'll get time, I'm just about to head on a work trip to the US for a 10 days.

[joerick]

One thing that would be worth discussing is if we let users control the behaviour via a different inherit setting e.g. merge rather than append for this style. That would have the benefit that we could document merge only works when the settings are TOML tables, not strings.

[henryiii]

I'd prefer to keep this inline with scikit-build-core, where it will never convert a string to a list because you "appended" them. We also don't support "append" on string-only items; you don't know how to merge two arbitrary strings, only tables and lists.

I think it's fine if we require that inherit only works when users use tables/lists, and it would be fine to error out if they use raw strings if that makes it easier. And environment merging can use the config-settings behavior, it's just config-settings that can't use the current behavior that does happen to work on environment variables.

[joerick]

And environment merging can use the config-settings behavior

There was a use-case for the stringification behaviour that I neglected to mention. (I thought that I had, but actually I just made a unit test for it! apologies)

[tool.cibuildwheel]
environment = {PATH="/opt/bin:$PATH"}

[[tool.cibuildwheel.overrides]]
select = "cp37*"
inherit.environment = "append"
environment = {PATH="/opt/local/bin:$PATH"}

This has the nice behaviour that in the second environment assignment $PATH refers to the previously defined variable. That lets the overrides build on each other.

But of course this is the wrong behaviour for the config-settings option (and perhaps other dict-based options). Did you ship inherit rules in scikit-build-core yet? I do feel that 'append' is the most natural name for this string-style inheriting, but that there might be a different name for the list/dict style (e.g. 'extend'/'pre-extend'). But I'm happy to be swayed if you have strong opinions.

[henryiii]

inherit shipped about a week ago. I think it's fine if we handle environment and config-settings differently based on how they are used. I'd consider config-settings the correct behavior and environment a special case to allow this special usage.