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

Please include Protobuf into conan-center #381

Closed
Croydon opened this issue Jul 25, 2018 · 44 comments
Closed

Please include Protobuf into conan-center #381

Croydon opened this issue Jul 25, 2018 · 44 comments
Assignees
Labels
blocked blocked by other issue conan-center Review for inclusion on Conan Center enhancement

Comments

@Croydon
Copy link
Member

Croydon commented Jul 25, 2018

In order to move the creation of a gRPC recipe further, it would be really helpful to have Protobuf in conan-center. Please consider making an inclusion request for Protobuf.

Furthermore there seems to quite some demand for it anyway 😄

https://croydon.github.io/conan_inquiry/#!stats

Packages available in many remotes but not in conan-center
Protobuf has 7 remotes

@uilianries uilianries added enhancement packaging Plans to create a package within Bincrafters labels Jul 26, 2018
@uilianries uilianries self-assigned this Jul 26, 2018
@uilianries uilianries added the upstream update Packages need updates for new upstream versions label Jul 26, 2018
@uilianries

This comment has been minimized.

@uilianries uilianries removed the packaging Plans to create a package within Bincrafters label Jul 26, 2018
@uilianries

This comment has been minimized.

@SSE4
Copy link
Member

SSE4 commented Jul 26, 2018

/cc @zamazan4ik

@uilianries

This comment has been minimized.

@Croydon

This comment has been minimized.

@uilianries

This comment has been minimized.

@uilianries

This comment has been minimized.

@SSE4

This comment has been minimized.

@uilianries

This comment has been minimized.

@SSE4

This comment has been minimized.

@Croydon

This comment has been minimized.

@uilianries

This comment has been minimized.

@SSE4

This comment has been minimized.

@uilianries

This comment has been minimized.

@Croydon

This comment has been minimized.

@SSE4

This comment has been minimized.

@uilianries

This comment has been minimized.

@SSE4
Copy link
Member

SSE4 commented Aug 10, 2018

@uilianries are we done with it? :)

@uilianries
Copy link
Member

Yes! We could request for the inclusion on Conan center 😄

@Croydon
Copy link
Member Author

Croydon commented Aug 13, 2018

Please go ahead then :)

@uilianries
Copy link
Member

I just requested the inclusion 3 days ago 😄

@Croydon Croydon added the blocked blocked by other issue label Sep 16, 2018
@SSE4
Copy link
Member

SSE4 commented Oct 4, 2018

NOTE: protoc_installer couldn't be just build_require, it will be usual require, since all packages dependent on protobuf will need protoc, not just protobuf itself

@SSE4
Copy link
Member

SSE4 commented Oct 7, 2018

status update:
protoc_installer and protobuf are working good with cross-compilation on 3.6.1 branch.
now I'll take my time to backport changes into 3.5.1/3.5.2 branches, and we're ready for conan-center inclusion I believe.

@SSE4
Copy link
Member

SSE4 commented Oct 10, 2018

@danimtb I think we're ready to go. I've backported changes to 3.5.2 branch, and 3.6.1 is also done. I don't think we need 3.5.1 if we have 3.5.2 (I don't see the point into supporting two minor releases). so, please take a look.

@SSE4 SSE4 removed the blocked blocked by other issue label Oct 10, 2018
@Croydon
Copy link
Member Author

Croydon commented Oct 31, 2018

@danimtb Any news? 😄

@Croydon Croydon added conan-center Review for inclusion on Conan Center and removed upstream update Packages need updates for new upstream versions help wanted labels Oct 31, 2018
@uilianries
Copy link
Member

I updated both conan-protobuf and conan-protoc_installer

https://github.com/bincrafters/conan-protobuf/pulls
https://github.com/bincrafters/conan-protoc_installer/pulls

@danimtb
Copy link

danimtb commented Nov 5, 2018

Hi!

Update: We are currently discussing the dependency relationship among protobuf, protoc and consumers of protobuf library. We are making some progress to have it included in Conan Center but will still require some work.

Thanks @uilianries for updating the recipes.

@elizagamedev
Copy link
Member

