native: remove non required NATIVEINCLUDES #8652

cladmi · 2018-02-27T18:36:08Z

While trying to document more what is done in boards and cpu Makefile.include files I found this native 'NATIVEINCLUDES' special case and found no current reasons to have it.
Also, not real explanation on why it is needed.

Contribution description

Some modules used a 'NATIVEINCLUDES' with different include path and no other included directories.
It was defining basic 'include' in a different order and not using other things defined in INCLUDES.
After doing some checks with the given include path and possible conflicting files, there should be no conflict when using the default one.

No common headers between all the NATIVEINCLUDES directories
No common headers files between board/native/include, cpu/native/include and
other files in the repository (except other boards/cpus of course).

Notes

I found reference in a previous issue here #793 but it looks like its not the case anymore.

Maybe I am missing some things and would like to ask at least if it is still necessary.
Also they may be other solutions to not need this special case.

If its still necessary, I would change the PR to only use the same include order as the one currently used in RIOT as there is no reasons to have another one.

I tried to detect problems with this script:

#! /bin/sh

INCLUDE_DIRS+=" boards/native/include/"
INCLUDE_DIRS+=" cpu/native/include/"
INCLUDE_DIRS+=" core/include/"
INCLUDE_DIRS+=" drivers/include/"
INCLUDE_DIRS+=" sys/include/"
INCLUDE_DIRS+=" cpu/native/osx-libc-extra/"
INCLUDE_DIRS+=" /usr/include/valgrind/"

list_headers() {
    local include_dir="$1"
    echo "Listing files in $include_dir" >&2
    find "${include_dir}" -type f | sed "s|^${include_dir}||"
}

list_all_headers() {
    for i in $@; do
        list_headers "$i"
    done
}

list_conflicting_headers() {
    local directory="$1"
    echo "Listing files with same name as in ${directory}"
    for i in $(ls "$directory"); do
        find . -name $i | grep -v -e '^./cpu' -e '^./boards' | sed 's/^/  /'
    done
}

OUTPUT=/tmp/out
list_all_headers ${INCLUDE_DIRS} | sort | uniq -c -d | sed 's/^ */  /'
echo ""

list_conflicting_headers boards/native/include
list_conflicting_headers cpu/native/include
echo ""

NI_PATTERN="NATIVE.?INCLUDES"
echo "Listing ${NI_PATTERN}"
git grep -E ${NI_PATTERN} | sed 's/^/  /'

Issues/PRs references

None.

jnohlgard

ACK in general.
Did you compare the resulting command lines for gcc before and after?

miri64

Just for safety I tested with Valgrind and it still works ;-).

cladmi · 2018-02-27T18:58:09Z

@gebart Normally the gcc command line must be different before and after but I did not compared the real difference. I am first waiting for murdock result at least :)

I know my check is not against "system" includes directories and it could be a problem. I do not really know how to test this right now.

cladmi · 2018-03-01T14:31:35Z

The script output before my PR:

Listing files in boards/native/include/
Listing files in cpu/native/include/
Listing files in core/include/
Listing files in drivers/include/
Listing files in sys/include/
Listing files in cpu/native/osx-libc-extra/
Listing files in /usr/include/valgrind/
  2 can/doc.txt

Listing files with same name as in boards/native/include
  ./pkg/semtech-loramac/include/semtech-loramac/board.h
Listing files with same name as in cpu/native/include
  ./tests/openthread/bin/pkg/samr21-xpro/openthread/third_party/ti/cc26xxware/driverlib/cpu.h

