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:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
