boards/native: fix overriden INCLUDES leading to build failure #11572

aabadie · 2019-05-24T11:38:24Z

Contribution description

This PR fixes an issue when one loads the log_printfnoformat submodule into an application and builds for native. This use case works for all other boards.

I'm not sure this is the best fix and I'm also wondering why using INCLUDES = $(NATIVEINCLUDES) in Makefile.include is needed.

Testing procedure

Build an application with log_printfnoformat on native:

USEMODULE=log_printfnoformat make -C examples/hello-world/ clean all

With this PR it works, on master, you get the following error:

make: Entering directory '/home/aabadie/softs/src/riot/RIOT/examples/hello-world'
Building application "hello-world" for "native" with MCU "native".

"make" -C /home/aabadie/softs/src/riot/RIOT/boards/native
In file included from /home/aabadie/softs/src/riot/RIOT/boards/native/board.c:21:
/home/aabadie/softs/src/riot/RIOT/core/include/log.h:98:10: fatal error: log_module.h: No such file or directory
 #include "log_module.h"
          ^~~~~~~~~~~~~~
compilation terminated.

Issues/PRs references

Required by #11573, fixes #11603

cladmi

NATIVEINCLUDES is currently necessary here. At least, changing it is not trivial. And += should not be used at all.

The goal to use NATIVEINCLUDES is to not use the modules include directories and currently use the system posix headers for example.

It make sense for module giving a RIOT interface around system functionalities.

If boards needs both RIOT and system headers it means the system specific part may need to be split out.

cladmi · 2019-05-27T12:05:49Z

I checked in board.c and the issue is only for native_motor, so I think it should be moved outside.

cladmi · 2019-05-27T12:10:00Z

The main question for native, is what should be using the system headers instead of the RIOT ones. Restricting this to the lowest layer modules as possible would be good.
Another solution is to make our headers use the reference posix ones if available.

jcarrano · 2019-05-27T13:03:23Z

@cladmi So you say that the real solution is to move ´native_motor´ outside?

aabadie · 2019-05-27T14:22:56Z

I checked in board.c and the issue is only for native_motor, so I think it should be moved outside.

I don't understand the relation with the problem that this PR is trying to solve. Moving native_motor "outside" doesn't solve the build issue you get with USEMODULE=log_printfnoformat make -C examples/hello-world.

I checked info-build target to compare the list of directories included (in INCLUDES), using USEMODULE=log_printfnoformat make -C examples/hello-world info-build:

this PR:

INCLUDES: 
	-I/home/aabadie/softs/src/riot/RIOT/core/include  
	-I/home/aabadie/softs/src/riot/RIOT/drivers/include  
	-I/home/aabadie/softs/src/riot/RIOT/sys/include  
	-I/home/aabadie/softs/src/riot/RIOT/boards/native/include  
	-DNATIVE_INCLUDES  
	-I/home/aabadie/softs/src/riot/RIOT/boards/native/include/  
	-I/home/aabadie/softs/src/riot/RIOT/core/include/  
	-I/home/aabadie/softs/src/riot/RIOT/drivers/include/  
	-I/home/aabadie/softs/src/riot/RIOT/cpu/native/include  
	-I/home/aabadie/softs/src/riot/RIOT/sys/include  
	-I/home/aabadie/softs/src/riot/RIOT/cpu/native/include  
	-I/home/aabadie/softs/src/riot/RIOT/sys/log/log_printfnoformat

master:

INCLUDES: 
	-I/home/aabadie/softs/src/riot/RIOT/core/include  
	-I/home/aabadie/softs/src/riot/RIOT/drivers/include  
	-I/home/aabadie/softs/src/riot/RIOT/sys/include  
	-I/home/aabadie/softs/src/riot/RIOT/boards/native/include  
	-DNATIVE_INCLUDES  
	-I/home/aabadie/softs/src/riot/RIOT/boards/native/include/  
	-I/home/aabadie/softs/src/riot/RIOT/core/include/  
	-I/home/aabadie/softs/src/riot/RIOT/drivers/include/  
	-I/home/aabadie/softs/src/riot/RIOT/cpu/native/include  
	-I/home/aabadie/softs/src/riot/RIOT/sys/include  
	-I/home/aabadie/softs/src/riot/RIOT/cpu/native/include  
	-I/home/aabadie/softs/src/riot/RIOT/sys/log/log_printfnoformat

