Makefile.include: fix override headers from application by leandrolanzieri · Pull Request #20744 · RIOT-OS/RIOT

leandrolanzieri · 2024-06-11T12:44:02Z

Contribution description

As our drivers guide states:

The idea is that this file can be overridden by an application or a board, by simply putting a file with the same name in the application's or the board's include folders, while RIOT's build system takes care of preferring those files instead of the default params file.

For some reason this doesn't seem to be working. I'm not sure whether there was some uncaught regression at some point. Currently, when one places a <driver>_params.h this doesn't override the one define by RIOT, and is simply ignored.

This PR fixes the issue by including the APPDIR in the considered include paths, so that it has priority.

Testing procedure

On some driver test application, attempt to override default parameters by defining a <driver>_params.h file in your app directory.

Issues/PRs references

None

riot-ci · 2024-06-12T11:11:48Z

Murdock results

✔️ PASSED

6247c10 tests/drivers/mrf24j40: rename common.h

Success	Failures	Total	Runtime
151284	0	151284	02h:21m:56s

Artifacts

Documentation preview

leandrolanzieri · 2024-06-12T11:15:16Z

Hmm it's failing on a test that tries to override the irq.h file. Seems like an existing issue.

leandrolanzieri · 2024-06-12T11:22:46Z

I included a commit that seems to fix this (at least this failing case) locally.

maribu

ACK. Thanks for fixing this! ❤️

maribu · 2024-06-12T12:41:51Z

Makefile.include

 endif

 # Add standard include directories
+INCLUDES += -I$(APPDIR)


Thanks for fixing this! I have falling into this pitfall numerous times when I needed to quickly test something, but never really traced it down but instead did a quick-and-dirty hack of the system header instead.

I'm glad that next time I need to quickly test something, I won't fall into that pitfall again ❤️

leandrolanzieri · 2024-06-13T09:18:11Z

I wonder if the irq.h header introduced in #17066 ever had an effect. Currently seems to be breaking for some builds, and even some prototypes are not correct. Was the idea to prevent inline implementations from some platforms? Do you know @maribu ?

maribu · 2024-06-13T09:28:41Z

I'm almost certain that header was never included before, so it can be safely removed.

I think the idea was to have the signatures of the IRQ functions available, so that setting up the mockup wrapper functions is possible. Since the mock wrapper functions replace the actual calls, having a working IRQ header included was not needed. They just would need to match the API. (For the mock ups it would not matter if they are static inline or real symbol generating functions.)

But as the test worked just fine with wrapping the IRQ functions using the real signatures (including static inline for most platforms), I'd personally just drop the irq.h from the test without giving it a second thought.

leandrolanzieri · 2024-06-13T10:08:22Z

After some fiddling around, it turns out it's not so straight forward. Removing the header seems to uncover other issues:

Currently there's no way to override IRQ_API_INLINED
Most of the platforms include irq_arch.h directly instead of irq.h

IIUC the "official" interface is to always go through irq.h, which in turn includes irq_arch.h on platforms that set IRQ_API_INLINED. We'd also need to change that.

The test would then define IRQ_API_INLINED to 0, which would allow to provide mock implementations from the application.

maribu · 2024-06-13T12:07:06Z

