Skip to content
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.

Sign up for GitHub

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

Merged
merged 23 commits into from
Aug 24, 2023
Merged

Feature/483 Conan 2 Support #620

Show file tree
Hide file tree
Changes from 20 commits
Commits
Show all changes
23 commits
Select commit Hold shift + click to select a range
60b5941
Drop generators unsupported by Conan 2.0
PengZheng Aug 20, 2023
681c6ef
Merge branch 'master' into feature/483-conan-2-support
PengZheng Aug 20, 2023
2e42f25
Use conan2's conanrun.sh
PengZheng Aug 20, 2023
93d93ec
Stop gap for #618
PengZheng Aug 20, 2023
949ec5d
Specify dependency options in a way compatible with both Conan1 and C…
PengZheng Aug 21, 2023
6d25aa6
Implement dynamic defaults.
PengZheng Aug 21, 2023
c181c05
Simplify options settings.
PengZheng Aug 22, 2023
73f44ea
Fix version conflict of openssl caused by civetweb.
PengZheng Aug 22, 2023
0033657
Add test_package for Conan2.
PengZheng Aug 23, 2023
ff7ad59
Fix linking error caused by private linking of transitive dependencie…
PengZheng Aug 23, 2023
af02716
Use Conan2 for conan_create/ubuntu-build
PengZheng Aug 23, 2023
f292a83
Fix profile setup for conan_create/ubuntu-build
PengZheng Aug 23, 2023
dfae92e
Fix option setting error for conan_create/ubuntu-build
PengZheng Aug 23, 2023
e7d277a
Remove irrelevant dependency of unit_test_rsa_common.
PengZheng Aug 23, 2023
1c5605a
Fix missing `deps_cpp_info` for Conan 2
PengZheng Aug 23, 2023
a2d547a
Update documentation to reflect the new conan based workflow.
PengZheng Aug 23, 2023
2e48484
Remove unsupported `conan build --configure`.
PengZheng Aug 23, 2023
ff26503
Enable ninja for all conan build.
PengZheng Aug 24, 2023
ad18430
Run gcov by ninja.
PengZheng Aug 24, 2023
697e7ed
Run gcov by GNU make.
PengZheng Aug 24, 2023
a76ad79
Fix asan error and re-enable pubsub_udpmc_tests.
PengZheng Aug 24, 2023
9ea3246
Still recommend using `conan build` in our Conan-based CLion workflow…
PengZheng Aug 24, 2023
4e7c7a4
Avoid breaking downstream user who link to these targets.
PengZheng Aug 24, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
24 changes: 6 additions & 18 deletions .github/workflows/conan_create.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,44 +9,33 @@ on:
jobs:

