Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: Preserve additivity of cmake.define across overrides tables #564

Merged
merged 5 commits into from
Mar 4, 2024

Conversation

stubbiali
Copy link
Contributor

This light-weight PR adds a special rule when overriding cmake.define, so that additivity is retained and orthogonal CMake defines are accommodated. Consider the following TOML excerpt:

[[tool.scikit-build.overrides]]
if.env.SET_FOO = "ON"
cmake.define.FOO = "1"

[[tool.scikit-build.overrides]]
if.env.SET_BAR = "ON"
cmake.define.BAR = "1"

As of now, this would result in cmake.define being {"BAR": "1"}. With the PR, cmake.define will be {"FOO": 1, "BAR": "1"}.

@henryiii
Copy link
Collaborator

henryiii commented Dec 4, 2023

I've been thinking about this (also in cibuildwheel) for quite a while, but since there's no "null" option in TOML, there would be no way to un-set an environment variable once set. It's also inconsistent with how other options work. What I've thought of is having a special character/name to indicate that it's going to be additive. Probably "cmake.define+`?

@henryiii
Copy link
Collaborator

henryiii commented Dec 4, 2023

(Haven't thought through this at all yet, but how about something like {inherit = true} as an item in the list filling in the old version when it's encountered?) (Inspired a bit by the design of https://peps.python.org/pep-0735/)

Ehh, for some reason I was thinking this was a list. Is there an invalid environment variable name we could use as a key, though? "inherit+" = true, for example?

@stubbiali
Copy link
Contributor Author

What about '+cmake.define.FOO' = "1"? Of course, parsing of the keys will be needed. But this approach could be applied to cmake.args too, to append items to the list.

@stubbiali
Copy link
Contributor Author

The logic to support the syntax '+cmake.args' and '+cmake.define' has been implemented.

@henryiii
Copy link
Collaborator

henryiii commented Jan 2, 2024

(This is moving forward slowly, I'll get back to you once we are happy on a design! I've brought it up with the rest of the cibuildwheel devs at pypa/cibuildwheel#1716)

@henryiii
Copy link
Collaborator

If people like pypa/cibuildwheel#1730, we can do the same thing here.

@stubbiali
Copy link
Contributor Author

stubbiali commented Jan 29, 2024

Speaking on behalf of the team behind GHEX: we like the approach pursued in pypa/cibuildwheel#1730, and the end result will perfectly serve our needs, so green light from our side. If needed, I can gladly offer some help.

@henryiii
Copy link
Collaborator

henryiii commented Feb 3, 2024

There's some question about the final term ("inherit" vs. "extend", opinions welcome on that issue), but it's mostly finalized. I can look at adding it here too.

Signed-off-by: Henry Schreiner <henryschreineriii@gmail.com>
@henryiii henryiii merged commit e6ae46d into scikit-build:main Mar 4, 2024
47 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants