newlib-nano: fix include directory, 2nd attempt with pkg fix #9281
newlib-nano: fix include directory, 2nd attempt with pkg fix #9281smlng wants to merge 6 commits intoRIOT-OS:masterfrom
Conversation
|
btw. includes all commits originally by @toonst |
also: - raise error when trying to use newlib on native target - check for existence of newlib.h file iso existence of include directory - use nano.specs file whenever possible
stddef.h and stdarg.h are not supplied by newlib, but by the compiler, so we need the standard include directories to find them.
|
there seems to be problem with the MIPS toolchain and its |
|
However this is currently only a problem with |
josephnoir
left a comment
There was a problem hiding this comment.
I don't feel confident to review the newlib related changes in newlib.mk. And I don't think someone approved the previous PR.
| .PHONY: all | ||
|
|
||
| export RIOT_CFLAGS = $(CFLAGS) $(INCLUDES) | ||
| RIOT_CFLAGS := $(INCLUDES) |
There was a problem hiding this comment.
This should be probably be renamed to RIOT_INCLUDES to reflect its contents.
There was a problem hiding this comment.
mhm, those are still CFLAGS passed to the C compiler
There was a problem hiding this comment.
hmh, I can accept that. Still need someone else to review the newlib part, any suggestions who?
Not enough knowledge about the RIOT build system or newlib.
|
@smlng can you identify a proper additional reviewer? |
@smlng but essentially this breaks mips in conjunction with @neiljay are you familiar with the nano.specs newlib + MIPS or did you encounter a similar issue? |
| @@ -1,7 +1,15 @@ | |||
| ifeq ($(BOARD), native) | |||
| $(error Newlib is not supported on native target) | |||
There was a problem hiding this comment.
Why add artifical barriers? It is possible to build newlib for Linux and windows (cygwin)
| ifneq ($(NEWLIB_NANO_INCLUDE_DIR),) | ||
| # newlib-nano overrides newlib.h and its include dir should therefore go before | ||
| # the regular system include dirs, but after the other newlib includes. | ||
| NEWLIB_INCLUDES += -isystem $(NEWLIB_NANO_INCLUDE_DIR) |
There was a problem hiding this comment.
Why is the nano include dir after the regular newlib dir? The comment does not make sense
| # Test if nano.specs is available | ||
| ifeq ($(shell $(LINK) -specs=nano.specs -E - 2>/dev/null >/dev/null </dev/null ; echo $$?),0) | ||
| USE_NEWLIB_NANO = 1 | ||
| # Clang/llvm has no support for specs files |
There was a problem hiding this comment.
Shouldn't the test above already filter out any compiler which errors on -specs?
There was a problem hiding this comment.
Even though it has no support for it, Clang does not fail when passed the -specs option. Adding -Werror makes sure Clang fails when the -specs flag is passed.
| # the current PATH and use a relative path for the includes | ||
| ifeq (,$(NEWLIB_INCLUDE_DIR)) | ||
| ifeq (,$(NEWLIB_INCLUDE_FILE)) | ||
| $(warning Using relative include dir) |
There was a problem hiding this comment.
Suggestion: change the message to "newlib.h not found, guessing newlib include path from gcc path"
|
@cgundogan Hi, No I have no experience with newlib-nano I checked with our compiler team and they said they have not tried nanospecs either, with the MIPS toolchain we do supply a SmallClib (-mclib=small) and TinyClib (-mclib=tiny) which are newlib derived if you need something smaller, but these are not the same as newlib-nano though. |
|
closing in favour of original PR #9216 |
Contribution description
based on #9216, but including fixes for cmake based external packages
Issues/PRs references