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

pkg/nanopb: add nanoPb protocol buffers library package #11565

Merged
merged 3 commits into from
Oct 19, 2019

Conversation

kaspar030
Copy link
Contributor

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:

  • includes generic way to generate sources using tools like nanopb.
  • uses official test code
  • simpler package source compilation through using custom (one-line) Makefile instead of calling upstream cmake

Issues/PRs references

#11157

@kaspar030 kaspar030 added Type: new feature The issue requests / The PR implemements a new feature for RIOT Area: pkg Area: External package ports CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR labels May 23, 2019
pkg/nanopb/nanopb.inc.mk Outdated Show resolved Hide resolved
pkg/nanopb/nanopb.inc.mk Outdated Show resolved Hide resolved
@benpicco
Copy link
Contributor

I still want that package 😃
But I really think we should use the repo that gets downloaded through the pkg instead of having the user download it again manually.

If you don't have time for this right now I can also make a PR against your branch for this.

@benpicco
Copy link
Contributor

Looks good now 😉
You can squash.

@kaspar030 kaspar030 added the CI: run tests If set, CI server will run tests on hardware for the labeled PR label Sep 10, 2019
@kaspar030
Copy link
Contributor Author

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)

@benpicco benpicco added CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR and removed CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR labels Sep 10, 2019
@benpicco
Copy link
Contributor

Will it be enough to add protobuf-compiler to requirements.txt in riotdocker?

@kaspar030
Copy link
Contributor Author

Will it be enough to add protobuf-compiler to requirements.txt in riotdocker?

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!

@benpicco
Copy link
Contributor

benpicco commented Sep 11, 2019

Hm, is there a way to overwrite PKG_VERSION from the application?
I just realized that #11157 pointed to the master (soon to be 0.4.0) branch and it contains an API change from the 0.3.x branch.

But the package works as advertised :)

@miri64
Copy link
Member

miri64 commented Sep 11, 2019

There is #11533 (comment). However, setting the PKG_VERSION from outside the build system would also be great (needs some kind of scoping, though)

pkg/nanopb/nanopb.inc.mk Outdated Show resolved Hide resolved
pkg/nanopb/nanopb.inc.mk Outdated Show resolved Hide resolved
Makefile.base Outdated Show resolved Hide resolved
pkg/nanopb/nanopb.inc.mk Outdated Show resolved Hide resolved
@kaspar030
Copy link
Contributor Author

We need do check if pkg/nanopb/Makefile.gensrc as is would work if multiple modules make use of GENSRC. Also I'm not sure as is if non-application modules can use protobuf files the way it is implemented now.

@kaspar030 kaspar030 removed the CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR label Sep 12, 2019
@benpicco benpicco added the CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR label Sep 23, 2019
@kaspar030 kaspar030 added the CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR label Oct 10, 2019
@kaspar030
Copy link
Contributor Author

  • squashed

@benpicco this might just make it into the release. 😉

@kaspar030
Copy link
Contributor Author

@benpicco this might just make it into the release. wink

Hm seems like the python support is still missing... :(

Copy link
Contributor

@benpicco benpicco left a 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.

@kaspar030
Copy link
Contributor Author

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.

@benpicco benpicco added CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR and removed CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR CI: run tests If set, CI server will run tests on hardware for the labeled PR labels Oct 10, 2019
@kaspar030 kaspar030 removed the CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR label Oct 10, 2019
@kaspar030
Copy link
Contributor Author

Sorry @benpicco, python-protobuf is actually needed for building ;(

@kaspar030
Copy link
Contributor Author

@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?

@benpicco
Copy link
Contributor

So with RIOT-OS/riotdocker#89 merged, can we give Murdock another try?

@benpicco
Copy link
Contributor

You can already squash, so hopefully everything will be green now :)

pkg/nanopb/Makefile Outdated Show resolved Hide resolved
@kaspar030 kaspar030 added CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR CI: run tests If set, CI server will run tests on hardware for the labeled PR labels Oct 18, 2019
@kaspar030
Copy link
Contributor Author

This passed all tests. @benpicco I'll give you the honor! 😉

@benpicco benpicco merged commit 9f0ccde into RIOT-OS:master Oct 19, 2019
@benpicco
Copy link
Contributor

Thank you for providing the package & build system integration!

@kaspar030 kaspar030 deleted the pr/pkg/nanopb branch October 19, 2019 08:37
@kaspar030
Copy link
Contributor Author

Thank you for providing the package & build system integration!

Thanks for the review!

@Citrullin Citrullin mentioned this pull request Oct 26, 2019
@fjmolinas fjmolinas added this to the Release 2020.01 milestone Dec 13, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area: pkg Area: External package ports CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR CI: run tests If set, CI server will run tests on hardware for the labeled PR Type: new feature The issue requests / The PR implemements a new feature for RIOT
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants