Makefile.features: add declarative FEATURES_ variables definition by cladmi · Pull Request #11492 · RIOT-OS/RIOT

cladmi · 2019-05-06T16:59:48Z

Contribution description

Use Makefile.features to define all the calculated FEATURES_ variables.
This gives a name on the resolved parsing of the BSP FEATURES_ variables.

Then use them in Makefile.include/makefiles/info.inc.mk/makefiles/info-global.inc.mk`.

Reviewing procedure

The review is mainly on Makefile.features the other changes being the consequence of the changes but they can explain what changed.

The goal is does the name makes sense and matches with what is evaluated.

The fact that the migration is correct can come in a second time.

Testing procedure

All the supported applications should still be the same.

I added make info-applications-supported-boards that can be run in the base directory to compare a commit with another for listing all the boards returned by info-boards-supported. (I will split this one).

The output of this one should be the same with the end commit and the commit introducing it.

Info-build output

I will add some outputs for the info-build to verify the new variables in some defined cases.

Difference between master and this pull request

diff -u  info_build_examples_default_samr21_xpro_*
--- info_build_examples_default_samr21_xpro_master      2019-05-27 17:47:39.169868988 +0200
+++ info_build_examples_default_samr21_xpro_pr  2019-05-27 17:47:18.273697122 +0200
@@ -21,19 +21,23 @@
 HEXFILE: /home/harter/work/git/RIOT/examples/default/bin/samr21-xpro/default.hex
 FLASHFILE: /home/harter/work/git/RIOT/examples/default/bin/samr21-xpro/default.bin
 
-FEATURES_REQUIRED (excl. optional features):
+FEATURES_USED:
+         periph_cpuid periph_gpio periph_gpio_irq periph_pm periph_rtc periph_spi periph_timer periph_uart
+FEATURES_REQUIRED:
          periph_gpio periph_gpio_irq periph_spi periph_timer periph_uart
-FEATURES_OPTIONAL (strictly "nice to have"):
+FEATURES_OPTIONAL_ONLY (optional that are not required, strictly "nice to have"):
          periph_cpuid periph_hwrng periph_pm periph_rtc
+FEATURES_OPTIONAL_MISSING (missing optional features):
+         periph_hwrng
 FEATURES_PROVIDED (by the board or USEMODULE'd drivers):
          cpp cpu_check_address periph_adc periph_cpuid periph_flashpage periph_flashpage_raw periph_flashpage_rwee periph_gpio periph_gpio_irq periph_i2c periph_pm periph_pwm periph_rtc periph_rtt periph_spi periph_timer periph_uart periph_usbdev puf_sram riotboot
-FEATURES_MISSING (incl. optional features):
-         -none-
-FEATURES_MISSING (only non-optional features):
+FEATURES_MISSING (only non optional features):
          -none-
 
 FEATURES_CONFLICT:     
 FEATURES_CONFLICT_MSG: 
+FEATURES_CONFLICTING:
+         -none-
 
 INCLUDES: 
        -isystem

BOARD=samr21-xpro make info-build | grep -C 1 'FEATURES' # master

FEATURES_REQUIRED (excl. optional features):
         periph_gpio periph_gpio_irq periph_spi periph_timer periph_uart
FEATURES_OPTIONAL (strictly "nice to have"):
         periph_cpuid periph_hwrng periph_pm periph_rtc
FEATURES_PROVIDED (by the board or USEMODULE'd drivers):
         cpp cpu_check_address periph_adc periph_cpuid periph_flashpage periph_flashpage_raw periph_flashpage_rwee periph_gpio periph_gpio_irq periph_i2c periph_pm periph_pwm periph_rtc periph_rtt periph_spi periph_timer periph_uart periph_usbdev puf_sram riotboot
FEATURES_MISSING (incl. optional features):
         -none-
FEATURES_MISSING (only non-optional features):
         -none-

FEATURES_CONFLICT:     
FEATURES_CONFLICT_MSG:

BOARD=samr21-xpro make info-build | grep -C 1 'FEATURES' # PR

FEATURES_USED:
         periph_cpuid periph_gpio periph_gpio_irq periph_pm periph_rtc periph_spi periph_timer periph_uart
FEATURES_REQUIRED:
         periph_gpio periph_gpio_irq periph_spi periph_timer periph_uart
FEATURES_OPTIONAL_ONLY (optional that are not required, strictly "nice to have"):
         periph_cpuid periph_hwrng periph_pm periph_rtc
FEATURES_OPTIONAL_MISSING (missing optional features):
         periph_hwrng
FEATURES_PROVIDED (by the board or USEMODULE'd drivers):
         cpp cpu_check_address periph_adc periph_cpuid periph_flashpage periph_flashpage_raw periph_flashpage_rwee periph_gpio periph_gpio_irq periph_i2c periph_pm periph_pwm periph_rtc periph_rtt periph_spi periph_timer periph_uart periph_usbdev puf_sram riotboot
FEATURES_MISSING (only non optional features):
         -none-

FEATURES_CONFLICT:     
FEATURES_CONFLICT_MSG: 
FEATURES_CONFLICTING:
         -none-

Output with conflicting features:

BOARD=stm32f4discovery make -C tests/warn_conflict/ info-build | grep -C 1 'FEATURES' 

FEATURES_USED:
         periph_dac periph_gpio periph_pm periph_spi periph_uart
FEATURES_REQUIRED:
         periph_dac periph_spi periph_uart
FEATURES_OPTIONAL_ONLY (optional that are not required, strictly "nice to have"):
         periph_gpio periph_pm
FEATURES_OPTIONAL_MISSING (missing optional features):
         -none-
FEATURES_PROVIDED (by the board or USEMODULE'd drivers):
         arduino cpp cpu_check_address periph_adc periph_cpuid periph_dac periph_gpio periph_gpio_irq periph_hwrng periph_i2c periph_pm periph_pwm periph_rtc periph_spi periph_timer periph_uart periph_uart_modecfg puf_sram
FEATURES_MISSING (only non optional features):
         -none-

FEATURES_CONFLICT:     periph_spi:periph_dac
FEATURES_CONFLICT_MSG: "On stm32f4discovery boards there are the same pins for the DAC and/or SPI_0."
FEATURES_CONFLICTING:
         periph_dac periph_spi

Issues/PRs references

Side work of #9913 that added this Makefile.features to group the features handling.

Could help with #9081 to have the definition in only one place.

info-applications-supported-boards makefiles/app_dirs.inc.mk: target to list supported applications/boards #11590

fjmolinas · 2019-05-19T19:40:20Z

@cladmi Is this PR waiting for another PR because of your TODO list or because is actually depends on one?

cladmi · 2019-05-20T10:57:41Z

My bad, it does not depend on anything. Must have misclicked…

cladmi · 2019-05-20T10:58:29Z

Oh yes it depends on splitting the first two commits, now that I remember it. So was indeed right, I just forgot to do it.

cladmi · 2019-05-20T11:03:46Z

It could also be merged without these two commits at all. So lets say they should either be removed before merging, or split in another pull request but not block it.

fjmolinas

@cladmi Can you reword your last commit details to :

Define a variable for FEATURES_USED that conflict and use it in
Makefie.include.

It could also be merged without these two commits at all. So lets say they should either be removed before merging, or split in another pull request but not block it.

About the first two commits, do you see more use for the recipe besides testing this PR? If there is one lets keep it, but if not remove them from the PR.

Otherwise I tested and got the same supported-applications output.

I like the general re-ordering introduced by this PR, I do have a couple of comments regarding naming. See below and FEATURES_CONFLICTING isn't working for me.

fjmolinas · 2019-05-20T13:29:43Z

Makefile.features

+FEATURES_OPTIONAL_ONLY = $(sort $(filter-out $(FEATURES_REQUIRED),$(FEATURES_OPTIONAL)))
+FEATURES_OPTIONAL_USED = $(sort $(filter $(FEATURES_PROVIDED),$(FEATURES_OPTIONAL_ONLY)))
+# Optional features that will not be used because they are unavailable
+FEATURES_OPTIONAL_UNUSED = $(sort $(filter-out $(FEATURES_PROVIDED),$(FEATURES_OPTIONAL_ONLY)))


I think FEATURES_OPTIONAL_MISSING would be clearer in this case, FEATURES_OPTIONAL_UNUSED doesn't let you know why they are unused. What do you think?

Thank you for going in the semantic details, that is what I want to be challenged :)

I wanted to separate missing which for me sounds like an error from they are not available but optional anyway/or not used. And even changed it from the previous FEATURES_MISSING message that was in info-build.

https://github.com/RIOT-OS/RIOT/pull/11492/files#diff-cb9c0ad59ebaddd68b4a35e09c6d47f5R56

With a new general FEATURES_BLACKLIST #9081 variable or specific FEATURES_OPTIONAL_DISABLE #10047 (comment) variable, an optional feature could be provided but not included.

But maybe I would indeed need a specific variable for this case or the OPTIONAL_MISSING.

About the first two commits, do you see more use for the recipe besides testing this PR? If there is one lets keep it, but if not remove them from the PR.

Testing other refactoring Pull requests that touch the makefiles/info-global.inc.mkand can changeinfo-boards-supported`.
I would prefer to have them in a separate PR anyway.

