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

Makefile.{include,base},pkg/nanopb: fix target deps & code generation #20400

Conversation

DanielLockau-MLPA
Copy link
Contributor

Contribution description

This PR aims to

  • make the results of GENSRC available to all modules (and be robust against multiple dependencies against the same protobuf schema)
  • compile protobuf schema outside the current directory without caring much about the shortcomings of protoc

To achieve that

  • The GENSRC target is moved into a more central place. Before, it did run on every module build. GENSRC is known already much earlier and can be run once for all modules. This PR suggests to run it once after pkg-prepare.
  • The output of nanopb is put into a central place in $(BINDIR)/nanopb to make it accessible to all modules which depend on nanopb. While not normally required, this allows for multiple modules using the generated sources. Without the changes of this PR the options for usage by multiple modules was to live with a race condition during build because one module (the application) would generate sources which another one then wanted to use for compiling. Naming of the same protobuf dependency by multiple modules would lead to multiple definitions of the same symbols.

Testing procedure

none

Issues/PRs references

none

@github-actions github-actions bot added Area: build system Area: Build system Area: pkg Area: External package ports labels Feb 19, 2024
@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 Feb 19, 2024
pkg/nanopb/Makefile.gensrc Outdated Show resolved Hide resolved
pkg/nanopb/Makefile.gensrc Outdated Show resolved Hide resolved
pkg/nanopb/Makefile.gensrc Outdated Show resolved Hide resolved
@riot-ci
Copy link

riot-ci commented Feb 19, 2024

Murdock results

✔️ PASSED

9c9d739 Makefile.{include,base},pkg/nanopb: fix target dependencies & code generation

Success Failures Total Runtime
10008 0 10009 08m:01s

Artifacts

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.

Please squash

@DanielLockau-MLPA DanielLockau-MLPA force-pushed the dl/fix/20240219_nanopb_code_generation branch from 9c44697 to 9c9d739 Compare March 12, 2024 08:53
@benpicco benpicco added this pull request to the merge queue Mar 12, 2024
Merged via the queue into RIOT-OS:master with commit ed89e34 Mar 12, 2024
25 checks passed
@benpicco benpicco deleted the dl/fix/20240219_nanopb_code_generation branch April 5, 2024 12:45
@@ -2,29 +2,40 @@ PROTOC ?= protoc
PROTOC_GEN_NANOPB ?= $(PKGDIRBASE)/nanopb/generator/protoc-gen-nanopb

PROTOBUF_FILES ?= $(wildcard *.proto)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In our branch we have

Suggested change
PROTOBUF_FILES ?= $(wildcard *.proto)
PROTOBUF_FILES += $(wildcard *.proto)

and apps that don't build with the upstream version.

@MrKevinWeiss MrKevinWeiss added this to the Release 2024.04 milestone Apr 30, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area: build system Area: Build system 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
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants