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

Support Protobuf 22 #2546

Merged
merged 4 commits into from
Mar 13, 2024
Merged

Support Protobuf 22 #2546

merged 4 commits into from
Mar 13, 2024

Conversation

wasphin
Copy link
Member

@wasphin wasphin commented Feb 25, 2024

What problem does this PR solve?

Issue Number: close #2537

Problem Summary: compatible with protobuf >= 22

What is changed and the side effects?

Changed:

Side effects:

  • Performance effects(性能影响):

  • Breaking backward compatibility(向后兼容性):


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.(请遵循贡献者准则).

@wasphin wasphin marked this pull request as draft February 25, 2024 14:01
@wasphin wasphin force-pushed the feature/protobuf-22 branch from dac0b3e to 5fa8037 Compare February 25, 2024 14:02
@wasphin
Copy link
Member Author

wasphin commented Feb 25, 2024

还有几个问题需要讨论一下:

  1. 目前编译方式有多种, 有一定维护成本:
    • cmake 和 bazel 相对标准, 依赖外部工具;
    • config_brpc.sh 是自定义的用于生成 makefile 的配置脚本, 是否有必要再保留?
  2. Protobuf 22 Log 迁移 absl 后, 需要自定义 absl 的 LogSink 实现自定义的 Log 文件写, 是否可以不在再 brpc 中配置, 改由应用层自定义?

@wasphin wasphin changed the title WIP: Support Protbuf 22 Support Protobuf 22 Feb 25, 2024
# required by absl
set(CMAKE_CXX_STANDARD 17)

find_package(absl REQUIRED CONFIG)
Copy link
Member Author

Choose a reason for hiding this comment

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

基于 CONFIG 方式查找 protobuf 可以推导依赖, 但不是所有 protobuf 都提供, 同时 protoc 相关的任务也需要改

@wwbmmm
Copy link
Contributor

wwbmmm commented Feb 26, 2024

还有几个问题需要讨论一下:

  1. 目前编译方式有多种, 有一定维护成本:

    • cmake 和 bazel 相对标准, 依赖外部工具;
    • config_brpc.sh 是自定义的用于生成 makefile 的配置脚本, 是否有必要再保留?
  2. Protobuf 22 Log 迁移 absl 后, 需要自定义 absl 的 LogSink 实现自定义的 Log 文件写, 是否可以不在再 brpc 中配置, 改由应用层自定义?

config_brpc.sh还是保留吧,挺方便的,不用依赖cmake/bazel

@wasphin wasphin force-pushed the feature/protobuf-22 branch from bd059d8 to df8380b Compare March 4, 2024 15:49
@wasphin wasphin marked this pull request as ready for review March 4, 2024 15:58
@wasphin
Copy link
Member Author

wasphin commented Mar 4, 2024

未进行生产环境验证,需谨慎使用。

@wwbmmm
Copy link
Contributor

wwbmmm commented Mar 11, 2024

LGTM

@Superskyyy
Copy link
Member

请 merge一下 我这里可以验证是不是ok 谢谢

@wasphin wasphin merged commit 37814d2 into apache:master Mar 13, 2024
20 checks passed
@wasphin wasphin deleted the feature/protobuf-22 branch March 13, 2024 05:31
@ketor
Copy link
Contributor

ketor commented Mar 18, 2024

请问这个PR使用的protobuf的版本是多少呢?

用prototbuf 3.22.5版本验证,cmake编译会报错:
/home/mengsz/git/other/brpc/src/json2pb/json_to_pb.cpp: In function ‘bool json2pb::JsonValueToProtoMessage(const butil::rapidjson::Value&, google::protobuf::Message*, const Json2PbOptions&, std::string*, bool)’:
/home/mengsz/git/other/brpc/src/json2pb/json_to_pb.cpp:540:42: error: ‘const struct google::protobuf::Descriptor::ExtensionRange’ has no member named ‘start_number’
540 | for (int tag_number = ext_range->start_number(); tag_number < ext_range->end_number(); ++tag_number)

这个地方是不是代码里判断的值不对啊?

@ketor
Copy link
Contributor

ketor commented Mar 18, 2024

#if GOOGLE_PROTOBUF_VERSION < 4025000 for (int tag_number = ext_range->start; tag_number < ext_range->end; ++tag_number) #else for (int tag_number = ext_range->start_number(); tag_number < ext_range->end_number(); ++tag_number) #endif

在Rocky 8.9的环境实际验证了一下,3.22-3.24版本这个地方都应该维持旧的处理方式,3.25版本开始才改为了start_number()。

所以pb_to_json.cpp:78 和 json_to_pb.cpp:537 的判断都应该改成 GOOGLE_PROTOBUF_VERSION < 4025000 。

@wasphin
Copy link
Member Author

wasphin commented Mar 19, 2024

https://github.com/protocolbuffers/protobuf/blob/v3.24.0/src/google/protobuf/descriptor.h#L469

确实是 24 版本开始的 start_number, 当时改糊涂了

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.

Support protobuf 22+
4 participants