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.

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

[BUILD] CURL versions discrepancies with bazel #2520

Open
ecourreges-orange opened this issue Feb 1, 2024 · 5 comments
Open

[BUILD] CURL versions discrepancies with bazel #2520

ecourreges-orange opened this issue Feb 1, 2024 · 5 comments
Labels
bug Something isn't working Stale triage/accepted Indicates an issue or PR is ready to be actively worked on.

Comments

@ecourreges-orange
Copy link
Contributor

ecourreges-orange commented Feb 1, 2024

Describe your environment
Ubuntu 20.04 with its curl/libcurl v 7.68.0-1ubuntu2.21
bazel 5.4.1

Steps to reproduce
Created an OtlpHttpExporter

What is the expected behavior?
No problem connecting, unsupported options not being used

What is the actual behavior?

[Error] File: external/io_opentelemetry_cpp/ext/src/http/client/curl/http_operation_curl.cc:526 CURL, set option <20312> failed: <An unknown option was passed in to libcurl>
[Error] File: external/io_opentelemetry_cpp/exporters/otlp/src/otlp_http_client.cc:200 [OTLP HTTP Client] Session state: connection failed.An unknown option was passed in to libcurl
[Error] File: external/io_opentelemetry_cpp/exporters/otlp/src/otlp_http_client.cc:168 [OTLP HTTP Client] Session state: session create failed.
[Error] File: external/io_opentelemetry_cpp/exporters/otlp/src/otlp_http_exporter.cc:118 [OTLP HTTP Client] ERROR: Export 4 trace span(s) error: 1 

20312 is CURLOPT_PREREQFUNCTION you can see here that it is supported in curl 7.80.0 and above, although otlp exporter tries to use it for 7.5 and above which is exactly my case for producing the bug 7.5<7.68<7.80

So I think simply changing the test for the curl version will fix this bug, and I will post a PR for that in a couple of minutes.

#if LIBCURL_VERSION_NUM >= 0x075000

#if LIBCURL_VERSION_NUM >= 0x075000

The other option CURLOPT_PREREQDATA which is also concerned appeared in the same curl version 7.80.0, so all should be fine in one fix.

Not sure I can unit test that though...

@ecourreges-orange ecourreges-orange added the bug Something isn't working label Feb 1, 2024
@github-actions github-actions bot added the needs-triage Indicates an issue or PR lacks a `triage/foo` label and requires one. label Feb 1, 2024
@marcalff
Copy link
Member

marcalff commented Feb 1, 2024

Bonjour @ecourreges-orange

Please double check which version of curl headers are used, and which version of the curl library is used to link, in your build: I suspect a discrepancy there.

See comments in the PR.

@ecourreges-orange
Copy link
Contributor Author

Bonjour @marcalff
Thanks for your expertise, the problem is I guess only on the bazel build, because the curl version downloaded is fixed and is 8.4.0, and my system (Ubuntu 20.04 focal) has 7.68.0

"https://github.com/curl/curl/releases/download/curl-8_4_0/curl-8.4.0.tar.gz",

In the cmake build, the user brings his own curl version.
I am running an apache module which is dynamically loaded, I can't afford to have a different curl than the system, because other apache modules probably use libcurl of the system.
An ldd of my app confirms that curl is loaded dynamically from the system, which causes the version discrepancy.

As far as the solutions, I suppose the bazel build could use the system curl if it founds it, instead of downloading it?

@marcalff
Copy link
Member

marcalff commented Feb 5, 2024

Thanks for the details.

This issue needs to stay opened, to see how to address bazel.

@marcalff marcalff changed the title otlp http exporter fails to connect: curl option CURLOPT_PREREQFUNCTION is used on unsupported versions of curl [BUILD] CURL versions discrepancies with bazel Feb 5, 2024
@ecourreges-orange
Copy link
Contributor Author

For now I have a working version in my "nobazelcurl" fork, replacing libcurl deps like this to use the system version of curl.
Maybe just having a switch between this and the existing code would make it a decent enough solution for a PR.

https://github.com/ecourreges-orange/opentelemetry-cpp/blob/88a5f141628c5de7c52cd9eb31d8dfee66918c3a/bazel/repository.bzl#L155

    native.new_local_repository(
        name = "curl",
        path = "/usr",
        build_file = "@io_opentelemetry_cpp//bazel:systemcurl.BUILD"
    )

https://github.com/ecourreges-orange/opentelemetry-cpp/blob/nobazelcurl/bazel/systemcurl.BUILD

cc_import(
    name = "curl",
    shared_library = "lib/x86_64-linux-gnu/libcurl.so",
    static_library = "lib/x86_64-linux-gnu/libcurl.a",
    visibility = ["//visibility:public"],
)

@marcalff marcalff added triage/accepted Indicates an issue or PR is ready to be actively worked on. and removed needs-triage Indicates an issue or PR lacks a `triage/foo` label and requires one. labels Feb 7, 2024
Copy link

github-actions bot commented Apr 8, 2024

This issue was marked as stale due to lack of activity.

@github-actions github-actions bot added the Stale label Apr 8, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working Stale triage/accepted Indicates an issue or PR is ready to be actively worked on.
Projects
None yet
2 participants