Conversation
jcarrano
left a comment
jcarrano
left a comment
There was a problem hiding this comment.
The processing of the proto files should be moved outside the package. Currently our handling of generated files is less than optimal, I ran into similar issues with the Lua package, and I'm still not satisfied with the results.
The package's makefile should be written so that it does nothing if it should do nothing.
|
|
||
| # Todo: Not sure if that really works on Windows. Somebody should test it. | ||
| # Check if we are running on Windows | ||
| ifdef windir |
There was a problem hiding this comment.
ifeq ($(strip $(OS)), Windows_NT)
BLABLABLA
endif| rm -rf $(PROTO_BUILD_DIR) && \ | ||
| mkdir $(PROTO_BUILD_DIR) && \ | ||
| cp $(PKG_DIR)/Makefile.template $(PKG_BUILDDIR)/Makefile && \ | ||
| $(MAKE) -e PROTO_BUILD_DIR="$(PROTO_BUILD_DIR)" -e PROTO_FILES="$(PROTO_FILES)" \ |
There was a problem hiding this comment.
The processing of the .proto files should not be done by the package's makefile.
There was a problem hiding this comment.
Why shouldn't it? Otherwise the user has to take care of it or has to keep some code for it in the applications Makefile. I want to keep it as simple as possible for the user.
There was a problem hiding this comment.
The package's makefile should only take care of building the package. The rest of the rules must be in Makefile.include so that they are added to the main makefile. It is possible to define it so that the user does not have to worry about anything: the same way it is done for .c files:
- Use a wildcard to collect
.protofiles - From this collection, create a target for each .proto.
- Define an implicit rule so that make knows how to build a
pb.{c,h}from a.proto
| @@ -0,0 +1 @@ | |||
| INCLUDES += -L$(PKGDIRBASE)/nanopb/ -I$(PKGDIRBASE)/nanopb/ | |||
There was a problem hiding this comment.
This is the place to include the Makefile.template so that the rules for processing proto files are executed by the top level make.
There was a problem hiding this comment.
The processing of the proto files should be moved outside the package. Currently our handling of generated files is less than optimal, I ran into similar issues with the Lua package, and I'm still not satisfied with the results.
The package's makefile should be written so that it does nothing if it should do nothing.
Okay, I take a look into the Lua package. Thanks for the info. I just don't want to have this in the applications Makefile. The user shouldn't maintain this. Should just run out of the box after including the package into the project.
There was a problem hiding this comment.
In lua I had to keep it there because the integration was a bit more tricky and because I could not get #9565 in. Your case should be easier, but if it isn't and we need to change stuff (read fix stuff) in the build system to make it work, we will.
| endif | ||
| endif | ||
|
|
||
| $(PROTO_FILES): |
There was a problem hiding this comment.
typing make clean does not clean these files.
| TOOLCHAIN_FILE = $(PKG_BUILDDIR)/xcompile-toolchain.cmake | ||
|
|
||
| PKG_DIR=$(CURDIR) | ||
| PROTO_DIR=$(APPDIR)/proto |
There was a problem hiding this comment.
"compiled" .proto files should go under bin/. Unfortunately, there is currently no place to put "target-independent" files (like generated files) so they will have to go under bin/${BOARD}/proto_whatever.
| all: $(PKG_BUILDDIR)/Makefile | ||
| cp $(PKG_BUILDDIR)/libnanopb.a $(BINDIR)/nanopb.a | ||
|
|
||
| $(PKG_BUILDDIR)/Makefile: $(TOOLCHAIN_FILE) |
There was a problem hiding this comment.
Because this depends on $(TOOLCHAIN_FILE), which depends on git-download which is a PHONY target, this rules is being executed every time.
| -e PROTO_DIR="$(PROTO_DIR)" -e PKG_BUILDDIR="$(PKG_BUILDDIR)" -C $(PKG_BUILDDIR) proto lib | ||
|
|
||
| $(TOOLCHAIN_FILE): git-download | ||
| $(RIOTTOOLS)/cmake/generate-xcompile-toolchain.sh > $(TOOLCHAIN_FILE) |
There was a problem hiding this comment.
Because of the dependency on a phony target, this gets executed every time. Move this to all.
PeterKietzmann
added
Type: new feature
|
@Citrullin we seem to be working on similar stuff... I had a branch with nanopb RIOT integration lying around. I wouldn't have PR'ed it after seeing this PR, but I think the integration in #11157 is somewhat cleaner. |
|
@kaspar030 Wrong PR. It is #11157. Yes, I will take a look. |
|
Closed, due to merged implementation in #11565 |
4 participants
Contribution description
Add nanopb package. See example of how it works. Just create proto directory with your proto files. Include the package in your app:
And that's it. The Makefile will do the rest for you. It compiles the header and c files for you.
Testing procedure