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

fix thrift version gt 0.13.0 #2257

Merged
merged 3 commits into from
Sep 7, 2023
Merged

Conversation

ZjuYTW
Copy link
Contributor

@ZjuYTW ZjuYTW commented May 19, 2023

What problem does this PR solve?

Issue Number: #1328

Problem Summary:
When using thrift version greater than 0.13.0, brpc can't compile correctly due to deprecate of boost.

What is changed and the side effects?

Changed:

Now compile logic works as follow:
For version < 0.11, set THRIFT_VERSION_LOWER_THAN_0_11_0 in cmake and using boost
For 0.11 <= version < 0.13, THRIFT_STDCXX_H is set inside thrift/stdcxx.h and use apache::thrift::stdcxx
For later, just use std

Side effects:

  • Performance effects(性能影响):
    No
  • Breaking backward compatibility(向后兼容性):
    No, later version of thrift use std as well.

Check List:

  • Please make sure your changes are compilable(请确保你的更改可以通过编译).
  • When providing us with a new feature, it is best to add related tests(如果你向我们增加一个新的功能, 请添加相关测试).
  • Please follow Contributor Covenant Code of Conduct.(请遵循贡献者准则).

@ZjuYTW ZjuYTW force-pushed the fix_thrift_version branch 2 times, most recently from 4cc5992 to 57024f8 Compare May 19, 2023 15:03
@ZjuYTW
Copy link
Contributor Author

ZjuYTW commented May 19, 2023

seems also need to address other two compile methods

@ZjuYTW ZjuYTW changed the title fix thrift version gt 0.13.0 [WIP]fix thrift version gt 0.13.0 May 19, 2023
@ZjuYTW ZjuYTW changed the title [WIP]fix thrift version gt 0.13.0 fix thrift version gt 0.13.0 May 22, 2023
@ZjuYTW
Copy link
Contributor Author

ZjuYTW commented May 22, 2023

No so sure if I need to get bazel compatible for thrift >= 0.13, seems there is no enough doc about how to build by bazel...
And maybe later I should add some CI tests for thrift >= 0.13?

@wwbmmm
Copy link
Contributor

wwbmmm commented Jun 25, 2023

No so sure if I need to get bazel compatible for thrift >= 0.13, seems there is no enough doc about how to build by bazel...
And maybe later I should add some CI tests for thrift >= 0.13?

The bazel build use thrift 0.15.0 and CI is success. It seems that bazel build already supports thrift > 0.13.0 ?

@ZjuYTW
Copy link
Contributor Author

ZjuYTW commented Jul 2, 2023

@wwbmmm
In BUILD.bazel L44 says -DTHRIFT_STDCXX=std on default condition.

so already compatible.

@wwbmmm
Copy link
Contributor

wwbmmm commented Aug 16, 2023

This PR has conflicts with master, please resolve @ZjuYTW

Signed-off-by: wangyitao <wangyitao999@outlook.com>
Signed-off-by: wangyitao <wangyitao999@outlook.com>
@ZjuYTW
Copy link
Contributor Author

ZjuYTW commented Aug 26, 2023

This PR has conflicts with master, please resolve @ZjuYTW

done @wwbmmm

@wwbmmm
Copy link
Contributor

wwbmmm commented Aug 30, 2023

LGTM

@ZjuYTW
Copy link
Contributor Author

ZjuYTW commented Sep 4, 2023

maybe we can merge it? @wwbmmm

@wwbmmm wwbmmm merged commit 5dc6562 into apache:master Sep 7, 2023
16 checks passed
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.

2 participants