-
Notifications
You must be signed in to change notification settings - Fork 102
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
cmake changes #1048
cmake changes #1048
Conversation
@@ -31,20 +34,6 @@ set( | |||
"${MANIFOLD_VERSION_MAJOR}.${MANIFOLD_VERSION_MINOR}.${MANIFOLD_VERSION_PATCH}" | |||
) | |||
|
|||
# Correct MANIFOLD_PAR values to on/off (previous NONE/TBB values should still |
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.
We will be having a new release, so better remove the compatibility code here and start fresh.
CMakeLists.txt
Outdated
@@ -111,34 +96,23 @@ if(MANIFOLD_FUZZ) | |||
# enable fuzztest fuzzing mode | |||
set(FUZZTEST_FUZZING_MODE ON) | |||
# address sanitizer required | |||
set(CMAKE_CXX_FLAGS "${CMAKE_CXX_FLAGS} -fsanitize=address") | |||
add_compile_options("-fsanitize=address") |
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.
I think using add_compile_options
has the same effect as modifying CMAKE_CXX_FLAGS
? And this looks cleaner.
CMakeLists.txt
Outdated
endif() | ||
|
||
if(EMSCRIPTEN) | ||
message("Building for Emscripten") | ||
add_link_options("-s ALLOW_MEMORY_GROWTH=1") |
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.
Similarly, add_link_options
seems to be cleaner than using CMAKE_EXE_LINKER_FLAGS
.
if(CMAKE_SYSTEM_NAME STREQUAL "Windows") | ||
if(CMAKE_CXX_COMPILER_ID STREQUAL "GNU") | ||
list(APPEND WARNING_FLAGS -Wno-format) | ||
endif() | ||
else() | ||
elseif(PROJECT_IS_TOP_LEVEL) |
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.
We only do -Werror
when we are not the top level project.
@@ -7,13 +20,15 @@ include_directories( | |||
|
|||
add_library( | |||
manifoldc | |||
SHARED |
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.
I'm thinking maybe rust users will want to build the library statically and link it, so we shouldn't force this as a shared library.
bindings/c/CMakeLists.txt
Outdated
manifoldc.cpp | ||
conv.cpp | ||
box.cpp | ||
cross.cpp | ||
rect.cpp | ||
) | ||
add_library(manifold::manifoldc ALIAS manifoldc) |
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.
Namespace alias so users can just do manifold::manifold
/manifold::manifoldc
when they use our library, regardless of whether it is a system install or subdirectory.
) | ||
target_compile_options(manifoldc PRIVATE ${MANIFOLD_FLAGS}) | ||
target_compile_features(manifoldc PRIVATE cxx_std_17) |
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.
Because we have the global CMAKE_CXX_STANDARD
, this is now redundant.
bindings/python/CMakeLists.txt
Outdated
include(FetchContent) | ||
logmissingdep("nanobind" , "MANIFOLD_PYBIND") |
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.
Make sure we will not do download when MANIFOLD_DOWNLOAD
is disabled.
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 I thought of moving this to cmake/manifoldDeps.cmake
, but not sure if that is the best thing to do...
src/CMakeLists.txt
Outdated
set( | ||
MANIFOLD_INCLUDE_DIRS | ||
${PROJECT_SOURCE_DIR}/include | ||
$<$<BOOL:${TBB_INCLUDE_DIRS}>:${TBB_INCLUDE_DIRS}> |
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 now uses generator expressions to reduce the number of lines. Not sure if this is what everyone likes, but I feel this is now somewhat easier to read.
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #1048 +/- ##
==========================================
- Coverage 91.84% 84.69% -7.15%
==========================================
Files 37 62 +25
Lines 4976 9685 +4709
Branches 0 1050 +1050
==========================================
+ Hits 4570 8203 +3633
- Misses 406 1482 +1076 ☔ View full report in Codecov by Sentry. |
This looks like the last PR for v3.0 - agreed? I don't know enough CMake to review this one particularly, but it certainly looks to be going in a good direction. How much feedback do you want to wait for before we merge? And after all the commits, do you still feel this is a preliminary attempt? |
Changes here are mostly internal, except the target alias that is already covered by #1049. I also changed Also, this includes things from #1045, which is probably not that ready for a release. I am fine for merging this after v3.0, and get some feedback from the users, who have more experience working with cmake than I do. |
And I feel like merging a 1.5k line refactor a few days before a major version release is a bit too risky for my liking :P (Though technically the changes are quite trivial.) |
list( | ||
APPEND | ||
WARNING_FLAGS | ||
-Wall | ||
-Wno-unknown-warning-option | ||
-Wno-unused | ||
-Wno-shorten-64-to-32 | ||
) |
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.
Are these compiler flags now supported across all supported versions of (gcc and clang) compilers?
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.
I think the most important flag is -Wno-unknown-warning-option
, it allows us to use some other warning options not supported by the compiler. We should check when gcc and clang supports this...
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 seems to be a clang only flag, which is supported at least 13 years ago (https://bugzilla.mozilla.org/show_bug.cgi?id=731316).
GCC will not complain about unknown warning options anyway:
When an unrecognized warning option is requested (e.g., -Wunknown-warning), GCC emits a diagnostic stating that the option is not recognized. However, if the -Wno- form is used, the behavior is slightly different: no diagnostic is produced for -Wno-unknown-warning unless other diagnostics are being produced. This allows the use of new -Wno- options with old compilers, but if something goes wrong, the compiler warns that an unrecognized option is present.
These flags should be fine.
One think that I'm uncertain how to deal with:
This can be solved by never using builtins, but it feels wrong the way it's done today. Not sure how easy it is to work around this, either from within manifold or inside the target build environment. |
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.
So, it looks like we want to merge this for v3.0 - but I have a few questions first:
src/parallel.h
Outdated
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.
I thought you had put this in include so OpenSCAD could use our parallel functions. Did something change?
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.
Yes this was the plan, but later I found that I can just use tbb functions directly there. Having this as a public header will require us to make tbb a public dependency, which is problematic when we build the fetched dependency as a static library...
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.
Okay, sounds good to me - the less in our public includes the better, as far as I'm concerned.
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.
I wasn't plugged in this weekend, so it's too late for 3.0, but IIRC iters.h was only public in the headers for parallel.h so as long as that's still true we can make that header private too for the next round.
Would be great if someone with mac can check if our build already does that. @elalish @kintel |
I have a mac, but I'm not sure what to check. Also, I'm going to bed. |
@hzeller I moved the |
@elalish I guess we can wait for kintel to test that :) |
SGTM, thanks! |
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.
LGTM, though I'm no CMake expert. I'll go update our required tests so merging isn't blocked.
# library conflict. | ||
# When the system package is unavailable, the option will be automatically set | ||
# to true. | ||
option(MANIFOLD_USE_BUILTIN_TBB "Use builtin tbb" OFF) |
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.
Users can choose not to use the system version of the dependencies. I saw this in the openscad wishlist, so I added it.
Exported library and binary files are now placed under Configuration info is now logging the full set of compile and link options for manifold for better debugging... |
Look good from here,
|
I think this should be ready to merge? |
cmake/version.h.in
Outdated
|
||
#define MANIFOLD_VERSION_MAJOR @MANIFOLD_VERSION_MAJOR@ | ||
#define MANIFOLD_VERSION_MINOR @MANIFOLD_VERSION_MINOR@ | ||
#define MANIFOLD_VERSION_PATH @MANIFOLD_VERSION_PATCH@ |
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.
Typo: PATH -> PATCH
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.
Ups, I take full responsibility for that typo...
Re. compile time version. It could be nice with a single integer version number in addition. I can find examples from other libs if needed for inspiration |
I think this should be merged soon, as it is getting big and allows folks to easier play with it, and final polish-ups can be done in a follow-up. |
Examples (all taken from the OpenSCAD codebase):
|
Okay, I fixed the typo, so I'm going to go ahead and merge this. I'm not quite sure how you want a single version number int exposed, so I'll wait for a PR from @kintel or @hzeller for that. When it sounds like things have settled, I'll cut v3.0. Please let me know - I'd like to avoid an immediate 3.0.1 if possible. |
I have put the comparable version number suggestion in #1054 |
if(MANIFOLD_EXPORT) | ||
list(APPEND MANIFOLD_PUBLIC_HDRS include/manifold/meshIO.h) | ||
endif() | ||
list(TRANSFORM MANIFOLD_PUBLIC_HDRS PREPEND include/manifold/) |
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.
Slick! That's a new one for me.
${PROJECT_SOURCE_DIR}/src/sort.cpp | ||
${PROJECT_SOURCE_DIR}/src/cross_section/cross_section.cpp | ||
${PROJECT_SOURCE_DIR}/src/polygon.cpp | ||
${PROJECT_SOURCE_DIR}/include/manifold/common.h |
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.
Ah, good catch. Using CMAKE_SOURCE_DIR is a habit I have left from the old days, looks like PROJECT_SOURCE_DIR is probably what one would want most of the time.
I was looking at the changes required to better support including this project as a subdirectory in a cmake project, and went down the rabbit hole of changing stuff...
This PR is very radical, and I do not expect everything to be merged as-is. This is mostly for testing in the CI as well as gather some feedback from the users.
@starseeker @kintel will be nice if you can provide some feedback. I will comment on things that are a bit more interesting than moving things around.