PengZheng commented on code in PR #620:
URL: https://github.com/apache/celix/pull/620#discussion_r1304161309
##########
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:
> 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.
Fortunately, all these targets (zmq/czmq/libzip/rapidjson) need not to be
specified explicitly by our downstream users, since they are all linked
privately by their users, for exmaple `RsaConfiguredDiscovery`. For a container
containing `RsaConfiguredDiscovery`, e.g. `RemoteCalculatorConsumer`, there is
no need to refer to rapidjson/RapidJSON::RapidJSON directly. All needed
information is encoded in `DT_NEEDED` tag, the dynamic loader will find them
automatically provided the needed library is in the canonical search path.
I have removed many such unnecessary extra linkages in tests in the past two
years. They were needed because we have no BUILD_RPATH in the past. When they
are installed, even without BUILD_RPATH, our user will encounter no issue,
since Conan setup LD_LIBRARY_PATH to containing all dependencies (direct and
indirect).
> I believe this issue could be addressed with an additional alias:
add_library(RapidJSON::RapidJSON ALIAS rapidjson)
Our user may have encounter the same issue we had before, and resort to
explicit linking as we did before.
So I will add these alias in find modules to avoid breaking these usage.
There is indeed a interesting corner case: Conan does not describe CMake
private linkage well enough, we have the following workaround in Celix:
```
# the following is workaround
https://github.com/conan-io/conan/issues/7192
if self.settings.os == "Linux":
cmake.definitions["CMAKE_EXE_LINKER_FLAGS"] =
"-Wl,--unresolved-symbols=ignore-in-shared-libs"
elif self.settings.os == "Macos":
cmake.definitions["CMAKE_EXE_LINKER_FLAGS"] = "-Wl,-undefined
-Wl,dynamic_lookup"
```
Note that the above is to avoid build time linker error, there is nothing
wrong at run time (all information is encoded in DT_NEEDED correctly).
For more on this, see https://github.com/conan-io/conan/issues/13302
--
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
