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

Feature/conan package proto files #110

Closed

Conversation

kevswims
Copy link

Public-Facing Changes

None

Description

Adds a conanfile and cmake project to compile the protobuf code and package it.

We are using this in a couple of ways:

  • Dependent code such as (https://github.com/foxglove/ws-protocol/tree/main/cpp/examples) no longer has to compile the protobuf files. They can just consume the package with conan.
  • The cmake project is being used standalone from a Yocto Bitbake recipe to package the proto files for consumption as a yocto package.

TODO:

Questions:

  • How does Foxglove typically contribute packages to conan center?

@CLAassistant
Copy link

CLAassistant commented May 23, 2023

CLA assistant check
All committers have signed the CLA.

@jtbandes
Copy link
Member

Hi, thanks for your contribution!

We've had a good experience so far with publishing Python packages for the schemas (foxglove-schemas-protobuf and foxglove-schemas-flatbuffer), making it easy to get up and running with the schemas in a new Python project. So it makes sense to try and offer a good experience for C++ users in this way as well. We would probably want the name of the package to be protobuf-specific, since the schemas can also be used with other encodings like flatbuffers.

It's my understanding that publishing a package via Conan usually requires making a PR to conan-io/conan-center-index. For example, here's our mcap recipe and our foxglove-websocket recipe. We've followed their contributing guide in the past in order to set up these packages. Do you think that making a PR to conan-center-index would be a better first step to making this available? (Off the top of my head I'm not sure whether any changes in this repo would be required to get it working.)

We do sometimes use conan in our source repos in order to build and test the packages (for example: https://github.com/foxglove/mcap/blob/main/cpp/build.sh) but I don't think it's required, though of course there is value in setting up CI checks to ensure as best we can that the published conan package would continue to work as we make changes to the source repo.

@kevswims
Copy link
Author

Hi @jtbandes

Thanks for the responses.

I like the idea of including protobuf in the name. I'll update that to foxglove-protobuf-schemas

As far as for publishing to conan-center-index that is the right process. I've done that before for other packages. Not sure if you want that contribution to come from Foxglove or from me. I'm happy to do it if you would like.

It makes sense to me to have the conan file also live in this repo for CI as you said. I also created the CMakeLists.txt file which build all the pb.cc files into a library. That file should definitely live here.

Here's the next steps for me on this:

  • Clean up and doc the conanfile so that it is fully filled out
  • Get this merged in here
  • Create a PR in conan-center with the recipe
  • Add some info in the ws-protocol library to consume this

Long term goals for this:

  • Setup CI for the conan package in this repo

@jtbandes
Copy link
Member

Makes sense – do you think the conanfile needs to be included in this PR if it's primarily going to live in the conan-center-index?

I do think we'd want some form of CI testing in order to accept the CMakeLists being added here.

I'm actually not totally convinced the CMakeLists should live here at all. Right now we have nothing in the repo specific to C++. We have some Python-specific stuff because the pypi package is actually published from this repo. But if the conan package lives in the conan-center-index repo, maybe the CMakeLists should live there too? What do you think? I don't have a lot of experience with other conan packages to know how they handle this.

@kevswims
Copy link
Author

I'll try submitting to conan-center with both the conanfile and cmakelists and see what the reviewers say on it. They have a patch mechanism that will make it work. If they are fine with it then we can do that. Otherwise we can explore other options for putting it in here.

@kevswims
Copy link
Author

Conan center PR is up here: conan-io/conan-center-index#17707

@jtbandes
Copy link
Member

What do you think of naming it foxglove-schemas-protobuf to match the python package? And I think it's nice because the repo name is foxglove/schemas.

@kevswims
Copy link
Author

kevswims commented May 25, 2023 via email

@jtbandes
Copy link
Member

Hey, what's the latest on this? Still waiting for review on the conan-center-index PR?

@kevswims
Copy link
Author

@jtbandes Yeah, that PR is still pending. I got everything on it building so I think it should be good to go. It just can take a while to get through the review queue.

@jtbandes
Copy link
Member

jtbandes commented Jun 29, 2023

It looks like things are moving along on the conan center PR 🎉 – so I'm going to close this ticket for now to indicate that our team doesn't have any more work planned to support this.

Once that package is published, I think a good action item would be to update the websocket protocol example c++ server to use the new package instead of having the proto files and CMake stuff duplicated in that repo. It might also make sense to update the MCAP docs and/or Studio docs to include some info about how to get set up with C++ & protobuf. I've created internal tickets to track both of these and we will revisit them in a few weeks.

Feel free to let us know if you have any other desired outcomes beyond the publishing of the package to conan-center. Thanks for working on this!

@jtbandes jtbandes closed this Jun 29, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants