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 of Celix 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 issues 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
> 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.
We don't need to do this. Our user should live happily without knowing a
private dependency of Celix if they don't use it directly. It's a pure
implementation detail of Celix. Yes, at runtime, we need these dependencies at
the right place so that dynamic linker could find them. The good news is that
Conan helps a lot with this scenario:
`conan import` will fetch all direct and indirect dependencies from the
cache. Users only need to copy all of them collected by Conan into the library
path when deploying their application. Conan 2 facilitate this usage even
further.
--
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
