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: auto cmake version #804

Merged
merged 8 commits into from
Jul 15, 2024
Merged

feat: auto cmake version #804

merged 8 commits into from
Jul 15, 2024

Conversation

henryiii
Copy link
Collaborator

@henryiii henryiii commented Jul 11, 2024

Close #777.

This is on by default for 0.10+. Setting "CMakeLists.txt" explicitly though will force it to be found, while the default will fallback on 3.15+.

@henryiii henryiii force-pushed the henryiii/feat/autocmakever branch 3 times, most recently from 5ac758c to e47052f Compare July 11, 2024 04:32
Copy link

codecov bot commented Jul 11, 2024

Codecov Report

Attention: Patch coverage is 85.33333% with 22 lines in your changes missing coverage. Please review.

Project coverage is 82.98%. Comparing base (054e0cc) to head (43d135e).
Report is 74 commits behind head on main.

Files with missing lines Patch % Lines
...cikit_build_core/settings/skbuild_read_settings.py 70.00% 9 Missing ⚠️
src/scikit_build_core/ast/ast.py 87.09% 8 Missing ⚠️
src/scikit_build_core/ast/tokenizer.py 91.11% 4 Missing ⚠️
...c/scikit_build_core/settings/auto_cmake_version.py 90.00% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main     #804      +/-   ##
==========================================
+ Coverage   82.90%   82.98%   +0.08%     
==========================================
  Files          69       73       +4     
  Lines        3926     4074     +148     
==========================================
+ Hits         3255     3381     +126     
- Misses        671      693      +22     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@henryiii
Copy link
Collaborator Author

I think I'll try this as default for 0.10+. I think the only way to break the parsing is to have something like

something([=[
cmake_minimum_required(VERSION 3.99)
]=]
)
cmake_minimum_required(VERSION 3.15)

Which is really weird, and would have to be above the first min required one to break it anyway. I'd have to write a tokenizer to fix this. But if someone does have something really weird, they can just set cmake.version and we won't look anymore.

@LecrisUT
Copy link
Collaborator

LecrisUT commented Jul 11, 2024

Wait, what about comments and comment blocks? E.g. in a TODO comment

#[=[ TODO:
Support newer version of CMake, blocked by ...
cmake_minimum_required(VERSION 3.99)
]=]

cmake_minimum_required(VERSION 3.15)

@henryiii
Copy link
Collaborator Author

That should be handled, # at the front won't match this. IIRC the only multiline structure is a [=[ string.

@henryiii
Copy link
Collaborator Author

Ahh, forgot about block comments. That would confuse it, yes.

@henryiii
Copy link
Collaborator Author

And if statements could confuse it, including if(FALSE), etc.

@henryiii henryiii force-pushed the henryiii/feat/autocmakever branch from 833a61d to c146250 Compare July 12, 2024 16:35
@henryiii
Copy link
Collaborator Author

I've added a simple streaming tokenizer and AST for CMake, this allows block comments as well as blocks (like if, foreach, macro) to be ignored. The first top-level statement is taken if found.

@henryiii henryiii force-pushed the henryiii/feat/autocmakever branch 3 times, most recently from 73313dd to 204a646 Compare July 12, 2024 21:40
@LecrisUT
Copy link
Collaborator

Oh, Fedora CI caught this that needs to be migrated

[tool.scikit-build]
ninja.minimum-version = "1.10"
cmake.minimum-version = "3.17.2"

Incidentally, probably this section needs a -Werror

rlRun "pip install --user . --config-settings=cmake.verbose=true --no-index --no-build-isolation" 0 "Build the python project"

Was wondering why the nox tests were not picking it up, don't they lack --no-build-issolation?

session.install(".", "--config-settings=cmake.verbose=true", "pytest")

"BRACKET_QUOTE": r"\[(?P<bq1>=*)\[(?s:.)*?\](?P=bq1)\]",
"OPEN_PAREN": r"\(",
"CLOSE_PAREN": r"\)",
"LEGACY": r'\b\w+=[^\s"()$\\]*(?:"[^"\\]*"[^\s"()$\\]*)*|"(?:[^"\\]*(?:\\.[^"\\]*)*)*"',
Copy link
Collaborator

Choose a reason for hiding this comment

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

Didn't know about this legacy part. Couldn't find the documentation of it and its depreciation/removal. Is it compatible with the minimum CMake that scikit-build-core requires?

Just for navigability, can the bracket section (and maybe other parts) be extracted out of the dict and introduced in as raw f-strings?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

https://cmake.org/cmake/help/latest/manual/cmake-language.7.html#unquoted-argument

I think it's in the "not really recommended for new code" stage.

I've downloaded every CMakeLists.txt from PyPI, so I have 35,000 test cases. I haven't quite finished, but it's passing about 10,000 before it hits its first failure currently. I sometimes have to skip a file because it's not valid. (One literally was a C++ file named CMakeLists.txt, for example).

Copy link
Collaborator Author

@henryiii henryiii Jul 14, 2024

Choose a reason for hiding this comment

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

Found one more small issue, now passes 32,000+ files (parsing the whole thing without crashing). Had to skip 20 files, mostly templates, or other clearly broken files. https://github.com/henryiii/pystats/blob/main/cmakelists-proc.py

Note that this a streaming tokenizer and AST generator, so if cmake_minimum_required is found, the rest of the file is not processed.

@henryiii henryiii force-pushed the henryiii/feat/autocmakever branch 2 times, most recently from 9bf8240 to 8a1aced Compare July 14, 2024 05:19
@henryiii
Copy link
Collaborator Author

Was wondering why the nox tests were not picking it up

Those don't have warnings as errors, and also yes, I think it just grabs a pre-built version. I'm trying to fix that in #810.

@henryiii henryiii force-pushed the henryiii/feat/autocmakever branch from 26faa75 to 43d135e Compare July 15, 2024 15:55
@henryiii henryiii merged commit c299548 into main Jul 15, 2024
51 checks passed
@henryiii henryiii deleted the henryiii/feat/autocmakever branch July 15, 2024 16:22
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.

feat: auto
2 participants