Add basic Conan support #401
Conversation
It used GPLv3ed zmq by default, which cannot be included in most proprietary softwares deployed in embedded device.
This reverts commit 58a2f88.
There are still some rough edges in CMake in-project dependency management.
Use find_dependency without REQUIRED to handle find_package(Celix) correctly (i.e., use Celix as an optional package).
Avoid redefining libzip::libzip and support Jansson target for etcdlib.
…rdingly.
Replace the broken automatic dependency deduction in celix_subproject with automatic error detection. Use celix_subproject systematically to reflect the current modular structure, and thus allowing more to be opted out. Essential and non-essential usages are carefully differentiated: e.g., PUBSUB_PSA_WS uses HTTP_ADMIN essentially, while all bundles use log_helper non-essentially. Now it's possible to turn off all bundles.
Minor cmake style improvements, including: replacing add_dependencies with add_celix_bundle_dependencies; use ${CMAKE_INSTALL_INCLUDEDIR} instead of fixed `include`; replacing $<INSTALL_INTERFACE:> with INCLUDES DESTINATION.
conanfile.py is also updated according to the current project structure. The default options reflect a reasonably minimal and functionally stable configuration: framework without etcd, log_admin, http_admin, pubsub_pas_ws with no need of external discovery mechanism, rsa with preconfigured discovery.
| @@ -908,8 +908,9 @@ function(install_celix_bundle_targets) | |||
|
|
|||
| get_target_property(EXPORT_BUNDLES celix-bundles EXPORT_${EXPORT_NAME}_BUNDLES) | |||
|
|
|||
| if (NOT DEFINED EXPORT_BUNDLES) | |||
| message(FATAL_ERROR "Export ${EXPORT_NAME} not defined. Did you forgot to use a install_celix_bundle with the 'EXPORT ${EXPORT_NAME}' option?") | |||
| if (NOT DEFINED EXPORT_BUNDLES OR NOT EXPORT_BUNDLES) | |||
There was a problem hiding this comment.
Now that it is possible to turn off all bundles, this is not FATAL error any more.
| set(CELIX_MAJOR "2") | ||
| set(CELIX_MINOR "2") | ||
| set(CELIX_MICRO "1") | ||
| if (NOT DEFINED CELIX_MAJOR) |
There was a problem hiding this comment.
if not defined externally (by conan)
| @@ -15,7 +15,7 @@ | |||
| # specific language governing permissions and limitations | |||
| # under the License. | |||
|
|
|||
| celix_subproject(DEPLOYMENT_ADMIN "Option to enable building the Deployment Admin Service bundles" ON DEPS framework launcher shell_tui log_writer) | |||
There was a problem hiding this comment.
Framework is always built. shell_tui and log_writer are only used for the demo application, thus are of non-essential usage.
|
test_package for three components added, more to come later. |
Codecov Report
@@ Coverage Diff @@
## master #401 +/- ##
==========================================
- Coverage 73.13% 71.78% -1.36%
==========================================
Files 205 180 -25
Lines 31274 29281 -1993
==========================================
- Hits 22872 21018 -1854
+ Misses 8402 8263 -139
Continue to review full report at Codecov.
|
…ntrusive usage of conan in CMakeLists.txt.
…ions from conan package ID calculation.
…e for etcdlib, launcher, and promises.
…dCppUTest.cmake compatible with the one generated by conan.
…ound for CMake shared library private linking issue. For the CMake private linking issue, see conan-io/conan#7192
| if self.options.enable_testing: | ||
| self.options['gtest'].shared = True | ||
| if self.options.enable_address_sanitizer: | ||
| self.options["cpputest"].with_leak_detection = False |
There was a problem hiding this comment.
Without Leak Detection Plugin, clang address sanitizer now works with cpputest, the following is addressed:
celix/.github/workflows/ubuntu.yml
Line 16 in 9cf8bc3
We cannot modify distribution's cpputest package, but we can easily do so in Conan's virtual environment.
| set(CMAKE_C_FLAGS "-fsanitize=address -fno-omit-frame-pointer ${CMAKE_C_FLAGS}") | ||
| set(CMAKE_CXX_FLAGS "-fsanitize=address -fno-omit-frame-pointer ${CMAKE_CXX_FLAGS}") | ||
| set(CMAKE_C_FLAGS "-DCPPUTEST_MEM_LEAK_DETECTION_DISABLED -fsanitize=address -fno-omit-frame-pointer ${CMAKE_C_FLAGS}") | ||
| set(CMAKE_CXX_FLAGS "-DCPPUTEST_MEM_LEAK_DETECTION_DISABLED -fsanitize=address -fno-omit-frame-pointer ${CMAKE_CXX_FLAGS}") |
There was a problem hiding this comment.
When enabling ASAN, it makes no sense to use the inferior CPPUTest memory leak detection.
…nan zeromq override warning.
This reverts commit 9198422.
…sing cpputest with build-in memory leak detector.
| @@ -135,7 +135,7 @@ function (setup_target_for_coverage) | |||
| # Capturing lcov counters and generating report | |||
| COMMAND ${CMAKE_COMMAND} -E make_directory ${CMAKE_BINARY_DIR}/coverage | |||
| COMMAND ${LCOV_PATH} --directory ${COVERAGE_SCAN_DIR} --capture --output-file ${OUTPUT_FILE} | |||
| COMMAND ${LCOV_PATH} --remove ${OUTPUT_FILE} '**/mock/*' '**/test/*' '**/gtest/*' '**/tst/*' '**/celix/gen/*' '**/googletest_project/*' '**/glog/*' '/usr/*' --output-file ${OUTPUT_FILE}.cleaned | |||
| COMMAND ${LCOV_PATH} --remove ${OUTPUT_FILE} '**/mock/*' '**/.conan/*' '**/test/*' '**/gtest/*' '**/tst/*' '**/celix/gen/*' '**/googletest_project/*' '**/glog/*' '/usr/*' --output-file ${OUTPUT_FILE}.cleaned | |||
There was a problem hiding this comment.
To remove all external dependencies.
|
@PengZheng: I wanted to create a separate PR with some changes to this PR, but I accidentally pushed directly on this PR. |
|
I started testing the Celix Conan setup on a macOS system and something did not work:
I still have some troubles with running the Celix tests and I will look into this a bit later. |
No problem. Let's address remaining issues together. |
This is the error I am getting if openssl is not fixed to 1.1.1n: I tried changing the version of cmake and libzip, but I could not find version combination that does not raise this issue. |
… for macos and fixes openssl version
|
When using conan create with a profile for build and host, I get the same error: This implies that the build tools and build requirements are not in different contexts. |
|
@PengZheng I reverted the changes to version. For my system the fixed openssl is still needed. |
Using the command line you provided, I successfully reproduced the problem. The root cause is If you want to build missing requirement, the command line should be Resolving dependency conflicts in downstream is generally correct. Here the mentioned conflict should not appear at the first place. |
|
IDE integration is essential for Conan-based CBD. Adding a requirement, then happily including header and invoking APIs are huge productivity improvement. Here I summarized my own Clion setup to facilitate such use cases. Hope that will help other developers. @pnoltes
Here
|
|
Thanks for the extra info. Now I also got it running in CLion 😄. I am not very happy that this is needed for all the run/debug configurations, but (again) this is something we can try to tackle later. I will try to also include the conan information in the coming updated documentation pr. |
Oops! We should setup the All CTest target rather than any specific test target. Run All CTest target once, then we can right click any specific test in Run(alt+4) window to debug it, an Run/Debug Configuration will be created automatically for the test we select. HTH @pnoltes |
|
I'm writing to notify that there is a recently introduced regression of CLion 2022.1, which affects environment variable setting: It will be fixed for the next release, and a workaround is provided by the above link. @pnoltes |
Now we support Conan package manager, and the modular structure of cmake project is improved along the way:
add_dependencieswithadd_celix_bundle_dependencies; useCMAKE_INSTALL_INCLUDEDIRinstead of fixedinclude; replacing$<INSTALL_INTERFACE:>withINCLUDES DESTINATION.Now
test_packagecovers all stable functionalities. This PR is finished, and is ready for review. @pnoltesThis PR may seems a little scary at a first glance, considering its size. However, I shall emphasize that this PR does not change fundamentally how Celix builds, and that CMake build system can be used without conan as before. Though more
celix_subprojects are used, it should change nothing with a defaultON.