ci: Resolving nginx & prometheus build issue - #667
thompson-tomo wants to merge 38 commits into
Conversation
| include(FetchContent) | ||
|
|
||
| link_directories(${CMAKE_ARCHIVE_OUTPUT_DIRECTORY}) | ||
| set(ENABLE_TESTING OFF CACHE BOOL "") |
There was a problem hiding this comment.
Can you check this once - it seems to be disabling the test for both dependencies and the exporter. Also seen this in CI:
Updating test list for fixtures
Added 0 tests to meet fixture requirements
Checking test dependency graph...
Checking test dependency graph end
No tests were found!!! ??
| include(CMakePackageConfigHelpers) | ||
| file(MAKE_DIRECTORY "${CMAKE_LIBRARY_OUTPUT_DIRECTORY}/cmake/${PROJECT_NAME}") | ||
|
|
||
| configure_package_config_file( |
There was a problem hiding this comment.
With this removal, the downstream CMake projects can no longer discover and link the installed exporter through its existing package target.
There was a problem hiding this comment.
Has this been resolved with the newer commits?
There was a problem hiding this comment.
When built as a main project these lines are required to install the package. Please revert these removals for that case.
To keep the import target name the same please update the project namespace to be opentelemetry-cpp-contrib::
There was a problem hiding this comment.
These were causing build issues when moving away from using a local version of otel cpp & also other packages didn't have this but did have the add otel_component.
My impression was that is the newer pattern?
There was a problem hiding this comment.
Can you share more about the build issues? I'm hopeful we can get the independent install working.
The otel_add_component function is part of the main opentelemtry-cpp project's cmake utilities (added in v1.21) and was used in the user_events and geneva CMake files to allow injecting those contrib components into the main opentelemetry-cpp package when creating the vcpkg port.
I don't think we should propagate that pattern to inject external components into the core package any further in this repo. An alternative approach is to create a top level project for opentelemetry-cpp-contrib with its own utility functions to manage the install of the components. I started prototyping this in #550.
There was a problem hiding this comment.
From memory it was related to when switching to fetching of the libraries as opposed to installing them it caused the generated *-config.cmake to fail or something like that.
I don't think we should propagate that pattern to inject external components into the core package any further in this repo. An alternative approach is to create a top level project for opentelemetry-cpp-contrib with its own utility functions to manage the install of the components.
That sounds like a good solution as it would also enable testing of the non main project path in ci.
|
Thanks for continuing to improve the CI across contrib - these fixes are really appreciated. For this PR, the NGINX changes look good. Please revisit the Prometheus CMake workflow and dependency changes so we preserve the existing test, dependency, and packaging contracts. |
|
I am quite confused about the prometheus package purpose on the surface it should be an exporter just like user_events, however when you look at the cmake its name indicates otherwise. If the name is correct then shouldn't there be a file in the root for the contrib repo & enable the prometheus to mirror other exporters. In the case of the later then the name etc should be updated. |
|
CI is failing since debian:11 is now EOL. Update to debian 13 ? |
|
@marcalff have bumped it to debian 12 |
NGINX instrumentation build failures have been addressed by explicitly using buildx & switching tests to use php docker image which exists. Have also updated the tests to use nil for remote-parent-id rather than empty string.
Prometheus bazel build resolved by specifying an older bazel version to use.
Prometheus cmake build resolved by changing zlib adding process as well as switching to a native cmake build.
This is a replacement for #633 which i accidentally broke which triggered the closing of it.