pnoltes commented on code in PR #620:
URL: https://github.com/apache/celix/pull/620#discussion_r1304038077
##########
documents/building/dev_celix_with_clion.md:
##########
@@ -47,26 +47,33 @@ conan profile update settings.build_type=Debug debug
#generate and configure cmake-build-debug directory
conan install . celix/2.3.0 -pr:b default -pr:h debug -if cmake-build-debug/
-o celix:enable_testing=True -o celix:enable_address_sanitizer=True -o
celix:build_all=True -b missing
-conan build . -bf cmake-build-debug/ --configure
-#optional build
+#invoke the exact cmake command `conan install` shows to configure the build
directory
cd cmake-build-debug
+cmake .. -G "Unix Makefiles"
-DCMAKE_TOOLCHAIN_FILE=/home/peng/Downloads/git/mycelix/cmake-build-debug/conan_toolchain.cmake
-DENABLE_TESTING=ON -DENABLE_CODE_COVERAGE=OFF -DENABLE_ADDRESS_SANITIZER=ON
-DENABLE_UNDEFINED_SANITIZER=OFF -DENABLE_THREAD_SANITIZER=OFF
-DENABLE_TESTING_DEPENDENCY_MANAGER_FOR_CXX11=OFF
-DENABLE_TESTING_FOR_CXX14=OFF -DBUILD_ALL=ON -DBUILD_DEPLOYMENT_ADMIN=ON
-DBUILD_HTTP_ADMIN=ON -DBUILD_LOG_SERVICE=ON -DBUILD_LOG_HELPER=ON
-DBUILD_LOG_SERVICE_API=ON -DBUILD_SYSLOG_WRITER=ON -DBUILD_PUBSUB=ON
-DBUILD_PUBSUB_WIRE_PROTOCOL_V1=ON -DBUILD_PUBSUB_WIRE_PROTOCOL_V2=ON
-DBUILD_PUBSUB_JSON_SERIALIZER=ON -DBUILD_PUBSUB_AVROBIN_SERIALIZER=ON
-DBUILD_PUBSUB_PSA_ZMQ=ON -DBUILD_PUBSUB_EXAMPLES=ON
-DBUILD_PUBSUB_INTEGRATION=ON -DBUILD_PUBSUB_PSA_TCP=ON
-DBUILD_PUBSUB_PSA_UDP_MC=ON -DBUILD_PUBSUB_PSA_WS=ON
-DBUILD_PUBSUB_DISCOVERY_ETCD=ON -DBUILD_CXX_REMOTE_SERVICE_ADMIN=ON
-DBUILD_CXX_RSA_INTEGRATION=ON -DBUILD_REMOTE_SERVICE_ADMIN=ON
-DBUILD_RSA_REMOTE_SERVICE_ADMIN_DF
I=ON -DBUILD_RSA_DISCOVERY_COMMON=ON -DBUILD_RSA_DISCOVERY_CONFIGURED=ON
-DBUILD_RSA_DISCOVERY_ETCD=ON -DBUILD_RSA_REMOTE_SERVICE_ADMIN_SHM_V2=ON
-DBUILD_RSA_JSON_RPC=ON -DBUILD_RSA_DISCOVERY_ZEROCONF=ON -DBUILD_SHELL=ON
-DBUILD_SHELL_API=ON -DBUILD_REMOTE_SHELL=ON -DBUILD_SHELL_BONJOUR=ON
-DBUILD_SHELL_TUI=ON -DBUILD_SHELL_WUI=ON -DBUILD_COMPONENTS_READY_CHECK=ON
-DBUILD_EXAMPLES=ON -DBUILD_CELIX_ETCDLIB=ON -DBUILD_LAUNCHER=ON
-DBUILD_PROMISES=ON -DBUILD_PUSHSTREAMS=ON -DBUILD_EXPERIMENTAL=ON
-DBUILD_CELIX_DFI=ON -DBUILD_DEPENDENCY_MANAGER=ON
-DBUILD_DEPENDENCY_MANAGER_CXX=ON -DBUILD_FRAMEWORK=ON -DBUILD_RCM=ON
-DBUILD_UTILS=ON -DCELIX_CXX14=ON -DCELIX_CXX17=ON
-DCELIX_INSTALL_DEPRECATED_API=ON -DCELIX_USE_COMPRESSION_FOR_BUNDLE_ZIPS=ON
-DENABLE_CMAKE_WARNING_TESTS=OFF -DENABLE_TESTING_ON_CI=OFF
-DFRAMEWORK_CURLINIT=ON -DBUILD_ERROR_INJECTOR_MDNSRESPONDER=ON
-DCELIX_ERR_BUFFER_SIZE=512
-DCMAKE_EXE_LINKER_FLAGS=-Wl,--unresolved-symbols=ignore-in-shared-libs
-DCELIX_MAJOR=2 -DCELIX_M
INOR=3 -DCELIX_MICRO=0 -DCMAKE_POLICY_DEFAULT_CMP0091=NEW
-DCMAKE_BUILD_TYPE=Debug
Review Comment:
This command contains user-specific info (`/home/peng/...`).
I also think that this command it too large and error prone for changes in
build options.
Maybe better to just call conan build?
##########
cmake/Modules/FindRapidJSON.cmake:
##########
@@ -28,8 +28,8 @@ find_package_handle_standard_args(RapidJSON DEFAULT_MSG
mark_as_advanced(RapidJSON_INCLUDE_DIR)
-if(RapidJSON_FOUND AND NOT TARGET RapidJSON::RapidJSON)
- add_library(RapidJSON::RapidJSON INTERFACE IMPORTED)
- set_target_properties(RapidJSON::RapidJSON PROPERTIES
INTERFACE_INCLUDE_DIRECTORIES
+if(RapidJSON_FOUND AND NOT TARGET rapidjson)
+ add_library(rapidjson INTERFACE IMPORTED)
+ set_target_properties(rapidjson PROPERTIES INTERFACE_INCLUDE_DIRECTORIES
"${RapidJSON_INCLUDE_DIRS}")
Review Comment:
Technically, in my opinion, this is a breaking change.
The reason is that downstream Apache Celix containers need to link against
rapidjson to ensure a functional C++ Remote Services. This requirement also
applies to ZerMQ and czmq, but then for PubSub ZMQ usage.
I believe this issue could be addressed with an additional alias:
`add_library(RapidJSON::RapidJSON ALIAS rapidjson)`
A more optimal solution would have been to introduce an Apache Celix library
with an INTERFACE link to the required library, thereby abstracting the
underlying library target names. This approach is used with `civetweb` for
cmake target `Celix::http_admin_api`. However, since this wasn't done for
rapidjson, ZeroMQ or czmq usage, changing the container-required library names
now, is in my view, a breaking change.
Perhaps we should add a paragraph to the coding convention. It could mention
that an Apache Celix INTERFACE library should be added if a bundle requires the
associated executable to link against an external library. This ensures the
specific required library is abstracted away.
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