Currently the protobuf recipe exposes all of libprotobuf, libprotobuf-lite, and libprotoc to consumers. As I've discovered, linking to both libprotobuf and libprotobuf-lite will result in undefined behavior. Additionally, linking to libprotoc is not a common use case. To resolve these problems, would this design be acceptable?

  • A new boolean option lite which defaults to False and appends either libprotobuf or libprotobuf-lite depending on its value to cpp_info.libs
  • A new boolean option libprotoc which defaults to False and, if true, appends libprotoc to cpp_info.libs

@uilianries
Copy link
Member

/cc @jgsogo

@jgsogo
Copy link
Member

jgsogo commented Nov 12, 2018

Hi!

I think that this should be the dependency graph associated with these packages (meaning of arrows is requires/depends on):
image

As @elizagamedev says, we cannot link both with libprotobuf and libprotobuf-lite. Talking about CMake, consumer can use targets and link against CONAN_PCK::libprotobuf or CONAN_PCK::libprotobuf-lite, but using the direct approach with ${CONAN_LIBS}, we should have a way to select which one to link (and yes, by default I would use the no-lite one). About libprotoc, as it is contained in another package (I think it will usually be a build_requires) it won't be exposed in ${CONAN_LIBS}

@uilianries
Copy link
Member

Hi @jgsogo, thanks for you help! 😸

So we could create two packages as well, but protoc_installer will depend on protobuf, if I get it.

Also, we will need to add an option for protobuf-lite and exclude libprotobuf from CONAN_LIBS, but if on same build the customer requires protoc and protobuf there will be a problem.

Will we need to create a new package for libprotobuf-lite only? 🤔

As libprotoc is only required by protoc, we could use static link.

@jgsogo
Copy link
Member

jgsogo commented Nov 12, 2018

Yes, protoc_installer depends on protobuf (it happens the other way around (not binary dep, just workflow) if we were to compile tests, as most of the tests for the protobuf package requires protoc binary to generate code based on .proto files, but we are not building nor running tests).

protobuf package must allow the consumer to choose between linking with libprotobuf or libprotobuf-lite (usually the first one, I think, but I've always developed for systems plenty of resources).

If the consumer just want protoc to generate code, it will be ok to link statically, but someone may want to link against libprotoc in order to make his own protobuf generator (who knows...).

So the problem will be if the consumer requires both of them. We will have a diamond and we will transitively link to libprotobuf, so there will be a conflict if we want to link to the -lite version too. I've always considered ${CONAN_LIBS} a shorthand, user can select which libraries to link.


One more issue we could have with this library is how to deploy the CMake macros protobuf_generate_cpp, how the consumer will include them. These usually are included through the find_package(Protobuf) because they are defined inside the FindProtobuf.cmake file, where are they when requiring protoc_installer?

@sourcedelica
Copy link

Agreed that protoc_installer needs a CMake macro to generate C++ and Python code. Ideally it would work with find_package(Protobuf), or at least the macros would be named protobuf_generate_cpp and protobuf_generate_python and would work the same way as the macros of the same names from FindProtobuf.

@Croydon
Copy link
Member Author

Croydon commented Jan 3, 2019

Blocked by #598 & #604

@Croydon Croydon added the blocked blocked by other issue label Jan 3, 2019
@SSE4
Copy link
Member

SSE4 commented Feb 25, 2019

Now in conan center

@Croydon
Copy link
Member Author

Croydon commented Feb 25, 2019

There are still several huge issues, aren't there? Why was it added to conan-center now?

@uilianries
Copy link
Member

Most simple cases are working and protobuf is very popular. We can continue fixing and updating.

@SSE4
Copy link
Member

SSE4 commented Feb 26, 2019

okay, as we have other issues to track, can we close this one?

@danimtb
Copy link

danimtb commented Mar 11, 2019

I think this can be closed as the packages are already in Conan Center

@SSE4 SSE4 closed this as completed Mar 11, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
blocked blocked by other issue conan-center Review for inclusion on Conan Center enhancement
Projects
None yet
Development

No branches or pull requests

7 participants