Skip to content

Conversation

cladmi
Copy link
Contributor

@cladmi cladmi commented Aug 15, 2019

Contribution description

This moves CPU and CPU_MODEL to Makefile.features for a selection of board that currently cause no difference at all in the dependency parsing.

This was accepted in "Tracking: move CPU/CPU_MODEL to Makefile.features" #11477 and more precisely by this PR "Makefile.features: prerequisites for moving CPU/CPU_MODEL to boards/Makefile.features" #11478

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.

Review procedure

This PR modifications are only

  • moving CPU, CPU_MODEL to a different file
  • removing the include $(RIOTCPU)/cpu_name/...
  • Removing the export CPU and export CPU_MODEL as they are globally exported in
    export CPU # The CPU, set by the board's Makefile.features.
    export CPU_MODEL # The specific identifier of the used CPU, used for some CPU implementations to differentiate between different memory layouts. Set by the board's Makefile.features.
  • It removed the comment saying "we set the cpu" as it is quite obvious. If it is not it should be part of a board porting guide.

The testing procedure dumps all variables used for dependency parsing and compare that the new value is consistent.

The dependency to #12004 is only for testing and could be merged without it.

Testing procedure

Review of the new state

In the PR run

./dist/tools/buildsystem_sanity_check/save_all_dependencies_resolution_variables.sh  build/deps/cpu_model
...
# This takes more than 2 hours on my machine

Then run the aggregation script:

generate_aggregated.sh
#! /bin/bash

set -ue

readonly DIRECTORY=$1

set -v
rm -f ${DIRECTORY}/tests/nordic_softdevice/info-global/dependencies_info-boards-supported_reel  # Remove as not generated by normal compilation
rm -rf ${DIRECTORY}/tests/cpu_efm32_features # Remove as issues with BOARD being unconditionally set https://github.com/RIOT-OS/RIOT/pull/12009
find "${DIRECTORY}" -type f -name 'dependencies_info_*' | sort | xargs cat > ${DIRECTORY}/deps_info
find "${DIRECTORY}" -type f -name 'dependencies_info-boards-supported_*' | sort | xargs cat > ${DIRECTORY}/deps_info-boards-supported
./generate_aggregated.sh build/deps/cpu_model

Now the value of CPU and CPU_MODEL info-boards-supported and the normal dependency parsing are the same except for boards that are not migrated by this PR:

Difference in `CPU/CPU_MODEL/BOARD`
diff -W 80 -y --suppress-common-lines build/deps/cpu_model/deps_info-boards-supported build/deps/cpu_model/deps_info | grep -e BOARD -e 'CPU ' -e CPU_MODEL | awk '!x[$0]++'
CPU =                                 | CPU = nrf52
CPU =                                 | CPU = nrf51
CPU =                                 | CPU = stm32f1
CPU_MODEL =                           | CPU_MODEL = stm32f103c8
CPU =                                 | CPU = cc2538
CPU_MODEL =                           | CPU_MODEL = cc2538nf53
CPU =                                 | CPU = lm4f120
CPU_MODEL =                           | CPU_MODEL = LM4F120H5QR
CPU =                                 | CPU = esp32
CPU_MODEL =                           | CPU_MODEL = esp32
CPU =                                 | CPU = esp8266
CPU_MODEL =                           | CPU_MODEL = esp8266
CPU =                                 | CPU = fe310
CPU_MODEL =                           | CPU_MODEL = fe310_g000
CPU_MODEL =                           | CPU_MODEL = fe310_g002
CPU_MODEL =                           | CPU_MODEL = lpc1768
CPU =                                 | CPU = kinetis
CPU_MODEL =                           | CPU_MODEL = mk60dn512vll10
CPU_MODEL =                           | CPU_MODEL = nrf51x22xxaa
CPU =                                 | CPU = mips_pic32mx
CPU_MODEL =                           | CPU_MODEL = p32mx470f512h
CPU =                                 | CPU = mips_pic32mz
CPU_MODEL =                           | CPU_MODEL = p32mz2048efg100
CPU =                                 | CPU = efm32
CPU_MODEL =                           | CPU_MODEL = efr32mg12p332f1024gl125

Note that currently there are still differences in both parsing with

DISABLE_MODULE DEFAULT_MODULE USEMODULE FEATURES_REQUIRED FEATURES_USED FEATURES_OPTIONAL FEATURES_CONFLICT FEATURES_MISSING CPU_FAM

Comparison with the previous state

On the commit: "squash! make: add targets to debug dependencies variables " run the same generation:

./dist/tools/buildsystem_sanity_check/save_all_dependencies_resolution_variables.sh  build/deps/ref
# This takes more than 2 hours on my machine
./generate_aggregated.sh build/deps/ref

It did not change the variables used when normal dependency parsing:

diff build/deps/ref/deps_info build/deps/cpu_model/deps_info
# no difference

And in general nothing changed except CPU and CPU_MODEL values being now declared in info-boards-supported:

diff -r -W 80 -y  --suppress-common-lines build/deps/ref/ build/deps/cpu_model/ -I 'CPU ' -I 'CPU_MODEL ' --exclude='deps_info*'

I need to exclude the aggregated file as diff reports a few lines with CPU / CPU_MODEL even if I asked to ignore them.

