drivers: add driver specific Makefile.dep by aabadie · Pull Request #14386 · RIOT-OS/RIOT

aabadie · 2020-06-29T08:49:25Z

Contribution description

This PR splits the main drivers/Makefile.dep in driver specific Makefile.deps.

The dependency resolution for drivers modules defined as pseudo-modules still have to be done in the main drivers/Makefile.dep but just add the "real" module as a dependency.

There was a dependency resolution for nrfmin in drivers/Makefile.dep but it has nothing to do here, this PR moves it to cpu/nrf5x_common/Makefile.dep.

Testing procedure

A green Murdock
Use build system tools to ensure no dependency has changed (help: what are the commands already ?)

Issues/PRs references

Similar to #14369 but for Makefile.dep

aabadie · 2020-06-29T08:56:03Z

I fear that this one will be hard to maintain in the long run...

gschorcht · 2020-06-29T11:16:17Z

Use build system tools to ensure no dependency has changed (help: what are the commands already ?)

Would be dist/tools/buildsystem_sanity_check/save_all_dependencies_resolution_variables.sh sufficient?

fjmolinas · 2020-06-29T11:29:47Z

I fear that this one will be hard to maintain in the long run...

You mean this PR, or the actual changes? If no dependencies have changed I think we can move quickly with this one, but is @miri64 OK with us merging this one in soft feature freeze?

fjmolinas · 2020-06-29T11:31:54Z

You mean this PR, or the actual changes? If no dependencies have changed I think we can move quickly with this one, but is @miri64 OK with us merging this one in soft feature freeze?

I'm currently running the script for #14353 I can run i for this PR after wards.

gschorcht · 2020-06-29T11:37:59Z

drivers/Makefile.dep

-  FEATURES_REQUIRED += periph_spi
 endif

 ifneq (,$(filter ccs811_full,$(USEMODULE)))


I wonder if it would be better to always address driver variants defined by pseudomodules with %, for example.

Suggested change

ifneq (,$(filter ccs811_full,$(USEMODULE)))

ifneq (,$(filter ccs811%,$(USEMODULE)))

