-
Notifications
You must be signed in to change notification settings - Fork 89
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Feature/483 Conan 2 Support #620
Conversation
Drop generators unsupported by Conan 2.0
Dynamic defaults is unsupported by Conan2 according to conan-io/conan#14528
Codecov Report
@@ Coverage Diff @@
## master #620 +/- ##
==========================================
+ Coverage 80.00% 81.61% +1.60%
==========================================
Files 260 260
Lines 34674 34674
==========================================
+ Hits 27741 28298 +557
+ Misses 6933 6376 -557
... and 10 files with indirect coverage changes 📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice to see an upgrade to conan 2, I do have some remarks.
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}") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 or LD_LIBRARY_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 conan-io/conan#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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Clear and thanks for explanation.
I agree that linked system libraries is mostly an implementation details, but the libraries need to be available runtime so there is some leakage and therefore I prefer no changes to libs target name .. if possible.
… but mention a quicker way to skip full building.
Once https://youtrack.jetbrains.com/issue/CPP-34818 is addressed, using Conan with Clion will be enjoyable. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
With the added lib alias, LGTM.
This PR updates the whole project to support both Conan 1.x and the current Conan 2:
conanrun.sh
/deactivate_conanrun.sh
configure()
method will be forbidden by Conan 2 but allowed by Conan 1. For an extensive discussion with the Conan upstream, see [question] [2.0] Options are frozen when calling configure conan-io/conan#14528configure()
method. Previously this is done inrequirements()
method, which is not supported by Conan 2.self.requires("openssl/1.1.1t", override=True)
to resolve version conflict of openssl caused by civetweb and libcurl. Note that it does not introduce any dependency of openssl into Celix. For an extensive discusson, see [question] Resolve Version Conflicts Caused by Indirect Dependencies conan-io/conan#14535 (comment)conan build . -bf cmake-build-debug --configure
does not work anymore. It will always lead to a full build.conan install
is used instead to configure the build directory.tc.user_presets_path = False
is added to suppress its production. I reported it to JetBrains (https://youtrack.jetbrains.com/issue/CPP-34818). Once this issue solved, we can expect excellent user experience by combining Conan and CLion.