apache / celix

Apache Celix is a framework for C and C++14 to develop dynamic modular software applications using component and in-process service-oriented programming.
https://celix.apache.org/
Apache License 2.0
162 stars 86 forks source link

Dubious usage of `BUILD_WITH_INSTALL_RPATH` in add_celix_bundle #571

Closed PengZheng closed 1 year ago

PengZheng commented 1 year ago

Current usage causes loader error when running tests.

Suppose I have a project named MiniSysRT that depends on Celix, and it contains a bundle named MiniSysRT::cli, which depend on another shared library MiniSysRT::net_base. When the framework tries to load the bundle, the following error occurs:

"Could not load library: .../libcli.so.1"
Cause: libnet_base.so.1: No such file or directory

To fix this, I add the following workaround:

if (ENABLE_TESTING)
    celix_bundle_private_libs(cli MiniSysRT::net_base)

This way dynamic loader does find libnet_base.so.1 using the install RPATH $ORIGIN. Is there any specific reason why we need it at the first place? It seems to me that this usage only brings inconvenience to downstream users, and ourselves:

    if (ENABLE_TESTING)
        celix_bundle_private_libs(rsa_json_rpc Celix::dfi)
# Celix::framework makes bundles loadable (otherwise we need an copy of framework in each bundle, celix_bundle_private_libs)
target_link_libraries(test_framework PRIVATE framework_cut Celix::framework CURL::libcurl GTest::gtest GTest::gtest_main)

Celix::framework is added as a workaround.

pnoltes commented 1 year ago

If I call correctly the BUILD_WITH_INSTALL_RPATH is needed to ensure $ORIGIN works.

See snippet:

set_target_properties(${BUNDLE_TARGET_NAME} PROPERTIES "BUILD_WITH_INSTALL_RPATH" true)

if(APPLE)
    set_target_properties(${BUNDLE_TARGET_NAME} PROPERTIES INSTALL_RPATH "@loader_path")
else()
    set_target_properties(${BUNDLE_TARGET_NAME} PROPERTIES INSTALL_RPATH "$ORIGIN")
endif()

And $ORIGIN should IMO be the preferred path to load (private) bundle libraries, so that as result a executable using a Celix framework should: a) Link against the required bundle dependency libraries so that during bundle loading the needed libraries can be found or b) Add required bundle dependency libraries as a private lib / file to the bundle using the libraries

The usage Celix::dfi in remote services is an example of optoin A and the usage of cmzq in pubsub is an example of option B.

Maybe it would be good to document this better ?

PengZheng commented 1 year ago

See snippet:

I understand why we need $ORIGIN set for private library. But I don't quite understand we need it at build time: Conan test_package can help ensure $ORIGIN in the installed binary works as expected.

BUILD_RPATH is just a convenience feature. See https://github.com/apache/celix/commit/eccbb7f9be7958b5724e8569164e8c14052abcae and https://github.com/apache/celix/pull/569/commits/514a71e8fc55501976f2867bc5359d9635d484d2 for examples.

a) Link against the required bundle dependency libraries so that during bundle loading the needed libraries can be found

target_link_libraries(rsa_json_rpc PRIVATE ${RSA_JSON_RPC_DEPS}) has already done this for Celix::dfi, if we have the default BUILD_RPATH in place.
rsa_json_rpc does not really need a private copy of Celix::dfi. IMHO, BUILD_RPATH is just a convenience feature.

Will these commits (https://github.com/apache/celix/commit/eccbb7f9be7958b5724e8569164e8c14052abcae and https://github.com/apache/celix/pull/569/commits/514a71e8fc55501976f2867bc5359d9635d484d2) bring about any harm?

b) Add required bundle dependency libraries as a private lib / file to the bundle using the libraries

As documented in celix_bundle_private_libs, the idea of private library does not really work reliably due to the limitation of ld.so. A more reliable way of using private library is to link the private library statically into the shared object built with symbol visibility control and -BSymbolic, so that the shared object does not interfere with the outside world. To mitigate resulting code bloating, the techniques in https://github.com/apache/celix/issues/442 can be utilized.

the usage of cmzq in pubsub is an example of option B.

I checked the source code, and found no such usage of czmq as private library.

Maybe it would be good to document this better ?

If there is no harm, let's bring the default BUILD_RPATH back. If necessary, we can add more Conan test_package code.