They are exactly the same but the latter doesn't work. So for me the use of INCLUDES = $(NATIVEINCLUDES) in the board Makefile is somehow breaking the list of includes.

Now if I use the verbose build, one can see that the log_printfnoformat is not in the list of -I arguments of gcc, which is in contradiction with what info-build returns:

on master (and it fails right after of course):

gcc \
	-DRIOT_FILE_RELATIVE=\"boards/native/board_init.c\" \
	-DRIOT_FILE_NOPATH=\"board_init.c\" \
	-Werror -Wall -Wextra -pedantic -std=gnu99 -m32 -fstack-protector-all -ffunction-sections -fdata-sections -fno-delete-null-pointer-checks -fdiagnostics-color -Wstrict-prototypes -Wold-style-definition -fno-common -Wall -Wextra -Wformat=2 -Wformat-overflow -Wformat-truncation -Wmissing-include-dirs -include '/home/aabadie/softs/src/riot/RIOT/examples/hello-world/bin/native/riotbuild/riotbuild.h'  -DNATIVE_INCLUDES -I/home/aabadie/softs/src/riot/RIOT/boards/native/include/ -I/home/aabadie/softs/src/riot/RIOT/core/include/ -I/home/aabadie/softs/src/riot/RIOT/drivers/include/ -I/home/aabadie/softs/src/riot/RIOT/cpu/native/include -I/home/aabadie/softs/src/riot/RIOT/sys/include -MD -MP -c -o /home/aabadie/softs/src/riot/RIOT/examples/hello-world/bin/native/board/board_init.o /home/aabadie/softs/src/riot/RIOT/boards/native/board_init.c
gcc \
	-DRIOT_FILE_RELATIVE=\"boards/native/board.c\" \
	-DRIOT_FILE_NOPATH=\"board.c\" \
	-Werror -Wall -Wextra -pedantic -std=gnu99 -m32 -fstack-protector-all -ffunction-sections -fdata-sections -fno-delete-null-pointer-checks -fdiagnostics-color -Wstrict-prototypes -Wold-style-definition -fno-common -Wall -Wextra -Wformat=2 -Wformat-overflow -Wformat-truncation -Wmissing-include-dirs -include '/home/aabadie/softs/src/riot/RIOT/examples/hello-world/bin/native/riotbuild/riotbuild.h'  -DNATIVE_INCLUDES -I/home/aabadie/softs/src/riot/RIOT/boards/native/include/ -I/home/aabadie/softs/src/riot/RIOT/core/include/ -I/home/aabadie/softs/src/riot/RIOT/drivers/include/ -I/home/aabadie/softs/src/riot/RIOT/cpu/native/include -I/home/aabadie/softs/src/riot/RIOT/sys/include -MD -MP -c -o /home/aabadie/softs/src/riot/RIOT/examples/hello-world/bin/native/board/board.o /home/aabadie/softs/src/riot/RIOT/boards/native/board.c
In file included from /home/aabadie/softs/src/riot/RIOT/boards/native/board.c:21:
/home/aabadie/softs/src/riot/RIOT/core/include/log.h:98:10: fatal error: log_module.h: No such file or directory
 #include "log_module.h"

with this PR:

