makefiles: add FEATURES_OPTIONAL_ANY #15855

fjmolinas · 2021-01-26T08:53:42Z

Contribution description

From looking at #15836 and #15817, it's clear that some dependencies are better modeled in Kconfig with choices. This PR looks to give a similar option to model this behavior in make while the migration is ongoing.

So similar to #13349:

FEATURES_OPTIONAL_ALT are nice to have FEATURES that fulfill a similar
function. Alternatives are separated by a pipe (|) in order of preference,
e.g.: FEATURES_OPTIONAL_ALT += puf_sram|periph_hwrng if both are provided
then only puf_sram will be used.

Testing procedure

#12166 Shows a test case for this (I want to rebase that one on top of #15836 and this one)

The save_all_dependencies script should have the same output as before, as this PR does not make use of the feature.

Issues/PRs references

Taken from #12166
Related to #15836

fjmolinas · 2021-02-04T09:30:03Z

makefiles/features_check.inc.mk

+# Additionally optional features due to the "one out of" dependencies,
+#   For each features list in $(FEATURES_OPTIONAL_ALT):
+#     ==> If one (or more) features is usable select the first one in list
+#     ==> If no feature is usable return the list
+FEATURES_OPTIONAL_ONE_OUT_OF := $(call _features_one_out_of,\
+                                  $(FEATURES_OPTIONAL_ALT),\
+                                    $(FEATURES_OPTIONAL) \
+                                    $(FEATURES_USABLE))
+
+FEATURES_OPTIONAL_ONLY := $(sort $(filter-out $(FEATURES_REQUIRED),\
+                            $(FEATURES_OPTIONAL) \
+                            $(FEATURES_OPTIONAL_ONE_OUT_OF)))
+FEATURES_OPTIONAL_USED := $(sort $(filter $(FEATURES_PROVIDED), \
+                            $(FEATURES_OPTIONAL_ONLY)))


After a comment from @leandrolanzieri offline, this should probably be:

Suggested change

# Additionally optional features due to the "one out of" dependencies,

# For each features list in $(FEATURES_OPTIONAL_ALT):

# ==> If one (or more) features is usable select the first one in list

# ==> If no feature is usable return the list

FEATURES_OPTIONAL_ONE_OUT_OF := $(call _features_one_out_of,\

$(FEATURES_OPTIONAL_ALT),\

$(FEATURES_OPTIONAL) \

$(FEATURES_USABLE))

FEATURES_OPTIONAL_ONLY := $(sort $(filter-out $(FEATURES_REQUIRED),\

$(FEATURES_OPTIONAL) \

$(FEATURES_OPTIONAL_ONE_OUT_OF)))

FEATURES_OPTIONAL_USED := $(sort $(filter $(FEATURES_PROVIDED), \

$(FEATURES_OPTIONAL_ONLY)))

# FEATURES_OPTIONAL that have not been required

FEATURES_OPTIONAL_ONLY_SO_FAR := $(sort $(filter-out $(FEATURES_REQUIRED),\

$(FEATURES_OPTIONAL)))

# Only add FEATURES that are usable

FEATURES_OPTIONAL_USED_SO_FAR := $(sort $(filter $(FEATURES_USABLE) \

$(FEATURES_OPTIONAL_ONLY_SO_FAR)))

# Additionally optional features due to the "one out of" dependencies,

# For each features list in $(FEATURES_OPTIONAL_ALT):

# ==> If one (or more) features is already added to OPTIONAL_USED

then the firs of the used ones is returned

# ==> If one (or more) features is usable select the first one in list

# ==> If no feature is usable return the list

FEATURES_OPTIONAL_ONE_OUT_OF := $(call _features_one_out_of,\

$(FEATURES_OPTIONAL_ALT),\

$(FEATURES_OPTIONAL_USED_SO_FAR) \

$(FEATURES_USABLE))

# FEATURES_OPTIONAL that have not been required

FEATURES_OPTIONAL_ONLY := $(sort $(filter-out $(FEATURES_REQUIRED),\

$(FEATURES_OPTIONAL_ONE_OUT_OF)))

# Only add FEATURES that are usable

FEATURES_OPTIONAL_USED := $(sort $(filter $(FEATURES_USABLE) \

$(FEATURES_OPTIONAL_ONLY )))

This way do not add to FEATURES_OPTIONAL_USED non-usable features, and do not add with the FEATURES_OPTIONAL_ALT a feature that will be added bt the OPTIONAL mechanism. In other words:

FEATURES_OPTIONAL has precedence over FEATURES_OPTIONAL_ALT

leandrolanzieri

