foxglove / schemas

Message schemas supported by Foxglove Studio
MIT License
55 stars 29 forks source link

Feature/conan package proto files #110

Closed kevswims closed 1 year ago

kevswims commented 1 year ago

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:

TODO:

Questions:

CLAassistant commented 1 year ago

CLA assistant check
All committers have signed the CLA.

jtbandes commented 1 year ago

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 commented 1 year ago

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:

Long term goals for this:

jtbandes commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

Conan center PR is up here: https://github.com/conan-io/conan-center-index/pull/17707

jtbandes commented 1 year ago

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 commented 1 year ago

That works for me. I'll update the Conan center pr tomorrow with the new name.

On Thu, May 25, 2023, 4:37 PM Jacob Bandes-Storch @.***> wrote:

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.

— Reply to this email directly, view it on GitHub https://github.com/foxglove/schemas/pull/110#issuecomment-1563594781, or unsubscribe https://github.com/notifications/unsubscribe-auth/ADG2EGJYAHX6D4JGEXRQVM3XH7NLVANCNFSM6AAAAAAYMIANI4 . You are receiving this because you authored the thread.Message ID: @.***>

jtbandes commented 1 year ago

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

kevswims commented 1 year ago

@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 commented 1 year ago

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!