build: relax the version constraint for Protobuf #2132
Conversation
|
@mickey298 could you help test this change? |
tchaikov
force-pushed
the
cmake-protobuf
branch
2 times, most recently
from
d795e35
to
5d34d6b
Compare
cmake/SeastarDependencies.cmake
Outdated
| @@ -159,11 +159,11 @@ macro (seastar_find_dependencies) | |||
| # ProtobufConfig.cmake provided by protobuf defines this linkage. so we try | |||
| # the CMake package configuration file first, and fall back to CMake's | |||
| # FindProtobuf module. | |||
| find_package (Protobuf 2.5.0 | |||
| find_package (Protobuf 22.0...25.3 | |||
There was a problem hiding this comment.
I don't understand how 2.5.0 worked before and now it's 22.0..22.3. I understood there are two versioning systems, but you don't change versioning systems here.
There was a problem hiding this comment.
I don't understand how 2.5.0 worked before and now it's 22.0..22.3. I understood there are two versioning systems, but you don't change versioning systems here.
22.0 is the first version which uses abseil. so i wanted to make this explicit. and i didn't want to mix two versioning systems in a single condition, like find_package (Protobuf 2.5...25.3) even if it would yield the expected outcome. but it's just weird..
Also, does 26 not work?
yeah, i asked myself the same question when writing these numbers. but i wanted to avoid adding versions in future and wanted to avoid pretending that we support versions we haven't even compiled with. that's why it looks this way. but it didn't pan out.
anyway, i am pushing an updated version.
There was a problem hiding this comment.
What I didn't understand is the version jump from 2 to 22. Was the numbering changed? Or do the version numbers refer to different things?
There was a problem hiding this comment.
please note, in the latest revision i stopped using 22 as the version number, because it's not only confusing but also, more importantly, it didn't work due to upstream failed to mark compatibility with the CMake configuration file -- so, v23 is only compatible with v23, etc.. and sorry, i failed to explain my reasoning on the previous revision in an understandable way. let me rephrase it.
more like it refers to a different thing.
actually i didn't bumped the version from 2 to 22. it was because, the CMake configuration file shipped by Protobuf uses the library's version, i.e., 22 and 23..., while the FindProtobuf module uses the language binding's version, like 2.5, 3.12, 3.21, 4.22, .... . the cutting edge distros are more likely to use the CMake configuration file shipped by Protobuf, so i intended to use the library's version when finding the package using the config mode, and i need to pick a library version . instead of trying to map 2.5 to "protobuf v5.0" which does not exst at all. i guess the reason is that it predates the version when protobuf developers decided to using this new versioning approach. so, i used v22, which is the first version which uses abseil, in hope that i could assure that the change works.
|
Is it somehow aligned with the version Prometheus uses (CC @amnonh ) ? |
and add it as a dependency for generating C++ bindings. simpler this way. protobuf::protoc was added to FindProtobuf in CMake 3.10, and our minimal required CMake version is 3.13, so we should be able to have access to this variable as long as Protobuf is found. Signed-off-by: Kefu Chai <kefu.chai@scylladb.com>
tchaikov
force-pushed
the
cmake-protobuf
branch
2 times, most recently
from
9f20ba8
to
efc814e
Compare
as higher versions of Protobuf does not claim to be compatible with v2.5.0, so we should not specify a version number which is acceptable by Seastar as a parameter of `find_package()`. and instead, we just check the version number after calling `find_package()` in config mode. if we find the package using config mode, we search it again, for printing out the version number to the stdout, otherwise we search using its C++ API version. Refs #scylladb#2113 Signed-off-by: Kefu Chai <kefu.chai@scylladb.com>
|
v2:
|
@mykaul hi Yaniv, this change is intended to address the FTBFS on a very bleeding edge distro which
the metrics' on-wire protocol is defined by https://buf.build/gogo/protobuf/file/4df00b267f944190a229ce3695781e99:gogoproto/gogo.proto and https://github.com/prometheus/prometheus/blob/main/prompb/io/prometheus/client/metrics.proto. when it comes to the C++ language binding. i think protobuf compiler should be backward compatible in this regards. |
|
per @mickey298 's test at #2113 (comment) . the change does address the FTBFS. |
|
@scylladb/seastar-maint hello maintainers, could you review this change? |
as higher versions of Protobuf does not claim to be compatible with v2.5.0, so we should not specify a version number which is acceptable by Seastar as a parameter of `find_package()`. and instead, we just check the version number after calling `find_package()` in config mode. if we find the package using config mode, we search it again, for printing out the version number to the stdout, otherwise we search using its C++ API version. Refs #scylladb#2113 Closes scylladb#2132 * github.com:scylladb/seastar: build: relax the version constraint for Protobuf build: use protobuf::protoc for the path to protoc compiler
as higher versions of Protobuf does not claim to be compatible with
v2.5.0, so we should not specify a version number which is acceptable
by Seastar as a parameter of
find_package(). and instead, we justcheck the version number after calling
find_package()in config mode.if we find the package using config mode, we search it again, for
printing out the version number to the stdout, otherwise we search
using its C++ API version.
Refs ##2113