I quickly tried this on the hello-world example (samr21-xpro), and I am not getting the expected result, the last feature of the list is selected. Am I using it wrongly?

Normal used features

arch_32bit
arch_arm
cpu_core_cortexm
no_idle_thread
periph_gpio
periph_pm
periph_uart

After adding FEATURES_OPTIONAL_ALT += periph_cpuid|periph_timer|periph_i2c to the application Makefile:

New used features

arch_32bit
arch_arm
cpu_core_cortexm
no_idle_thread
periph_gpio
periph_i2c
periph_pm
periph_uart

fjmolinas · 2021-02-04T17:16:50Z

@leandrolanzieri I addressed the issue you pointed out in the first fixup e148d7b, the comment I inlined is addressed in 6c9983b

FEATURES_OPTIONAL_ALT="periph_cpuid|periph_timer|periph_pwm" make -C examples/hello-world/ info-modules
auto_init
board
core
core_idle_thread
core_init
core_msg
core_panic
cpu
native_drivers
periph
periph_common
periph_cpuid
periph_gpio
periph_gpio_linux
periph_init
periph_init_cpuid
periph_init_gpio
periph_init_gpio_linux
periph_init_pm
periph_init_uart
periph_pm
periph_uart
stdio_native
sys

fjmolinas · 2021-02-04T17:17:28Z

Note that in master the issue you pointed out was also present:

FEATURES_REQUIRED_ANY="periph_cpuid|periph_timer|periph_pwm" make -C examples/hello-world/ info-modules
auto_init
board
core
core_idle_thread
core_init
core_msg
core_panic
cpu
native_drivers
periph
periph_common
periph_gpio
periph_gpio_linux
periph_init
periph_init_gpio
periph_init_gpio_linux
periph_init_pm
periph_init_timer
periph_init_uart
periph_pm
periph_timer
periph_uart
stdio_native
sys

fjmolinas · 2021-02-04T17:18:07Z

Note that in master the issue you pointed out was also present:

So nice catch!

maribu

looks good to me. I wonder if we shouldn't name it FEATURES_OPTIONAL_ANY for consistency. Otherwise the relation to FEATURES_REQUIRED_ANY is IMO less obvious.

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

maribu · 2021-02-09T13:47:56Z

Please squash :-) (Also note that commit message needs updating.)

maribu · 2021-02-09T16:44:21Z

Wait, I found an issue:

FEATURES_OPTIONAL_ANY="periph_i2c|periph_gpio" BOARD=samr21-xpro make info-modules -C examples/
hello-world/
[...]
periph_gpio
periph_i2c
[...]

The periph_i2c shoudln't be pulled in. Same issue with FEATURES_REQUIRED_ANY. It does work as expected with FEATURES_REQUIRED_ANY in master.

aabadie · 2021-02-09T17:05:29Z

I think there's an issue with this PR: the number of jobs on Murdock is suspiciously low. The total run took 20 minutes, compared to the others, some applications are not built anymore.

compile job results (0 failed, 27546 passed, 27546 total)

Nowadays, there +80k jobs on Murdock.

fjmolinas · 2021-02-09T17:21:33Z

I think there's an issue with this PR: the number of jobs on Murdock is suspiciously low. The total run took 20 minutes, compared to the others, some applications are not built anymore.
compile job results (0 failed, 27546 passed, 27546 total)
Nowadays, there +80k jobs on Murdock.

Indeed that is strange, I'll take a look.

fjmolinas · 2021-02-09T17:41:03Z

Hmm In Murdock I see that some jobs did not get built for a bunch of BOARDs, tests/saul does not get built for any Nucleo. But the info-board-supported output matches what is expected. Might it have something to do with the fact that a conflict showed up during the build? I'll rebase and run the dependencies script in any case.

FEATURES_OPTIONAL_ANY are nice to have FEATURES that fulfill a similar function. Alternatives are separated by a pipe (|) in order of preference. e.g.: FEATURES_OPTIONAL_ANY += puf_sram|periph_hwrng if both are provided then only puf_sram will be used.

…atures

fjmolinas · 2021-02-22T08:10:40Z

After all these bugs were uncovered with the any algorithm I've realized that to implement this some question would need answering first;

In what order should a FEATURES_OPTIONAL_ANY feature be resolved:

e.g.: if there is an overlap with FEATURES_REQUIRED_ANY I would expect only the overlap to be selected, but that means resolving OPTIONAL_ANY after REQUIRED_ANY.

But since the dependency process is iterative there are no guarantees of this anyway. So I think this is going to become messy very fast, any I'm not sure its worth the effort anymore. I'll still try to take a look at how it would look as soon as I have some time, but as I said I'm not sure its worth it.

maribu · 2021-02-22T08:19:17Z

