cpu: move cpu level dependencies in dedicated Makefile.dep files by aabadie · Pull Request #12898 · RIOT-OS/RIOT

aabadie · 2019-12-07T17:31:38Z

Contribution description

This PR moves some modules dependencies defined at CPU level (with USEMODULE, FEATURES_REQUIRED, DEFAULT_MODULE) from Makefile.include to Makefile.dep.

Note that $(RIOTCPU)/$(CPU)/Makefile.dep is automatically included in $(RIOTBASE)/Makefile.dep:

RIOT/Makefile.dep

Lines 8 to 9 in f2dbd3b

    
           # include cpu dependencies 
        
           -include $(RIOTCPU)/$(CPU)/Makefile.dep

That's why new Makefile.dep files were introduced (for stm32, mips, etc).

Some CPU families are not fully "fixed": kinetis, efm32 and esp. They should be handled in separate PRs because more work is needed there.

Each CPU family is modified in a separate commit, that should simply the review.

Testing procedure

At least a green Murdock
Maybe we should also check the modules loaded for each boards on each application ?

Issues/PRs references

Part of the work needed in #9913

gschorcht · 2019-12-07T19:11:16Z

Should Makefile.dep really contain all USEMODULE or only those that depend on other modules? I'm not sure.

aabadie · 2019-12-07T19:17:24Z

Should Makefile.dep really contain all USEMODULE or only those that depend on other modules? I'm not sure.

If the modules defined in those Makefile.dep are used later to resolve some dependencies then I would say yes. This will make the "normal" dependency resolution equal to the one performed by the info-boards-supported target (see info-global.inc.mk file).

gschorcht · 2019-12-08T07:59:55Z

Should Makefile.dep really contain all USEMODULE or only those that depend on other modules? I'm not sure.

If the modules defined in those Makefile.dep are used later to resolve some dependencies then I would say yes.

Yes, of course, you are right.

gschorcht

Seems to be OK for me.

fjmolinas · 2019-12-08T20:45:37Z

Maybe we should also check the modules loaded for each boards on each application ?

@aabadie you could use save_all_dependencies_resolution_variables.sh for this.

leandrolanzieri · 2019-12-09T16:09:11Z

Maybe we should also check the modules loaded for each boards on each application ?

@aabadie you could use save_all_dependencies_resolution_variables.sh for this.

I'm running the script on this PR, it takes a while. I'll post the results when it's done.

aabadie · 2019-12-10T10:31:34Z

I'm running the script on this PR, it takes a while. I'll post the results when it's done.

Thanks a lot @leandrolanzieri !

leandrolanzieri · 2019-12-10T11:08:35Z

@aabadie here there is a diff only on the results for the examples. The diff is not really easy to follow, the script does not order the variables :- /

I ran the script after applying the PR (584f24c) and before (f2dbd3b). I'm seeing various false positives (e.g. oneway_malloc appearing more than once) but also some real differences (arduino_hello-world/info-global/dependencies_info-boards-supported_acd52832).

aabadie · 2019-12-11T06:47:02Z

Thanks for providing your results @leandrolanzieri. The differences are indeed hard to read but, except some false positives with DEFAULT_MODULE, the other differences are normal I would say. For example, now it's reported that acd52832 (and in fact all nrf boards) are pulling the nrf5x_common_periph module. Before these modules were not appearing. So I think that it's a good point for this PR.
I saw the same thing with stm32.

There's another fix I could see from the results: the dependency to periph_uart/stdio_uart for lpc2387 based boards is now explicit because the newlib dependency appears (and it depends on stdio_uart). Same thing with riscv.

fjmolinas · 2019-12-15T18:43:55Z

@aabadie The differences here are really hard to read and verify, it might be easier if we split and address the changes individually.

aabadie · 2020-01-27T11:58:19Z

rebased

fjmolinas · 2020-01-28T10:46:53Z

I re-ran dist/tools/buildsystem_sanity_check/save_all_dependencies_resolution_variables.sh with sorted variable output, I'll PR this change separately. I get a lot of differences but only for mips and because of the addition of FEATURES_REQUIRED += periph_timer.

The diff can be found in this https://gist.github.com/fjmolinas/b34d9d28b87915c5a904efcc381b3861

fjmolinas

ACK!

fjmolinas · 2020-01-28T10:59:04Z

If wanted I can also provide the whole dependency dump.

fjmolinas · 2020-01-28T10:59:59Z

I would like someone else to ACK this, since it affects so many modules. But otherwise I think it is good to go.

aabadie · 2020-01-28T12:11:18Z

cpu/mips32r2_common/Makefile.include

  #Use UHI to handle syscalls
  LINKFLAGS += -luhi
-  export USEMODULE += newlib_syscalls_mips_uhi
+  USEMODULE += newlib_syscalls_mips_uhi


This line should be removed, this was introduced by a wrong rebase

you mean the exports had already been removed?

There was an PR for removing the export, but this PR is moving the line in Makefile.dep. In the current state, the line is duplicated.

This commit also removes not needed use of export

aabadie · 2020-01-28T12:28:11Z

@fjmolinas, there's still one USE_PKG in cpu/efm32, see here but I'm not sure how to handle the condition on the CPU_ARCH variable in Makefile.dep. What do you think ?

fjmolinas · 2020-01-28T12:39:20Z

@fjmolinas, there's still one USE_PKG in cpu/efm32, see here but I'm not sure how to handle the condition on the CPU_ARCH variable in Makefile.dep. What do you think ?

I would just leave it, #13174 will move it out.

fjmolinas · 2020-01-28T12:41:20Z

I would just leave it, #13174 will move it out.

Or review #13174 :D

aabadie · 2020-01-28T12:46:09Z