gcc \
	-DRIOT_FILE_RELATIVE=\"examples/hello-world/main.c\" \
	-DRIOT_FILE_NOPATH=\"main.c\" \
	-Werror -Wall -Wextra -pedantic -std=gnu99 -m32 -fstack-protector-all -ffunction-sections -fdata-sections -fno-delete-null-pointer-checks -fdiagnostics-color -Wstrict-prototypes -Wold-style-definition -fno-common -Wall -Wextra -Wformat=2 -Wformat-overflow -Wformat-truncation -Wmissing-include-dirs -include '/home/aabadie/softs/src/riot/RIOT/examples/hello-world/bin/native/riotbuild/riotbuild.h'  -I/home/aabadie/softs/src/riot/RIOT/core/include -I/home/aabadie/softs/src/riot/RIOT/drivers/include -I/home/aabadie/softs/src/riot/RIOT/sys/include -I/home/aabadie/softs/src/riot/RIOT/boards/native/include -DNATIVE_INCLUDES -I/home/aabadie/softs/src/riot/RIOT/boards/native/include/ -I/home/aabadie/softs/src/riot/RIOT/core/include/ -I/home/aabadie/softs/src/riot/RIOT/drivers/include/ -I/home/aabadie/softs/src/riot/RIOT/cpu/native/include -I/home/aabadie/softs/src/riot/RIOT/sys/include -I/home/aabadie/softs/src/riot/RIOT/cpu/native/include -I/home/aabadie/softs/src/riot/RIOT/sys/log/log_printfnoformat -MD -MP -c -o /home/aabadie/softs/src/riot/RIOT/examples/hello-world/bin/native/application_hello-world/main.o /home/aabadie/softs/src/riot/RIOT/examples/hello-world/main.c
"make" -C /home/aabadie/softs/src/riot/RIOT/boards/native
make[2]: Entering directory '/home/aabadie/softs/src/riot/RIOT/boards/native'
mkdir -p /home/aabadie/softs/src/riot/RIOT/examples/hello-world/bin/native/board/
gcc \
	-DRIOT_FILE_RELATIVE=\"boards/native/board_init.c\" \
	-DRIOT_FILE_NOPATH=\"board_init.c\" \
	-Werror -Wall -Wextra -pedantic -std=gnu99 -m32 -fstack-protector-all -ffunction-sections -fdata-sections -fno-delete-null-pointer-checks -fdiagnostics-color -Wstrict-prototypes -Wold-style-definition -fno-common -Wall -Wextra -Wformat=2 -Wformat-overflow -Wformat-truncation -Wmissing-include-dirs -include '/home/aabadie/softs/src/riot/RIOT/examples/hello-world/bin/native/riotbuild/riotbuild.h'  -I/home/aabadie/softs/src/riot/RIOT/core/include -I/home/aabadie/softs/src/riot/RIOT/drivers/include -I/home/aabadie/softs/src/riot/RIOT/sys/include -I/home/aabadie/softs/src/riot/RIOT/boards/native/include -DNATIVE_INCLUDES -I/home/aabadie/softs/src/riot/RIOT/boards/native/include/ -I/home/aabadie/softs/src/riot/RIOT/core/include/ -I/home/aabadie/softs/src/riot/RIOT/drivers/include/ -I/home/aabadie/softs/src/riot/RIOT/cpu/native/include -I/home/aabadie/softs/src/riot/RIOT/sys/include -I/home/aabadie/softs/src/riot/RIOT/cpu/native/include -I/home/aabadie/softs/src/riot/RIOT/sys/log/log_printfnoformat -DNATIVE_INCLUDES -I/home/aabadie/softs/src/riot/RIOT/boards/native/include/ -I/home/aabadie/softs/src/riot/RIOT/core/include/ -I/home/aabadie/softs/src/riot/RIOT/drivers/include/ -I/home/aabadie/softs/src/riot/RIOT/cpu/native/include -I/home/aabadie/softs/src/riot/RIOT/sys/include -MD -MP -c -o /home/aabadie/softs/src/riot/RIOT/examples/hello-world/bin/native/board/board_init.o /home/aabadie/softs/src/riot/RIOT/boards/native/board_init.c

