-
Notifications
You must be signed in to change notification settings - Fork 50
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: add header dir to target #37
Conversation
Would you mind rebasing this pull request? Maybe we can get a maintainer to merge it. |
CMakeLists.txt
Outdated
@@ -28,6 +28,7 @@ set(STRING_ENCODING_TYPE "ICONV" CACHE STRING "Set the way strings have to be en | |||
|
|||
add_library (${PROJECT_NAME} SHARED ${HEADERS} ${SOURCES}) | |||
set_target_properties(${PROJECT_NAME} PROPERTIES PUBLIC_HEADER "kaitai/kaitaistream.h;kaitai/kaitaistruct.h") | |||
target_include_directories(${PROJECT_NAME} 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.
Can you explain why you believe that CMAKE_CURRENT_SOURCE_DIR
is the right variable to use here?
Some people use CMAKE_CURRENT_LIST_DIR
in this context instead, see https://stackoverflow.com/a/40242257/12940655. And the official CMake tutorial uses PROJECT_SOURCE_DIR
, see https://cmake.org/cmake/help/latest/guide/tutorial/Adding%20a%20Library.html#build-and-run:
Finally we need to specify the library's header file location. Modify
target_include_directories()
to add theMathFunctions
subdirectory as an include directory so that theMathFunctions.h
header file can be found.TODO 4: CMakeLists.txt
target_include_directories(Tutorial PUBLIC "${PROJECT_BINARY_DIR}" "${PROJECT_SOURCE_DIR}/MathFunctions" )
Intuitively, PROJECT_SOURCE_DIR
sounds most predictable and robust to me and it's probably what we actually want (given that the official tutorial uses it too). The word "current" in CMAKE_CURRENT_*
variable names suggests that the contents of these variables may change under some circumstances (which might be some other CMake way to include kaitai_struct_cpp_stl_runtime
to someone else's application, I don't really know) and that could break things.
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.
Honestly I have never seen PROJECT_SOURCE_DIR
in the wild. If it works then great. Like with everything in Cmake there is 1e6 different ways to do the same thing.
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.
In this case, all three are equivalent. How I understand it:
CMAKE_CURRENT_SOURCE_DIR
is the source directory being processed. It is "current" because that can change withadd_subdirectory
;CMAKE_CURRENT_LIST_DIR
is the directory containing the current CMake file. In this case, the listing is in the source dir;PROJECT_SOURCE_DIR
is the source directory from the last call toproject
.
So, IMO using either CMAKE_CURRENT_SOURCE_DIR
or PROJECT_SOURCE_DIR
would be sensible (and equivalent) choices, but I do agree that PROJECT_SOURCE_DIR
may be clearer.
I don't have anything to test it on anymore given this was 3 years ago. |
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.
@russkel @LHLaurini Thank you both!
This simplifies use of the runtime in CMake projects. With this mod it's now possible to simply do:
Before I was getting this error, due to it not finding the includes correctly.