-
Notifications
You must be signed in to change notification settings - Fork 311
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
[SofaExporter] FIX: out-of-tree include of SofaExporter header files #975
[SofaExporter] FIX: out-of-tree include of SofaExporter header files #975
Conversation
[ci-build][with-all-tests] |
|
@@ -8,4 +8,6 @@ if(NOT TARGET SofaExporter) | |||
include("${CMAKE_CURRENT_LIST_DIR}/SofaExporterTargets.cmake") | |||
endif() | |||
|
|||
# set(@PROJECT_NAME@_INCLUDE_DIRS @CMAKE_CURRENT_SOURCE_DIR@/src/) |
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.
commented ?
@@ -99,7 +99,9 @@ target_compile_options(${PROJECT_NAME} PRIVATE "-DSOFA_BUILD_CIMGPLUGIN") | |||
target_link_libraries(${PROJECT_NAME} SofaCore ${EXTERNAL_LIBS}) | |||
target_include_directories(${PROJECT_NAME} PUBLIC "$<BUILD_INTERFACE:${CMAKE_CURRENT_BINARY_DIR}>/include") | |||
target_include_directories(${PROJECT_NAME} PUBLIC "$<BUILD_INTERFACE:${CMAKE_CURRENT_SOURCE_DIR}>") | |||
target_include_directories(${PROJECT_NAME} PUBLIC "$<BUILD_INTERFACE:${CMAKE_CURRENT_SOURCE_DIR}/..>") |
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.
Not sure this is needed.
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 it kind of is:
There's .h & .cpp files present directly in CImgPlugin dir, so if, from external code, you want to include CImgPlugin.h for instance, by typing: #include <CImgPlugin/CImgPlugin.h>
you'd have to include ../ no?
@guparan: Looks like it's used also that way in the SOFA codebase. How should I proceed? should I put back the include of |
Let me know if my latest commit broke something. |
Trying to include the SofaExporter module in an external project (out-of-tree build of a plugin || test) was not functional since include directories were not exposed properly.
I took inspiration from CImgPlugin for this PR, which uses CMAKE_CURRENT_SOURCE_DIR to expose the target's include directory.. but I believe this is actually wrong as this corresponds to the path to the sources (
/home/bruno/dev/sofa/modules/SofaExporter/src/
in my case for instance) and not to the installed include directory, (which should be/home/bruno/dev/sofa/build/install/include/SofaExporter/src/SofaExporter
on my machine)I took a look at other plugins & pluginized modules & they all seem to do this, so Idk if it's a mistake or done on purpose..?
This PR:
Reviewers will merge only if all these checks are true.