cladmi · 2019-05-27T14:27:54Z

NATIVEINCLUDES is not as simple as this. It is there for a good reason.

#8940

And indeed, the modules using NATIVEINCLUDES cannot use any riot modules includes. It is on purpose.

cladmi · 2019-05-28T07:51:20Z

@jcarrano currently it is using APIs that are not available for boards/native/board.c as it uses restrictive includes, so the placement is wrong.
As native/boards is not using any system posix headers it should be possible to use the normal INCLUDES there. Doing a rationalization of the NATIVEINCLUDES handling in general could be a good thing.

However, in practice, why is there even a native_motor_driver_qdec_simulation in boards/native/board.c?
If there is a need for a fake test device, why not put it in its own module ?
It is always compiled even if motor_driver features is not used.

aabadie · 2019-05-28T07:54:28Z

And indeed, the modules using NATIVEINCLUDES cannot use any riot modules includes. It is on purpose.

Can you explain this a bit more ?

cladmi · 2019-05-28T08:21:18Z

And indeed, the modules using NATIVEINCLUDES cannot use any riot modules includes. It is on purpose.

Can you explain this a bit more ?

From #8940.

The goal of NATIVEINCLUDES, as I understood it, is to handle conflicts between RIOT headers and the system headers which may contain different values.
(I know about POSIX for example)
POSIX is only an API and not an ABI, so "names" are the same but not the values for the enums/defines and even differ between linux/mac.

For native our POSIX headers do not match with the ones from the system.
Using INCLUDES directories can make the system use RIOT posix values, which are not the one for the host system. So not working when communication with the system.

Every modules that need to communicate with the Linux/mac underlying system, must use the system headers, not the ones we provide if they can be different.

Currently who should use NATIVEINCLUDES is not that clear and maybe too general as here board for native does not interact directly with Host system.

aabadie · 2019-05-29T05:34:20Z

@cladmi, see #11603 where there is an alternative to the change proposed here: extend NATIVEINCLUDES from the log submodule.
I have no idea what would be better. Do you have an even better suggestion ?

cladmi · 2019-06-03T09:49:13Z

