You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@qpid.apache.org by GitBox <gi...@apache.org> on 2021/05/14 16:58:20 UTC
[GitHub] [qpid-proton] astitcher commented on a change in pull request #312: This seems to work to allow linking with static Proton from Dispatch from within CMake
astitcher commented on a change in pull request #312:
URL: https://github.com/apache/qpid-proton/pull/312#discussion_r632580725
##########
File path: c/CMakeLists.txt
##########
@@ -487,16 +492,18 @@ if (BUILD_STATIC_LIBS)
target_compile_definitions(qpid-proton-static PUBLIC PROTON_DECLARE_STATIC)
target_link_libraries (qpid-proton-static
qpid-proton-core-static
- $<$<BOOL:${HAS_PROACTOR}>:qpid-proton-proactor-static>
+ ${TARTET_qpid-proton-proactor-static}
Review comment:
What problem does this change solve? The generator works well with 2.8.12 already AFAIK.
Also spelling as above.
##########
File path: c/CMakeLists.txt
##########
@@ -456,6 +458,9 @@ if (qpid-proton-proactor)
add_library (qpid-proton-proactor-static STATIC ${qpid-proton-proactor})
target_compile_definitions(qpid-proton-proactor-static PUBLIC PROTON_DECLARE_STATIC)
target_link_libraries (qpid-proton-proactor-static ${PLATFORM_LIBS} ${PROACTOR_LIBS})
+ set_target_properties(qpid-proton-proactor-static PROPERTIES
+ POSITION_INDEPENDENT_CODE ON)
+ set(TARTET_qpid-proton-proactor-static qpid-proton-proactor-static)
Review comment:
Spelling TARTET->TARGET?
##########
File path: c/CMakeLists.txt
##########
@@ -456,6 +458,9 @@ if (qpid-proton-proactor)
add_library (qpid-proton-proactor-static STATIC ${qpid-proton-proactor})
target_compile_definitions(qpid-proton-proactor-static PUBLIC PROTON_DECLARE_STATIC)
target_link_libraries (qpid-proton-proactor-static ${PLATFORM_LIBS} ${PROACTOR_LIBS})
+ set_target_properties(qpid-proton-proactor-static PROPERTIES
+ POSITION_INDEPENDENT_CODE ON)
Review comment:
As above I don't think this should be PIC for a static library.
##########
File path: c/CMakeLists.txt
##########
@@ -557,11 +564,25 @@ if(HAS_PROACTOR)
configure_lib(PROTONPROACTORLIB qpid-proton-proactor)
endif(HAS_PROACTOR)
+# CMake is happy to generate a lot of the *config.cmake file for us, see
+# https://gitlab.kitware.com/cmake/cmake/-/issues/19560
+# https://cmake.org/cmake/help/latest/manual/cmake-packages.7.html#creating-packages
+# https://cmake.org/cmake/help/latest/module/CMakePackageConfigHelpers.html#example-generating-package-files
Review comment:
No need for this comment here - put it in the commit message.
Just make sure that the generated files are substitutable for the current one in all cases. I think they should be the only reason they are not use is because cmake earlier than 2.8.12 didn't support this as well.
##########
File path: c/CMakeLists.txt
##########
@@ -487,16 +492,18 @@ if (BUILD_STATIC_LIBS)
target_compile_definitions(qpid-proton-static PUBLIC PROTON_DECLARE_STATIC)
target_link_libraries (qpid-proton-static
qpid-proton-core-static
- $<$<BOOL:${HAS_PROACTOR}>:qpid-proton-proactor-static>
+ ${TARTET_qpid-proton-proactor-static}
${SSL_LIB} ${SASL_LIB} ${TIME_LIB} ${PLATFORM_LIBS} ${PROACTOR_LIBS})
+ set_target_properties(qpid-proton-static PROPERTIES
+ POSITION_INDEPENDENT_CODE ON)
endif(BUILD_STATIC_LIBS)
# Install executables and libraries
if (BUILD_STATIC_LIBS)
set(STATIC_LIBS qpid-proton-static qpid-proton-core-static)
endif()
install(TARGETS qpid-proton qpid-proton-core ${STATIC_LIBS}
- EXPORT proton
+ EXPORT ProtonTargets
Review comment:
No objection, I just can't remember what this is for - can you say what the EXPORT directive does?
##########
File path: c/CMakeLists.txt
##########
@@ -419,6 +419,8 @@ if (BUILD_STATIC_LIBS)
add_library (qpid-proton-core-static STATIC ${qpid-proton-core-src})
target_compile_definitions(qpid-proton-core-static PUBLIC PROTON_DECLARE_STATIC)
target_link_libraries (qpid-proton-core-static ${SSL_LIB} ${SASL_LIB} ${PLATFORM_LIBS})
+ set_target_properties(qpid-proton-core-static PROPERTIES
+ POSITION_INDEPENDENT_CODE ON)
Review comment:
I think this is probably wrong. You should *not* be building a dynamic library from static library parts.
This implies that the final dispatch router executable should be statically linked into qdrouter.
Ie: static qpid-proton-core static qpid-proton-proactor -> static qpid-dispatch -> qdrouter
Mixing static and dynamic libs to build a library is IMO a really bad idea just waiting for build problems. Also if you statically link all the way you've a much better chance to get something like LTO working for a much bigger part of the final executable.
##########
File path: c/CMakeLists.txt
##########
@@ -487,16 +492,18 @@ if (BUILD_STATIC_LIBS)
target_compile_definitions(qpid-proton-static PUBLIC PROTON_DECLARE_STATIC)
target_link_libraries (qpid-proton-static
qpid-proton-core-static
- $<$<BOOL:${HAS_PROACTOR}>:qpid-proton-proactor-static>
+ ${TARTET_qpid-proton-proactor-static}
${SSL_LIB} ${SASL_LIB} ${TIME_LIB} ${PLATFORM_LIBS} ${PROACTOR_LIBS})
+ set_target_properties(qpid-proton-static PROPERTIES
+ POSITION_INDEPENDENT_CODE ON)
Review comment:
PIC as above
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@qpid.apache.org
For additional commands, e-mail: dev-help@qpid.apache.org