cmake: set CAPNP_INCLUDE_DIR #1071
Conversation
|
The windows CI checks fail for some reason that seems unrelated (test executables don't get built) and I have too little experience to be able to debug it |
c++/src/capnp/CMakeLists.txt
Outdated
| @@ -168,6 +168,7 @@ if(NOT CAPNP_LITE) | |||
| set_target_properties(capnp_tool PROPERTIES CAPNP_INCLUDE_DIRECTORY | |||
| $<JOIN:$<BUILD_INTERFACE:${CMAKE_CURRENT_SOURCE_DIR}/..>,$<INSTALL_INTERFACE:${CMAKE_INSTALL_BINDIR}/..>> | |||
| ) | |||
| set_target_properties(capnp_tool PROPERTIES COMPILE_FLAGS -DCAPNP_INCLUDE_DIR='"${CMAKE_INSTALL_FULL_INCLUDEDIR}"') | |||
There was a problem hiding this comment.
nitpick: Seems like this should be indented to match the surrounding lines?
There was a problem hiding this comment.
ahh ugly, sorry, am I blind or did vim betray me few minutes before push?
|
The Cygwin test failure is expected (it has... bitrotted), but the other failures seem to be caused by this change. |
p4l1ly
force-pushed
the
includedir
branch
4 times, most recently
from
6a82675
to
538cb7b
Compare
|
Can you please make this change without changing the format of the macro? Otherwise this breaks people who are already customizing the macro in their own builds. |
|
The problem is that I cannot figure out how to pass the double quotes to Anyway, I think that requiring double quotes around the argument is quite weird and as it moreover violates platform independency, it might be a good idea to remove it. The version is still unstable... |
|
ok, the escaping |
p4l1ly
force-pushed
the
includedir
branch
2 times, most recently
from
c585c3b
to
46abd32
Compare
c++/src/capnp/CMakeLists.txt
Outdated
| @@ -168,6 +168,7 @@ if(NOT CAPNP_LITE) | |||
| set_target_properties(capnp_tool PROPERTIES CAPNP_INCLUDE_DIRECTORY | |||
| $<JOIN:$<BUILD_INTERFACE:${CMAKE_CURRENT_SOURCE_DIR}/..>,$<INSTALL_INTERFACE:${CMAKE_INSTALL_BINDIR}/..>> | |||
| ) | |||
| set_target_properties(capnp_tool PROPERTIES COMPILE_FLAGS -DCAPNP_INCLUDE_DIR="\\\"${CMAKE_INSTALL_FULL_INCLUDEDIR}\\\"") | |||
There was a problem hiding this comment.
Any reason to use COMPILE_FLAGS over COMPILE_DEFINITIONS?
The latter seems more appropriate, and I think the escaping syntax will be lighter:
set_target_properties(capnp_tool PROPERTIES
COMPILE_DEFINITIONS CAPNP_INCLUDE_DIR=\"${CMAKE_INSTALL_FULL_INCLUDEDIR}\")I don't know if the capnproto installation is relocatable today, but using CMAKE_INSTALL_FULL_XXX in the binary may harm relocatability of the package.
Unfortunately, I think the solution to this is more complex.
To do so, the binary should be able to locate it's own installation prefix at runtime, which is platform specific.
There was a problem hiding this comment.
Thank you for the hint, I didn't know about COMPILE_DEFINITIONS.
As for the relocatability, the installation was not relocatable before in the context of default include paths. Actually, the default include paths were hardcoded to /usr/include and /usr/local/include. The proposed change is not the ideal solution but is a bit more dynamic than before. The installation cannot be relocated but it can be at least compiled and installed locally with the correct default include paths. It was not possible before if using cmake.
There was a problem hiding this comment.
Yeah, this makes the cmake build match what the autotools build has done for a long time, which seems like an improvement.
Making the binary relocatable was discussed a bit in #1062, but it's not completely obvious what the right approach is.
|
@harrishancock Can you verify this still looks good to you and, if so, feel free to merge? |
when installing capnp to a non-standard location (out of
/usror/usr/local), thecapnp compilemust be issued with the-Ioption even for the standard/capnp/c++.capnpimport. Some projects that usecapnp compiledirectly, not via cmake (e.g. capnproto-java) cannot be built in this configuration.