Listing NATIVE.?INCLUDES
  boards/native/Makefile:INCLUDES = $(NATIVEINCLUDES)
  boards/native/Makefile.include:export NATIVEINCLUDES += -DNATIVE_INCLUDES
  boards/native/Makefile.include:export NATIVEINCLUDES += -I$(RIOTBOARD)/$(BOARD)/include/
  boards/native/Makefile.include:export NATIVEINCLUDES += -I$(RIOTBASE)/core/include/
  boards/native/Makefile.include:export NATIVEINCLUDES += -I$(RIOTBASE)/drivers/include/
  boards/native/Makefile.include:all-valgrind: export NATIVEINCLUDES += $(shell pkg-config valgrind --cflags)
  boards/native/Makefile.include:export INCLUDES += $(NATIVEINCLUDES)
  boards/native/drivers/Makefile:INCLUDES = $(NATIVEINCLUDES)
  core/include/kernel_types.h:#ifndef NATIVE_INCLUDES
  cpu/native/Makefile:INCLUDES = $(NATIVEINCLUDES)
  cpu/native/Makefile.include:export NATIVEINCLUDES += -I$(RIOTCPU)/native/include -I$(RIOTBASE)/sys/include
  cpu/native/Makefile.include:    export NATIVEINCLUDES += -I$(RIOTCPU)/native/osx-libc-extra
  cpu/native/mtd/Makefile:INCLUDES = $(NATIVEINCLUDES)
  cpu/native/netdev_tap/Makefile:INCLUDES = $(NATIVEINCLUDES)
  cpu/native/socket_zep/Makefile:INCLUDES = $(NATIVEINCLUDES)

cladmi · 2018-03-01T14:31:57Z

The script output with the PR:

Listing files in boards/native/include/
Listing files in cpu/native/include/
Listing files in core/include/
Listing files in drivers/include/
Listing files in sys/include/
Listing files in cpu/native/osx-libc-extra/
Listing files in /usr/include/valgrind/
  2 can/doc.txt

Listing files with same name as in boards/native/include
  ./pkg/semtech-loramac/include/semtech-loramac/board.h
Listing files with same name as in cpu/native/include
  ./tests/openthread/bin/pkg/samr21-xpro/openthread/third_party/ti/cc26xxware/driverlib/cpu.h

Listing NATIVE.?INCLUDES
  boards/native/Makefile.include:CFLAGS += -DNATIVE_INCLUDES
  core/include/kernel_types.h:#ifndef NATIVE_INCLUDES

cladmi · 2018-03-01T14:43:27Z

For the difference between before and after by using #8714

The CFLAGS are the same, only includes changed.

Before

INCLUDES: 
        -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/native/include  
        -I/home/harter/work/git/RIOT/boards/native/include  
        -DNATIVE_INCLUDES  
        -I/home/harter/work/git/RIOT/boards/native/include/  
        -I/home/harter/work/git/RIOT/core/include/  
        -I/home/harter/work/git/RIOT/drivers/include/  
        -I/home/harter/work/git/RIOT/cpu/native/include  
        -I/home/harter/work/git/RIOT/sys/include

After (I moved the -DNATIVE_INCLUDES to CFLAGS and so its now in riotbuild.h)

INCLUDES: 
	-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/native/include  
	-I/home/harter/work/git/RIOT/boards/native/include

For all modules using INCLUDES = $(NATIVEINCLUDES) where receiving only:

        -DNATIVE_INCLUDES  
        -I/home/harter/work/git/RIOT/boards/native/include/  
        -I/home/harter/work/git/RIOT/core/include/  
        -I/home/harter/work/git/RIOT/drivers/include/  
        -I/home/harter/work/git/RIOT/cpu/native/include  
        -I/home/harter/work/git/RIOT/sys/include

cladmi · 2018-03-15T17:53:51Z

Some more testing by analyzing the generated .d files showed me that files in sys/posix/include where used instead of the ones in /usr/include which could indeed still be a problem.

I added a commit that replace NATIVEINCLUDES by only removing -I sys/posix/include for the native Makefile. And the generated .d files are exactly the same for all examples and tests.

cladmi · 2018-03-15T17:54:49Z

Just pushing for testing on other machines, it should still be cleaned and tested.

jnohlgard · 2018-03-15T18:41:06Z

You don't need the special filter out of includes for native because the headers in sys/posix/include already have a preprocessor conditional include_next that forwards the include to the system header

cladmi · 2018-03-16T14:26:46Z

Without it, it was not compiling on @kYc0o mac for native. It was saying there was a type definition conflict or something.
We should re-test and add the error here.

kYc0o · 2018-03-19T16:15:04Z

Without the last commit, while building examples/posix_sockets I have this error:

/Users/facosta/git/RIOT-OS/RIOT/sys/posix/include/sys/bytes.h:31:16: error: typedef redefinition with different types ('size_t'
      (aka 'unsigned long') vs '__darwin_socklen_t' (aka 'unsigned int'))
typedef size_t socklen_t;           /**< socket address length */
               ^
/usr/include/sys/_types/_socklen_t.h:30:28: note: previous definition is here
typedef __darwin_socklen_t      socklen_t;
                                ^
