Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Makefile: Newlib: Added pattern to NEWLIB_INCLUDE_PATTERN #5131

Merged
merged 1 commit into from
Apr 1, 2016

Conversation

jia200x
Copy link
Member

@jia200x jia200x commented Mar 22, 2016

Hi

Some arm toolchains in OSX are installed in /usr/local/opt/gcc-arm/$TARGET_ARCH
When the current Makefile tries to search for the location of include files, it doesn't find them and the variable NEWLIB_INCLUDE_DIR is blank.

In the end, this produces errors like:

_Building application "default" for "samr21-xpro" with MCU "samd21".

/Users/jia200x/Development/RIOT/examples/default/main.c:28:20: fatal error: thread.h: No such file or directory
#include "thread.h"
^
compilation terminated.
make[1]: *** [/Users/jia200x/Development/RIOT/examples/default/bin/samr21-xpro/default/main.o] Error 1
make: *** [all] Error 2_

I added the /usr/local/opt/gcc-arm/$TARGET_ARCH pattern to Makefile.

Also, do you think there should be an error message or something if newlib dirs are not found?
Best regards

@OlegHahm OlegHahm added this to the Release 2016.04 milestone Mar 22, 2016
@OlegHahm OlegHahm added Area: build system Area: Build system OS: Mac OS X Host OS: This PR/issue concerns usage of RIOT with Mac OS X as a host system labels Mar 22, 2016
NEWLIB_INCLUDE_PATTERNS ?= \
/etc/alternatives/gcc-$(TARGET_ARCH)-include \
/usr/$(TARGET_ARCH)/include \
/usr/local/opt/$(TARGET_ARCH)*/$(TARGET_ARCH)/include \
/usr/local/opt/gcc-arm/$(TARGET_ARCH)/include \
Copy link
Contributor

Choose a reason for hiding this comment

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

Probably this pattern is not only used for ARM, so how about gcc-* instead of gcc-arm?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, you're right.
I will commit again.

Thanks

@cgundogan cgundogan added the CI: needs squashing Commits in this PR need to be squashed; If set, CI systems will mark this PR as unmergable label Mar 23, 2016
@cgundogan
Copy link
Member

@kYc0o can you also verify this on your OSX machine?

@kYc0o
Copy link
Contributor

kYc0o commented Mar 24, 2016

I didn't experienced the same problem and my $PATH for my toolchain is set differently. For some reason my machine is able to find newlib without this change...

Anyways, I don't think it's a good idea to put all "possible" paths on this file and if it's not found to throw a compilation error without any insights.

@cgundogan your toolchain's path coincide with one of those that appear on the file?

@jia200x
Copy link
Member Author

jia200x commented Mar 24, 2016

Hi again

I think this is not happening on every OS X. It seems it's not a typical PATH.
Anyway in case of not finding NEWLIB include files, the NEWLIB_INCLUDE variable will be only "-isystem". That causes the whole compilation to crash due to not finding some header files (And was not easy to trace the root of the crash).

For me this started to happen in commit 2b51e4b

With the older version of this file, my NEWLIB_INCLUDES variable is blank. With the current version, NEWLIB_INCLUDES is "-isystem /nano -isystem", so INCLUDES variable is:

-isystem /nano -isystem -I/Users/jia200x/Development/RIOT/core/include ..._

The second -isystem causes the crash, for testing I tried to set NEWLIB_INCLUDES variable to "-isystem /nano" and compiles without problems.

I think one way to fix this could be to add something like:
ifneq(,NEWLIB_INCLUDE_DIR)
NEWLIB_INCLUDES := -isystem $(NEWLIB_INCLUDE_DIR)
endif
So that way you won't have an -isystem without argument.

Does it seem reasonable?

Also, I noticed my compile process was not including newlib header files. Are they mandatory for RIOT?
Thank you

@cgundogan
Copy link
Member

@jia200x can you squash the commits? The change looks sane and it should not break the previous behaviour. So I'd say ACK

@jia200x
Copy link
Member Author

jia200x commented Apr 1, 2016

Squashed. Thank you.

Cheers!

@cgundogan cgundogan 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 Apr 1, 2016
@cgundogan
Copy link
Member

@kYc0o I don't think that we will fill up the include path with many different possible paths. I can imagine that different installation procedures will reflect differently on the filesystem. The same could happen due to a different version of OSX, or maybe because the maintainer of the gcc-arm package (I assume such a thing exists on OSX) has decided to change the path.

As long as nobody adds a path to a user defined directoty (~/my-gcc-arm/), I am fine with merging this.

@@ -28,11 +28,12 @@ export LINKFLAGS += -lc -lnosys
# Ubuntu seem to put a copy of the newlib headers in the same place as
# Gentoo crossdev, but we prefer to look at /etc/alternatives first.
# On OSX, newlib includes are possibly located in
# /usr/local/opt/arm-none-eabi*/arm-none-eabi/include
# /usr/local/opt/arm-none-eabi*/arm-none-eabi/include or /local/opt/gcc-arm/arm-none-eabi/include
Copy link
Member

Choose a reason for hiding this comment

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

you forgot the /usr here

@kYc0o
Copy link
Contributor

kYc0o commented Apr 1, 2016

@cgundogan agree.

@jia200x
Copy link
Member Author

jia200x commented Apr 1, 2016

Oops. Just fixed.

@cgundogan cgundogan merged commit 909c018 into RIOT-OS:master Apr 1, 2016
@cgundogan cgundogan added the Type: bug The issue reports a bug / The PR fixes a bug (including spelling errors) label Apr 1, 2016
@jia200x
Copy link
Member Author

jia200x commented Apr 1, 2016

Cool!
Thank you

@jia200x jia200x deleted the newlib_makefile_fix branch April 1, 2016 13:51
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 OS: Mac OS X Host OS: This PR/issue concerns usage of RIOT with Mac OS X as a host system Type: bug The issue reports a bug / The PR fixes a bug (including spelling errors)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants