Skip to content

newlib-nano: add include directory#9080

Merged
jnohlgard merged 1 commit intoRIOT-OS:masterfrom
OTAkeys:pr/newlib_incl_dir
May 27, 2018
Merged

newlib-nano: add include directory#9080
jnohlgard merged 1 commit intoRIOT-OS:masterfrom
OTAkeys:pr/newlib_incl_dir

Conversation

@toonst
Copy link
Copy Markdown
Member

@toonst toonst commented May 4, 2018

Contribution description

In libnewlib-arm-none-eabi version 2.4.0.20160527-3 the include directory is called nano iso newlib-nano. This fix checks for the existence of either directories and adds it to the include path.

newlib-nano: add include directory
5c9a523 
The newlib-nano include directory can be named either newlib-nano or
nano, so we should make sure it is added in both cases.
jnohlgard
jnohlgard reviewed May 4, 2018
View reviewed changes
Copy link
Copy Markdown
Member

@jnohlgard jnohlgard left a comment

Choose a reason for hiding this comment

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

Looks good! Untested ACK

@jnohlgard jnohlgard added Area: build system Area: Build system Impact: minor The PR is small in size and might only require a quick look of a knowledgeable reviewer CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR labels May 4, 2018
@jnohlgard jnohlgard added this to the Release 2018.07 milestone May 4, 2018
@vincent-d vincent-d requested a review from kaspar030 May 7, 2018 09:31
@kaspar030 kaspar030 added CI: run tests If set, CI server will run tests on hardware for the labeled PR 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 7, 2018
@vincent-d vincent-d added CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR and removed CI: run tests If set, CI server will run tests on hardware for the labeled PR CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR labels May 22, 2018
@tcschmidt
Copy link
Copy Markdown
Member

tcschmidt commented May 26, 2018

@gebart @kaspar030 ready for approval?

jnohlgard
jnohlgard approved these changes May 27, 2018
View reviewed changes
Copy link
Copy Markdown
Member

@jnohlgard jnohlgard left a comment

Choose a reason for hiding this comment

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

Looks fine. ACK

@jnohlgard jnohlgard merged commit 88aaa62 into RIOT-OS:master May 27, 2018
@kYc0o
Copy link
Copy Markdown
Contributor

kYc0o commented May 28, 2018

I'm sorry to come until now, but this PR broke compilation for ARM platforms on OS X. I'll investigate for a solution.

@kYc0o
Copy link
Copy Markdown
Contributor

kYc0o commented May 28, 2018

@smlng @emmanuelsearch can you verify just to check if it's not only my local setup?

@jcarrano
Copy link
Copy Markdown
Contributor

jcarrano commented May 28, 2018

This PR broke compilation in ArchLinux. As far as I can tell, it's not really the fault of this change, but rather the newlib package in Arch is not installing the nano directory properly (see https://bugs.archlinux.org/task/50481 reported by someone at OTAkeys). @kYc0o Maybe in OSX there is a similar problem.

As a workaround, can we fall back to what we were doing previousl if neither the "nano" or "newlib-nano" directories are found?

@kYc0o kYc0o mentioned this pull request May 28, 2018
Closed
@kYc0o
Copy link
Copy Markdown
Contributor

kYc0o commented May 28, 2018

@jcarrano please report your output in #9214 .

@jcarrano jcarrano mentioned this pull request May 28, 2018
Closed
@toonst toonst mentioned this pull request May 28, 2018
Closed
@toonst
Copy link
Copy Markdown
Member Author

toonst commented May 28, 2018

I found a more elegant way to fix the issue. See #9216.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@jnohlgard jnohlgard jnohlgard approved these changes

@kaspar030 kaspar030 Awaiting requested review from kaspar030

Assignees

No one assigned

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 Impact: minor The PR is small in size and might only require a quick look of a knowledgeable reviewer

Projects

None yet

Milestone

Release 2018.07

Development

Successfully merging this pull request may close these issues.

7 participants

@toonst @tcschmidt @kYc0o @jcarrano @jnohlgard @vincent-d @kaspar030