Ah, OK. So irq.h was included, but only from the tests C source (as #include "irq.h" would also search the current folder).

Now, the header is overriding the systems irq.h.

Maybe just copy-paste the contents of irq.h in the test source file and drop the header then?

leandrolanzieri · 2024-06-15T18:20:40Z

Maybe just copy-paste the contents of irq.h in the test source file and drop the header then?

Now this is implemented on 78413fe. It feels a bit off, but the alternative (I believe) would be a whole restructuring of how irq.h and irq_arch.h are included throughout multiple CPUs, just to mock this.

mguetschow

Looks good to me, thanks! Do you know what was failing in the last Murdock build? Any chance we can get this in before hard-feature freeze next week?

Teufelchen1 · 2024-09-25T19:36:04Z

@leandrolanzieri what do you need to move this forward?

maribu · 2025-04-24T09:32:49Z

tests/pkg/lwip/utilities.h

+ *
+ * @return  Length of @p out.
+ */
+size_t hex2ints(uint8_t *out, const char *in);


This could be replaced with calls to scn_buf_hex() now, but this could also be a follow-up.

I think that goes a bit out of scope

leandrolanzieri · 2025-05-06T15:29:20Z

Rebased and applied changes to fix the latest CI errors.

leandrolanzieri · 2025-05-15T11:42:36Z

Rebased and applied changes to fix the latest CI errors.

Again

Teufelchen1

👍

leandrolanzieri · 2025-05-15T12:47:01Z

:(

To avoid name clash, rename this application header file to test_common.h

To avoid name clash, rename this application header file to test_common.h.

maribu · 2025-05-15T19:12:27Z

Nice! Thx a lot! ❤️

leandrolanzieri requested a review from Teufelchen1 June 11, 2024 12:44

github-actions bot added the Area: build system Area: Build system label Jun 11, 2024

leandrolanzieri requested review from MichelRottleuthner and aabadie as code owners June 12, 2024 11:22

github-actions bot added the Area: tests Area: tests and testing framework label Jun 12, 2024

leandrolanzieri requested a review from miri64 as a code owner June 12, 2024 11:47

leandrolanzieri requested a review from maribu June 12, 2024 11:48

maribu approved these changes Jun 12, 2024

View reviewed changes

leandrolanzieri force-pushed the pr/makefile_include/include-app-headers branch from 42212cd to 78413fe Compare June 15, 2024 18:17

mguetschow approved these changes Jul 9, 2024

View reviewed changes

mguetschow 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 Apr 3, 2025

mguetschow 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 Apr 24, 2025

maribu reviewed Apr 24, 2025

View reviewed changes

leandrolanzieri force-pushed the pr/makefile_include/include-app-headers branch from 3120b0c to 6010fb4 Compare May 6, 2025 15:28

leandrolanzieri force-pushed the pr/makefile_include/include-app-headers branch from 6010fb4 to 6eb59fc Compare May 15, 2025 11:41

Teufelchen1 approved these changes May 15, 2025

View reviewed changes

Teufelchen1 added this pull request to the merge queue May 15, 2025

github-merge-queue bot removed this pull request from the merge queue due to failed status checks May 15, 2025

leandrolanzieri added 6 commits May 15, 2025 14:51

Makefile.include: fix override headers from application

3c19f8a

tests/pkg/lwip: rename local header

b6b92cd

tests/core/irq_cpp: remove irq.h override

a29efa2

tests/net/ieee802154_hal: rename common.h

f77b414

To avoid name clash, rename this application header file to test_common.h

test/drivers/kw2xrf: rename common.h

044b1f5

To avoid name clash, rename this application header file to test_common.h.

tests/drivers/mrf24j40: rename common.h

6247c10

To avoid name clash, rename this application header file to test_common.h.

crasbe added CI: full build disable CI build filter CI: no fast fail don't abort PR build after first error and removed CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR labels May 15, 2025

leandrolanzieri force-pushed the pr/makefile_include/include-app-headers branch from 6eb59fc to 6247c10 Compare May 15, 2025 12:51

crasbe added the CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR label May 15, 2025

crasbe added this pull request to the merge queue May 15, 2025

Merged via the queue into RIOT-OS:master with commit 65a8b47 May 15, 2025
26 checks passed

leandrolanzieri deleted the pr/makefile_include/include-app-headers branch May 16, 2025 06:55

Teufelchen1 added this to the Release 2025.07 milestone Jul 14, 2025

Conversation

leandrolanzieri commented Jun 11, 2024

Contribution description

Testing procedure

Issues/PRs references

Uh oh!

riot-ci commented Jun 12, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Murdock results

Artifacts

Uh oh!

leandrolanzieri commented Jun 12, 2024

Uh oh!

leandrolanzieri commented Jun 12, 2024

Uh oh!

maribu left a comment

Choose a reason for hiding this comment

Uh oh!

maribu Jun 12, 2024

Choose a reason for hiding this comment

Uh oh!

leandrolanzieri commented Jun 13, 2024

Uh oh!

maribu commented Jun 13, 2024

Uh oh!

leandrolanzieri commented Jun 13, 2024

Uh oh!

maribu commented Jun 13, 2024

Uh oh!

leandrolanzieri commented Jun 15, 2024

Uh oh!

mguetschow left a comment

Choose a reason for hiding this comment

Uh oh!

Teufelchen1 commented Sep 25, 2024

Uh oh!

maribu Apr 24, 2025

Choose a reason for hiding this comment

Uh oh!

leandrolanzieri May 6, 2025

Choose a reason for hiding this comment

Uh oh!

leandrolanzieri commented May 6, 2025

Uh oh!

leandrolanzieri commented May 15, 2025

Uh oh!

Teufelchen1 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

leandrolanzieri commented May 15, 2025

Uh oh!

Uh oh!

maribu commented May 15, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

riot-ci commented Jun 12, 2024 •

edited

Loading