[RFC] make: introduce driver specific Makefiles for .dep and .include#6923
[RFC] make: introduce driver specific Makefiles for .dep and .include#6923smlng wants to merge 2 commits intoRIOT-OS:masterfrom
Conversation
|
I'm not a Makefile expert either but the general idea here sounds very interesting. |
|
I think it would be somewhat easier if we would distinguish between drivers modules and other (system/feature) modules. For instance by introducing a |
Yes, would be great, if we could make this possible for any module ;-). |
|
(not just drivers.) |
|
As you are only including files based on |
|
For modules, I think it would require having definitions of |
|
@cladmi can you align this to a future-proof build? |
|
@cladmi what to do here: elaborate or close? |
cladmi
left a comment
There was a problem hiding this comment.
I go back on my previous remark of removing the ifneq protections.
With more background on how it is done in other places, they should be kept and ensured to be mandatory (within a static-test).
And all files should be processed all the time, not like packages.
This proposal changes the include directory. It is somehow an other change and could be done before or after alone, but also makes sense to migrate at the same time. It would prevent seeing headers when a module is not used.
However I would want feedback from the drivers maintainers about this.
I would not support any RIOTMODULESTREES right now, there are still issues in the "dependencies" definition order (and even more issues) so would advertise a feature with a lot of side effects. Changing the "definitions" order for lwip modules changes the behavior. There is now EXTERNAL_MODULE_DIRS that allows this too but forces the user to provide the parsing order.
It is in my plans to address it but only after dependencies are resolved in a separate step before parsing board/Makefile.include (#9913).
| include $(RIOTBASE)/drivers/Makefile.dep | ||
|
|
||
| # pull dependencies from drivers | ||
| -include $(USEMODULE:%=$(RIOTBASE)/drivers/%/Makefile.dep) |
There was a problem hiding this comment.
This should always process all files with a $(sort $(wildcard)).
There are already weird cases with at86rf2xx that implements multiple drivers and would break with a pattern like this.
It looks like there is not reasons to have issues with the processing order with the current definitions.
| $(RIOTBASE)/drivers/%/Makefile.include:: | ||
| $(Q)"$(MAKE)" -C $(RIOTBASE)/drivers/$* Makefile.include | ||
|
|
||
| .PHONY: $(USEMODULE:%=$(RIOTBASE)/drivers/%/Makefile.include) |
There was a problem hiding this comment.
This is a remaining mistake from packages that should be removed.
I just did not find an opportunity to remove it without only saying "it's bad".
We should never generate files in the source directory and Makefile.include should be BOARD/build configuration independent so not have any reasons to be generated at compile time.
| $(Q)"$(MAKE)" -C $(RIOTBASE)/drivers/$* Makefile.include | ||
|
|
||
| .PHONY: $(USEMODULE:%=$(RIOTBASE)/drivers/%/Makefile.include) | ||
| -include $(USEMODULE:%=$(RIOTBASE)/drivers/%/Makefile.include) |
There was a problem hiding this comment.
Same here, using a wildcard.
| @@ -0,0 +1,3 @@ | |||
| ifneq (,$(filter hd44780,$(USEMODULE))) | |||
| USEMODULE_INCLUDES += $(RIOTBASE)/drivers/hd44780/include | |||
There was a problem hiding this comment.
This changes the include directory and should get specific feedback from people maintaining this.
If the directory is modified like this, I would also like a static test that the include is in this directory, it would prevent issues when people copy paste or rename in the middle of a PR.
|
Any progress here or shall we close? |
|
This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. If you want me to ignore this issue, please mark it with the "State: don't stale" label. Thank you for your contributions. |
This PR introduces driver specific Makefiles for dependencies and includes, as an example its implemented for
drivers/hd44780.The motivation behind this is to have self-contained driver modules without editing several global Makefiles (like
drivers/Makefile.depanddrivers/Makefile.include). Further, this would also makedrivers/includeobsolete; and, if we could introduce something likeRIOTDRVpath variable -which allows for multiple dirs like system env variablePATH- this would also make the addition of external driver modules (not in RIOT code base) very simple, as not edits of other files is required anymore.I'm not a Makefile expert, so this is more proof of concept than best implementation: @kaspar030 and @gebart I rely on your expertise here.