Everything else is done by drivers/*/Makefile.dep. For most of the pseudomodules it's already done in that way, but for some of them direct names are used.

Sure, I started with the brute force way but we can refine for sure !

Since we touch it once, we should change it with this PR. If I'm right, there are less than 10 occurrences.

Addressed, I prefer to use ccs811_%.

drivers/mhz19/Makefile.dep

aabadie · 2020-06-29T11:53:00Z

I'm currently running the script for #14353 I can run i for this PR after wards.

Thanks!

gschorcht · 2020-06-29T12:07:40Z

@aabadie I went through all the changes. From what I have seen, it should be the same. But we should rely on tools. If it helps, I could run the script if @fjmolinas run for PR #14353 takes too long.

fjmolinas · 2020-06-29T13:30:10Z

@aabadie I went through all the changes. From what I have seen, it should be the same. But we should rely on tools. If it helps, I could run the script if @fjmolinas run for PR #14353 takes too long.

The script is still running on my side, so feel free to run it yourself. I would probably run it tonight and look at the results tomorrow/

gschorcht · 2020-06-29T13:44:37Z

@aabadie I went through all the changes. From what I have seen, it should be the same. But we should rely on tools. If it helps, I could run the script if @fjmolinas run for PR #14353 takes too long.

The script is still running on my side, so feel free to run it yourself. I would probably run it tonight and look at the results tomorrow/

Ok, I will start it immediatly. My computer is ~~borrowing~~ 😉 (edit: bored 😆)

gschorcht · 2020-06-29T13:52:15Z

@fjmolinas for each application I get the two warnings

boards/arduino-nano-33-ble/Makefile.include:27: warning: overriding recipe for target 'term-delay'
makefiles/tools/usb_board_reset.mk:35: warning: ignoring old recipe for target 'term-delay'

Is that normal?

fjmolinas · 2020-06-29T14:14:15Z

@fjmolinas for each application I get the two warnings

boards/arduino-nano-33-ble/Makefile.include:27: warning: overriding recipe for target 'term-delay'
makefiles/tools/usb_board_reset.mk:35: warning: ignoring old recipe for target 'term-delay'

Is that normal?

Not sure if normal, but unrelated. I'll take a look.

fjmolinas · 2020-06-30T07:11:07Z

The output of running ./dist/tools/buildsystem_sanity_check/save_all_dependencies_resolution_variables.sh diff.txt, there are only differences in FEATURES_REQUIRED and FEATURES_OPTIONAL, but no diff in FEATURES_USED or USEMODULE. So there is no dependencies have changed, it is probably parsed more times than before. I wan't to try to understand why, but I dont' necessarily see this as a problem.

Wow, you have already finished it. My one is still running since I forgot to start the second run yesterday in the evening 😟

I have a second machine, useful for running this kind of things while keeping my laptop free.

gschorcht · 2020-06-30T08:22:09Z

The output of running ./dist/tools/buildsystem_sanity_check/save_all_dependencies_resolution_variables.sh diff.txt, there are only differences in FEATURES_REQUIRED and FEATURES_OPTIONAL

FEATURES_REQUIRED and FEATURES_OPTIONAL look really weird at times. But as I understand it, it is important that the sorted versions of these lists do not differ.

gschorcht · 2020-07-10T16:52:22Z

Now that the release branch was created after the hard feature freeze, we should merge this PR as soon as possible before new drivers are merged.

@aabadie Please squash.
@fjmolinas Based on the results of your comparison in #14386 (comment), can we be sure that the dependencies are resolved correctly, even if FEATURES_REQUIRED and FEATURES_OPTIONAL sometimes look very strange (the complete list of features multiple times)? Sorted versions of these variables don't differ.

aabadie · 2020-07-10T16:57:22Z

I had conflicts when rebasing the last time (a couple of hours ago) so I think it's safer to re-run the checks. I rebased again and squashed the branch.

benpicco

Soft feature freeze is over, Murdock is happy and this is a good improvement.
Let's get this in before new merge conflicts pop up.

aabadie · 2020-07-15T18:43:24Z

rebased once again.

gschorcht · 2020-07-15T21:18:15Z

Again, looks good from my point of view.

Ping @fjmolinas Based on the results of your comparison in #14386 (comment), can we be sure that the dependencies are resolved correctly, even if FEATURES_REQUIRED and FEATURES_OPTIONAL sometimes look very strange (the complete list of features is in multiple times)? Sorted versions of these variables don't differ.

aabadie · 2020-07-20T11:13:41Z

rebased to solve the conflict introduced when #14497 was just merged.

gschorcht · 2020-07-20T11:24:48Z

We should merge it before further conflicts pop up. But I'm a bit unsure without the final statement from @fjmolinas 😟

aabadie · 2020-07-20T11:26:28Z

But I'm a bit unsure without the final statement from @fjmolinas

I can ping him IRL :D

gschorcht · 2020-07-20T11:32:52Z

But I'm a bit unsure without the final statement from @fjmolinas

I can ping him IRL :D

That would be good. My concern was that although the sorted versions of FEATURES_REQUIRED and FEATURES_OPTIONAL were the same in his test, the unsorted versions of these variables sometimes looked really strange (the same list of features multiple times). I would rely on his oppinion if he says that it doesn't matter.

fjmolinas

Found why there are repeated features, you can squash right away.

fjmolinas · 2020-07-20T11:43:12Z

Makefile.dep

 include $(RIOTBASE)/drivers/Makefile.dep

+# pull Makefile.dep of each driver modules if they exist
+-include $(USEMODULE:%=$(RIOTBASE)/drivers/%/Makefile.dep)


Suggested change

-include $(USEMODULE:%=$(RIOTBASE)/drivers/%/Makefile.dep)

-include $(sort $(USEMODULE:%=$(RIOTBASE)/drivers/%/Makefile.dep))

This will fix having the list included multiple times.

Only keep the dependency resolution for drivers declared as pseudo-modules

fjmolinas

ACK

nagrawal63 · 2020-07-27T15:13:24Z

driver-guide.md is not updated to include these changes since it still says that you have to add the driver dependencies in the Makefile.dep file in $(RIOTBASE)/drivers/Makefile.dep (at line no. 307).

Could you kindly check into it ?

aabadie requested a review from fjmolinas June 29, 2020 08:49

aabadie requested review from basilfx, benpicco, bergzand, daniel-k, gschorcht, haukepetersen, jia200x, leandrolanzieri, maribu, miri64 and smlng as code owners June 29, 2020 08:49

aabadie force-pushed the pr/drivers/makefile_dep branch from 72f2063 to d9e4301 Compare June 29, 2020 08:55

gschorcht reviewed Jun 29, 2020

View reviewed changes

drivers/mhz19/Makefile.dep Outdated Show resolved Hide resolved

aabadie force-pushed the pr/drivers/makefile_dep branch from d9e4301 to ffbcb8a Compare June 29, 2020 14:21

aabadie force-pushed the pr/drivers/makefile_dep branch from 32c4817 to 56ad5a0 Compare July 10, 2020 14:34

aabadie force-pushed the pr/drivers/makefile_dep branch from 56ad5a0 to 7d8d89a Compare July 10, 2020 16:57

benpicco approved these changes Jul 15, 2020

View reviewed changes

aabadie force-pushed the pr/drivers/makefile_dep branch from 7d8d89a to 3b8ac51 Compare July 15, 2020 18:43

aabadie force-pushed the pr/drivers/makefile_dep branch from 3b8ac51 to de3d388 Compare July 15, 2020 18:53

aabadie 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 Jul 15, 2020

aabadie force-pushed the pr/drivers/makefile_dep branch from de3d388 to 0357b44 Compare July 20, 2020 11:13

fjmolinas requested changes Jul 20, 2020

View reviewed changes

aabadie added 3 commits July 20, 2020 14:32

Makefile.dep: include per driver Makefile.dep if used and exist

54d51bb

drivers: configure per driver dependency resolution

304bce3

drivers/Makefile.dep: remove global dependency resolution

0c86b72

Only keep the dependency resolution for drivers declared as pseudo-modules

aabadie force-pushed the pr/drivers/makefile_dep branch from 0357b44 to 0c86b72 Compare July 20, 2020 12:32

fjmolinas approved these changes Jul 20, 2020

View reviewed changes

fjmolinas merged commit 14e232d into RIOT-OS:master Jul 20, 2020

aabadie deleted the pr/drivers/makefile_dep branch July 20, 2020 13:27

benpicco mentioned this pull request Nov 16, 2020

drivers/at24mac: fix dependency #15449

Closed

	ifneq (,$(filter ccs811_full,$(USEMODULE)))
	ifneq (,$(filter ccs811%,$(USEMODULE)))

	-include $(USEMODULE:%=$(RIOTBASE)/drivers/%/Makefile.dep)
	-include $(sort $(USEMODULE:%=$(RIOTBASE)/drivers/%/Makefile.dep))

Conversation

aabadie commented Jun 29, 2020

Contribution description

Testing procedure

Issues/PRs references

Uh oh!

aabadie commented Jun 29, 2020

Uh oh!

gschorcht commented Jun 29, 2020

Uh oh!

fjmolinas commented Jun 29, 2020

Uh oh!

fjmolinas commented Jun 29, 2020

Uh oh!

gschorcht Jun 29, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

aabadie Jun 29, 2020

Choose a reason for hiding this comment

Uh oh!

gschorcht Jun 29, 2020

Choose a reason for hiding this comment

Uh oh!

aabadie Jun 29, 2020

Choose a reason for hiding this comment

Uh oh!

Uh oh!

aabadie commented Jun 29, 2020

Uh oh!

gschorcht commented Jun 29, 2020

Uh oh!

fjmolinas commented Jun 29, 2020

Uh oh!

gschorcht commented Jun 29, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

gschorcht commented Jun 29, 2020

Uh oh!

fjmolinas commented Jun 29, 2020

Uh oh!

fjmolinas commented Jun 30, 2020

Uh oh!

gschorcht commented Jun 30, 2020

Uh oh!

gschorcht commented Jul 10, 2020

Uh oh!

aabadie commented Jul 10, 2020

Uh oh!

benpicco left a comment

Choose a reason for hiding this comment

Uh oh!

aabadie commented Jul 15, 2020

Uh oh!

gschorcht commented Jul 15, 2020

Uh oh!

aabadie commented Jul 20, 2020

Uh oh!

gschorcht commented Jul 20, 2020

Uh oh!

aabadie commented Jul 20, 2020

Uh oh!

gschorcht commented Jul 20, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

fjmolinas left a comment

Choose a reason for hiding this comment

Uh oh!

fjmolinas Jul 20, 2020

Choose a reason for hiding this comment

Uh oh!

aabadie Jul 20, 2020

Choose a reason for hiding this comment

Uh oh!

fjmolinas left a comment

Choose a reason for hiding this comment

Uh oh!

nagrawal63 commented Jul 27, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

gschorcht Jun 29, 2020 •

edited

Loading

gschorcht commented Jun 29, 2020 •

edited

Loading

gschorcht commented Jul 20, 2020 •

edited

Loading

nagrawal63 commented Jul 27, 2020 •

edited

Loading