Add support for EGADS geometry system #330
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,33 @@ | ||
| # - Try to find EGADS | ||
| # Once done this will define | ||
| # EGADS_FOUND - System has EGADS | ||
| # EGADS_INCLUDE_DIRS - The EGADS include directories | ||
| # EGADS_LIBRARIES - The libraries needed to use EGADS/EGADSlite | ||
| # EGADSLITE_LIBRARIES - The libraries needed to use EGADSlite | ||
| # EGADS_DEFINITIONS - Compiler switches required for using EGADS | ||
|
|
||
| find_path(EGADS_INCLUDE_DIR egads.h PATHS "${EGADS_DIR}/include") | ||
| set(EGADS_INCLUDE_DIRS ${EGADS_INCLUDE_DIR}) | ||
|
|
||
| if(${PUMI_USE_EGADSLITE}) | ||
| find_library(EGADS_LIBRARY | ||
| NAMES egadslite libegadslite.so libegadslite.dylib | ||
| PATHS "${EGADS_DIR}/lib") | ||
| else() | ||
| find_library(EGADS_LIBRARY | ||
| NAMES egads libegads.so libegads.dylib | ||
| PATHS "${EGADS_DIR}/lib") | ||
| endif() | ||
|
|
||
| set(EGADS_LIBRARIES ${EGADS_LIBRARY}) | ||
|
|
||
| include(FindPackageHandleStandardArgs) | ||
| # handle the QUIETLY and REQUIRED arguments and set EGADS_FOUND to TRUE | ||
| # if all listed variables are TRUE | ||
| find_package_handle_standard_args( | ||
| EGADS | ||
| DEFAULT_MSG | ||
| EGADS_INCLUDE_DIR EGADS_LIBRARY | ||
| ) | ||
|
|
||
| mark_as_advanced(EGADS_INCLUDE_DIR EGADS_LIBRARY) | ||
tuckerbabcock marked this conversation as resolved.
Show resolved
Hide resolved
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,43 @@ | ||
| if(DEFINED TRIBITS_PACKAGE) | ||
| include(pkg_tribits.cmake) | ||
| return() | ||
| endif() | ||
|
|
||
| if(NOT ENABLE_EGADS) | ||
| return() | ||
| endif() | ||
|
|
||
| # this file brings PUMI_USE_EGADSLITE from CMake to C++ | ||
| configure_file("${CMAKE_CURRENT_SOURCE_DIR}/gmi_egads_config.h.in" | ||
| "${CMAKE_CURRENT_BINARY_DIR}/gmi_egads_config.h") | ||
|
There was a problem hiding this comment. @jacobmerson this config file only defines There was a problem hiding this comment.
An example of its use is here: Line 159 in 58a8832 There was a problem hiding this comment. @cwsmith would you prefer the target be the interface target For my above thought on having There was a problem hiding this comment. I spent some more time looking at the code and have a hopefully more complete response. IIUC, the user needs to know if PUMI was installed with EGADS and/or EGADSLite support and To answer your questions:
There was a problem hiding this comment. @cwsmith I've switched over all the EGADS stuff to use |
||
|
|
||
| #Sources & Headers | ||
| set(SOURCES gmi_egads.c) | ||
| set(HEADERS gmi_egads.h) | ||
|
|
||
| add_library(gmi_egads ${SOURCES}) | ||
|
|
||
| target_include_directories(gmi_egads | ||
| PRIVATE | ||
| ${EGADS_INCLUDE_DIR} | ||
| ) | ||
|
|
||
| target_link_libraries(gmi_egads | ||
| PUBLIC | ||
| gmi | ||
| PRIVATE | ||
| ${EGADS_LIBRARIES} | ||
| ) | ||
|
|
||
| # Include directories | ||
| target_include_directories(gmi_egads PUBLIC | ||
| $<BUILD_INTERFACE:${CMAKE_CURRENT_SOURCE_DIR}> | ||
| $<INSTALL_INTERFACE:include> | ||
| ) | ||
| # make sure the compiler can find the config header | ||
| target_include_directories(gmi_egads PRIVATE | ||
| $<BUILD_INTERFACE:${CMAKE_CURRENT_BINARY_DIR}>) | ||
|
|
||
| scorec_export_library(gmi_egads) | ||
|
|
||
| bob_end_subdir() | ||
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.
To add compiler definitions you should be
target_compile_definitions. Since you use this option in the header it will need to be public. Because this leaks into the user's downstream code you should use a prefixed name. I.e.PUMI_HAVE_EGADSThere 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 for taking another look at this @jacobmerson. I wrote the
-DHAVE_EGADSdefinition to be consistent with the simmetrix definition:core/CMakeLists.txt
Lines 97 to 99 in 30a332d
Based on the way that
HAVE_SIMMETRIXis used, it seems convenient to useadd_definitionsso that it applies to all the executables so that multiple calls totarget_compile_definitionscan be skipped, but I am aware that this approach is considered bad practice in modern cmake. In any case I thought it would be best to stay consistent, though I'm not tied to the current approach.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 would prefer that all of those other
HAVE_XXXdefinitions get prefixed eventually and you have a valid point about consistency. The problem with adding compile definitions without a namespace is that any other consuming project linking againstpumi_egadshas that compile definition on their target. So, what if a consumer wants to use PUMI compiled with egads but they have conditional code behindHAVE_EGADSthat they don't want to compile for some reason...currently they cannot do that since compile definitions are infectious. I think @cwsmith has last word on this sort of thing since he has to deal with all the xsdk people/rules.The issue with the non-target version of
add_definitions(especially in the top level cmake file) is that it will be applied to every single target and library at the current or lower sub-directory level. So, even random libraries that have nothing to do with egads are getting that compile definition added. Since these compile definitions will be infectious to anyone who uses any SCOREC target it has the potential to cause issues for down stream users.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.
@cwsmith do you have any thoughts on the concerns raised by @jacobmerson?
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.
@jacobmerson is correct. We should have all the
*HAVE*,*ENABLE*, etc... variables prefixed withPUMI_to avoid conflicts with packages that use PUMI. +1 for usingtarget_compile_definitionsas well.I created an issue to clean this up in
developafter this is merged. #337