build system: Restructure dependency resolution #13349

maribu · 2020-02-11T20:01:53Z

Contribution description

Added `FEATURES_REQUIRED_ANY`

Modules can now express a dependency relation in which only one out of a set of features are required. E.g. FEATURES_REQUIRED_ANY += periph_uart|periph_usbdev could added to the corresponding Makefile.dep if a module requires the feature periph_uart OR periph_usbdev. (Note: The or is not an exclusive or, so due to other dependencies both features might be used.)

Restructured Dependency Resolution

The addition of FEATURES_REQUIRED_ANY introduced huge performance issues. A restructuring of the dependency resolution was done to address these and also make the dependency resolution a bit easier to understand.

In detail, the changes are:

Makefile.dep
- Dropped handling of default modules and recursion
- Now only dependencies of the current set of used modules and pkgs are added
  ==> External recursion is needed to catch transitive dependencies
Changed Makefile.features:
- Dropped checking of provided features
- Dropped populating FEATURES_USED with provided features that are required or optional
- Dropped populating FEATURES_MISSING with required but not provided features
- Dropped adding modules implementing used features to USE_MODULE
  ==> This now only populates FEATURES_PROVIDED, nothing more
Added makefiles/features_check.inc.mk:
- This performs the population of FEATURES_USED and FEATURES_MISSING now
Added makefiles/features_modules.inc.mk:
- This performs now the addition of modules implementing used features
Added makefiles/dependency_resolution.inc.mk:
- This now performs the recursion required to catch transitive dependencies
- Also the feature check is performed recursively to handle also required and optional features of the transitive dependencies
Use simply expanded variables instead of recursively expended variables
(foo := $(bar) instead foo = $(bar)) for internal variables during feature resolution. This improves performance significantly for make info-boards-supported.
Reduce dependency resolution steps in make info-boards-supported
- Globally resolve dependencies without any features (including arch) provided
  ==> This results in the common subset of feature requirements and modules used
  - But for individual boards additional modules might be used on top due to architecture specific dependencies or optional features
- Boards not supporting this subset of commonly required features are not supported, so no additional dependency resolution is needed for them
- For each board supporting the common set of requirements a complete dependency resolution is still needed to also catch architecture specific hacks
  - But this resolution is seeded with the common set of dependencies to speed this up

Testing procedure

tests/driver_ws281x should still have the same output for make info-boards-supported
Compilation of no other app is broken (Murdock will check this)
The save_all_dependencies script should have the same output as before, except for FEATURES_REQUIRED_ANY and the auto_init_% modules that are explicitly blacklisted in three applications now

Issues/PRs references

Alternative to #13343

aabadie · 2020-02-12T08:18:40Z

Interesting idea but I would call the new variable FEATURES_REQUIRED_ANY.

fjmolinas · 2020-02-12T08:28:18Z

Nice, I like the idea. How does it interact with FEATURES_BLACKLIST?

maribu · 2020-02-12T09:03:26Z

How does it interact with FEATURES_BLACKLIST?

Currently not well, but it certainly should!

I will adapt the code to only consider non-blacklisted and provided features for addition.

aabadie

Thanks for adopting FEATURES_REQUIRED_ANY :)

There are left-overs in the porting board documentation.

doc/doxygen/src/build-system-basics.md

maribu · 2020-02-12T12:08:39Z

I think I now addressed all comments (and fixed the leftovers in the doc). Please have a second look :-)

drivers/Makefile.dep

fjmolinas · 2020-02-12T12:37:49Z