1 error generated.

cladmi · 2018-03-19T19:00:37Z

Fixed the two compilation problems found for native/osx and posix

A conflicting type for socklen_t which has a different size uint32_t instead of size_t.
A missing definition of AF_LINK used for compiling netdev_tap

A last round of test could be cool on different setups:

BOARDS=native dist/tools/compile_test/compile_test.py

kYc0o · 2018-03-20T15:11:20Z

Re-tested and everything works OK.

kYc0o · 2018-03-20T15:11:47Z

You may squash it to get Murdock green.

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

@LudwigKnuepfer while you are online atm: do you remember why we did it this way?

In another header file, `socklen_t` is defined to `__darwin_socklen_t` which is an `uint32_t` and it conflicts. Preparation to remove NATIVEINCLUDES.

netdev_tap.c uses AF_LINK when compiled on OSX native. Preparation to remove NATIVEINCLUDES.

Some modules used a 'NATIVEINCLUDES' with different include path and no other included directories. It was defining basic 'include' in a different order and not using other things defined in INCLUDES. After doing some checks with the given include path and possible conflicting files, there should be no conflict when using the default one. * No common headers between all the NATIVEINCLUDES directories * No common headers files between board/native/include, cpu/native/include and other files in the repository (except other boards/cpus of course).

LudwigKnuepfer · 2018-03-21T09:52:16Z

Probably something with OSX... why don't you check the history?

miri64 · 2018-03-21T10:38:36Z

Well... I did, but

commit 2cd3f04fc6c391f1c137a9bd2236eea88dc22453
Author: Ludwig Ortmann <[email protected]>
Date:   Thu Feb 27 10:30:30 2014 +0100

    Don't use INCLUDES for building any native at all.
    
    native modules will never need the dynamic INCLUDES, so we define our
    own NATIVEINCLUDES. Due to the current make structure, the only way to
    not use INCLUDES is to redefine the build rules.

is less then helpful as a description ;-).

cladmi · 2018-03-21T14:30:18Z

It does make sense, as a module only needs to see its dependencies INCLUDES, not other unrelated modules INCLUDES. So in that case native board does not need to see the pkg/libfixmath includes which could be added by the build system to INCLUDES..
For that, having per module INCLUDES is a good idea and it could be a long term goal.
But then, why not adding COREINCLUDES also.

The reason I proposed to see if could be removed, is for cleanup purposes in the current build system.

Why in practice, does native need different INCLUDES ? It works for all modules, why not native ?
It had an order that did not match what is done in RIOT for other boards/modules (includes = board, core, driver, cpu, sys), is it needed or can it be changed to the same as RIOT ? So is it really only to use the dependencies include ?
Is supporting this kind of specific case needed in the RIOT build system and future evolutions ?

Another solution for this could be to just match the default RIOT include order and document why its required. I started this PR to know why it was needed. So I could create another PR to fix it in this way.

The provided patches fixes two definitions issues when building on osx.
For me using the same headers requires our posix headers to be better.
Also if a normal module importing from sys/posix/include and the board cpu/board functions using system headers, they could have different definitions for the same type (like socklen_t on osx) and if used in a shared struct definition it is a nasty bug to find that a type has different definition in two C files.

Also it removes a specific configuration in the build system that needs to be understood.

kYc0o

ACK, looks ok for me. @gebart you want to approve this too?

miri64 · 2018-03-21T15:56:33Z

Why in practice, does native need different INCLUDES ? It works for all modules, why not native ?

IIRC: RIOT is an operating system that has some degree of POSIX support (but not complete and that is important here!), the host system of RIOT native is also an operating system that has POSIX support => it might happen that a host system header has a function defined that is not implemented by RIOT => things may burn!

kYc0o · 2018-03-21T15:59:50Z

IIRC: RIOT is an operating system that has some degree of POSIX support (but not complete and that is important here!), the host system of RIOT native is also an operating system that has POSIX support => it might happen that a host system header has a function defined that is not implemented by RIOT => things may burn!

I thought the same and I tested this PR thoroughly, no errors so far both in compilation and tests (for those which have tests). So it may happen in the future, but it will break before it gets into RIOT.

miri64 · 2018-03-21T16:31:26Z

@kYc0o did you also test the tooling around native?

miri64 · 2018-03-21T16:32:27Z

