Makefile.include: fix board/cpu inconsistent include order by cladmi · Pull Request #8717 · RIOT-OS/RIOT

cladmi · 2018-03-01T15:48:26Z

Contribution description

Re-organize headers include directories order to be consistent to include first all things related to board then all things related to CPU.

I did a check to verify there were no header names conflicts between boards and cpu so changing the order has no impact. Output and verification script below.

Details

Include order for board and cpu was

cpu include
board include
board common includes
cpu common includes

Its now changed to:

board include
board common includes
cpu include
cpu common includes

Further steps

I plan to move the two lines with "INCLUDE +=" and "include ../Makefile.include" into dedicated files in boards and cpu to allow further cleanup, but it requires the include order to be solved before.

Changes INCLUDES output

Using #8714 with examples/gnrc_networking for iotlab-m3:

Without the PR, notice the cpu/stm32f1 before the boards include:

INCLUDES: 
        -isystem  
        /usr/arm-none-eabi/include/newlib-nano  
        -I/home/harter/work/git/RIOT/core/include  
        -I/home/harter/work/git/RIOT/drivers/include  
        -I/home/harter/work/git/RIOT/sys/include  
        -I/home/harter/work/git/RIOT/cpu/stm32f1/include  
        -I/home/harter/work/git/RIOT/boards/iotlab-m3/include  
        -I/home/harter/work/git/RIOT/boards/common/iotlab/include  
        -I/home/harter/work/git/RIOT/boards/iotlab-m3/include  # This is currently re-added in the board/Makefile.include
        -I/home/harter/work/git/RIOT/cpu/stm32_common/include  
        -I/home/harter/work/git/RIOT/cpu/cortexm_common/include  
        -I/home/harter/work/git/RIOT/cpu/cortexm_common/include/vendor  
        -I/home/harter/work/git/RIOT/sys/libc/include  
        -I/home/harter/work/git/RIOT/drivers/at86rf2xx/include

With the PR

INCLUDES:                                                                                                                                                     
        -isystem  
        /usr/arm-none-eabi/include/newlib-nano  
        -I/home/harter/work/git/RIOT/core/include  
        -I/home/harter/work/git/RIOT/drivers/include  
        -I/home/harter/work/git/RIOT/sys/include  
        -I/home/harter/work/git/RIOT/boards/iotlab-m3/include  
        -I/home/harter/work/git/RIOT/boards/common/iotlab/include  
        -I/home/harter/work/git/RIOT/boards/iotlab-m3/include  # This is currently re-added in the board/Makefile.include
        -I/home/harter/work/git/RIOT/cpu/stm32f1/include  
        -I/home/harter/work/git/RIOT/cpu/stm32_common/include  
        -I/home/harter/work/git/RIOT/cpu/cortexm_common/include  
        -I/home/harter/work/git/RIOT/cpu/cortexm_common/include/vendor  
        -I/home/harter/work/git/RIOT/sys/libc/include  
        -I/home/harter/work/git/RIOT/drivers/at86rf2xx/include

Verifications

There are no common headers names between boards and cpus.
Except native that has a 'periph_conf.h' in cpu instead of being in board.

Checking boards and cpu uniq headers names

wc -l ${BOARDS_HEADERS} 50
wc -l ${CPU_HEADERS} 630

Common headers between boards and cpu
      2 periph_conf.h

Culprit for common header
cpu/native/include/periph_conf.h

I used the following script to test this.

#! /bin/bash

find_headers_in_dir() {
    local directory="$1"
    find "${directory}" -name '*.h' -exec basename {} \;
}

BOARDS_HEADERS=$(find_headers_in_dir boards | sort -u)
CPU_HEADERS=$(find_headers_in_dir cpu | sort -u)

echo Checking boards and cpu uniq headers names
echo ""

# echo "${BOARDS_HEADERS}"
# echo "${CPU_HEADERS}"