Why does this get a special character (|) and other feature lists not? I think parsing (and declaring) will become way easier, if we just use whitespaces (the construct above need to take whitespaces into account anyway as when FEATURES_REQUIRED_ANY is already set to something it will become something arch_avr8|arch_native.

Using | is what we do for FEATURES_CONFLICT with :, FEATURES_REQUIRED_ANY are lists of lists, so you need the extra character.

You would never have FEATURES_REQUIRED_ANY += something you would jsut use FEATURES_REQUIRED for that, you could have arch_avr8|arch_native perih_rtc|periph_rtt, and here | becomes useful.

maribu · 2020-02-12T12:51:15Z

BTW: Technically, there is no need to distinguish between FEATURES_REQUIRED and FEATURES_REQUIRED_ANY. I just did so for consistence with FEATURES_OPTIONAL and FEATURES_BLACKLIST.

So if anyone would prefer to be able to do something like: FEATURES_REQUIRED += periph_spi arch_arm|arch_avr8 to represent periph_spi && (arch_arm || arch_avr8) I could adept to that.

fjmolinas

I think this looks good overall, documentation needs some improvements and I think with some functions the handling can be made more explicit.

makefiles/info-global.inc.mk and makefiles/info.inc.mk need updating as well.
I think the function name could be better to document the behavior because
- _features_any : selects the first one of the foo|bar|bas that is also PROVIDED, so maybe _features_use_first_one or something like that. I suggested some changes below.

Makefile.features

maribu · 2020-02-16T08:41:05Z

* `makefiles/info-global.inc.mk` and `makefiles/info.inc.mk` need updating as well.

Note that an unsatisfied FEATURES_REQUIRED_ANY is just placed into FEATURES_MISSING. Thus, make info-boards-features-missing prints those and by the use of the | marker, they are distinguishable from normal features.

I updated makefiles/info.inc.mk so that make info-build also prints FEATURES_REQUIRED_ANY.

maribu · 2020-02-26T18:11:52Z

I rebased to fix the merge conflict, but didn't squash to not interfere with the review process.

fjmolinas

Some minor nitpicking naming comments, otherwise all good. The handling and steps are a lot more clear now!

Makefile.features

doc/doxygen/src/build-system-basics.md

Makefile.features

fjmolinas · 2020-03-10T07:51:02Z

All my changes requests have been addresssed, @aabadie can you take a second look at yours?

fjmolinas · 2020-03-13T18:47:54Z

ping @aabadie !!

maribu · 2020-04-02T07:09:50Z

F***ck. Fixed one bug, open hundred new ones :-D

fjmolinas · 2020-04-02T07:11:52Z

But: The unit tests actually use some auto_init submodules, namely: auto_init_gnrc_ipv6 auto_init_gnrc_ipv6_nib auto_init_gnrc_pktbuf auto_init_gnrc_sixlowpan auto_init_gnrc_udp auto_init_random auto_init_xtimer. These are also used in master. But I doubt that any of these get properly executed without auto_init being used.

They don't need any auto_init module, they get through because of the issue mentioned above, this was just not detected in #13089, and so these auto_init modules where added although not wanted.

This prevents disabling this modules e.g. due to missing auto_init. I think we should just have both DEFAULT_MODULE and USEMODULE as directly expended variables to make sure such mind blowing behavior does not manifest.

Not sure about USEMODULE, but what you say is what I did for DEFAULT_MODULE in #13785. Here I would rather just add DISABLE_MODULE += auto_init_% in all applications with DISABLE_MODULE += auto_init for now.

Lets handle the rest of the cleanup in a different PR.

fjmolinas · 2020-04-02T07:13:13Z

Not sure about USEMODULE, but what you say is what I did for DEFAULT_MODULE in #13785. Here I would rather just add DISABLE_MODULE += auto_init_% in all applications with DISABLE_MODULE += auto_init for now.

Immediately expanding USEMODULE could have many other consequence I would rather not deal with right now, lets introduce this changes one at a time :)

maribu · 2020-04-02T07:14:46Z

In Makefile.dep (at root of the Repo) e.g. this snipped relies on DEFAULT_MODULES getting turned into USEMODULE during dependency resolution.

ifneq (,$(filter auto_init_saul,$(USEMODULE)))                                   
  USEMODULE += saul_init_devs                                                    
endif 
``

:-/

maribu · 2020-04-02T07:16:33Z

Lets handle the rest of the cleanup in a different PR.

👍. This PR is already complex enough

fjmolinas · 2020-04-02T07:17:30Z

In Makefile.dep (at root of the Repo) e.g. this snipped relies on DEFAULT_MODULES getting turned into USEMODULE during dependency resolution.

Damn I ACK'ed that, it slip through.

maribu · 2020-04-02T07:24:52Z

Damn I ACK'ed that, it slip through.

I would have also ACK'ed that. I only just now got aware of the issues this could have.

fjmolinas · 2020-04-02T07:47:01Z

@maribu all good now, I would squash the last 3 commits into 1.

