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

added public link of opentelemetry_proto_grpc against [shared] gRPC lib #2268

Merged
merged 3 commits into from
Aug 21, 2023

Conversation

ays7
Copy link
Contributor

@ays7 ays7 commented Aug 16, 2023

only if gRPC library is shared

Fixes #1891

Changes implemented as alternative to PR #1891 do not resolve issues with mac builds against shared gRPC library.
Furthermore, code generated by recent protobuf-grpc compiler contain references into gRPC library, hence opentelemetry_proto_grpc should be linked against gRPC library.

Since it is believed that STATIC gRPC library should only be [privately] linked into a single library, I suppose there are no issues with linking SHARED gRPC library wherever it is needed.

@ays7 ays7 requested a review from a team August 16, 2023 21:36
@lalitb
Copy link
Member

lalitb commented Aug 16, 2023

Thanks. I think the fix is in-line with what we discussed here - #1940 (comment)

@codecov
Copy link

codecov bot commented Aug 16, 2023

Codecov Report

Merging #2268 (81e2867) into main (c0092c4) will decrease coverage by 0.01%.
Report is 1 commits behind head on main.
The diff coverage is n/a.

❗ Current head 81e2867 differs from pull request most recent head cd12356. Consider uploading reports for the commit cd12356 to get more accurate results

Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #2268      +/-   ##
==========================================
- Coverage   87.52%   87.50%   -0.01%     
==========================================
  Files         199      199              
  Lines        5981     5981              
==========================================
- Hits         5234     5233       -1     
- Misses        747      748       +1     

see 1 file with indirect coverage changes

@lalitb lalitb enabled auto-merge (squash) August 21, 2023 20:34
@lalitb lalitb merged commit 4bfddde into open-telemetry:main Aug 21, 2023
42 checks passed
waywardmonkeys added a commit to waywardmonkeys/vcpkg that referenced this pull request Jan 2, 2024
The patch to add a dependency of opentelemetry-proto on gRPC should
not be needed after the upstream changes in:

open-telemetry/opentelemetry-cpp#2268
waywardmonkeys added a commit to waywardmonkeys/vcpkg that referenced this pull request Jan 2, 2024
The patch to add a dependency of opentelemetry-proto on gRPC should
not be needed after the upstream changes in:

open-telemetry/opentelemetry-cpp#2268

Additionally, the "zpages" feature is removed as it is no longer
present upstream after having been deprecated in a previous release.
waywardmonkeys added a commit to waywardmonkeys/vcpkg that referenced this pull request Jan 2, 2024
The patch to add a dependency of opentelemetry-proto on gRPC should
not be needed after the upstream changes in:

open-telemetry/opentelemetry-cpp#2268

Additionally, the "zpages" feature is removed as it is no longer
present upstream after having been deprecated in a previous release.

Fixes microsoft#35992.
waywardmonkeys added a commit to waywardmonkeys/vcpkg that referenced this pull request Jan 4, 2024
The patch to add a dependency of opentelemetry-proto on gRPC should
not be needed after the upstream changes in:

open-telemetry/opentelemetry-cpp#2268

Additionally, the "zpages" feature is removed as it is no longer
present upstream after having been deprecated in a previous release.

A patch is added extracted from open-telemetry/opentelemetry-cpp#2475
which fixes problems that arose after `NOMINMAX` was no longer
defined within opentelemetry-cpp on Windows.

Fixes microsoft#35992.
waywardmonkeys added a commit to waywardmonkeys/vcpkg that referenced this pull request Jan 4, 2024
The patch to add a dependency of opentelemetry-proto on gRPC should
not be needed after the upstream changes in:

open-telemetry/opentelemetry-cpp#2268

Additionally, the "zpages" feature is removed as it is no longer
present upstream after having been deprecated in a previous release.

A patch is added extracted from open-telemetry/opentelemetry-cpp#2475
and open-telemetry/opentelemetry-cpp#2449
which fix problems that arose after `NOMINMAX` was no longer
defined within opentelemetry-cpp on Windows.

Fixes microsoft#35992.
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.

3 participants