[riot-notifications] [RIOT-OS/RIOT] Add nanopb package (#11157)

Juan I Carrano notifications at github.com
Tue Mar 12 12:41:58 CET 2019


jcarrano requested changes on this pull request.

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.

> @@ -0,0 +1,51 @@
+# This is an include file for Makefiles. It provides rules for building
+# .pb.c and .pb.h files out of .proto, as well the path to nanopb core.
+
+# Path to the nanopb root directory
+NANOPB_DIR := $(PKG_BUILDDIR)
+
+# Files for the nanopb core
+NANOPB_OBJ = pb_encode pb_decode pb_common
+#NANOPB_CORE = $(NANOPB_DIR)/pb_encode.c $(NANOPB_DIR)/pb_decode.c $(NANOPB_DIR)/pb_common.c
+
+PROTOBUF_OBJ = $(subst .proto,,$(PROTO_FILES))
+
+# Todo: Not sure if that really works on Windows. Somebody should test it.
+# Check if we are running on Windows
+ifdef windir

```make
ifeq ($(strip $(OS)), Windows_NT)
  BLABLABLA
endif
```

> +
+TOOLCHAIN_FILE = $(PKG_BUILDDIR)/xcompile-toolchain.cmake
+
+PKG_DIR=$(CURDIR)
+PROTO_DIR=$(APPDIR)/proto
+PROTO_BUILD_DIR=$(APPDIR)/proto_compiled
+PROTO_FILES=$(shell ls $(PROTO_DIR))
+
+all: $(PKG_BUILDDIR)/Makefile
+	cp $(PKG_BUILDDIR)/libnanopb.a $(BINDIR)/nanopb.a
+
+$(PKG_BUILDDIR)/Makefile: $(TOOLCHAIN_FILE)
+	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)" \

The processing of the `.proto` files should not be done by the package's makefile.

> @@ -0,0 +1 @@
+INCLUDES += -L$(PKGDIRBASE)/nanopb/ -I$(PKGDIRBASE)/nanopb/

This is the place to include the Makefile.template so that the rules for processing proto files are executed by the top level make.

> +# system-supplied python interpreter.
+ifneq "$(wildcard $(NANOPB_DIR)/generator-bin)" ""
+	# Binary package
+	PROTOC = $(NANOPB_DIR)/generator-bin/protoc
+	PROTOC_OPTS =
+else
+	# Source only or git checkout
+	PROTOC = protoc
+	ifdef WINDOWS
+		PROTOC_OPTS = --plugin=protoc-gen-nanopb=$(NANOPB_DIR)/generator/protoc-gen-nanopb.bat
+	else
+		PROTOC_OPTS = --plugin=protoc-gen-nanopb=$(NANOPB_DIR)/generator/protoc-gen-nanopb
+	endif
+endif
+
+$(PROTO_FILES):

typing `make clean` does not clean these files.

> @@ -0,0 +1,30 @@
+PKG_NAME=nanopb
+PKG_URL=https://github.com/nanopb/nanopb.git
+PKG_VERSION=be0dd4b3aacdc9c2bbe21568b6a37218f2bb959a
+PKG_LICENSE=zlib
+
+.PHONY: all
+
+RIOT_CFLAGS = $(INCLUDES) -std=c99
+
+TOOLCHAIN_FILE = $(PKG_BUILDDIR)/xcompile-toolchain.cmake
+
+PKG_DIR=$(CURDIR)
+PROTO_DIR=$(APPDIR)/proto

"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`.

> +
+.PHONY: all
+
+RIOT_CFLAGS = $(INCLUDES) -std=c99
+
+TOOLCHAIN_FILE = $(PKG_BUILDDIR)/xcompile-toolchain.cmake
+
+PKG_DIR=$(CURDIR)
+PROTO_DIR=$(APPDIR)/proto
+PROTO_BUILD_DIR=$(APPDIR)/proto_compiled
+PROTO_FILES=$(shell ls $(PROTO_DIR))
+
+all: $(PKG_BUILDDIR)/Makefile
+	cp $(PKG_BUILDDIR)/libnanopb.a $(BINDIR)/nanopb.a
+
+$(PKG_BUILDDIR)/Makefile: $(TOOLCHAIN_FILE)

Because this depends on  $(TOOLCHAIN_FILE), which depends on git-download which is a PHONY target, this rules is being executed every time.

> +PROTO_DIR=$(APPDIR)/proto
+PROTO_BUILD_DIR=$(APPDIR)/proto_compiled
+PROTO_FILES=$(shell ls $(PROTO_DIR))
+
+all: $(PKG_BUILDDIR)/Makefile
+	cp $(PKG_BUILDDIR)/libnanopb.a $(BINDIR)/nanopb.a
+
+$(PKG_BUILDDIR)/Makefile: $(TOOLCHAIN_FILE)
+	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)" \
+	 -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)

Because of the dependency on a phony target, this gets executed every time. Move this to `all`.

-- 
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/RIOT-OS/RIOT/pull/11157#pullrequestreview-213330218
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.riot-os.org/pipermail/notifications/attachments/20190312/0e1f9e2c/attachment.html>


More information about the notifications mailing list