(this isn't tested by Murdock, as far as I know)

kYc0o · 2018-03-22T15:24:11Z

@kYc0o did you also test the tooling around native?

Just tested and works as good/bad than in master. Anything special thing/case that should I test? I just tested examples/posix_sockets which is the one caused trouble before.

kYc0o · 2018-03-22T15:26:02Z

For info, the output:

valgrind --leak-check=full --track-origins=yes --fullpath-after=/Users/facosta/git/RIOT-OS/RIOT/ --read-var-info=yes /Users/facosta/git/RIOT-OS/RIOT/examples/posix_sockets/bin/native/posix_sockets_example.elf tap0
==73341== Memcheck, a memory error detector
==73341== Copyright (C) 2002-2017, and GNU GPL'd, by Julian Seward et al.
==73341== Using Valgrind-3.13.0 and LibVEX; rerun with -h for copyright info
==73341== Command: /Users/facosta/git/RIOT-OS/RIOT/examples/posix_sockets/bin/native/posix_sockets_example.elf tap0
==73341== 
--73341-- run: /usr/bin/dsymutil "/Users/facosta/git/RIOT-OS/RIOT/examples/posix_sockets/bin/native/posix_sockets_example.elf"

parse DIE(readdwarf3.c:3651): confused by:
 <2><80>: Abbrev Number: 7 (DW_TAG_subrange_type)
     DW_AT_type        : <87>	
     DW_AT_count       : 2	
parse_type_DIE:
--73341-- WARNING: Serious error when reading debug info
--73341-- When reading debug info from /Users/facosta/git/RIOT-OS/RIOT/examples/posix_sockets/bin/native/posix_sockets_example.elf:
--73341-- confused by the above DIE
RIOT native interrupts/signals initialized.
LED_RED_OFF
LED_GREEN_ON
RIOT native board initialized.
RIOT native hardware initialization complete.

main(): This is RIOT! (Version: 2018.07-devel-656-ga0017-snake.local)
RIOT socket example application
All up, running the shell now
> ^C
native: exiting

native: exiting
==73341== 
==73341== HEAP SUMMARY:
==73341==     in use at exit: 0 bytes in 0 blocks
==73341==   total heap usage: 0 allocs, 0 frees, 0 bytes allocated
==73341== 
==73341== All heap blocks were freed -- no leaks are possible
==73341== 
==73341== For counts of detected and suppressed errors, rerun with: -v
==73341== ERROR SUMMARY: 0 errors from 0 contexts (suppressed: 0 from 0)
==73343== 
==73343== HEAP SUMMARY:
==73343==     in use at exit: 0 bytes in 0 blocks
==73343==   total heap usage: 0 allocs, 0 frees, 0 bytes allocated
==73343== 
==73343== All heap blocks were freed -- no leaks are possible
==73343== 
==73343== For counts of detected and suppressed errors, rerun with: -v
==73343== ERROR SUMMARY: 0 errors from 0 contexts (suppressed: 0 from 0)

miri64

I guess this should be enough (also tested the same with Ubuntu 17.04). Then let's finally merge this!

LudwigKnuepfer · 2018-03-24T12:18:01Z

is less then helpful as a description ;-).

I disagree, but admit it's not as elaborative as it should have been. I guess the discussion here covered all points, at least as far as my (refreshed) memory goes...
So yes, the key point is conflicting definitions.