For reference without excluding the aggregated files
$ diff -r -W 80 -y  --suppress-common-lines build/deps/ref/ build/deps/cpu_model/ -I 'CPU ' -I 'CPU_MODEL '
diff -r -W 80 -y --suppress-common-lines -I 'CPU ' -I 'CPU_MODEL ' build/deps/ref/deps_info-boards-supported build/deps/cpu_model/deps_info-boards-supported
BOARD = opencm904                     | BOARD = opencm904
CPU =                                 | CPU = stm32f1
CPU_MODEL =                           | CPU_MODEL = stm32f103cb
BOARD = nz32-sc151                    | BOARD = nz32-sc151
CPU =                                 | CPU = stm32l1
CPU_MODEL =                           | CPU_MODEL = stm32l151rc
BOARD = pba-d-01-kw2x                 | BOARD = pba-d-01-kw2x
CPU =                                 | CPU = kinetis
CPU_MODEL =                           | CPU_MODEL = mkw21d256vha5
BOARD = slwstk6220a                   | BOARD = slwstk6220a
CPU =                                 | CPU = ezr32wg
CPU_MODEL =                           | CPU_MODEL = ezr32wg330f256r60
BOARD = stm32f0discovery              | BOARD = stm32f0discovery
CPU =                                 | CPU = stm32f0
CPU_MODEL =                           | CPU_MODEL = stm32f051r8
BOARD = saml21-xpro                   | BOARD = saml21-xpro
CPU =                                 | CPU = saml21
CPU_MODEL =                           | CPU_MODEL = saml21j18a
BOARD = remote-reva                   | BOARD = remote-reva
CPU =                                 | CPU = cc2538
CPU_MODEL =                           | CPU_MODEL = cc2538sf53

Issues/PRs references

@cladmi cladmi changed the title Pr/cpu cpu model/migration/straightforward ones boards: move CPU/CPU_MODEL definition to Makefile.features Aug 15, 2019
@cladmi
Copy link
Contributor Author

cladmi commented Aug 15, 2019

I ran the validation script on a previous version of the branch as I needed to rebase it at the end to master and tweak some things.
I will re-run the testing procedure this evening and post the confirmation it is still valid tomorrow (or get back at fixing).

@jcarrano
Copy link
Contributor

jcarrano commented Aug 16, 2019

I'm running the script and getting tons of

/bin/sh: + : syntax error: operand expected (error token is "+ ")

in standard output.

Edit: it only seems to happen when evaluating bootloaders/riotboot.

@cladmi
Copy link
Contributor Author

cladmi commented Aug 16, 2019

I'm running the script and getting tons of

/bin/sh: + : syntax error: operand expected (error token is "+ ")

in standard output.

This is an issue with the riotboot handling in sys/riotboot/Makefile.include and that CFLAGS are evaluated even when not needed.
For a board that does not have riotboot, calling make in the directory gives these errors.

And for comparing the dependency resolution, I call it on all boards (that are not blacklisted).

BOARD=native make --no-print-directory -C bootloaders/riotboot/ | head
BOARD=native make --no-print-directory -C bootloaders/riotboot/ | head
/bin/sh: 1: arithmetic expression: expecting primary: " + "
There are unsatisfied feature requirements: riotboot


EXPECT ERRORS!


/bin/sh: 1: arithmetic expression: expecting primary: " + "
/bin/sh: 1: arithmetic expression: expecting primary: " + "
/bin/sh: 1: arithmetic expression: expecting primary: " + "
/bin/sh: 1: arithmetic expression: expecting primary: " + "
/bin/sh: 1: arithmetic expression: expecting primary: " + "
"make" -C /home/harter/work/git/RIOT/boards/native
"make" -C /home/harter/work/git/RIOT/boards/native/drivers
"make" -C /home/harter/work/git/RIOT/core
"make" -C /home/harter/work/git/RIOT/cpu/native
"make" -C /home/harter/work/git/RIOT/cpu/native/periph
"make" -C /home/harter/work/git/RIOT/cpu/native/vfs
"make" -C /home/harter/work/git/RIOT/drivers
"make" -C /home/harter/work/git/RIOT/drivers/periph_common
"make" -C /home/harter/work/git/RIOT/sys
"make" -C /home/harter/work/git/RIOT/sys/checksum

@cladmi
Copy link
Contributor Author

cladmi commented Aug 16, 2019

I compared again the output with the last versions of this PR and I get the same output as the testing description.

Copy link
Contributor

@jcarrano jcarrano left a comment

Choose a reason for hiding this comment

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

Ok.

@jcarrano jcarrano added Type: enhancement The issue suggests enhanceable parts / The PR enhances parts of the codebase / documentation Area: build system Area: Build system CI: needs squashing Commits in this PR need to be squashed; If set, CI systems will mark this PR as unmergable labels Aug 20, 2019
@jcarrano
Copy link
Contributor

Squash please.

@cladmi
Copy link
Contributor Author

cladmi commented Aug 20, 2019

I will remove the test script as it is part of #12004

cladmi added 2 commits August 20, 2019 16:11
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.
@cladmi cladmi force-pushed the pr/cpu_cpu_model/migration/straightforward_ones branch from e391308 to 636285e Compare August 20, 2019 14:12
@jcarrano jcarrano added CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR and removed CI: needs squashing Commits in this PR need to be squashed; If set, CI systems will mark this PR as unmergable labels Aug 21, 2019
@jcarrano jcarrano merged commit e2b2b8e into RIOT-OS:master Aug 21, 2019
@cladmi
Copy link
Contributor Author

cladmi commented Aug 21, 2019

Thank you for the review. Now I need to tackle the complex ones.

@cladmi cladmi deleted the pr/cpu_cpu_model/migration/straightforward_ones branch August 21, 2019 16:54
@kb2ma kb2ma added this to the Release 2019.10 milestone Sep 16, 2019
TorbenPetersen added a commit to ibr-cm/RIOT that referenced this pull request Oct 8, 2019
TorbenPetersen added a commit to ibr-cm/RIOT that referenced this pull request Nov 1, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
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 Type: enhancement The issue suggests enhanceable parts / The PR enhances parts of the codebase / documentation
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants