-
Notifications
You must be signed in to change notification settings - Fork 98
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
format cmake files #975
format cmake files #975
Conversation
This uses https://github.com/cheshirekow/cmake_format @starseeker see if you have any comments. The issue now is that I don't have a nice way to check for cmake format in the CI. |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #975 +/- ##
==========================================
- Coverage 91.84% 88.21% -3.63%
==========================================
Files 37 62 +25
Lines 4976 8671 +3695
Branches 0 1044 +1044
==========================================
+ Hits 4570 7649 +3079
- Misses 406 1022 +616 ☔ View full report in Codecov by Sentry. |
@pca006132 https://github.com/marketplace/actions/cmake-format-lint might be useful here? I'm not 100% sure, but it looks as if they may be checking for differences as a result of formatting. Their code is at https://github.com/neg-c/cmake-format-action |
Did you check https://github.com/BlankSpruce/gersemi as well? It's got a --diff option that might be handy for checking for changes. |
Or actually, in gersemi's case, the -c option might be exactly what we want. |
hmmm I can try that |
yeah gersemi looks better, and easier to integrate with CI |
src/CMakeLists.txt
Outdated
@@ -113,31 +114,24 @@ if(MANIFOLD_EXPORT) | |||
endif() | |||
target_compile_features(manifold PUBLIC cxx_std_17) | |||
if(MANIFOLD_PAR) | |||
target_compile_options(manifold INTERFACE -DMANIFOLD_PAR) | |||
target_compile_options(manifold PUBLIC -DMANIFOLD_PAR) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is an interesting one, INTERFACE
will make the compile option only available to external code linking with our library, so it is not actually using parallelization. Changing to public fixes that.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
and this makes me thinking if we should always set these to some non-zero values and assert the thing should be non-zero in our code, to avoid this sort of issue (and potential typos, which we had before)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Urk. That's my mistake - I was misunderstanding the PUBLIC vs. INTERFACE settings. I thought PRIVATE was just for the target, PUBLIC was for all other targets in the current CMake project linking that target, and INTERFACE meant everything... I see now that is incorrect, apologies.
For future reference (mostly for me) here's the link to CMake's docs on those three keywords:
https://cmake.org/cmake/help/latest/manual/cmake-buildsystem.7.html#target-command-scope
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@pca006132 Were you thinking something like this for asserting non-zero for parallel values? starseeker@c94e8a2
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@starseeker maybe a bit simpler, 1 for true and -1 for false. This will work for other macros as well.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@pca006132 Updated. Pull request up: #980
@starseeker regarding the |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks!
I think I was conflating that with some discussions I recollected about issues building it as a static library. You can go ahead and remove that TODO. |
Sure, we can fix that when we fix our documentation in another clean up. |
Use
cmake-format
for formatting, and changed some of the oldset
calls tolist(APPEND ...)
which should be nicer.