Skip to content

build system: fix FEATURES_REQUIRED_ANY#15957

Merged
maribu merged 1 commit intoRIOT-OS:masterfrom
maribu:features_required_any_order_fix
Feb 9, 2021
Merged

build system: fix FEATURES_REQUIRED_ANY#15957
maribu merged 1 commit intoRIOT-OS:masterfrom
maribu:features_required_any_order_fix

Conversation

@maribu
Copy link
Member

@maribu maribu commented Feb 9, 2021

Contribution description

Previously, FEATURES_REQUIRED_ANY didn't honor the order of the alternatives
provided, if none of the features were already in used and multiple options
are provided. This fixes this.

Testing procedure

See #15855 (comment)

Issues/PRs references

Found in #15855

@maribu
build system: fix FEATURES_REQUIRED_ANY
f41dcc9 
Previously, FEATURES_REQUIRED_ANY didn't honor the order of the alternatives
provided, if none of the features were already in used and multiple options
are provided. This fixes this.
@maribu maribu added Type: bug The issue reports a bug / The PR fixes a bug (including spelling errors) Area: build system Area: Build system Impact: minor The PR is small in size and might only require a quick look of a knowledgeable reviewer labels Feb 9, 2021
@maribu maribu requested review from aabadie and fjmolinas February 9, 2021 09:48
@benpicco 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 9, 2021
@fjmolinas
Copy link
Contributor

fjmolinas commented Feb 9, 2021

This is already fixed in #15855 , why not review and get that one in instead?

@maribu
Copy link
Member Author

maribu commented Feb 9, 2021

If #15855 get's reverted for some reason, also the bugfix get's reverted. So fixing the bug first makes sense IMO. Also, if @jia200x would consider a point release, this would be relatively trivial to backport.

@fjmolinas
Copy link
Contributor

fjmolinas commented Feb 9, 2021

If #15855 get's reverted for some reason, also the bugfix get's reverted. So fixing the bug first makes sense IMO. Also, if @jia200x would consider a point release, this would be relatively trivial to backport.

Ok makes sense.

@fjmolinas
Copy link
Contributor

fjmolinas commented Feb 9, 2021

-master

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
  • PR
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_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
fjmolinas approved these changes Feb 9, 2021
View reviewed changes
Copy link
Contributor

@fjmolinas fjmolinas left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ACK

@jia200x
Copy link
Member

jia200x commented Feb 9, 2021

Also, if @jia200x would consider a point release, this would be relatively trivial to backport.

Sure! Considering #15961 is also there

@jia200x jia200x added the Process: needs backport Integration Process: The PR is required to be backported to a release or feature branch label Feb 9, 2021
@jia200x
Copy link
Member

jia200x commented Feb 9, 2021

I added the backport label :)

@maribu maribu merged commit 633dc78 into RIOT-OS:master Feb 9, 2021
@maribu maribu deleted the features_required_any_order_fix branch February 9, 2021 16:35
@maribu maribu mentioned this pull request Feb 9, 2021
Closed
@maribu
Copy link
Member Author

maribu commented Feb 9, 2021

Backport provided in #15967

@maribu
Copy link
Member Author

maribu commented Feb 9, 2021

Thanks for the review

fjmolinas
fjmolinas reviewed Feb 9, 2021
View reviewed changes
makefiles/features_check.inc.mk
Comment on lines +35 to +37
$(filter $(FEATURES_USED_SO_FAR) $(FEATURES_USABLE),\
$(subst |, ,$(item)))\
$(item)))
Copy link
Contributor

@fjmolinas fjmolinas Feb 9, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more. 

FEATURES_REQUIRED_ONE_OUT_OF := $(foreach item,\
                                  $(FEATURES_REQUIRED_ANY),\
                                  $(word 1,\
                                    $(filter $(FEATURES_USED_SO_FAR),\
                                      $(subst |, ,$(item)))\
                                    $(filter $(FEATURES_USABLE),\
                                     $(subst |, ,$(item)))\
                                    $(item)))

This seems to work properly ...

Copy link
Member Author

@maribu maribu Feb 9, 2021 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Interestingly, this introduced this regression :-/

Edit: This PR introduced it. So I swapped one bug for another.

This was referenced Feb 9, 2021
Merged
Merged
@aabadie aabadie mentioned this pull request Feb 12, 2021
Merged
@kaspar030 kaspar030 added this to the Release 2021.01 milestone Apr 23, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@fjmolinas fjmolinas fjmolinas approved these changes

@aabadie aabadie Awaiting requested review from aabadie

Assignees

No one assigned

Labels

Area: build system Area: Build system CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR Impact: minor The PR is small in size and might only require a quick look of a knowledgeable reviewer Process: needs backport Integration Process: The PR is required to be backported to a release or feature branch Type: bug The issue reports a bug / The PR fixes a bug (including spelling errors)

Projects

None yet

Milestone

Release 2021.01

Development

Successfully merging this pull request may close these issues.

5 participants

@maribu @fjmolinas @jia200x @benpicco @kaspar030