echo -n 'wc -l ${BOARDS_HEADERS} '
echo "${BOARDS_HEADERS}" | wc -l
echo -n 'wc -l ${CPU_HEADERS} '
echo "${CPU_HEADERS}" | wc -l
echo

echo "Common headers between boards and cpu"
echo "${BOARDS_HEADERS}" "${CPU_HEADERS}" | sort | uniq -c -d
echo ""

echo "Culprit for common header"
find cpu -name periph_conf.h

Issues/PRs references

Part of Issue #8713

jnohlgard · 2018-03-02T08:49:52Z

Makefile.include

+
+# Include Board configuration
 INCLUDES += -I$(RIOTBOARD)/$(BOARD)/include
+include $(RIOTBOARD)/$(BOARD)/Makefile.include


There could be a reason to include defaultmodules.inc.mk and pseudomodules.inc.mk in the same order as before, in case they are used by the board and cpu makefiles.

There are currently no difference, but I agree there is no reason to change the order. I will fix it.

jnohlgard

I agree with the general idea, but see my inline comment regarding makefile include order

miri64 · 2018-03-15T15:20:58Z

ping @gebart

cladmi · 2018-03-15T18:15:54Z

I developed some scripts to check the difference in included headers for #8652. I will run them for this PR too to check that indeed no includes changed and give some more tested feedback.

cladmi · 2018-03-19T16:02:28Z

I ran buildtest on all examples without cleaning intermediates and generated .d files are the same on master and on this branch rebased meaning the same headers are included.

I did not run on tests at the same time because it was taking too much space as packages are re-downloaded many times.

I could re-run it only on tests if you want.

kaspar030 · 2018-04-11T20:50:55Z

Not release critical, untagging.

Include order for board and cpu was 1. cpu include 2. board include 3. board common includes 4. cpu common includes Its now changed to: 1. board include 2. board common includes 3. cpu include 4. cpu common includes Verifications: There are no common headers names between boards and cpus. Except native that has a 'periph_conf.h' in cpu instead of being in board.

cladmi · 2018-07-04T07:22:51Z

Rebased, and I re-ran the test mentioned and there is still no names conflicts.

smlng · 2018-07-04T07:48:33Z

tested on macOS for native, output for make -C examples/gnrc_networking info-build INCLUDES:

master

INCLUDES: 
	-I/Volumes/devel/github/smlng/RIOT/core/include  
	-I/Volumes/devel/github/smlng/RIOT/drivers/include  
	-I/Volumes/devel/github/smlng/RIOT/sys/include  
	-I/Volumes/devel/github/smlng/RIOT/cpu/native/include  
	-I/Volumes/devel/github/smlng/RIOT/boards/native/include  
	-DNATIVE_INCLUDES  
	-I/Volumes/devel/github/smlng/RIOT/boards/native/include/  
	-I/Volumes/devel/github/smlng/RIOT/core/include/  
	-I/Volumes/devel/github/smlng/RIOT/drivers/include/  
	-I/Volumes/devel/github/smlng/RIOT/cpu/native/include  
	-I/Volumes/devel/github/smlng/RIOT/sys/include  
	-I/Volumes/devel/github/smlng/RIOT/cpu/native/osx-libc-extra  
	-I/Volumes/devel/github/smlng/RIOT/sys/net/gnrc/netif/include

This PR

INCLUDES: 
	-I/Volumes/devel/github/smlng/RIOT/core/include  
	-I/Volumes/devel/github/smlng/RIOT/drivers/include  
	-I/Volumes/devel/github/smlng/RIOT/sys/include  
	-I/Volumes/devel/github/smlng/RIOT/boards/native/include  
	-DNATIVE_INCLUDES  
	-I/Volumes/devel/github/smlng/RIOT/boards/native/include/  
	-I/Volumes/devel/github/smlng/RIOT/core/include/  
	-I/Volumes/devel/github/smlng/RIOT/drivers/include/  
	-I/Volumes/devel/github/smlng/RIOT/cpu/native/include  
	-I/Volumes/devel/github/smlng/RIOT/sys/include  
	-I/Volumes/devel/github/smlng/RIOT/cpu/native/osx-libc-extra  
	-I/Volumes/devel/github/smlng/RIOT/cpu/native/include  
	-I/Volumes/devel/github/smlng/RIOT/sys/net/gnrc/netif/include