ubuntu-build:
runs-on: ubuntu-20.04
runs-on: ubuntu-22.04
strategy:
fail-fast: false
matrix:
compiler: [ [gcc,g++], [clang,clang++] ]
type: [ Debug ]
type: [ Release ]
timeout-minutes: 120
steps:
- name: Checkout source code
uses: actions/checkout@v3.3.0
- name: Install build dependencies
run: |
sudo pip install -U conan==1.59.0
sudo pip install -U conan
sudo apt-get install -yq --no-install-recommends ninja-build
- name: Setup Conan Profile
env:
CC: ${{ matrix.compiler[0] }}
CXX: ${{ matrix.compiler[1] }}
run: |
# build profile
conan profile new release --detect
conan profile update settings.build_type=Release release
#Note no backwards compatiblity for gcc5 needed, setting libcxx to c++11.
conan profile update settings.compiler.libcxx=libstdc++11 release
conan profile show release
# host profile
conan profile new default --detect
conan profile update settings.build_type=${{ matrix.type }} default
#Note no backwards compatiblity for gcc5 needed, setting libcxx to c++11.
conan profile update settings.compiler.libcxx=libstdc++11 default
conan profile show default
conan profile detect -f
- name: Create Celix
env:
CC: ${{ matrix.compiler[0] }}
CXX: ${{ matrix.compiler[1] }}
CONAN_CMAKE_GENERATOR: Ninja
run: |
conan inspect . -a options | awk 'BEGIN { FS="[\t:]+" } /build/ && !/build_all/ { print $1}' | while read option; do conan create . -b missing -o celix:${option}=True -pr:b release -pr:h default -tf examples/conan_test_package -tbf test-build -o celix:celix_cxx17=True -o celix:celix_install_deprecated_api=True --require-override=libcurl/7.64.1 --require-override=openssl/1.1.1s --require-override=zlib/1.2.13 || exit 1; done
conan inspect . | awk 'BEGIN { FS="[\t:]+"; output=0 } /build/ && !/build_all/ { if(output) print $1} /^options/ {output=1} /^options_definitions/ {output=0}' | while read option; do conan create . -c tools.cmake.cmaketoolchain:generator=Ninja -b missing -o celix/*:${option}=True -pr:b default -pr:h default -s:h build_type=${{ matrix.type }} -tf examples/conan_test_package_v2 -o celix/*:celix_cxx17=True -o celix/*:celix_install_deprecated_api=True || exit 1; done

mac-build:
runs-on: macOS-11
Expand All @@ -66,6 +55,5 @@ jobs:
env:
CC: ${{ matrix.compiler[0] }}
CXX: ${{ matrix.compiler[1] }}
CONAN_CMAKE_GENERATOR: Ninja
run: |
conan inspect . -a options | awk 'BEGIN { FS="[\t:]+" } /build/ && !/build_all/ { print $1}' | while read option; do conan create . -b missing -o celix:${option}=True -pr:b default -pr:h default -tf examples/conan_test_package -tbf test-build -o celix:celix_cxx17=True -o celix:celix_install_deprecated_api=True --require-override=libcurl/7.64.1 --require-override=openssl/1.1.1s --require-override=zlib/1.2.13 || exit 1; done
conan inspect . -a options | awk 'BEGIN { FS="[\t:]+" } /build/ && !/build_all/ { print $1}' | while read option; do conan create . -c tools.cmake.cmaketoolchain:generator=Ninja -b missing -o celix:${option}=True -pr:b default -pr:h default -tf examples/conan_test_package -tbf test-build -o celix:celix_cxx17=True -o celix:celix_install_deprecated_api=True --require-override=libcurl/7.64.1 --require-override=openssl/1.1.1s --require-override=zlib/1.2.13 || exit 1; done
7 changes: 3 additions & 4 deletions .github/workflows/coverage.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -33,14 +33,13 @@ jobs:
conan install . celix/ci -pr:b default -pr:h default -if build ${CONAN_BUILD_OPTIONS} -b missing -b cpputest --require-override=libcurl/7.64.1 --require-override=openssl/1.1.1s
- name: Build
run: |
conan build . -bf build --configure
conan build . -bf build --build
conan build . -bf build
- name: Test with coverage
run: |
cd build
source activate_run.sh
source conanrun.sh
make coverage
source deactivate_run.sh
source deactivate_conanrun.sh
lcx="lcov --output-file=coverage.info " && for i in `find . -name "*.info.cleaned"`; do lcx+=" --add-tracefile=$i"; done && $lcx
- name: Codecov
uses: codecov/codecov-action@e156083f13aff6830c92fc5faa23505779fbf649
Expand Down
11 changes: 5 additions & 6 deletions .github/workflows/macos.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,7 @@ jobs:
uses: actions/checkout@v3.3.0
- name: Install conan
run: |
brew install python
brew install python ninja
pip3 install -U conan==1.59.0
- name: Setup Conan Profile
run: |
Expand All @@ -35,18 +35,17 @@ jobs:
-o celix:framework_curlinit=False
run: |
#force require libcurl 7.64.1, due to a sha256 verify issue in libcurl/7.87.0
conan install . celix/ci -pr:b default -pr:h default -if build ${CONAN_BUILD_OPTIONS} -b missing -b cpputest --require-override=libcurl/7.64.1 --require-override=openssl/1.1.1s
conan install . celix/ci -c tools.cmake.cmaketoolchain:generator=Ninja -pr:b default -pr:h default -if build ${CONAN_BUILD_OPTIONS} -b missing -b cpputest --require-override=libcurl/7.64.1 --require-override=openssl/1.1.1s
- name: Build
run: |
conan build . -bf build --configure
conan build . -bf build --build
conan build . -bf build

- name: Test
run: |
cd build
source activate_run.sh
source conanrun.sh
ctest --output-on-failure
source deactivate_run.sh
source deactivate_conanrun.sh

macos-build-brew:
runs-on: macOS-latest
Expand Down
9 changes: 4 additions & 5 deletions .github/workflows/ubuntu.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -55,21 +55,20 @@ jobs:
-o celix:framework_curlinit=False
run: |
#force require libcurl 7.64.1, due to a sha256 verify issue in libcurl/7.87.0
conan install . celix/ci -pr:b release -pr:h default -if build ${CONAN_BUILD_OPTIONS} -b missing -b cpputest --require-override=libcurl/7.64.1 --require-override=openssl/1.1.1s
conan install . celix/ci -c tools.cmake.cmaketoolchain:generator=Ninja -pr:b release -pr:h default -if build ${CONAN_BUILD_OPTIONS} -b missing -b cpputest --require-override=libcurl/7.64.1 --require-override=openssl/1.1.1s
- name: Build
env:
CC: ${{ matrix.compiler[0] }}
CXX: ${{ matrix.compiler[1] }}
CONAN_CMAKE_GENERATOR: Ninja
run: |
conan build . -bf build --configure
conan build . -bf build --build
conan build . -bf build
- name: Test
run: |
cd build
source activate_run.sh
source conanrun.sh
ctest --output-on-failure
source deactivate_run.sh
source deactivate_conanrun.sh

linux-build-apt:
runs-on: ubuntu-22.04
Expand Down
2 changes: 1 addition & 1 deletion bundles/cxx_remote_services/discovery_configured/CMakeLists.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -38,7 +38,7 @@ target_link_libraries(RsaConfiguredDiscovery PRIVATE
Celix::framework
Celix::rsa_spi
Celix::log_helper
RapidJSON::RapidJSON
rapidjson
)

target_include_directories(RsaConfiguredDiscovery PRIVATE src)
Expand Down
2 changes: 1 addition & 1 deletion bundles/pubsub/pubsub_admin_zmq/CMakeLists.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -47,7 +47,7 @@ if (PUBSUB_PSA_ZMQ)
)

target_link_libraries(celix_pubsub_admin_zmq PRIVATE
Celix::log_helper ZeroMQ::ZeroMQ czmq::czmq libuuid::libuuid ${OPTIONAL_OPENSSL_LIB})
Celix::log_helper libzmq czmq libuuid::libuuid ${OPTIONAL_OPENSSL_LIB})
target_link_libraries(celix_pubsub_admin_zmq PRIVATE Celix::shell_api)
target_link_libraries(celix_pubsub_admin_zmq PRIVATE Celix::pubsub_spi Celix::pubsub_utils)
target_include_directories(celix_pubsub_admin_zmq PRIVATE src)
Expand Down
2 changes: 1 addition & 1 deletion bundles/remote_services/discovery_zeroconf/gtest/CMakeLists.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -64,7 +64,7 @@ if (LINKER_WRAP_SUPPORTED)
GTest::gtest_main
)

target_compile_definitions(unit_test_discovery_zeroconf PRIVATE -DMDNSD="${DNSSD_LIB_DIRS}/../bin/mdnsd")
target_compile_definitions(unit_test_discovery_zeroconf PRIVATE -DMDNSD="${DNSSD_INCLUDE_DIR}/../bin/mdnsd")


add_test(NAME run_unit_test_discovery_zeroconf COMMAND sudo $<TARGET_FILE:unit_test_discovery_zeroconf>)
Expand Down
1 change: 0 additions & 1 deletion bundles/remote_services/rsa_common/gtest/CMakeLists.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,6 @@ if (LINKER_WRAP_SUPPORTED)

target_link_libraries(unit_test_rsa_common PRIVATE
rsa_common_cut
Celix::rsa_spi
Celix::framework
Celix::utils
Celix::malloc_ei
Expand Down
12 changes: 6 additions & 6 deletions cmake/Modules/FindCppUTest.cmake
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -44,18 +44,18 @@ FIND_PACKAGE_HANDLE_STANDARD_ARGS(CppUTest DEFAULT_MSG CppUTest_LIBRARY CppUTest
IF(CppUTest_FOUND)
SET(CppUTest_LIBRARIES ${CppUTest_LIBRARY})
SET(CppUTest_INCLUDE_DIRS ${CppUTest_INCLUDE_DIR})
if(NOT TARGET CppUTest::CppUTest)
add_library(CppUTest::CppUTest STATIC IMPORTED)
set_target_properties(CppUTest::CppUTest PROPERTIES
if(NOT TARGET CppUTest)
add_library(CppUTest STATIC IMPORTED)
set_target_properties(CppUTest PROPERTIES
IMPORTED_LOCATION "${CppUTest_LIBRARY}"
INTERFACE_INCLUDE_DIRECTORIES "${CppUTest_INCLUDE_DIR}"
)
endif()
SET(CppUTest_EXT_LIBRARIES ${CppUTest_EXT_LIBRARY})
SET(CppUTest_EXT_INCLUDE_DIRS ${CppUTest_EXT_INCLUDE_DIR})
if(NOT TARGET CppUTest::CppUTestExt)
add_library(CppUTest::CppUTestExt STATIC IMPORTED)
set_target_properties(CppUTest::CppUTestExt PROPERTIES
if(NOT TARGET CppUTestExt)
add_library(CppUTestExt STATIC IMPORTED)
set_target_properties(CppUTestExt PROPERTIES
IMPORTED_LOCATION "${CppUTest_EXT_LIBRARY}"
INTERFACE_INCLUDE_DIRECTORIES "${CppUTest_EXT_INCLUDE_DIR}"
)
Expand Down
6 changes: 3 additions & 3 deletions cmake/Modules/FindRapidJSON.cmake
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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}")
Copy link
Contributor

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.

Copy link
Contributor Author

@PengZheng PengZheng Aug 24, 2023 •
edited
Loading

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.

Copy link
Contributor

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.

endif()
6 changes: 3 additions & 3 deletions cmake/Modules/FindZeroMQ.cmake
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -42,9 +42,9 @@ find_package_handle_standard_args(ZeroMQ DEFAULT_MSG

mark_as_advanced(ZEROMQ_INCLUDE_DIR ZEROMQ_LIBRARY )

if (ZeroMQ_FOUND AND NOT TARGET ZeroMQ::ZeroMQ)
add_library(ZeroMQ::ZeroMQ SHARED IMPORTED)
set_target_properties(ZeroMQ::ZeroMQ PROPERTIES
if (ZeroMQ_FOUND AND NOT TARGET libzmq)
add_library(libzmq SHARED IMPORTED)
set_target_properties(libzmq PROPERTIES
IMPORTED_LOCATION "${ZEROMQ_LIBRARY}"
INTERFACE_INCLUDE_DIRECTORIES "${ZEROMQ_INCLUDE_DIR}"
)
Expand Down
8 changes: 4 additions & 4 deletions cmake/Modules/Findczmq.cmake
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,7 @@
# CZMQ_INCLUDE_DIRS - The czmq include directories
# CZMQ_LIBRARIES - The libraries needed to use czmq
# CZMQ_DEFINITIONS - Compiler switches required for using czmq
# czmq::czmq - Imported CMake target for the library (include path + library)
# czmq - Imported CMake target for the library (include path + library)

find_path(CZMQ_INCLUDE_DIR czmq.h
/usr/include
Expand All @@ -42,9 +42,9 @@ find_package_handle_standard_args(czmq DEFAULT_MSG

mark_as_advanced(CZMQ_INCLUDE_DIR CZMQ_LIBRARY)

if (czmq_FOUND AND NOT TARGET czmq::czmq)
add_library(czmq::czmq SHARED IMPORTED)
set_target_properties(czmq::czmq PROPERTIES
if (czmq_FOUND AND NOT TARGET czmq)
add_library(czmq SHARED IMPORTED)
set_target_properties(czmq PROPERTIES
IMPORTED_LOCATION "${CZMQ_LIBRARY}"
INTERFACE_INCLUDE_DIRECTORIES "${CZMQ_INCLUDE_DIR}"
)
Expand Down
6 changes: 3 additions & 3 deletions cmake/Modules/Findlibzip.cmake
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -37,9 +37,9 @@ include(FindPackageHandleStandardArgs)
find_package_handle_standard_args(libzip DEFAULT_MSG
LIBZIP_LIBRARY LIBZIP_INCLUDE_DIR)

if(libzip_FOUND AND NOT TARGET libzip::libzip)
add_library(libzip::libzip IMPORTED STATIC GLOBAL)
set_target_properties(libzip::libzip PROPERTIES
if(libzip_FOUND AND NOT TARGET libzip::zip)
add_library(libzip::zip IMPORTED STATIC GLOBAL)
set_target_properties(libzip::zip PROPERTIES
IMPORTED_LOCATION "${LIBZIP_LIBRARY}"
INTERFACE_INCLUDE_DIRECTORIES "${LIBZIP_INCLUDE_DIR}"
)
Expand Down
Loading