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] Fix build without vcpkg on Windows when gRPC is disabled #3016

Merged
merged 1 commit into from
Aug 2, 2024

Conversation

Corristo
Copy link
Contributor

@Corristo Corristo commented Aug 2, 2024

Currently, if support for gRPC is disabled by configuring the build with -DWITH_OTLP_GRPC=OFF, you are on Windows and do not use a toolchain file then the build will always use all third-party dependencies from vcpkg, regardless of whether nlohmann-json, abseil and protobuf were found or not.

The root cause for this behavior is a bug in the condition meant to check that all dependencies have been found, which unconditionally checks for NOT gRPC_FOUND, but the corresponding find_package call is only performed if WITH_OTLP_GRPC is true, and thus causing that condition to always evaluate to true if WITH_OTLP_GRPC is false.

@Corristo Corristo requested a review from a team August 2, 2024 20:07
Copy link

linux-foundation-easycla bot commented Aug 2, 2024

CLA Signed

The committers listed above are authorized under a signed CLA.

  • ✅ login: Corristo / name: Manuel Bergler (8aac60c)

@Corristo Corristo force-pushed the fix-build-without-grpc branch from 4f76968 to 96334b9 Compare August 2, 2024 20:11
Currently, if support for gRPC is disabled by configuring the build
with `-DWITH_OTLP_GRPC=OFF`, you are on Windows and do not use
a toolchain file then the build will always use all third-party
dependencies from vcpkg, regardless of whether nlohmann-json, abseil
and protobuf were found or not.

The root cause for this behavior is a bug in the condition meant
to check that all dependencies have been found, which unconditionally
checks for `NOT gRPC_FOUND`, but the corresponding `find_package`
call is only performed if `WITH_OTLP_GRPC` is true, and thus causing
that condition to always evaluate to `true` if `WITH_OTLP_GRPC` is
false.
@Corristo Corristo force-pushed the fix-build-without-grpc branch from 96334b9 to 8aac60c Compare August 2, 2024 20:40
@marcalff marcalff changed the title [BUILD] Fix bug preventing build without vcpkg on Windows if gRPC is disabled [BUILD] Fix vcpkg build on Windows when gRPC is disabled Aug 2, 2024
@marcalff marcalff changed the title [BUILD] Fix vcpkg build on Windows when gRPC is disabled [BUILD] Fix build without vcpkg on Windows when gRPC is disabled Aug 2, 2024
@marcalff
Copy link
Member

marcalff commented Aug 2, 2024

Thanks for the PR.

I hope I got the title right, waiting for @lalitb or @ThomsonTan to review (Windows).

Copy link

codecov bot commented Aug 2, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 87.59%. Comparing base (497eaf4) to head (8aac60c).
Report is 111 commits behind head on main.

Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #3016      +/-   ##
==========================================
+ Coverage   87.12%   87.59%   +0.47%     
==========================================
  Files         200      190      -10     
  Lines        6109     5870     -239     
==========================================
- Hits         5322     5141     -181     
+ Misses        787      729      -58     

see 122 files with indirect coverage changes

Copy link
Member

@lalitb lalitb left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks.

@ThomsonTan ThomsonTan merged commit 34ca855 into open-telemetry:main Aug 2, 2024
52 checks passed
@Corristo Corristo deleted the fix-build-without-grpc branch August 3, 2024 08:26
msiddhu added a commit to msiddhu/opentelemetry-cpp that referenced this pull request Aug 20, 2024
* [EXPORTER] Ignore exception when create thread in OTLP file exporter. (open-telemetry#3012)

* [BUILD] Update MODULE.bazel (open-telemetry#3015)

* [BUILD] Fix build without vcpkg on Windows when gRPC is disabled (open-telemetry#3016)

* [BUILD] Add abi_version_no bazel flag. (open-telemetry#3020)

* [Code health] Expand iwyu coverage to include unit tests. (open-telemetry#3022)

* [BUILD] Version opentelemetry_proto/proto_grpc shared libraries (open-telemetry#2992)

* [SEMANTIC CONVENTIONS] Upgrade semantic conventions to 1.27.0 (open-telemetry#3023)

* [SDK] Support empty histogram buckets (open-telemetry#3027)

* support empty buckets

* Update histogram_test.cc

* Update histogram_test.cc

* test for negative values

* fix count

* [TEST] Fix sync problems in OTLP File exporter tests. (open-telemetry#3031)

---------

Co-authored-by: WenTao Ou <[email protected]>
Co-authored-by: Carbo Kuo <[email protected]>
Co-authored-by: Manuel Bergler <[email protected]>
Co-authored-by: Marc Alff <[email protected]>
Co-authored-by: Troels Hoffmeyer <[email protected]>
Co-authored-by: Lalit Kumar Bhasin <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants