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.

Sign up for GitHub

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

Jump to bottom

Feature/add protobuf 3.11.3 version #5127

Merged
merged 4 commits into from
Apr 6, 2021
Merged

Feature/add protobuf 3.11.3 version #5127

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
5f1419d
[boost] Add 'algorithm' lib #3961
CAMOBAP Dec 21, 2020
99b049c
Merge remote-tracking branch 'upstream/master'
CAMOBAP Apr 3, 2021
89c0a21
Revert "[boost] Add 'algorithm' lib #3961"
CAMOBAP Apr 5, 2021
fa118a8
conan-io/conan-center-index#4806 add protobuf/3.11.3 to be used by on…
CAMOBAP Apr 5, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 3 additions & 0 deletions recipes/protobuf/all/conandata.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,9 @@ sources:
"3.9.1":
sha256: 98e615d592d237f94db8bf033fba78cd404d979b0b70351a9e5aaff725398357
url: https://github.com/protocolbuffers/protobuf/archive/v3.9.1.tar.gz
"3.11.3":
sha256: cf754718b0aa945b00550ed7962ddc167167bd922b842199eeb6505e6f344852
url: https://github.com/protocolbuffers/protobuf/archive/v3.11.3.tar.gz
"3.11.4":
sha256: a79d19dcdf9139fa4b81206e318e33d245c4c9da1ffed21c87288ed4380426f9
url: https://github.com/protocolbuffers/protobuf/archive/v3.11.4.tar.gz
Expand Down
2 changes: 2 additions & 0 deletions recipes/protobuf/config.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,8 @@
versions:
"3.9.1":
folder: all
"3.11.3":
folder: all
"3.11.4":
folder: all
Comment on lines +4 to 7
Copy link
Contributor

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?

Copy link
Contributor Author

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)

Copy link
Contributor

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)

Copy link
Contributor

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.

Copy link
Contributor

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.

Copy link
Contributor Author

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?

Copy link
Contributor

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

Copy link
Contributor

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

Copy link
Contributor Author

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

"3.12.4":
Expand Down