Order looks better and works but for native there are several duplicated includes which looks a bit strange - this just an info and might be worth looking into in a followup PR

smlng

tested ACK

cladmi · 2018-07-04T08:36:35Z

Murdock is happy, thank you for testing.

cladmi · 2018-07-04T09:02:15Z

The duplicate includes for native are there because currently NATIVEINCLUDES are added to INCLUDES:

RIOT/boards/native/Makefile.include

Line 127 in 40c28d7

export INCLUDES += $(NATIVEINCLUDES)

cladmi added Area: build system Area: Build system Type: cleanup The issue proposes a clean-up / The PR cleans-up parts of the codebase / documentation CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR labels Mar 1, 2018

cladmi requested a review from jnohlgard March 1, 2018 15:48

cladmi mentioned this pull request Mar 1, 2018

Cleanup of boards and cpus makefiles. #8713

Open

10 tasks

jnohlgard reviewed Mar 2, 2018

View reviewed changes

jnohlgard added this to the Release 2018.04 milestone Mar 2, 2018

jnohlgard self-assigned this Mar 2, 2018

kaspar030 removed this from the Release 2018.04 milestone Apr 11, 2018

cladmi added this to the Release 2018.07 milestone Apr 12, 2018

cladmi force-pushed the pr/makefile/change_include_order branch from 5f93b8b to f7f2494 Compare July 4, 2018 07:21

cladmi requested a review from smlng July 4, 2018 07:31

smlng approved these changes Jul 4, 2018

View reviewed changes

cladmi merged commit 499d6b4 into RIOT-OS:master Jul 4, 2018

cladmi deleted the pr/makefile/change_include_order branch July 4, 2018 08:38

cladmi mentioned this pull request Aug 21, 2018

Support for self contained modules #9811

Closed

18 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Makefile.include: fix board/cpu inconsistent include order#8717

Makefile.include: fix board/cpu inconsistent include order#8717
cladmi merged 1 commit intoRIOT-OS:masterfrom
cladmi:pr/makefile/change_include_order

cladmi commented Mar 1, 2018

Uh oh!

jnohlgard Mar 2, 2018

Uh oh!

cladmi Mar 2, 2018

Uh oh!

jnohlgard left a comment

Uh oh!

miri64 commented Mar 15, 2018

Uh oh!

cladmi commented Mar 15, 2018

Uh oh!

cladmi commented Mar 19, 2018

Uh oh!

kaspar030 commented Apr 11, 2018

Uh oh!

cladmi commented Jul 4, 2018

Uh oh!

smlng commented Jul 4, 2018

Uh oh!

smlng left a comment

Uh oh!

cladmi commented Jul 4, 2018

Uh oh!

cladmi commented Jul 4, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

Conversation

cladmi commented Mar 1, 2018

Contribution description

Details

Further steps

Changes INCLUDES output

Verifications

Issues/PRs references

Uh oh!

jnohlgard Mar 2, 2018

Choose a reason for hiding this comment

Uh oh!

cladmi Mar 2, 2018

Choose a reason for hiding this comment

Uh oh!

jnohlgard left a comment

Choose a reason for hiding this comment

Uh oh!

miri64 commented Mar 15, 2018

Uh oh!

cladmi commented Mar 15, 2018

Uh oh!

cladmi commented Mar 19, 2018

Uh oh!

kaspar030 commented Apr 11, 2018

Uh oh!

cladmi commented Jul 4, 2018

Uh oh!

smlng commented Jul 4, 2018

master

This PR

Uh oh!

smlng left a comment

Choose a reason for hiding this comment

Uh oh!

cladmi commented Jul 4, 2018

Uh oh!

cladmi commented Jul 4, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants