PengZheng commented on code in PR #524: URL: https://github.com/apache/celix/pull/524#discussion_r1175144174
########## libs/framework/gtest/src/BundleArchiveWithErrorInjectionTestSuite.cc: ########## @@ -27,9 +27,9 @@ #include "asprintf_ei.h" //extern declarations for testing purposes. Note signatures are not correct, but that is not important for the test. -extern "C" celix_status_t bundleArchive_create(void); +extern "C" celix_status_t celix_bundleArchive_create(void); extern "C" celix_status_t manifest_create(void); -extern "C" celix_status_t bundleRevision_create(void); +extern "C" celix_status_t celix_bundleRevision_create(void); Review Comment: Now that the new celix api is used, why not just include corresponding headers? ########## libs/etcdlib/CMakeLists.txt: ########## @@ -54,32 +55,39 @@ endif () if (CELIX_ETCDLIB OR ETCDLIB_STANDALONE) find_package(jansson REQUIRED) - add_library(etcdlib SHARED - src/etcd.c - ) - target_include_directories(etcdlib PUBLIC - $<BUILD_INTERFACE:${CMAKE_CURRENT_LIST_DIR}/api> - ) - target_include_directories(etcdlib PRIVATE src) - set_target_properties(etcdlib PROPERTIES SOVERSION 1) - set_target_properties(etcdlib PROPERTIES VERSION 1.0.0) + add_library(etcdlib SHARED src/etcd.c) + target_include_directories(etcdlib PUBLIC $<BUILD_INTERFACE:${CMAKE_CURRENT_LIST_DIR}/api>) + target_include_directories(etcdlib PRIVATE src) + set_target_properties(etcdlib + PROPERTIES + VERSION 1.0.0 + SOVERSION 1 + C_VISIBILITY_PRESET hidden + ) target_link_libraries(etcdlib PUBLIC CURL::libcurl jansson::jansson ${CELIX_OPTIONAL_EXTRA_LIBS}) - add_library(etcdlib_static STATIC - src/etcd.c - ) - target_include_directories(etcdlib_static PUBLIC - $<BUILD_INTERFACE:${CMAKE_CURRENT_LIST_DIR}/api> - ) + generate_export_header(etcdlib + BASE_NAME "ETCDLIB" + EXPORT_FILE_NAME "${CMAKE_BINARY_DIR}/celix/gen/includes/etcdlib/etcdlib_export.h") + target_include_directories(etcdlib PUBLIC $<BUILD_INTERFACE:${CMAKE_BINARY_DIR}/celix/gen/includes/etcdlib>) + + + add_library(etcdlib_static STATIC src/etcd.c) + target_include_directories(etcdlib_static PUBLIC $<BUILD_INTERFACE:${CMAKE_CURRENT_LIST_DIR}/api>) target_include_directories(etcdlib_static PRIVATE src) - set_target_properties(etcdlib_static PROPERTIES "SOVERSION" 1) + target_include_directories(etcdlib_static PUBLIC $<BUILD_INTERFACE:${CMAKE_BINARY_DIR}/celix/gen/includes/etcdlib>) + set_target_properties(etcdlib_static + PROPERTIES + POSITION_INDEPENDENT_CODE 1 Review Comment: It is unnecessary. I think what we really need (but missing) is `target_compile_definitions(ETCDLIB PRIVATE ETCDLIB_STATIC_DEFINE)` Professional CMake describes this situation using an example: ```cmake # Same source list, different library types add_library(MyShared SHARED ${mySources}) add_library(MyStatic STATIC ${mySources}) # Shared target used for generating export header # with the name mytools_export.h, which will be suitable # for both the shared and static targets include(GenerateExportHeader) generate_export_header(MyShared BASE_NAME MyTools) # Static target needs special preprocessor define # to prevent symbol import/export keywords being added target_compile_definitions(MyStatic PRIVATE MYTOOLS_STATIC_DEFINE ) ``` ########## libs/framework/src/bundle_revision_private.h: ########## @@ -60,7 +62,7 @@ struct bundleRevision { * - CELIX_SUCCESS when no errors are encountered. * - CELIX_ENOMEM If allocating memory for <code>bundle_revision</code> failed. */ -celix_status_t bundleRevision_create(celix_framework_t* fw, const char *root, const char *location, manifest_pt manifest, bundle_revision_pt *bundle_revision); +CELIX_FRAMEWORK_EXPORT celix_status_t celix_bundleRevision_create(celix_framework_t* fw, const char *root, const char *location, manifest_pt manifest, bundle_revision_pt *bundle_revision); Review Comment: Use object target for unit test instead of export private symbol. ########## libs/framework/src/celix_framework_utils_private.h: ########## @@ -20,17 +20,20 @@ #ifndef CELIX_FRAMEWORK_UTILS_PRIVATE_H_ #define CELIX_FRAMEWORK_UTILS_PRIVATE_H_ +#include <time.h> #include "celix_framework_utils.h" - -#include <time.h> +#include "celix_framework_export.h" #ifdef __cplusplus extern "C" { #endif /** * @brief Checks whether the provided bundle url is newer than the provided time. + * + * @note Symbol export is required for unit testing. Review Comment: Using object target for unit test should be enough. ########## libs/framework/CMakeLists.txt: ########## @@ -36,29 +36,80 @@ set(SOURCES src/celix_framework_utils.c src/celix_module_private.h) add_library(framework_obj OBJECT ${SOURCES}) -target_include_directories(framework_obj PUBLIC - $<BUILD_INTERFACE:${CMAKE_CURRENT_LIST_DIR}/include> -) -target_include_directories(framework_obj PRIVATE ${CMAKE_CURRENT_LIST_DIR}/include_deprecated) -target_compile_options(framework_obj PRIVATE -DUSE_FILE32API) -target_compile_options(framework_obj PRIVATE -Wno-deprecated-declarations) #note part of the api is deprecated, ignore this warning on own api -target_link_libraries(framework_obj PUBLIC Celix::utils ${CELIX_OPTIONAL_EXTRA_LIBS}) -target_link_libraries(framework_obj PUBLIC libuuid::libuuid CURL::libcurl ZLIB::ZLIB) -target_link_libraries(framework_obj PRIVATE ${CMAKE_DL_LIBS}) -celix_deprecated_utils_headers(framework_obj) -add_library(framework SHARED) -target_link_libraries(framework PUBLIC framework_obj) -set_target_properties(framework PROPERTIES OUTPUT_NAME "celix_framework") -set_target_properties(framework PROPERTIES "SOVERSION" ${CELIX_MAJOR}) +set(CELIX_FRAMEWORK_CMAKE_CONFIGURE_ALT_OPTION ON) Review Comment: Object target when installed is nothing more than an interface target carrying useful information for the shared library target. Thus it does not make sense to have export header for the object target. ########## libs/utils/CMakeLists.txt: ########## @@ -45,44 +45,106 @@ add_library(utils_obj OBJECT src/celix_file_utils.c src/celix_convert_utils.c ${MEMSTREAM_SOURCES} - ) -target_link_libraries(utils_obj PRIVATE libzip::libzip) -if (NOT OPEN_MEMSTREAM_EXISTS) - target_compile_definitions(utils_obj PUBLIC -DCELIX_UTILS_NO_MEMSTREAM_AVAILABLE) -endif () +) -if (ANDROID) - target_compile_definitions(utils_obj PRIVATE -DUSE_FILE32API) -endif () +set(CELIX_UTILS_CMAKE_CONFIGURE_ALT_OPTION ON) Review Comment: Same remarks for framework apply here. ########## libs/framework/src/bundle_archive_private.h: ########## @@ -41,9 +41,9 @@ extern "C" { /** * @brief Create bundle archive. - * + * @note Symbol exported for testing purposes. */ -celix_status_t bundleArchive_create(celix_framework_t* fw, const char *archiveRoot, long id, const char *location, bundle_archive_pt *bundle_archive); +CELIX_FRAMEWORK_EXPORT celix_status_t celix_bundleArchive_create(celix_framework_t* fw, const char *archiveRoot, long id, const char *location, bundle_archive_pt *bundle_archive); Review Comment: We could use the object library for unit test. Thus it should be unnecessary. ########## libs/framework/src/bundle_private.h: ########## @@ -53,11 +54,14 @@ celix_status_t celix_bundle_createFromArchive(celix_framework_t *framework, bundle_archive_pt archive, celix_bundle_t **bundleOut); /** - * Get the bundle archive. + * @brief Get the bundle archive. + * + * @note Symbol export is needed for unit tests. + * * @param[in] bundle The bundle. * @return The bundle archive. */ -bundle_archive_t *celix_bundle_getArchive(const celix_bundle_t *bundle); +CELIX_FRAMEWORK_EXPORT bundle_archive_t *celix_bundle_getArchive(const celix_bundle_t *bundle); Review Comment: Let's use object target for unit test and remove it. ########## libs/framework/CMakeLists.txt: ########## @@ -36,29 +36,80 @@ set(SOURCES src/celix_framework_utils.c src/celix_module_private.h) add_library(framework_obj OBJECT ${SOURCES}) -target_include_directories(framework_obj PUBLIC - $<BUILD_INTERFACE:${CMAKE_CURRENT_LIST_DIR}/include> -) -target_include_directories(framework_obj PRIVATE ${CMAKE_CURRENT_LIST_DIR}/include_deprecated) -target_compile_options(framework_obj PRIVATE -DUSE_FILE32API) -target_compile_options(framework_obj PRIVATE -Wno-deprecated-declarations) #note part of the api is deprecated, ignore this warning on own api -target_link_libraries(framework_obj PUBLIC Celix::utils ${CELIX_OPTIONAL_EXTRA_LIBS}) -target_link_libraries(framework_obj PUBLIC libuuid::libuuid CURL::libcurl ZLIB::ZLIB) -target_link_libraries(framework_obj PRIVATE ${CMAKE_DL_LIBS}) -celix_deprecated_utils_headers(framework_obj) -add_library(framework SHARED) -target_link_libraries(framework PUBLIC framework_obj) -set_target_properties(framework PROPERTIES OUTPUT_NAME "celix_framework") -set_target_properties(framework PROPERTIES "SOVERSION" ${CELIX_MAJOR}) +set(CELIX_FRAMEWORK_CMAKE_CONFIGURE_ALT_OPTION ON) +if (CELIX_FRAMEWORK_CMAKE_CONFIGURE_ALT_OPTION) + function (celix_configure_framework_target) + list(GET ARGN 0 FRAMEWORK_TARGET_NAME) -celix_deprecated_utils_headers(framework) + target_include_directories(${FRAMEWORK_TARGET_NAME} + PRIVATE + ${CMAKE_CURRENT_LIST_DIR}/include_deprecated + PUBLIC + $<BUILD_INTERFACE:${CMAKE_CURRENT_LIST_DIR}/include> + $<BUILD_INTERFACE:${CMAKE_BINARY_DIR}/celix/gen/includes/${FRAMEWORK_TARGET_NAME}> + $<INSTALL_INTERFACE:include/celix/framework>) + target_compile_options(${FRAMEWORK_TARGET_NAME} PRIVATE -DUSE_FILE32API) + target_compile_options(${FRAMEWORK_TARGET_NAME} PRIVATE -Wno-deprecated-declarations) #note part of the api is deprecated, ignore this warning on own api + target_link_libraries(${FRAMEWORK_TARGET_NAME} PUBLIC Celix::utils ${CELIX_OPTIONAL_EXTRA_LIBS}) + target_link_libraries(${FRAMEWORK_TARGET_NAME} PUBLIC libuuid::libuuid CURL::libcurl ZLIB::ZLIB) + target_link_libraries(${FRAMEWORK_TARGET_NAME} PRIVATE ${CMAKE_DL_LIBS}) + set_target_properties(${FRAMEWORK_TARGET_NAME} PROPERTIES C_VISIBILITY_PRESET hidden) + celix_deprecated_utils_headers(${FRAMEWORK_TARGET_NAME}) + + generate_export_header(${FRAMEWORK_TARGET_NAME} + BASE_NAME "CELIX_FRAMEWORK" + EXPORT_FILE_NAME "${CMAKE_BINARY_DIR}/celix/gen/includes/${FRAMEWORK_TARGET_NAME}/celix_framework_export.h") + endfunction () + + celix_configure_framework_target(framework_obj) + + add_library(framework SHARED $<TARGET_OBJECTS:framework_obj>) + celix_configure_framework_target(framework) + set_target_properties(framework + PROPERTIES + "VERSION" "${CELIX_MAJOR}.${CELIX_MINOR}.${CELIX_MICRO}" + "SOVERSION" ${CELIX_MAJOR} + OUTPUT_NAME "celix_framework") +else () + target_include_directories(framework_obj PUBLIC + $<BUILD_INTERFACE:${CMAKE_CURRENT_LIST_DIR}/include> + $<INSTALL_INTERFACE:include/celix/framework> Review Comment: Theoretically we should not need it, because no one will use this pure interface target. `INCLUDES DESTINATION` in `install` already carries the equivalent information. ########## libs/framework/CMakeLists.txt: ########## @@ -36,29 +36,80 @@ set(SOURCES src/celix_framework_utils.c src/celix_module_private.h) add_library(framework_obj OBJECT ${SOURCES}) -target_include_directories(framework_obj PUBLIC - $<BUILD_INTERFACE:${CMAKE_CURRENT_LIST_DIR}/include> -) -target_include_directories(framework_obj PRIVATE ${CMAKE_CURRENT_LIST_DIR}/include_deprecated) -target_compile_options(framework_obj PRIVATE -DUSE_FILE32API) -target_compile_options(framework_obj PRIVATE -Wno-deprecated-declarations) #note part of the api is deprecated, ignore this warning on own api -target_link_libraries(framework_obj PUBLIC Celix::utils ${CELIX_OPTIONAL_EXTRA_LIBS}) -target_link_libraries(framework_obj PUBLIC libuuid::libuuid CURL::libcurl ZLIB::ZLIB) -target_link_libraries(framework_obj PRIVATE ${CMAKE_DL_LIBS}) -celix_deprecated_utils_headers(framework_obj) -add_library(framework SHARED) -target_link_libraries(framework PUBLIC framework_obj) -set_target_properties(framework PROPERTIES OUTPUT_NAME "celix_framework") -set_target_properties(framework PROPERTIES "SOVERSION" ${CELIX_MAJOR}) +set(CELIX_FRAMEWORK_CMAKE_CONFIGURE_ALT_OPTION ON) +if (CELIX_FRAMEWORK_CMAKE_CONFIGURE_ALT_OPTION) + function (celix_configure_framework_target) + list(GET ARGN 0 FRAMEWORK_TARGET_NAME) -celix_deprecated_utils_headers(framework) + target_include_directories(${FRAMEWORK_TARGET_NAME} + PRIVATE + ${CMAKE_CURRENT_LIST_DIR}/include_deprecated + PUBLIC + $<BUILD_INTERFACE:${CMAKE_CURRENT_LIST_DIR}/include> + $<BUILD_INTERFACE:${CMAKE_BINARY_DIR}/celix/gen/includes/${FRAMEWORK_TARGET_NAME}> + $<INSTALL_INTERFACE:include/celix/framework>) + target_compile_options(${FRAMEWORK_TARGET_NAME} PRIVATE -DUSE_FILE32API) + target_compile_options(${FRAMEWORK_TARGET_NAME} PRIVATE -Wno-deprecated-declarations) #note part of the api is deprecated, ignore this warning on own api + target_link_libraries(${FRAMEWORK_TARGET_NAME} PUBLIC Celix::utils ${CELIX_OPTIONAL_EXTRA_LIBS}) + target_link_libraries(${FRAMEWORK_TARGET_NAME} PUBLIC libuuid::libuuid CURL::libcurl ZLIB::ZLIB) + target_link_libraries(${FRAMEWORK_TARGET_NAME} PRIVATE ${CMAKE_DL_LIBS}) + set_target_properties(${FRAMEWORK_TARGET_NAME} PROPERTIES C_VISIBILITY_PRESET hidden) + celix_deprecated_utils_headers(${FRAMEWORK_TARGET_NAME}) + + generate_export_header(${FRAMEWORK_TARGET_NAME} + BASE_NAME "CELIX_FRAMEWORK" + EXPORT_FILE_NAME "${CMAKE_BINARY_DIR}/celix/gen/includes/${FRAMEWORK_TARGET_NAME}/celix_framework_export.h") + endfunction () + + celix_configure_framework_target(framework_obj) + + add_library(framework SHARED $<TARGET_OBJECTS:framework_obj>) + celix_configure_framework_target(framework) + set_target_properties(framework + PROPERTIES + "VERSION" "${CELIX_MAJOR}.${CELIX_MINOR}.${CELIX_MICRO}" + "SOVERSION" ${CELIX_MAJOR} + OUTPUT_NAME "celix_framework") +else () + target_include_directories(framework_obj PUBLIC + $<BUILD_INTERFACE:${CMAKE_CURRENT_LIST_DIR}/include> + $<INSTALL_INTERFACE:include/celix/framework> + ) + target_include_directories(framework_obj PRIVATE ${CMAKE_CURRENT_LIST_DIR}/include_deprecated) + target_compile_options(framework_obj PRIVATE -DUSE_FILE32API) + target_compile_options(framework_obj PRIVATE -Wno-deprecated-declarations) #note part of the api is deprecated, ignore this warning on own api + target_link_libraries(framework_obj PUBLIC Celix::utils ${CELIX_OPTIONAL_EXTRA_LIBS}) + target_link_libraries(framework_obj PUBLIC libuuid::libuuid CURL::libcurl ZLIB::ZLIB) + target_link_libraries(framework_obj PRIVATE ${CMAKE_DL_LIBS}) + #Note visibility preset is also needed on OBJECT libraries, to work correctly + set_target_properties(framework_obj PROPERTIES C_VISIBILITY_PRESET hidden) + celix_deprecated_utils_headers(framework_obj) + + generate_export_header(framework_obj Review Comment: We should `generate_export_header` for the framework target only. We can add `target_compile_definitions(framework_obj PRIVATE framework_EXPORTS)`. It should solve the correctness issue. ########## libs/utils/src/hash_map_private.h: ########## @@ -27,16 +27,17 @@ #ifndef HASH_MAP_PRIVATE_H_ #define HASH_MAP_PRIVATE_H_ -#include "exports.h" +#include "celix_utils_export.h" #include "hash_map.h" -UTILS_EXPORT unsigned int hashMap_hashCode(const void* toHash); -UTILS_EXPORT int hashMap_equals(const void* toCompare, const void* compare); +//note functions used for testing, so export is needed +CELIX_UTILS_EXPORT unsigned int hashMap_hashCode(const void* toHash); Review Comment: Use object library instead? ########## libs/framework/CMakeLists.txt: ########## @@ -36,29 +36,80 @@ set(SOURCES src/celix_framework_utils.c src/celix_module_private.h) add_library(framework_obj OBJECT ${SOURCES}) -target_include_directories(framework_obj PUBLIC - $<BUILD_INTERFACE:${CMAKE_CURRENT_LIST_DIR}/include> -) -target_include_directories(framework_obj PRIVATE ${CMAKE_CURRENT_LIST_DIR}/include_deprecated) -target_compile_options(framework_obj PRIVATE -DUSE_FILE32API) -target_compile_options(framework_obj PRIVATE -Wno-deprecated-declarations) #note part of the api is deprecated, ignore this warning on own api -target_link_libraries(framework_obj PUBLIC Celix::utils ${CELIX_OPTIONAL_EXTRA_LIBS}) -target_link_libraries(framework_obj PUBLIC libuuid::libuuid CURL::libcurl ZLIB::ZLIB) -target_link_libraries(framework_obj PRIVATE ${CMAKE_DL_LIBS}) -celix_deprecated_utils_headers(framework_obj) -add_library(framework SHARED) -target_link_libraries(framework PUBLIC framework_obj) -set_target_properties(framework PROPERTIES OUTPUT_NAME "celix_framework") -set_target_properties(framework PROPERTIES "SOVERSION" ${CELIX_MAJOR}) +set(CELIX_FRAMEWORK_CMAKE_CONFIGURE_ALT_OPTION ON) +if (CELIX_FRAMEWORK_CMAKE_CONFIGURE_ALT_OPTION) + function (celix_configure_framework_target) + list(GET ARGN 0 FRAMEWORK_TARGET_NAME) -celix_deprecated_utils_headers(framework) + target_include_directories(${FRAMEWORK_TARGET_NAME} + PRIVATE + ${CMAKE_CURRENT_LIST_DIR}/include_deprecated + PUBLIC + $<BUILD_INTERFACE:${CMAKE_CURRENT_LIST_DIR}/include> + $<BUILD_INTERFACE:${CMAKE_BINARY_DIR}/celix/gen/includes/${FRAMEWORK_TARGET_NAME}> + $<INSTALL_INTERFACE:include/celix/framework>) + target_compile_options(${FRAMEWORK_TARGET_NAME} PRIVATE -DUSE_FILE32API) + target_compile_options(${FRAMEWORK_TARGET_NAME} PRIVATE -Wno-deprecated-declarations) #note part of the api is deprecated, ignore this warning on own api + target_link_libraries(${FRAMEWORK_TARGET_NAME} PUBLIC Celix::utils ${CELIX_OPTIONAL_EXTRA_LIBS}) + target_link_libraries(${FRAMEWORK_TARGET_NAME} PUBLIC libuuid::libuuid CURL::libcurl ZLIB::ZLIB) + target_link_libraries(${FRAMEWORK_TARGET_NAME} PRIVATE ${CMAKE_DL_LIBS}) + set_target_properties(${FRAMEWORK_TARGET_NAME} PROPERTIES C_VISIBILITY_PRESET hidden) + celix_deprecated_utils_headers(${FRAMEWORK_TARGET_NAME}) + + generate_export_header(${FRAMEWORK_TARGET_NAME} + BASE_NAME "CELIX_FRAMEWORK" + EXPORT_FILE_NAME "${CMAKE_BINARY_DIR}/celix/gen/includes/${FRAMEWORK_TARGET_NAME}/celix_framework_export.h") + endfunction () + + celix_configure_framework_target(framework_obj) + + add_library(framework SHARED $<TARGET_OBJECTS:framework_obj>) + celix_configure_framework_target(framework) + set_target_properties(framework + PROPERTIES + "VERSION" "${CELIX_MAJOR}.${CELIX_MINOR}.${CELIX_MICRO}" + "SOVERSION" ${CELIX_MAJOR} + OUTPUT_NAME "celix_framework") +else () + target_include_directories(framework_obj PUBLIC + $<BUILD_INTERFACE:${CMAKE_CURRENT_LIST_DIR}/include> + $<INSTALL_INTERFACE:include/celix/framework> + ) + target_include_directories(framework_obj PRIVATE ${CMAKE_CURRENT_LIST_DIR}/include_deprecated) + target_compile_options(framework_obj PRIVATE -DUSE_FILE32API) + target_compile_options(framework_obj PRIVATE -Wno-deprecated-declarations) #note part of the api is deprecated, ignore this warning on own api + target_link_libraries(framework_obj PUBLIC Celix::utils ${CELIX_OPTIONAL_EXTRA_LIBS}) + target_link_libraries(framework_obj PUBLIC libuuid::libuuid CURL::libcurl ZLIB::ZLIB) + target_link_libraries(framework_obj PRIVATE ${CMAKE_DL_LIBS}) + #Note visibility preset is also needed on OBJECT libraries, to work correctly + set_target_properties(framework_obj PROPERTIES C_VISIBILITY_PRESET hidden) + celix_deprecated_utils_headers(framework_obj) + + generate_export_header(framework_obj + BASE_NAME "CELIX_FRAMEWORK" + EXPORT_FILE_NAME "${CMAKE_BINARY_DIR}/celix/gen/includes/framework/celix_framework_export.h") + target_include_directories(utils_obj PUBLIC Review Comment: Should it be removed? -- 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. To unsubscribe, e-mail: dev-unsubscr...@celix.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org