Skip to content

newlib: include fixes for newlib-nano#9394

Merged
smlng merged 1 commit intoRIOT-OS:masterfrom
cgundogan:pr/newlib_includes
Jun 26, 2018
Merged

newlib: include fixes for newlib-nano#9394
smlng merged 1 commit intoRIOT-OS:masterfrom
cgundogan:pr/newlib_includes

Conversation

@cgundogan
Copy link
Copy Markdown
Member

@cgundogan cgundogan commented Jun 21, 2018 •
edited
Loading

Contribution description

At least for Ubuntu newlib-nano seems to be in a different location
(/usr/include/newlib/nano/newlib.h).

This PR fixes #9381 by adding /usr/include to the newlib includes and searching for both newlib-nano (Arch et al.) and newlib/nano (Ubuntu et al.) explicitly.

Tested on Arch and Ubuntu.

EDIT: The major change in this PR is now to autodetect newlib in the compiler's default search paths, using:

$(PREFIX)gcc -v -x c -E /dev/null 2>&1 | grep '^\s' | tr -d '\n'

Issues/PRs references

#9381

@cgundogan cgundogan added Type: bug The issue reports a bug / The PR fixes a bug (including spelling errors) Area: build system Area: Build system labels Jun 21, 2018
@cgundogan cgundogan requested review from cladmi and smlng June 21, 2018 14:59
@cgundogan cgundogan force-pushed the pr/newlib_includes branch from d1205e2 to afcfe9b Compare June 21, 2018 14:59
@cgundogan cgundogan changed the title newlib: include fixes newlib: include fixes for newlib-nano Jun 21, 2018
@cgundogan cgundogan mentioned this pull request Jun 21, 2018
Closed
@cgundogan
Copy link
Copy Markdown
Member Author

cgundogan commented Jun 22, 2018

@miri64 could you also give it a try with your ubuntu box?

@smlng
Copy link
Copy Markdown
Member

smlng commented Jun 22, 2018

doesn't help on macOS

@miri64
Copy link
Copy Markdown
Member

miri64 commented Jun 22, 2018

Works for me.

@miri64
Copy link
Copy Markdown
Member

miri64 commented Jun 22, 2018

Isn't there some way to get this platform independent?

@cgundogan
Copy link
Copy Markdown
Member Author

cgundogan commented Jun 22, 2018

Isn't there some way to get this platform independent?

yup => CMake or some other meta build system to autoconfigure Makefiles

@miri64
Copy link
Copy Markdown
Member

miri64 commented Jun 22, 2018

Even CMake needs to collect this info from somewhere, as it isn't plugged into the OSs package management either. Isn't this information in pkg-config or something?

@jnohlgard
Copy link
Copy Markdown
Member

jnohlgard commented Jun 22, 2018

Newlib does not normally provide a pkgconfig file and is part of the toolchain for each platform, not part of the host system, so it is not so straightforward to integrate into pkgconfig etc. Especially since we also want to support local installations where the user extracts a pre-built toolchain tarball (usually vendor supplied) into some directory and simply adds it to the PATH environment variable.
Any major refactoring of the build system will need to consider all kinds of toolchain installations, on all platforms.

@miri64
Copy link
Copy Markdown
Member

miri64 commented Jun 22, 2018

Any major refactoring of the build system will need to consider all kinds of toolchain installations, on all platforms.

Fun! ;-)

@jnohlgard
Copy link
Copy Markdown
Member

jnohlgard commented Jun 22, 2018

CMake usually requires a toolchain file with all these settings for cross compilations https://cmake.org/cmake/help/latest/manual/cmake-toolchains.7.html#cross-compiling

@cgundogan cgundogan force-pushed the pr/newlib_includes branch from 873ec2b to c2715f0 Compare June 22, 2018 11:33
@cgundogan
Copy link
Copy Markdown
Member Author

cgundogan commented Jun 22, 2018

I am now querying gcc for its standard search paths for includes and there I search for newlib and newlib-nano. This seems to work for Arch and Ubuntu. @smlng wanna test on MAC?

@cgundogan
Copy link
Copy Markdown
Member Author

cgundogan commented Jun 22, 2018

@gebart AFAIK you are using gentoo, wanna give it a try there?

You can verify with BOARD=iotlab-m3 make info-build and looking at the INCLUDES variable, whether the correct newlib-nano is included

@cgundogan cgundogan added the CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR label Jun 22, 2018
@cgundogan
Copy link
Copy Markdown
Member Author

cgundogan commented Jun 22, 2018

doesn't help on macOS

in an offline discussion with @smlng we figured that the current proposed diff helps on macOS

@cgundogan cgundogan force-pushed the pr/newlib_includes branch from f752c21 to de53997 Compare June 23, 2018 07:49
@basilfx
Copy link
Copy Markdown
Member

basilfx commented Jun 24, 2018