I wanted to separate missing which for me sounds like an error from they are not available but optional anyway/or not used. And even changed it from the previous FEATURES_MISSING message that was in info-build.

I agree with this. But FEATURES_OPTIONAL_UNUSED makes me think of features that are optional but that I (or the build system) have decided not to use. I had to read up the comment and code to understand what it meant since it went against my intuition of what the variable name made me think.

FEATURES_OPTIONAL_DISABLE #10047 (comment) variable, an optional feature could be provided but not included.

In that case to me its more intuitive to have:

FEATURES_OPTIONAL_UNUSED = $(sort $(filter $(FEATURES_OPTIONAL_DISABLE),$(FEATURES_OPTIONAL_ONLY)))

and

FEATURES_OPTIONAL_MISSING = $(sort $(filter-out $(FEATURES_PROVIDED),$(FEATURES_OPTIONAL_ONLY)))

Anyway this is just my interpretation, but I think is exactly the kind of questioning you wanted right?

So you would prefer two variables for this.
Currently there would be no FEATURES_OPTIONAL_UNUSED as none of the two DISABLE or BLACKLIST are there.

I will change the UNUSED to go back to the MISSING behavior and see for a nice more describing comment.

Makefile.features

fjmolinas · 2019-05-21T07:04:47Z

Testing other refactoring Pull requests that touch the makefiles/info-global.inc.mkand can changeinfo-boards-supported`.
I would prefer to have them in a separate PR anyway.

@cladmi Ok, can you open a separate PR for this then? Or are you thinking of optimizing it a bit first? I'm merging what you did now, I'm sure optimization will come later :). (I can review that one and get it moving quickly)

fjmolinas · 2019-05-23T09:31:27Z

@cladmi ping!

cladmi · 2019-05-24T09:49:26Z

@fjmolinas I am not 100% currently available, so I cannot focus on everything. But no worry, I do not miss it :)

fjmolinas · 2019-05-28T14:36:48Z

@cladmi This is not longer waiting for another PR right?

cladmi · 2019-05-28T15:38:50Z

Indeed, updated the description and the labels.

fjmolinas · 2019-05-28T16:32:22Z

I'm satisfied with the naming of the variables. As I stated before everything was working as expected. I will still wan't to repeat the tests afters you squash your commits! (and if you are also happy with the variables semantics)

Update the FEATURES_OPTIONAL meaning to be more in line since FEATURES_USED is defined. Handle FEATURES_OPTIONAL as a configuration from the BSP/build that should not be changed anymore after. `FEATURES_OPTIONAL` are by definition optional so are not supposed to cause a build to fail. Only the 'REQUIRED' ones that are not 'PROVIDED' are 'MISSING'. * Do not change FEATURES_OPTIONAL to remove REQUIRED features * Prepare for having a different variable for the previous value * Update dependency resolution/info-build as FEATURES_OPTIONAL cannot be missing

Put the definition of `FEATURES_MISSING` in common and use the variable instead of duplicating code.

Put the definition of `FEATURES_USED` in common and use the variable instead of duplicating code. This required defining 'FEATURES_OPTIONAL_ONLY|USED' to not overwrite the value of 'FEATURES_OPTIONAL' as was done before. Also add 'FEATURES_OPTIONAL_MISSING' to list optional features that were not included as not provided. This removes the need to print FEATURES_MISSING with the optional features too.

Define a variable for used features that conflict and use it in `Makefie.include`. It was not used by `info-global.inc.mk` and is still currently not.

cladmi · 2019-05-28T17:23:28Z

Squashed and I am happy with the current names.

cladmi · 2019-05-28T17:51:57Z

At least I get the same output for make info-applications-supported-boards on this branch and the reference.

cladmi · 2019-06-14T14:01:39Z

@fjmolinas ping if you have time :)

fjmolinas

Tested again, everything is still working as expected and I agree with the semantic change. ACK!

fjmolinas · 2019-06-14T16:03:09Z

Sorry for the delay, GO!

cladmi · 2019-06-14T16:48:23Z

@fjmolinas no worry we all have a lot of things in parallel.

cladmi · 2019-06-14T16:48:30Z

Thank you for the review

cladmi added the State: waiting for other PR State: The PR requires another PR to be merged first label May 6, 2019

cladmi removed the State: waiting for other PR State: The PR requires another PR to be merged first label May 20, 2019

fjmolinas requested changes May 20, 2019

View reviewed changes

cladmi mentioned this pull request May 27, 2019

makefiles/app_dirs.inc.mk: target to list supported applications/boards #11590

Merged

cladmi force-pushed the pr/features/declarative branch from 6c1d9dc to b72930c Compare May 27, 2019 15:38

cladmi added State: waiting for other PR State: The PR requires another PR to be merged first Area: build system Area: Build system and removed CI: needs squashing Commits in this PR need to be squashed; If set, CI systems will mark this PR as unmergable labels May 27, 2019

cladmi force-pushed the pr/features/declarative branch from 4e3683c to 4afa19b Compare May 28, 2019 07:23

cladmi removed the State: waiting for other PR State: The PR requires another PR to be merged first label May 28, 2019

cladmi added 4 commits May 28, 2019 19:21

Makefile.features: declare FEATURES_MISSING and make use of it

e1c60ea

Put the definition of `FEATURES_MISSING` in common and use the variable instead of duplicating code.

Makefile.features: declare FEATURES_CONFLICTING and make use of it

6d7a70b

Define a variable for used features that conflict and use it in `Makefie.include`. It was not used by `info-global.inc.mk` and is still currently not.

cladmi force-pushed the pr/features/declarative branch from 4afa19b to 6d7a70b Compare May 28, 2019 17:22

fjmolinas added the CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR label May 28, 2019

fjmolinas approved these changes Jun 14, 2019

View reviewed changes

fjmolinas merged commit ff317f2 into RIOT-OS:master Jun 14, 2019

cladmi deleted the pr/features/declarative branch June 14, 2019 16:53

fjmolinas mentioned this pull request Oct 14, 2019

make: add architecture features and feature blacklisting #9081

Merged

Conversation

cladmi commented May 6, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Contribution description

Reviewing procedure

Testing procedure

Info-build output

Issues/PRs references

Uh oh!

fjmolinas commented May 19, 2019

Uh oh!

cladmi commented May 20, 2019

Uh oh!

cladmi commented May 20, 2019

Uh oh!

cladmi commented May 20, 2019

Uh oh!

fjmolinas left a comment • 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!

fjmolinas May 20, 2019

Choose a reason for hiding this comment

Uh oh!

cladmi May 20, 2019

Choose a reason for hiding this comment

Uh oh!

fjmolinas May 21, 2019

Choose a reason for hiding this comment

Uh oh!

cladmi May 24, 2019

Choose a reason for hiding this comment

Uh oh!

Uh oh!

fjmolinas commented May 21, 2019

Uh oh!

fjmolinas commented May 23, 2019

Uh oh!

cladmi commented May 24, 2019

Uh oh!

fjmolinas commented May 28, 2019

Uh oh!

cladmi commented May 28, 2019

Uh oh!

fjmolinas commented May 28, 2019

Uh oh!

cladmi commented May 28, 2019

Uh oh!

cladmi commented May 28, 2019

Uh oh!

cladmi commented Jun 14, 2019

Uh oh!

fjmolinas left a comment

Choose a reason for hiding this comment

Uh oh!

fjmolinas commented Jun 14, 2019

Uh oh!

cladmi commented Jun 14, 2019

Uh oh!

cladmi commented Jun 14, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

cladmi commented May 6, 2019 •

edited

Loading

fjmolinas left a comment •

edited

Loading