Goals: - Untangle dependency resolution and feature checking for better maintainability - Improve performance of "make info-boards-supported" Changes: - Makefile.dep - Dropped handling of default modules and recursion - Now only dependencies of the current set of used modules and pkgs are added ==> External recursion is needed to catch transient dependencies - Changed Makefile.features: - Dropped checking of provided features - Dropped populating FEATURES_USED with provided features that are required or optional - Dropped populating FEATURES_MISSING with required but not provided features - Dropped adding modules implementing used features to USE_MODULE ==> This now only populates FEATURES_PROVIDED, nothing more - Added makefiles/features_check.inc.mk: - This performs the population of FEATURES_USED and FEATURES_MISSING now - Added makefiles/features_modules.inc.mk: - This performs now the addition of modules implementing used features - Added makefiles/dependency_resolution.inc.mk: - This now performs the recursion required to catch transient dependencies - Also the feature check is performed recursively to handle also required and optional features of the transient dependencies - DEFAULT_MODULES are added repeatedly to allow it to be extended based on used features and modules ==> This allows modules to have optional dependencies, as these dependencies can be blacklisted - Use simply expanded variables instead of recursively expended variables (`foo := $(bar)` instead `foo = $(bar)`) for internal variables during feature resolution. This improves performance significantly for `make info-boards-supported`. - Reduce dependency resolution steps in `make info-boards-supported` - Globally resolve dependencies without any features (including arch) provided ==> This results in the common subset of feature requirements and modules used - But for individual boards additional modules might be used on top due to architecture specific dependencies or optional features - Boards not supporting this subset of commonly required features are not supported, so no additional dependency resolution is needed for them - For each board supporting the common set of requirements a complete dependency resolution is still needed to also catch architecture specific hacks - But this resolution is seeded with the common set of dependencies to speed this up

- Add FEATURES_REQUIRED_ANY to dependency-debug: Now `make dependency-debug` by default also stores the contents of `FEATURES_REQUIRED_ANY`. - makefiles/features_check.inc.mk: Break long lines - {tests/minimal,tests/unittests,bootloaders/riotboot}: Disable auto_init_% in addition to auto_init. This works around weird behavior due to the USEMODULE being recursively expended in the first iteration of dependency resolution: Modules added to DEFAULT_MODULE get automatically added to USEMODULE during the first run, but not for subsequent. This should be iron out later on.

fjmolinas · 2020-04-02T08:27:48Z

My ACK still stands.

fjmolinas · 2020-04-02T08:32:03Z

I changed the PR title to the main change of this PR.

fjmolinas · 2020-04-02T08:32:41Z

I changed the PR title to the main change of this PR.

I would be nice to update the PR description as well.

leandrolanzieri

Besides the original intention of the PR of allowing "one out of" dependencies, it turned into a restructuring of our dependency resolution process.

As @fjmolinas mentioned, the reviewing process of this PR brought attention to many existing issues, and the cleanup helps understanding the process better. Changes look good, follow-up work has been identified, and multiple tests and dependency checks have been run.

It's an ACK from my side. Let's get this to the freeze, I'm sure all of us will be on the ball if issues pop up during the next days ;-)

fjmolinas · 2020-04-02T08:46:50Z

Thanks for sticking through with this @maribu and for the good will to tackle many unrelated issues that surfaced because of your initial change! Thanks for the help reviewing the final mile @leandrolanzieri!

maribu · 2020-04-02T09:13:34Z

@fjmolinas, @leandrolanzieri: Thanks for the review and helping to understand the various issues that needed to be addressed :-)

maribu · 2020-04-02T09:29:31Z

It would be nice to update the PR description as well.

Done

maribu added Type: enhancement The issue suggests enhanceable parts / The PR enhances parts of the codebase / documentation Area: build system Area: Build system labels Feb 11, 2020

maribu requested review from benpicco, fjmolinas, aabadie and miri64 February 11, 2020 20:02

miri64 added the Discussion: RFC The issue/PR is used as a discussion starting point about the item of the issue/PR label Feb 11, 2020

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 11, 2020

basilfx changed the title ~~Makefile.featuers: Allow modeling "one out of" dependencies~~ Makefile.features: Allow modeling "one out of" dependencies Feb 11, 2020

aabadie requested changes Feb 12, 2020

View reviewed changes

doc/doxygen/src/build-system-basics.md Outdated Show resolved Hide resolved

doc/doxygen/src/build-system-basics.md Outdated Show resolved Hide resolved

doc/doxygen/src/build-system-basics.md Outdated Show resolved Hide resolved

miri64 reviewed Feb 12, 2020

View reviewed changes

drivers/Makefile.dep Show resolved Hide resolved

