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

Support TOML lists in cmake.define #921

Merged
merged 12 commits into from
Oct 2, 2024

Conversation

alexreinking
Copy link
Contributor

Adds support for specifying CMake lists in cmake.define using TOML lists:

[tool.scikit-build.cmake.define]
ONE_LEVEL_LIST = [
    "Foo",
    "Bar",
    "ExceptionallyLargeListEntryThatWouldOverflowTheLine",
    "Baz",
]
NESTED_LIST = [ "Apple", "Lemon;Lime", "Banana" ]

Escapes semicolons in list items so as to allow nested lists.

Fixes #874

@alexreinking alexreinking marked this pull request as draft October 1, 2024 17:03
@alexreinking alexreinking marked this pull request as ready for review October 1, 2024 21:03
@LecrisUT
Copy link
Collaborator

LecrisUT commented Oct 2, 2024

Can you add some docs on the change, particularly regarding the nested list syntax?

## Configuring CMake arguments and defines

Maybe as a :::{versionchanged} 0.11 inside the tab block:

````{tab} pyproject.toml
```toml
[tool.scikit-build.cmake.define]
SOME_DEFINE = "ON"
```
````

Copy link
Collaborator

@henryiii henryiii left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A mention in the docs, and one unit test would be nice, but not required. Thanks! Not sure I would have come up with this solution.

@alexreinking
Copy link
Contributor Author

@henryiii - I added a unit test to test_skbuild_settings.py that checks the lists in packages/cmake_defines/pyproject.toml are converted to the correct strings. I also updated the documentation. This is how I see it rendered locally:

image

@henryiii
Copy link
Collaborator

henryiii commented Oct 2, 2024

I've adjusted the docs style to match the existing style a bit better, otherwise looks fantastic.

@henryiii henryiii enabled auto-merge (squash) October 2, 2024 18:18
@alexreinking
Copy link
Contributor Author

I've adjusted the docs style to match the existing style a bit better, otherwise looks fantastic.

Thanks! Happy to contribute 🙂

@alexreinking
Copy link
Contributor Author

@henryiii -- not sure why one of the CI bots failed. Hoping it's just an intermittent failure. Merged in main to re-run.

@henryiii
Copy link
Collaborator

henryiii commented Oct 2, 2024

Yes, it is. Even with a rebuild PyPy is flaky in pretty much every repo.

@henryiii henryiii merged commit ef07739 into scikit-build:main Oct 2, 2024
63 checks passed
@alexreinking alexreinking deleted the feat/toml-lists branch October 2, 2024 20:51
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.

Allow toml lists in tool.scikit-build.cmake.define
3 participants