I think we should aim for best effort here (that is, after FEATURES_REQUIRED_ANY of the same Iteration). Without access to the full dependency graph, we cannot do much bettee anyway IMO.

stale · 2022-03-02T11:18:52Z

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.

maribu · 2022-04-16T09:43:46Z

I kind of assumed that this has already landed. @fjmolinas: Will you reboot this, or is it better to get KConfig finished so that we no longer need this?

fjmolinas added Area: build system Area: Build system Type: new feature The issue requests / The PR implemements a new feature for RIOT labels Jan 26, 2021

fjmolinas requested review from maribu and leandrolanzieri January 26, 2021 08:53

fjmolinas requested review from aabadie and jia200x as code owners January 26, 2021 08:53

fjmolinas 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 Feb 2, 2021

fjmolinas commented Feb 4, 2021

View reviewed changes

leandrolanzieri reviewed Feb 4, 2021

View reviewed changes

maribu mentioned this pull request Feb 9, 2021

build system: fix FEATURES_REQUIRED_ANY #15957

Merged

maribu reviewed Feb 9, 2021

View reviewed changes

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

maribu changed the title ~~makefiles: add FEATURES_OPTIONAL_ALT~~ makefiles: add FEATURES_OPTIONAL_ANY Feb 9, 2021

maribu added the CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR label Feb 9, 2021

fjmolinas force-pushed the pr_features_optional_alt branch from 5b8cf4d to 07b6384 Compare February 9, 2021 13:58

maribu mentioned this pull request Feb 9, 2021

build system: fix FEATURES_REQUIRED_ANY [backport 2021.01] #15967

Closed

maribu added the Reviewed: 3-testing The PR was tested according to the maintainer guidelines label Feb 9, 2021

maribu removed the Reviewed: 3-testing The PR was tested according to the maintainer guidelines label Feb 9, 2021

fjmolinas added 3 commits February 9, 2021 18:44

makefiles/features_check.inc.mk: add _features_one_out_of func

ded345d

build_system_check: add FEATURES_REQUIRED_ANY to check_not_parsing_fe…

66f3570

…atures

fjmolinas force-pushed the pr_features_optional_alt branch from 07b6384 to 66f3570 Compare February 9, 2021 17:44

fjmolinas removed the CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR label Feb 9, 2021

maribu mentioned this pull request Feb 9, 2021

tests/feature_resolution: add build system unit tests #15973

Merged

MrKevinWeiss added this to the Release 2021.07 milestone Jun 22, 2021

MrKevinWeiss removed this from the Release 2021.07 milestone Jul 15, 2021

stale bot added the State: stale State: The issue / PR has no activity for >185 days label Mar 2, 2022

stale bot closed this Apr 16, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

makefiles: add FEATURES_OPTIONAL_ANY #15855

makefiles: add FEATURES_OPTIONAL_ANY #15855

fjmolinas commented Jan 26, 2021

fjmolinas Feb 4, 2021 •

edited

Loading

leandrolanzieri left a comment •

edited

Loading

fjmolinas commented Feb 4, 2021

fjmolinas commented Feb 4, 2021

fjmolinas commented Feb 4, 2021

maribu left a comment

maribu commented Feb 9, 2021

maribu commented Feb 9, 2021

aabadie commented Feb 9, 2021

fjmolinas commented Feb 9, 2021

fjmolinas commented Feb 9, 2021

fjmolinas commented Feb 22, 2021

maribu commented Feb 22, 2021

stale bot commented Mar 2, 2022

maribu commented Apr 16, 2022

makefiles: add FEATURES_OPTIONAL_ANY #15855

makefiles: add FEATURES_OPTIONAL_ANY #15855

Conversation

fjmolinas commented Jan 26, 2021

Contribution description

Testing procedure

Issues/PRs references

fjmolinas Feb 4, 2021 • edited Loading

Choose a reason for hiding this comment

leandrolanzieri left a comment • edited Loading

Choose a reason for hiding this comment

fjmolinas commented Feb 4, 2021

fjmolinas commented Feb 4, 2021

fjmolinas commented Feb 4, 2021

maribu left a comment

Choose a reason for hiding this comment

maribu commented Feb 9, 2021

maribu commented Feb 9, 2021

aabadie commented Feb 9, 2021

fjmolinas commented Feb 9, 2021

fjmolinas commented Feb 9, 2021

fjmolinas commented Feb 22, 2021

maribu commented Feb 22, 2021

stale bot commented Mar 2, 2022

maribu commented Apr 16, 2022

fjmolinas Feb 4, 2021 •

edited

Loading

leandrolanzieri left a comment •

edited

Loading