Skip to content
This repository has been archived by the owner on Aug 30, 2022. It is now read-only.

[thrift]: drop stdcxx namespace #239

Merged
merged 3 commits into from
Sep 16, 2021
Merged

[thrift]: drop stdcxx namespace #239

merged 3 commits into from
Sep 16, 2021
+266 −267

Conversation

ideepika
Copy link
Contributor

@ideepika ideepika commented Aug 21, 2020 •
edited

since thrift has dropped support for stdcxx.h that makes jaeger
incompatible with latest thrift and hence jaeger failure:

/home/ideepika/ceph-primary/ceph/src/jaegertracing/jaeger-client-cpp/src/jaegertracing/thrift-gen/agent_types.h:18:10: fatal error: thrift/stdcxx.h: No such file or directory
18 | #include <thrift/stdcxx.h>
| ^~~~~~~~~~~~~~~~~
this change will make jaeger compatible with latest thrift.

see: apache/thrift#1677

Note: their won't be development and maintenance effort for this project henceforth, please see: https://medium.com/jaegertracing/jaeger-and-opentelemetry-1846f701d9f2

thrift: drop stdcxx namespace
0a7269b 
since thrift has dropped support for stdcxx.h that makes jaeger
incompatible with latest thrift and hence jaeger failure:

/home/ideepika/ceph-primary/ceph/src/jaegertracing/jaeger-client-cpp/src/jaegertracing/thrift-gen/agent_types.h:18:10: fatal error: thrift/stdcxx.h: No such file or directory
   18 | #include <thrift/stdcxx.h>
      |          ^~~~~~~~~~~~~~~~~
this change will make jaeger compatible with latest thrift.

see: apache/thrift#1677

Signed-off-by: Deepika Upadhyay <dupadhya@redhat.com>
@ideepika
Copy link
Contributor Author

we need to update the version to thrift 0.13.0 in hunter, I didn't have the bandwidth for it right now, maybe someone can do that.

@ideepika
Copy link
Contributor Author

solves: #195

@ideepika
Copy link
Contributor Author

ideepika commented Aug 21, 2020 •
edited

@mdouaihy @yurishkuro ping :)

@AppVeyorBot
Copy link

✅ Build jaeger-client-cpp 142 completed (commit d47b7a9107 by @ideepika)

thrift: fix warning due to difference in naming.
be19ad9 
fixes warning:

CMake Warning (dev) at /usr/share/cmake/Modules/FindPackageHandleStandardArgs.cmake:272 (message):
  The package name passed to `find_package_handle_standard_args` (THRIFT)
  does not match the name of the calling package (thrift).  This can lead to
  problems in calling code that expects `find_package` result variables
  (e.g., `_FOUND`) to follow a certain pattern.
Call Stack (most recent call first):
  cmake/Findthrift.cmake:92 (FIND_PACKAGE_HANDLE_STANDARD_ARGS)
  CMakeLists.txt:63 (find_package)
This warning is for project developers.  Use -Wno-dev to suppress it.

Signed-off-by: Deepika Upadhyay <dupadhya@redhat.com>
@AppVeyorBot
Copy link

✅ Build jaeger-client-cpp 143 completed (commit 3153d0cd8a by @ideepika)

@dvzubarev
Copy link

Hi, thanks for this PR.
I was trying to build it without Hunter and failed on linking stage.
I had to replace THRIFT_LIBRARIES with thrift_LIBRARIES in CMakeLists.txt. I guess, this change should be included in this PR too.

@ideepika
Copy link
Contributor Author

Hi, thanks for this PR.
I was trying to build it without Hunter and failed on linking stage.
I had to replace THRIFT_LIBRARIES with thrift_LIBRARIES in CMakeLists.txt. I guess, this change should be included in this PR too.

👍 yes, I have added it in a separate commit in this PR itself

@david-antiteum david-antiteum mentioned this pull request Jun 3, 2021
Closed
@bayarcher
Copy link

Hi,
This patch will help use newer version of thrift.
Can we expect this patch to be approved?

yurishkuro
yurishkuro reviewed Jul 2, 2021
View reviewed changes
cmake/Findthrift.cmake
# THRIFT_INCLUDE_DIR, where to find THRIFT headers
# THRIFT_COMPILER, thrift compiler executable
# THRIFT_FOUND, If false, do not try to use ant
# thrift_VERSION_STRING, version string of ant if found
Copy link
Member

Choose a reason for hiding this comment

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

why is this lower-case change needed?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fixes warning:

CMake Warning (dev) at /usr/share/cmake/Modules/FindPackageHandleStandardArgs.cmake:272 (message):
The package name passed to find_package_handle_standard_args (THRIFT)
does not match the name of the calling package (thrift). This can lead to
problems in calling code that expects find_package result variables
(e.g., _FOUND) to follow a certain pattern.
Call Stack (most recent call first):
cmake/Findthrift.cmake:92 (FIND_PACKAGE_HANDLE_STANDARD_ARGS)
CMakeLists.txt:63 (find_package)
This warning is for project developers. Use -Wno-dev to suppress it.

@mdouaihy
Copy link
Contributor

mdouaihy commented Jul 2, 2021

hello,

I ll review the change today and get back to you.

@ingomueller-net
Copy link

It seems like this PR is ready to be merged, no? Click the button?

@AppVeyorBot
Copy link

✅ Build jaeger-client-cpp 250 completed (commit 3049c925ed by @yurishkuro)

@ideepika
Copy link
Contributor Author

@yurishkuro ping!

@yurishkuro yurishkuro merged commit 3650fff into jaegertracing:master Sep 16, 2021
@ideepika ideepika deleted the thrift-upgrade branch September 16, 2021 21:59
@ideepika ideepika changed the title thrift: drop stdcxx namespace [there won't be much maintainence effort for jaeger cpp client henceforth, please consider migrating to OpenTelemetry C++ SDK instead] thrift: drop stdcxx namespace Oct 8, 2021
@ideepika ideepika changed the title [there won't be much maintainence effort for jaeger cpp client henceforth, please consider migrating to OpenTelemetry C++ SDK instead] thrift: drop stdcxx namespace [there won't be much maintenance effort for jaeger-cpp-client henceforth, please consider migrating to OpenTelemetry C++ SDK instead] thrift: drop stdcxx namespace Oct 8, 2021
@ideepika ideepika changed the title [there won't be much maintenance effort for jaeger-cpp-client henceforth, please consider migrating to OpenTelemetry C++ SDK instead] thrift: drop stdcxx namespace [please consider migrating to OpenTelemetry C++ SDK] thrift: drop stdcxx namespace Oct 8, 2021
@mbeaulie
Copy link

@yurishkuro are there plan to issue a new release/tag with this thrift update given thrift has a few security issues? We are looking at moving to openTelemetry C++ but we would like to quickly issue security fixes to customers while the port to openTelemetry gets done. If not, we might consider testing with the latest master.

@yurishkuro
Copy link
Member

https://github.com/jaegertracing/jaeger-client-cpp/releases/tag/v0.9.0

@yurishkuro yurishkuro changed the title [please consider migrating to OpenTelemetry C++ SDK] thrift: drop stdcxx namespace [thrift]: drop stdcxx namespace Jan 21, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@yurishkuro yurishkuro yurishkuro left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
8 participants
@ideepika @AppVeyorBot @dvzubarev @bayarcher @mdouaihy @ingomueller-net @mbeaulie @yurishkuro