fjmolinas reviewed Feb 12, 2020

View reviewed changes

Makefile.features Outdated Show resolved Hide resolved

Makefile.features Outdated Show resolved Hide resolved

Makefile.features Outdated Show resolved Hide resolved

Makefile.features Outdated Show resolved Hide resolved

Makefile.features Outdated Show resolved Hide resolved

benpicco mentioned this pull request Feb 26, 2020

tests/driver_ws281x: resolve weird feature dependencies #13343

Merged

maribu force-pushed the dependency-or branch from 70c8e09 to dd8f13b Compare February 26, 2020 18:11

maribu requested review from jia200x, leandrolanzieri, MichelRottleuthner and smlng as code owners February 26, 2020 18:11

fjmolinas reviewed Feb 28, 2020

View reviewed changes

Makefile.features Outdated Show resolved Hide resolved

doc/doxygen/src/build-system-basics.md Outdated Show resolved Hide resolved

Makefile.features Outdated Show resolved Hide resolved

maribu mentioned this pull request Mar 7, 2020

drivers/ws281x: Add GPIO ABC backend #12020

Closed

maribu removed the CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR label Apr 2, 2020

maribu added the CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR label Apr 2, 2020

maribu force-pushed the dependency-or branch from e5d45e0 to d77e0ab Compare April 2, 2020 07:55

maribu force-pushed the dependency-or branch from d77e0ab to af21f15 Compare April 2, 2020 07:59

fjmolinas changed the title ~~Makefile.features: Allow modeling "one out of" dependencies~~ build system: Restructure dependency resolution Apr 2, 2020

leandrolanzieri approved these changes Apr 2, 2020

View reviewed changes

leandrolanzieri added this to the Release 2020.04 milestone Apr 2, 2020

leandrolanzieri merged commit c74544c into RIOT-OS:master Apr 2, 2020

maribu deleted the dependency-or branch April 2, 2020 09:13

maribu mentioned this pull request Apr 2, 2020

*/Makefile.dep: remove usage of DEFAULT_MODULE += stdio% #13791

Merged

This was referenced May 25, 2020

makefiles/info-global: Remove dependency resolution cache #14132

Merged

[backport 2020.04] makefiles/info-global: Remove dependency resolution cache #14136

Merged

fjmolinas mentioned this pull request Sep 11, 2020

Makefile.include: initial features check before dependency resolution #15013

Merged

fjmolinas mentioned this pull request Jan 26, 2021

makefiles: add FEATURES_OPTIONAL_ANY #15855

Closed

build system: Restructure dependency resolution #13349

build system: Restructure dependency resolution #13349

Uh oh!

Conversation

maribu commented Feb 11, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Contribution description

Added FEATURES_REQUIRED_ANY

Restructured Dependency Resolution

Testing procedure

Issues/PRs references

Uh oh!

aabadie commented Feb 12, 2020

Uh oh!

fjmolinas commented Feb 12, 2020

Uh oh!

maribu commented Feb 12, 2020

Uh oh!

aabadie left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

maribu commented Feb 12, 2020

Uh oh!

Uh oh!

fjmolinas commented Feb 12, 2020

Uh oh!

maribu commented Feb 12, 2020

Uh oh!

fjmolinas left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

maribu commented Feb 16, 2020

Uh oh!

maribu commented Feb 26, 2020

Uh oh!

fjmolinas left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

fjmolinas commented Mar 10, 2020

Uh oh!

fjmolinas commented Mar 13, 2020

Uh oh!

maribu commented Apr 2, 2020

Uh oh!

fjmolinas commented Apr 2, 2020

Uh oh!

fjmolinas commented Apr 2, 2020

Uh oh!

maribu commented Apr 2, 2020

Uh oh!

maribu commented Apr 2, 2020

Uh oh!

fjmolinas commented Apr 2, 2020

Uh oh!

maribu commented Apr 2, 2020

Uh oh!

fjmolinas commented Apr 2, 2020

Uh oh!

fjmolinas commented Apr 2, 2020

Uh oh!

fjmolinas commented Apr 2, 2020

Uh oh!

fjmolinas commented Apr 2, 2020

Uh oh!

leandrolanzieri left a comment

Choose a reason for hiding this comment

Uh oh!

fjmolinas commented Apr 2, 2020

Uh oh!

maribu commented Feb 11, 2020 •

edited

Loading

Added `FEATURES_REQUIRED_ANY`