-
Notifications
You must be signed in to change notification settings - Fork 2k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
boards with side-effect: move CPU/CPU_MODEL definition to Makefile.features #12092
boards with side-effect: move CPU/CPU_MODEL definition to Makefile.features #12092
Conversation
Your computer seems to be much better than mine, running for 1 hour now.. hahaha |
@fjmolinas :D did you set the |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Changes look good and commit changes make sense.
I get the expected no diff when running on the top commit:
I can also see most differences where fixed when doing I also get the same results when normal parsing. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I tested the changes following the test procedure and got the expected results. Changes lookg good and are part of #11477. ACK, please remove unrelated commits and squash.
* CPU files should already have 'CPU' defined by the board. * Do not conditionally define CPU as it is not needed. This is part of cleanup prior to moving the CPU/CPU_MODEL to Makefile.features.
cpu/$(CPU)/Makefile.features and cpu/$(CPU)/Makefile.dep are automatically included Part of moving CPU/CPU_MODEL definition to Makefile.features to have it available before Makefile.include.
cpu/$(CPU)/Makefile.features and cpu/$(CPU)/Makefile.dep are automatically included Part of moving CPU/CPU_MODEL definition to Makefile.features to have it available before Makefile.include.
cpu/$(CPU)/Makefile.features and cpu/$(CPU)/Makefile.dep are automatically included Part of moving CPU/CPU_MODEL definition to Makefile.features to have it available before Makefile.include.
cpu/$(CPU)/Makefile.features and cpu/$(CPU)/Makefile.dep are automatically included Part of moving CPU/CPU_MODEL definition to Makefile.features to have it available before Makefile.include.
cpu/$(CPU)/Makefile.features and cpu/$(CPU)/Makefile.dep are automatically included Add the missing space when defining 'CPU_MODEL'. Part of moving CPU/CPU_MODEL definition to Makefile.features to have it available before Makefile.include.
cpu/$(CPU)/Makefile.features and cpu/$(CPU)/Makefile.dep are automatically included CPU_MODEL is currently kept uppercase. Making it lowercase is a future task. Part of moving CPU/CPU_MODEL definition to Makefile.features to have it available before Makefile.include.
cpu/$(CPU)/Makefile.features and cpu/$(CPU)/Makefile.dep are automatically included Part of moving CPU/CPU_MODEL definition to Makefile.features to have it available before Makefile.include.
cpu/$(CPU)/Makefile.features and cpu/$(CPU)/Makefile.dep are automatically included The multiple boards handling has been changed to use 'CPU_MODEL ?='. Part of moving CPU/CPU_MODEL definition to Makefile.features to have it available before Makefile.include.
f3675b6
to
cdca4d8
Compare
Thank you for testing. I removed the commits from #12004 |
It took me much longer than I want to admit to get the testing procedure right hahahaha |
I can imagine. Running tests with 2 separate versions is also always hard to explain concisely :/ Any feedback on what I should put more clearly? |
Thank you for the review. |
Contribution description
This moves CPU and CPU_MODEL to Makefile.features for a selection of board that only cause harmless side-effects in the dependency parsing like
?=
replaced by=
and spacingReview procedure
I would advise to look at each commit individually and the global output comparison.
This PR modifications are, like the first PR
CPU
,CPU_MODEL
to a different fileinclude $(RIOTCPU)/cpu_name
export CPU
andexport CPU_MODEL
as they are globally exported inRIOT/makefiles/vars.inc.mk
Lines 11 to 12 in 3e75383
And also the "side-effects":
include
order when moved to the global include location which change order in the variables?=
in espI can split in several PRs if wanted, it was just easier for me to run the validation once.
The dependency to #12004 is only for testing and could be merged without it.
Testing procedure
All the modified boards for this change are the following
So to run the comparison, it is easier to first limit the testing to these boards.
In the PR run
On the second commit: "squash! make: add targets to debug dependencies variables " run the same generation: (it is shown at last on github, but is not locally)
And generate the aggregated result
generate_aggregated.sh
Difference between normal and "global" dependency parsing
Both dependency parsing now have the same values for `FEATURES_PROVIDED`, `CPU`, `CPU_MODEL` for these boards
You can see that most differences have been fixed with this PR by checking the previous state with all
CPU
andCPU_MODEL
that were not defined ininfo-boards-supported
.There are still currently differences in some variables, so they are excluded, but this is out of this PR scope and was the case before too.
The difference between before and now for normal parsing
As the normal parsing already had
CPU
andCPU_MODEL
, there are only difference inFEATURES_REQUIRED
andFEATURES_PROVIDED
but the actual value is unchanged as the value of the sorted ones is the same.Looking in the difference in `examples/default`
And when excluding
^FEATURES_REQUIRED
and^FEATURES_PROVIDED
, so keeping thesorted
ones we have the same values after migration.Issues/PRs references