[CMAKE] Clean up pkg-config install methods and add tests #4528
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,60 @@ | ||
| # Copyright The OpenTelemetry Authors | ||
| # SPDX-License-Identifier: Apache-2.0 | ||
|
|
||
| cmake_minimum_required(VERSION 3.16) | ||
|
|
||
| project(opentelemetry-cpp-pkgconfig-install-test LANGUAGES CXX) | ||
|
Member
There was a problem hiding this comment. This test currently only covers the pkgconfig files in the ostream component dependency chain. Can it take on a form similar to the current package test to cover all components and pkgconfig files installed? |
||
|
|
||
| if(NOT DEFINED INSTALL_TEST_SRC_DIR) | ||
| message( | ||
| FATAL_ERROR | ||
| "INSTALL_TEST_SRC_DIR must be defined when running cmake on this test project" | ||
| ) | ||
| endif() | ||
|
|
||
| if(NOT INSTALL_TEST_COMPONENTS) | ||
| message(FATAL_ERROR "INSTALL_TEST_COMPONENTS is empty") | ||
| endif() | ||
|
|
||
| find_package(PkgConfig REQUIRED) | ||
| find_package(GTest CONFIG REQUIRED) | ||
|
|
||
| file(GLOB _pkgconfig_pc_files | ||
| "${CMAKE_PREFIX_PATH}/lib/pkgconfig/opentelemetry_*.pc") | ||
| if(NOT _pkgconfig_pc_files) | ||
| message( | ||
| FATAL_ERROR | ||
| "No installed opentelemetry pkg-config files found under ${CMAKE_PREFIX_PATH}/lib/pkgconfig" | ||
| ) | ||
| endif() | ||
|
|
||
| set(_pkgconfig_modules "") | ||
| foreach(_pc_file IN LISTS _pkgconfig_pc_files) | ||
| get_filename_component(_module "${_pc_file}" NAME_WE) | ||
| list(APPEND _pkgconfig_modules "${_module}") | ||
| endforeach() | ||
| list(SORT _pkgconfig_modules) | ||
|
|
||
| message(STATUS "Testing pkg-config install on modules = ${_pkgconfig_modules}") | ||
| message( | ||
| STATUS "Testing pkg-config install on components = ${INSTALL_TEST_COMPONENTS}" | ||
| ) | ||
|
|
||
| pkg_check_modules(OTEL_PKGCONFIG REQUIRED ${_pkgconfig_modules}) | ||
|
|
||
| add_executable(full_pkgconfig_test) | ||
|
|
||
| foreach(component IN LISTS INSTALL_TEST_COMPONENTS) | ||
| message(STATUS "Adding test source for component ${component}") | ||
| target_sources(full_pkgconfig_test | ||
| PRIVATE "${INSTALL_TEST_SRC_DIR}/test_${component}.cc") | ||
| endforeach() | ||
|
|
||
| target_include_directories(full_pkgconfig_test | ||
| PRIVATE ${OTEL_PKGCONFIG_INCLUDE_DIRS}) | ||
| target_compile_options(full_pkgconfig_test | ||
| PRIVATE ${OTEL_PKGCONFIG_CFLAGS_OTHER}) | ||
| target_link_libraries( | ||
| full_pkgconfig_test | ||
| PRIVATE ${OTEL_PKGCONFIG_LIBRARIES} ${OTEL_PKGCONFIG_LDFLAGS} GTest::gtest | ||
| GTest::gtest_main) | ||
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Uh oh!
There was an error while loading. Please reload this page.
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.
My idea with #4503 is to remove the separate opentelemetry_add_pkgconfig, which redefines export name and dependencies of the target. We would then rely on the
otel_add_componentfunction to create the pkgconfig files given the targets provided and their properties.The
opentelemetry_add_pkgconfigfunction then becomes unecessary and the install of the pkgconfig files is a private implementation detail of installing a component.Current mapping from the
opentelemetry_add_pkgconfigargs to target properties could be:EXPORT_NAMEproperty (this seems to be generally the case now but needs to be verified)opentelemetry-cpp::api)LINK_LIBRARIESandINTERFACE_LINK_LIBRARIESproperties. See_otel_collect_component_dependenciesfor how this is done now at the component level.Setting the target properties is still done in many places today (
EXPORT_NAME,VERSION, andSOVERSIONare separate function calls on each target). These can be consolidated to a functionotel_set_target_propertiesthat takes the export name and description.Looking at the current opentelemetry_metrics target we can already see divergence in the target dependencies between the pkgconfig function and the linked libraries.
Current:
Proposed refactor: