-
Notifications
You must be signed in to change notification settings - Fork 1.8k
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
Feature/add protobuf 3.11.3 version #5127
Feature/add protobuf 3.11.3 version #5127
Conversation
config.yml syntax error in build 1:
|
This reverts commit 5f1419d.
d9447d6
to
42f8e1a
Compare
42f8e1a
to
fa118a8
Compare
Failure in build 2 (
Note: To save resources, CI tries to finish as soon as an error is found. For this reason you might find that not all the references have been launched or not all the configurations for a given reference. Also, take into account that we cannot guarantee the order of execution as it depends on CI workload and workers availability. |
All green in build 3 (
|
"3.11.3": | ||
folder: all | ||
"3.11.4": | ||
folder: all |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Typically we discourage add older versions, I see in you comment message there's a motivation... but just to confirm is it absolutely required? It should work perfectly fine with the version 3.11.4
, is it known to be unsupported?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@prince-chrismc I have the same vision.
I am sure that there are no specific reason why onnxruntime
depends on protobuf/3.11.3
but for last stable depends on this specific version
Before I even reported ticket (to allow change protobuf version) microsoft/onnxruntime#4026 (comment) it possible to do it by changing submodule revision. I didn't tested it yet but this will force to use git
instead of zip
which I consider as not good solution (correct me if I'm wrong)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Without looking at that project, you should not require this old version based on the comment... the one that exists should be adequate
I am not sure about the second half of your comment but I can offer this bit of information...
Most often we patch projects so they can remove vendored third party and use conan instead (shouldn't be using submodules)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
AFAIK protobuf is special and requires the exactly the same version of the library, because generated .proto files contain version checks. if some library contains already generated .proto files (e.g. OpenCV), you can't just pick latest protobuf - it won't compile.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
a typical example:
https://raw.githubusercontent.com/opencv/opencv/master/modules/dnn/misc/caffe/opencv-caffe.pb.h
#if GOOGLE_PROTOBUF_VERSION < 3005000
#error This file was generated by a newer version of protoc which is
#error incompatible with your Protocol Buffer headers. Please update
#error your headers.
#endif
#if 3005001 < GOOGLE_PROTOBUF_MIN_PROTOC_VERSION
#error This file was generated by an older version of protoc which is
#error incompatible with your Protocol Buffer headers. Please
#error regenerate this file with a newer version of protoc.
#endif
it doesn't allow anything besides 3.5.0 or 3.5.1, you'll need exactly that version to compile.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, @SSE4 you are right, protobuf
do "exact" version check
I just tried to say that there is no real reason why onnxruntime
depends on protobuf
3.11.3 but not 3.11.4, I sure they can easily migrate to 3.11.4 but I think this should be up to ommxruntime
team.
@SSE4 @prince-chrismc to be on the same page, adding a new "legacy" version will create extra load on conan's infrastructure which we trying to avoid, right?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @SSE4 for that info!
We've avoid it because they are rarely (almost never used) in practice, this is an exception where an exact version is required so there's a motivation to have "more" offerings
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
3.11.4 has no documented changes for C/C++, so it might be fine to use that version. Have you tried it?
https://github.com/protocolbuffers/protobuf/releases/tag/v3.11.4
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@Croydon AFAIR even minor version mismatch will lear to compilation error
Shouldn't we add |
There are more packages which use v as prefix. It's not something new, we don't use it since conan-community and bincrafters. It can follow upstream, but removing it keep simpler. spirv-tools or really requires or was accepted by mistake. We can add it as recommendation on FAQs. |
* [boost] Add 'algorithm' lib conan-io#3961 * Revert "[boost] Add 'algorithm' lib conan-io#3961" This reverts commit 5f1419d. * conan-io#4806 add protobuf/3.11.3 to be used by onnxruntime/1.7.1 later
Specify library name and version: lib/1.0
This is also a good place to share with all of us why you are submitting this PR (specially if it is a new addition to ConanCenter): is it a dependency of other libraries you want to package? Are you the author of the library? Thanks!
conan-center hook activated.