Or review #13174 :D

I already did in fact, just forgot about it...

aabadie · 2020-01-28T13:25:40Z

All green here. @leandrolanzieri maybe you can give this one another brief look and eventually the second ACK ?

leandrolanzieri

Changes make sense, and according to the diff provided by @fjmolinas dependency resolution variables are the same (with the exception of periph_timer feature for mips). ACK.

gschorcht

ACK

gschorcht · 2020-01-28T17:09:58Z

@aabadie Thank you very much for this contribution. A big step towards changing the order of including the CPU Makefile.dep

aabadie added Area: build system Area: Build system Type: cleanup The issue proposes a clean-up / The PR cleans-up parts of the codebase / documentation Area: cpu Area: CPU/MCU ports labels Dec 7, 2019

aabadie requested review from fjmolinas and gschorcht December 7, 2019 17:31

aabadie mentioned this pull request Dec 7, 2019

Build dependencies - processing order issues #9913

Open

22 tasks

aabadie added the CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR label Dec 7, 2019

aabadie changed the title ~~cpu: move cpu level dependencies in a separate Makefile.dep~~ cpu: move cpu level dependencies in dedicated Makefile.dep files Dec 7, 2019

gschorcht reviewed Dec 8, 2019

View reviewed changes

aabadie force-pushed the pr/cpu/dependencies_cleanup branch from 584f24c to 8e94725 Compare December 11, 2019 06:50

aabadie force-pushed the pr/cpu/dependencies_cleanup branch from 8e94725 to 7b9c487 Compare January 27, 2020 11:58

fjmolinas approved these changes Jan 28, 2020

View reviewed changes

fjmolinas mentioned this pull request Jan 28, 2020

makefiles/dependencies_debug.inc.mk: add DEPENDENCY_DEBUG_SORT_VARS #13222

Merged

aabadie commented Jan 28, 2020

View reviewed changes

aabadie added 3 commits January 28, 2020 13:18

cpu/stm32: move all dependencies to Makefile.dep

a32c107

cpu/sam0: factorize dependencies in Makefile.dep

925445d

cpu/nrf5x: move dependencies to Makefile.dep

66d903c

aabadie added 9 commits January 28, 2020 13:20

cpu/mips*: move dependencies to Makefile.dep

e3b3ea0

This commit also removes not needed use of export

cpu/msp430: move dependencies to Makefile.dep

6d46621

cpu/cc26xx_cc13xx: move dependencies to Makefile.dep

1ae5a08

cpu/armv7: move dependencies to Makefile.dep

98780ea

cpu/lpc1768: move dependencies to Makefile.dep

32525b7

cpu/fe310: move dependencies to Makefile.dep

3869397

cpu/atmega_common: move avr-libc-extra dependency to Makefile.dep

90e10d3

cpu/efm32: move some dependencies to Makefile.dep

9b7f060

cpu/atmega_common: move common dependencies

e969b2f

aabadie force-pushed the pr/cpu/dependencies_cleanup branch from 7b9c487 to e969b2f Compare January 28, 2020 12:21

leandrolanzieri approved these changes Jan 28, 2020

View reviewed changes

gschorcht approved these changes Jan 28, 2020

View reviewed changes

gschorcht merged commit ddd7cb0 into RIOT-OS:master Jan 28, 2020

benpicco mentioned this pull request Feb 6, 2020

cpu/lpc2387: implement periph/i2c #13037

Merged

leandrolanzieri added this to the Release 2020.04 milestone Feb 20, 2020

aabadie deleted the pr/cpu/dependencies_cleanup branch March 5, 2020 09:09

	# include cpu dependencies
	-include $(RIOTCPU)/$(CPU)/Makefile.dep

Conversation

aabadie commented Dec 7, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Contribution description

Testing procedure

Issues/PRs references

Uh oh!

gschorcht commented Dec 7, 2019

Uh oh!

aabadie commented Dec 7, 2019

Uh oh!

gschorcht commented Dec 8, 2019

Uh oh!

gschorcht left a comment

Choose a reason for hiding this comment

Uh oh!

fjmolinas commented Dec 8, 2019

Uh oh!

leandrolanzieri commented Dec 9, 2019

Uh oh!

aabadie commented Dec 10, 2019

Uh oh!

leandrolanzieri commented Dec 10, 2019

Uh oh!

aabadie commented Dec 11, 2019

Uh oh!

fjmolinas commented Dec 15, 2019

Uh oh!

aabadie commented Jan 27, 2020

Uh oh!

fjmolinas commented Jan 28, 2020

Uh oh!

fjmolinas left a comment

Choose a reason for hiding this comment

Uh oh!

fjmolinas commented Jan 28, 2020

Uh oh!

fjmolinas commented Jan 28, 2020

Uh oh!

aabadie Jan 28, 2020

Choose a reason for hiding this comment

Uh oh!

fjmolinas Jan 28, 2020

Choose a reason for hiding this comment

Uh oh!

aabadie Jan 28, 2020

Choose a reason for hiding this comment

Uh oh!

aabadie Jan 28, 2020

Choose a reason for hiding this comment

Uh oh!

aabadie commented Jan 28, 2020

Uh oh!

fjmolinas commented Jan 28, 2020

Uh oh!

fjmolinas commented Jan 28, 2020

Uh oh!

aabadie commented Jan 28, 2020

Uh oh!

aabadie commented Jan 28, 2020

Uh oh!

leandrolanzieri left a comment

Choose a reason for hiding this comment

Uh oh!

gschorcht left a comment

Choose a reason for hiding this comment

Uh oh!

gschorcht commented Jan 28, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

aabadie commented Dec 7, 2019 •

edited

Loading