I can also confirm it works on macOS (10.13.5).

@PeterKietzmann
Copy link
Copy Markdown
Member

PeterKietzmann commented Jun 25, 2018

Works on Ubuntu 18.04. Would be great to have this fix merged soon.

@smlng
Copy link
Copy Markdown
Member

smlng commented Jun 25, 2018

how does this integrate (or interfere) with #9216?

@cgundogan
Copy link
Copy Markdown
Member Author

cgundogan commented Jun 25, 2018

how does this integrate (or interfere) with #9216?

looking at the diffs, it's a different approach and incompatible.

IMO using the proposed autodetection is superior to #9216. I am not sure whether #9216 is doing more then just fixing the includes for newlib-nano (@toonst ?) If so, then we can merge efforts by applying our PRs (either @toonst PRs to mine, or the other way around).

@A-Paul
Copy link
Copy Markdown
Member

A-Paul commented Jun 25, 2018

Successful fixes compile error on:
Linux mobi31 4.9.0-6-amd64 #1 SMP Debian 4.9.88-1+deb9u1 (2018-05-07) x86_64 GNU/Linux

With make BOARD=samr21-xpro info-build I get:

INCLUDES:                                                 
        -isystem                                           
        /usr/lib/gcc/arm-none-eabi/5.4.1/../../../arm-none-eabi/include/nano
        -I/home/aar680/git/RIOT/core/include                
        -I/home/aar680/git/RIOT/drivers/include                          
        -I/home/aar680/git/RIOT/sys/include              
        -I/home/aar680/git/RIOT/cpu/samd21/include            
        -I/home/aar680/git/RIOT/boards/samr21-xpro/include 
        -I/home/aar680/git/RIOT/cpu/sam0_common/include          
        -I/home/aar680/git/RIOT/cpu/cortexm_common/include  
        -I/home/aar680/git/RIOT/cpu/cortexm_common/include/vendor
        -I/home/aar680/git/RIOT/sys/libc/include

@toonst
Copy link
Copy Markdown
Member

toonst commented Jun 26, 2018

Hi guys, sorry for the delay. @cgundogan I incorporated your search in the standard include paths of the compiler in #9216 (see this commit: cad68c0). I thought it would be too much work to rebase your work on top of mine, so I just copied some lines. Are you OK with this?

I think it is cleaner to use the nano.spec file whenever it is available, which this PR does not do.

@cgundogan
Copy link
Copy Markdown
Member Author

cgundogan commented Jun 26, 2018

Are you OK with this?

in general yes (:

However, AFAIK we wanted to introduce the new features step by step #9216 (comment)

The proposed change in this PR is rather small and easy to digest, before we take another step for the nano.specs. What do you think?

@cgundogan cgundogan force-pushed the pr/newlib_includes branch from 5db8263 to db6cb1a Compare June 26, 2018 13:46
@toonst
Copy link
Copy Markdown
Member

toonst commented Jun 26, 2018

Ok, agreed. This PR seems fine by me then.

This was referenced Jun 26, 2018
Closed
Closed
@cgundogan
Copy link
Copy Markdown
Member Author

cgundogan commented Jun 26, 2018

Okay, if nobody objects, how about we merge it soonish?

@kYc0o
Copy link
Copy Markdown
Contributor

kYc0o commented Jun 26, 2018

Works for me in OS X.

kYc0o
kYc0o approved these changes Jun 26, 2018
View reviewed changes
Copy link
Copy Markdown
Contributor

@kYc0o kYc0o left a comment

Choose a reason for hiding this comment

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

Tested ACK.

@kYc0o
Copy link
Copy Markdown
Contributor

kYc0o commented Jun 26, 2018

Everything ok to merge then?

@smlng smlng merged commit d0a3817 into RIOT-OS:master Jun 26, 2018
@cgundogan cgundogan deleted the pr/newlib_includes branch June 26, 2018 17:35
@cladmi cladmi mentioned this pull request Jun 28, 2018
Closed
This was referenced Jul 6, 2018
Merged
Draft
@attdona attdona mentioned this pull request Jul 9, 2018
Closed
@cladmi cladmi added this to the Release 2018.07 milestone Jul 10, 2018
@cladmi cladmi mentioned this pull request Aug 2, 2018
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@kYc0o kYc0o kYc0o approved these changes

@cladmi cladmi Awaiting requested review from cladmi

@smlng smlng Awaiting requested review from smlng

Assignees

@cladmi cladmi

@smlng smlng

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 Type: bug The issue reports a bug / The PR fixes a bug (including spelling errors)

Projects

None yet

Milestone

Release 2018.07

Development

Successfully merging this pull request may close these issues.

newlib-nano: NEWLIB_INCLUDE_DIR set incorrectly

10 participants

@cgundogan @smlng @miri64 @jnohlgard @basilfx @PeterKietzmann @A-Paul @toonst @kYc0o @cladmi