[C++] Allow building against system opentelemetry #44982
Comments
assignUser
changed the title
assignUser
changed the title
|
Hm you explicitly set |
|
Looking at the code that area of the cmake could probably be substantially improved when we move to newer cmake as discussed in #44950 |
|
It seems that we can remove the Could you share full error log URL? |
|
I don't think we modify the cmake prefix path in anyway that should prevent otel from being found. I can confirm that a normal diff --git a/cpp/cmake_modules/ThirdpartyToolchain.cmake b/cpp/cmake_modules/ThirdpartyToolchain.cmake
index 35ad4089e..c12d6c7af 100644
--- a/cpp/cmake_modules/ThirdpartyToolchain.cmake
+++ b/cpp/cmake_modules/ThirdpartyToolchain.cmake
@@ -4966,8 +4966,9 @@ if(ARROW_WITH_OPENTELEMETRY)
# cURL is required whether we build from source or use an existing installation
# (OTel's cmake files do not call find_curl for you)
find_curl()
- set(opentelemetry-cpp_SOURCE "AUTO")
+ set(CMAKE_FIND_DEBUG_MODE TRUE)
resolve_dependency(opentelemetry-cpp)
+ set(CMAKE_FIND_DEBUG_MODE FLASE)
set(ARROW_OPENTELEMETRY_LIBS
opentelemetry-cpp::trace
opentelemetry-cpp::logs
|
|
@assignUser, here are the logs for your suggestion, lightly cleaned up by resubstituting common variables. I really don't know |
|
Does |
|
Yes, see the package contents I posted above, in particular (2nd line under "Files") (which is relative to |
|
Can we check just to be sure? For example, can we log installed locations as verbose log messages? |
|
|
It's 100% certain to be there. This is the environment location that everything gets installed in and the package content I keep referencing gets unpacked there. |
Let me rewrite this |
|
|
|
Ah, sorry. It's for Windows. |
This is in reaction to the admonition by CMake mentioned in the OP:
If you're referring to |
|
Is The following log doesn't include |
|
Yeah it's not there, I added So this is not on our end. |
I apologise, the recipe had changed enough since I initially opened the PR for otel that I must have missed a spot while rebasing. I had added it as a dependency to I've pushed a new run with more minimal changes (e.g. |
|
Good news: the builds have passed the config stage on all platforms! I left the patch removing I'll also need to double-check if a patch I had come with 1.5 years ago for this (that still applies) is still relevant diff --git a/cpp/src/arrow/util/tracing_internal.h b/cpp/src/arrow/util/tracing_internal.h
index 6ed731599..79907cf0c 100644
--- a/cpp/src/arrow/util/tracing_internal.h
+++ b/cpp/src/arrow/util/tracing_internal.h
@@ -122,13 +122,13 @@ struct Scope {
opentelemetry::trace::Scope scope_impl;
};
-opentelemetry::nostd::shared_ptr<opentelemetry::trace::Span>& UnwrapSpan(
+ARROW_EXPORT opentelemetry::nostd::shared_ptr<opentelemetry::trace::Span>& UnwrapSpan(
::arrow::util::tracing::SpanDetails* span);
-const opentelemetry::nostd::shared_ptr<opentelemetry::trace::Span>& UnwrapSpan(
+ARROW_EXPORT const opentelemetry::nostd::shared_ptr<opentelemetry::trace::Span>& UnwrapSpan(
const ::arrow::util::tracing::SpanDetails* span);
-opentelemetry::nostd::shared_ptr<opentelemetry::trace::Span>& RewrapSpan(
+ARROW_EXPORT opentelemetry::nostd::shared_ptr<opentelemetry::trace::Span>& RewrapSpan(
::arrow::util::tracing::SpanDetails* span,
opentelemetry::nostd::shared_ptr<opentelemetry::trace::Span> ot_span);It likely is though, as shared builds on windows tend to fail if required symbols aren't exported. |
|
Hm. Windows fails with something that sounds related to |
### Rationale for this change We don't need to disallow `-Dopentelemetry-cpp_SOURCE=SYSTEM`. ### What changes are included in this PR? Remove forcing `opentelemetry-cpp_SOURCE=AUTO`. ### Are these changes tested? Yes. ### Are there any user-facing changes? Yes. * GitHub Issue: #44982 Authored-by: Sutou Kouhei <[email protected]> Signed-off-by: Sutou Kouhei <[email protected]>
|
Issue resolved by pull request 44983 |
|
The Windows related problems are another problems. Let's work on them in another issue. |
Thanks a lot! 🙏 Sorry again for the false start.
How so? They appeared only once I enabled otel support...
Fine by me! |
|
The The |
Describe the enhancement requested
I've been wanting to add opentelemetry support in conda-forge for a long time, but given the amount of mutation that happens in packaging there, I wasn't comfortable in doing that before landing conda-forge/arrow-cpp-feedstock#1058.
Now that this is in, I've rebased the PR, but am running into problems pretty quickly. For one, arrow hardcodes
arrow/cpp/cmake_modules/ThirdpartyToolchain.cmake
Line 4974 in 8428c47
even if the config explicitly asks for
-Dopentelemetry-cpp_SOURCE="SYSTEM". With the following patchI then run into further problems in
ThirdpartyToolchain.cmakedespite the fact that our otel packages clearly contain the CMake metadata.
CC @lidavidm @kou
Component(s)
C++, Packaging
The text was updated successfully, but these errors were encountered: