-
Notifications
You must be signed in to change notification settings - Fork 2k
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
pkg/nanopb: add nanoPb protocol buffers library package #11565
Conversation
I still want that package 😃 If you don't have time for this right now I can also make a PR against your branch for this. |
Looks good now 😉 |
e02da61
to
dffbd77
Compare
There's one issue here: CI is gonna be missing protoc. Please don't merge yet (the build should fail now when running the tests anyways) |
dffbd77
to
53480aa
Compare
Will it be enough to add |
It should, apart from updating the murdock workers afterwards. I guess at that point we need to make sure that building in docker ("BUILD_IN_DOCKER=1") works as expected... BTW, thanks for helping out! |
Hm, is there a way to overwrite But the package works as advertised :) |
There is #11533 (comment). However, setting the |
We need do check if |
fcba523
to
9e1019f
Compare
9e1019f
to
3dd0975
Compare
@benpicco this might just make it into the release. 😉 |
Hm seems like the python support is still missing... :( |
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.
The code looks good and it works.
We're already pulling this branch into our development build, so it's being used already.
Perfect. I've added a commit to disable tests on CI for now. |
Sorry @benpicco, python-protobuf is actually needed for building ;( |
@benpicco I did a test run with the soon-to-be-live build container. It failed as we're installing the python 3 protobuf module, but nanopb's generator defaults to python2... I've worked around that by patching the generator to use python3. Seems to work fine. Could you take a look? |
So with RIOT-OS/riotdocker#89 merged, can we give Murdock another try? |
You can already squash, so hopefully everything will be green now :) |
c2976b3
to
f52bbd0
Compare
This passed all tests. @benpicco I'll give you the honor! 😉 |
Thank you for providing the package & build system integration! |
Thanks for the review! |
Contribution description
This PR adds a package for easy usage of google protocol buffers.
It adds some logic to automatically generate *.c files from *.proto files within a module's source folder.
In order to do so, a prerequisite commit adds generic support for generated sources.
Testing procedure
See
tests/pkg_nanopb/README.md
for usage info. Specifically, in order to test this, you'll need to install the official protobuf compiler, it's python bindings and the nanopb generator plugin.As that generator is not (yet) part of the build container, the test cannot be compiled on the CI.
This is an alternative to #11157. Differences:
Issues/PRs references
#11157