build system: Restructure dependency resolution

[maribu]

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]

Interesting idea but I would call the new variable FEATURES_REQUIRED_ANY.

[fjmolinas]

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

[maribu]

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.

[maribu]

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

[fjmolinas]

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]

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.

[maribu]

* `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]

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!

[fjmolinas]

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

[fjmolinas]

ping @aabadie !!

[maribu]

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

[fjmolinas]

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]

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]

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]

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

👍. This PR is already complex enough

[fjmolinas]

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]

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]

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

[fjmolinas]

My ACK still stands.

[fjmolinas]

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

[fjmolinas]

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]

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]

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

[maribu]

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

Done