@aabadie I think we can change that NATIVEINCLUDES is not used at all boards/native/* as neither board nor native-drivers implement direct interaction with the system. If another periph using system interaction must be implemented it should be put in its own directory .
So the definition of NATIVEINCLUDES could be moved to cpu/native too.

aabadie · 2019-06-03T11:44:26Z

@cladmi, I remove INCLUDES = $(NATIVEINCLUDES) from boards/native and cpu/native and it seems to work. I'm not sure if that was what you proposed.

I'll removed the previous commits if it's ok for you, just keeping the commit with the new test application.

cladmi · 2019-06-03T12:17:07Z

I was thinking about boards/native and boards/native/drivers.

cpu/native should keep it, async_read.c uses posix headers, and we have a custom fcntl.h. It may be ok but would not want to try.

When removing it needs to be reviewed that the included header did not change unexpectedly.
This can be checked by comparing the .d files output after compilation for all affected files like bin/native/board/board.d. So this needs its specific pull request.

aabadie · 2019-06-04T19:49:20Z

@cladmi, I updated this PR following your suggestion: cpu/native/Makefile change is reverted and only boards/native/Makefile/boards/native/drivers/Makefile are modified.

When removing it needs to be reviewed that the included header did not change unexpectedly.

After building examples/hello-world, I also checked the content of bin/native/board/board.d, it doesn't seem to have changed: same number of files included (using a combination of cat and grep with regexp).

So this needs its specific pull request.

I'm not sure to understand. Do you mean the included files verification ? e.g. a new static test or something ?

aabadie · 2019-06-26T07:26:26Z

ping @cladmi

cladmi · 2019-06-26T16:26:42Z

So this needs its specific pull request.

I'm not sure to understand. Do you mean the included files verification ? e.g. a new static test or something ?

I mean tests/log_printfnoformat: add test application & script should not stay in the pr removing NATIVE_INCLUDES from boards/native and boards/native/drivers.

I will test the update now.

cladmi · 2019-06-26T16:42:17Z

Comparing with your reference PR, it indeed did not change the .d files for board and native-led according to diff RIOT_CI_BUILD=1 make -C examples/hello-world/

diff -u bin_master/native/native-drivers/native-led.d bin_pr/native/native-drivers/native-led.d
diff -u bin_master/native/board/board.d bin_pr/native/board/board.d

And to be even more precise, the whole binary directories are the same

diff -r -a bin_master/ bin_pr/

And both the log_printfnoformat and the USEMODULE=log_printfnoformat make -C examples/hello-world/ clean all test case work.

Can you please rebase and only keep the commit removing NATIVEINCLUDES from native, and update the PR description.

As a justification somehow if you want inspiration

Both `board` and `native-drivers` do not interact with the host operating system so does not need to use system headers. So using restricted `NATIVEINCLUDES` is not needed.

It also fixes the issue that they use RIOT headers for, for example logging, which were not available in `NATIVEINCLUDES`.

aabadie · 2019-06-26T17:45:58Z

I mean tests/log_printfnoformat: add test application & script should not stay in the pr removing NATIVE_INCLUDES from boards/native and boards/native/drivers.

I'm sorry but I read 3 times your initial comment about "So this needs its specific pull request" and this cannot the reason. It seems at least that the test application had nothing to with "So this needs its specific pull request".
Can you explain this again better ?

Also I think it's fine to keep the test application in this PR event if it doesn't seem related: it gives a way to verify that log_printfnoformat can be build for native (which was the initial goal of this PR).

aabadie · 2019-08-06T07:50:16Z

I cut out the test application from this PR and opened #11968

miri64 · 2019-08-06T08:43:05Z

So where do NATIVEINCLUDES go now? IIRC native needs some different defines, otherwise their is a collision with our POSIX headers, and the host system's POSIX headers.

aabadie · 2019-08-06T08:53:13Z

So where do NATIVEINCLUDES go now?

They are not needed in boards/native

IIRC native needs some different defines, otherwise their is a collision with our POSIX headers, and the host system's POSIX headers.

See #11572 (comment)

miri64 · 2019-08-06T08:55:25Z

So where do NATIVEINCLUDES go now?

They are not needed in boards/native

IIRC native needs some different defines, otherwise their is a collision with our POSIX headers, and the host system's POSIX headers.

See #11572 (comment)

I see. I did not read the whole discussion.

miri64

ACK. The argumentation in this PR makes sense and I locally-tested locally with some apps I know had problems with POSIX headers in the past:

examples/posix_sockets
tests/gnrc_sock_dns
tests/posix_*
tests/pthreads_*

I also tested #11968 with and without this PR merged and it indeed fixes it fixes compiling for the test introduced there for native.

miri64 · 2019-08-06T09:02:52Z

Triggered rebuild, since last build was a few days back.

aabadie · 2019-08-06T10:05:21Z

All green, let's merge. Thanks @miri64 !

aabadie requested a review from kaspar030 May 24, 2019 11:38

aabadie mentioned this pull request May 24, 2019

sys/log: add module for colorized logging #11573

Merged

aabadie 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 May 24, 2019

aabadie requested a review from cladmi May 27, 2019 11:33

cladmi suggested changes May 27, 2019

View reviewed changes

cladmi requested a review from miri64 May 27, 2019 12:00

cladmi requested a review from jcarrano May 27, 2019 12:07

aabadie mentioned this pull request May 29, 2019

NATIVEINCLUDES Does not include USEMODULE_INCLUDES from log_printfnoformat #11603

Closed

aabadie force-pushed the pr/boards/native_fix_log branch from 304a7fe to e897634 Compare May 31, 2019 09:11

aabadie force-pushed the pr/boards/native_fix_log branch from e897634 to 4ad46e6 Compare June 3, 2019 11:36

aabadie added the CI: needs squashing Commits in this PR need to be squashed; If set, CI systems will mark this PR as unmergable label Jun 3, 2019

aabadie force-pushed the pr/boards/native_fix_log branch from 4ad46e6 to 580c6eb Compare June 4, 2019 19:38

nmeum mentioned this pull request Jun 24, 2019

boards/native: fix undefinied reference to qdecs_value #11733

Merged

aabadie force-pushed the pr/boards/native_fix_log branch from 580c6eb to b48bd22 Compare June 26, 2019 17:50

aabadie removed the CI: needs squashing Commits in this PR need to be squashed; If set, CI systems will mark this PR as unmergable label Jun 27, 2019

native: don't use NATIVEINCLUDES in board and board/drivers

d8746ad

aabadie force-pushed the pr/boards/native_fix_log branch from b48bd22 to d8746ad Compare August 6, 2019 07:45

aabadie mentioned this pull request Aug 6, 2019

tests/log_printfnoformat: add test application & script #11968

Merged

miri64 approved these changes Aug 6, 2019

View reviewed changes

miri64 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 Aug 6, 2019

aabadie merged commit b388a5b into RIOT-OS:master Aug 6, 2019

aabadie deleted the pr/boards/native_fix_log branch August 6, 2019 10:05

kb2ma added this to the Release 2019.10 milestone Sep 16, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

boards/native: fix overriden INCLUDES leading to build failure #11572

boards/native: fix overriden INCLUDES leading to build failure #11572

aabadie commented May 24, 2019 •

edited

Loading

cladmi left a comment

cladmi commented May 27, 2019

cladmi commented May 27, 2019

jcarrano commented May 27, 2019

aabadie commented May 27, 2019

cladmi commented May 27, 2019

cladmi commented May 28, 2019

aabadie commented May 28, 2019

cladmi commented May 28, 2019 •

edited

Loading

aabadie commented May 29, 2019

cladmi commented Jun 3, 2019

aabadie commented Jun 3, 2019

cladmi commented Jun 3, 2019

aabadie commented Jun 4, 2019

aabadie commented Jun 26, 2019

cladmi commented Jun 26, 2019

cladmi commented Jun 26, 2019

aabadie commented Jun 26, 2019

aabadie commented Aug 6, 2019

miri64 commented Aug 6, 2019

aabadie commented Aug 6, 2019

miri64 commented Aug 6, 2019

miri64 left a comment

miri64 commented Aug 6, 2019

aabadie commented Aug 6, 2019

boards/native: fix overriden INCLUDES leading to build failure #11572

boards/native: fix overriden INCLUDES leading to build failure #11572

Conversation

aabadie commented May 24, 2019 • edited Loading

Contribution description

Testing procedure

Issues/PRs references

cladmi left a comment

Choose a reason for hiding this comment

cladmi commented May 27, 2019

cladmi commented May 27, 2019

jcarrano commented May 27, 2019

aabadie commented May 27, 2019

cladmi commented May 27, 2019

cladmi commented May 28, 2019

aabadie commented May 28, 2019

cladmi commented May 28, 2019 • edited Loading

aabadie commented May 29, 2019

cladmi commented Jun 3, 2019

aabadie commented Jun 3, 2019

cladmi commented Jun 3, 2019

aabadie commented Jun 4, 2019

aabadie commented Jun 26, 2019

cladmi commented Jun 26, 2019

cladmi commented Jun 26, 2019

aabadie commented Jun 26, 2019

aabadie commented Aug 6, 2019

miri64 commented Aug 6, 2019

aabadie commented Aug 6, 2019

miri64 commented Aug 6, 2019

miri64 left a comment

Choose a reason for hiding this comment

miri64 commented Aug 6, 2019

aabadie commented Aug 6, 2019

aabadie commented May 24, 2019 •

edited

Loading

cladmi commented May 28, 2019 •

edited

Loading