In particular the ticket (#793) referenced in the PR that contains that commit (#804), mentions:

Since pnet redefines unistd.h native fails to build on some machines. ...

I guess it would be worthwhile to see if the compiler invocation has changed significantly in between now and then to understand why there appear to be no problems anymore with this. Also, make sure no compiler warnings (try and enable more warnings that fit the problem) regarding this thematic complex show up. (I won't have time to dig into it).

For that, having per module INCLUDES is a good idea and it could be a long term goal.

I think having only definitions declared in compilation units the active compilation unit is modelled to depend on is a great idea.

jnohlgard reviewed Feb 27, 2018

View reviewed changes

miri64 reviewed Feb 27, 2018

View reviewed changes

cladmi added CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR and removed 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 mentioned this pull request Mar 1, 2018

Cleanup of boards and cpus makefiles. #8713

Open

10 tasks

cladmi force-pushed the pr/remove_nativeincludes branch from 589c6c2 to f800ce2 Compare March 15, 2018 17:47

cladmi mentioned this pull request Mar 15, 2018

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

Merged

cladmi force-pushed the pr/remove_nativeincludes branch from f800ce2 to bd9668a Compare March 19, 2018 17:23

cladmi force-pushed the pr/remove_nativeincludes branch from 53911e7 to cd4384a Compare March 20, 2018 16:48

cladmi added 3 commits March 20, 2018 17:50

posix/osx: fix type conflict on OSX native

dcebfb1

In another header file, `socklen_t` is defined to `__darwin_socklen_t` which is an `uint32_t` and it conflicts. Preparation to remove NATIVEINCLUDES.

posix/osx: fix missing AF_LINK on OSX native

b5554bc

netdev_tap.c uses AF_LINK when compiled on OSX native. Preparation to remove NATIVEINCLUDES.

cladmi force-pushed the pr/remove_nativeincludes branch from cd4384a to 93a521c Compare March 20, 2018 16:51

kYc0o approved these changes Mar 21, 2018

View reviewed changes

miri64 approved these changes Mar 22, 2018

View reviewed changes

miri64 merged commit 690c36b into RIOT-OS:master Mar 22, 2018

vincent-d mentioned this pull request Apr 11, 2018

Native CAN device does not build when VFS is used #8922

Closed

cladmi mentioned this pull request Apr 12, 2018

REVERT native: remove non required NATIVEINCLUDES #8940

Merged

cladmi deleted the pr/remove_nativeincludes branch April 19, 2018 11:08

cladmi added this to the Release 2018.04 milestone Nov 7, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

native: remove non required NATIVEINCLUDES #8652

native: remove non required NATIVEINCLUDES #8652

cladmi commented Feb 27, 2018

jnohlgard left a comment

miri64 left a comment

cladmi commented Feb 27, 2018

cladmi commented Mar 1, 2018

cladmi commented Mar 1, 2018

cladmi commented Mar 1, 2018

cladmi commented Mar 15, 2018

cladmi commented Mar 15, 2018

jnohlgard commented Mar 15, 2018

cladmi commented Mar 16, 2018 •

edited

Loading

kYc0o commented Mar 19, 2018

cladmi commented Mar 19, 2018

kYc0o commented Mar 20, 2018

kYc0o commented Mar 20, 2018

miri64 commented Mar 20, 2018

LudwigKnuepfer commented Mar 21, 2018

miri64 commented Mar 21, 2018

cladmi commented Mar 21, 2018

kYc0o left a comment

miri64 commented Mar 21, 2018

kYc0o commented Mar 21, 2018

miri64 commented Mar 21, 2018

miri64 commented Mar 21, 2018

kYc0o commented Mar 22, 2018

kYc0o commented Mar 22, 2018

miri64 left a comment

LudwigKnuepfer commented Mar 24, 2018

native: remove non required NATIVEINCLUDES #8652

native: remove non required NATIVEINCLUDES #8652

Conversation

cladmi commented Feb 27, 2018

Contribution description

Notes

Issues/PRs references

jnohlgard left a comment

Choose a reason for hiding this comment

miri64 left a comment

Choose a reason for hiding this comment

cladmi commented Feb 27, 2018

cladmi commented Mar 1, 2018

cladmi commented Mar 1, 2018

cladmi commented Mar 1, 2018

cladmi commented Mar 15, 2018

cladmi commented Mar 15, 2018

jnohlgard commented Mar 15, 2018

cladmi commented Mar 16, 2018 • edited Loading

kYc0o commented Mar 19, 2018

cladmi commented Mar 19, 2018

kYc0o commented Mar 20, 2018

kYc0o commented Mar 20, 2018

miri64 commented Mar 20, 2018

LudwigKnuepfer commented Mar 21, 2018

miri64 commented Mar 21, 2018

cladmi commented Mar 21, 2018

kYc0o left a comment

Choose a reason for hiding this comment

miri64 commented Mar 21, 2018

kYc0o commented Mar 21, 2018

miri64 commented Mar 21, 2018

miri64 commented Mar 21, 2018

kYc0o commented Mar 22, 2018

kYc0o commented Mar 22, 2018

miri64 left a comment

Choose a reason for hiding this comment

LudwigKnuepfer commented Mar 24, 2018

cladmi commented Mar 16, 2018 •

edited

Loading