Skip to content

newlib: support nano specs#9415

Closed
toonst wants to merge 1 commit intoRIOT-OS:masterfrom
OTAkeys:pr/newlib_support_nano_specs
Closed

newlib: support nano specs#9415
toonst wants to merge 1 commit intoRIOT-OS:masterfrom
OTAkeys:pr/newlib_support_nano_specs

Conversation

@toonst
Copy link
Copy Markdown
Member

@toonst toonst commented Jun 26, 2018

  • When using newlib_nano and a nano.specs file is present and the compiler supports it, we use this file for letting the linker find the right include paths. If not, we manually try to find the right newlib (nano) paths.
  • When testing for the nano.specs file, we add -Werror so compilers which don't support .specs files give the correct output.
  • When none of the searched paths contain newlib.h, compilation fails.

I'm still unsure about the following points:

  • When deriving the newlib include path from predefined directories or from the compiler include path, we print a warning. --> should be silent maybe?
  • When using newlib_nano is specified, and we can not find a newlib nano directory, we print a warning that the regular newlib is used instead. --> should fail instead maybe?

Issues/PRs references

Based on @cgundogan 's #9394
Cleanup of #9216

@toonst toonst mentioned this pull request Jun 26, 2018
Closed
newlib: use nano.specs file when possible
c1795e5 
When using newlib_nano and a nano.specs file is present and the compiler
supports it, we use this file for letting the linker find the right
include paths. If not, we manually try to find the right newlib (nano)
paths.

When testing for the nano.specs file, we add -Werror so compilers which
don't support .specs files give the correct output.

When deriving the newlib include path from predefined directories or
from the compiler include path, we print a warning.

When none of the paths contain newlib.h, compilation fails.

When using newlib_nano is specified, and we can not find a newlib nano
directory, we print a warning that the regular newlib is used instead.
@toonst toonst force-pushed the pr/newlib_support_nano_specs branch from cef4189 to c1795e5 Compare June 27, 2018 08:28
@tcschmidt tcschmidt requested a review from cgundogan June 27, 2018 18:45
@cladmi cladmi mentioned this pull request Jun 29, 2018
Closed
cgundogan
cgundogan approved these changes Jul 2, 2018
View reviewed changes
Copy link
Copy Markdown
Member

@cgundogan cgundogan left a comment

Choose a reason for hiding this comment

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

Works fine for me. After applying this patch, my INCLUDES does not contain any path to newlib-nano, but LINKFLAGS and CFLAGS do contain specs=nano.specs. The binary size for default with samr21-xpro stays the same. I really want @smlng to check this for OSX before we merge, though.

@cgundogan cgundogan added CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR Type: enhancement The issue suggests enhanceable parts / The PR enhances parts of the codebase / documentation Area: build system Area: Build system labels Jul 2, 2018
@cgundogan
Copy link
Copy Markdown
Member

cgundogan commented Jul 2, 2018

we probably can get rid of the predefined search paths (in the near future). I don't think they are necessary anymore..

@cgundogan cgundogan mentioned this pull request Jul 2, 2018
Closed
@smlng
Copy link
Copy Markdown
Member

smlng commented Jul 3, 2018

FYI: works on macOS

cladmi
cladmi suggested changes Jul 6, 2018
View reviewed changes
Copy link
Copy Markdown
Contributor

@cladmi cladmi left a comment

Choose a reason for hiding this comment

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

For me this PR still does too many things at once, with some things that still need discussions and even more in the same commit.

  • add support for nano.specs
  • error if NEWLIB_INCLUDE_DIR is not found even if NEWLIB_NANO is not used
  • fix the NEWLIB_INCLUDE_DIR missing trailing slash
  • moves "NEWLIB_INCLUDES" out of the llvm conditional
  • Fix llvm + newlib nano not finding nano headers

You should add nano.specs alone, either the other changes which need a lot review will block the PR.

You can keep a global work in progress PR with "all I would like to have" and to split PRs for things easier to merge and review alone.

@cladmi cladmi added the Process: API change Integration Process: PR contains or issue proposes an API change. Should be handled with care. label Jul 6, 2018
@cladmi
Copy link
Copy Markdown
Contributor

cladmi commented Jul 6, 2018

I added the "API Change" label for the side effects.

@cladmi cladmi mentioned this pull request Jul 6, 2018
Draft
4 tasks
cladmi pushed a commit to cladmi/RIOT that referenced this pull request Jul 6, 2018
@cladmi
newlib: use addprefix to declare all variables
62eb720 
Taken out of RIOT-OS#9415
cladmi pushed a commit to cladmi/RIOT that referenced this pull request Jul 6, 2018
@cladmi
newlib.mk: use addprefix to declare all variables
9d36929 
Taken out of RIOT-OS#9415
cladmi pushed a commit to cladmi/RIOT that referenced this pull request Jul 20, 2018
@cladmi
newlib.mk: use addprefix to declare all variables
48717f8 
Taken out of RIOT-OS#9415
cladmi pushed a commit to cladmi/RIOT that referenced this pull request Jul 25, 2018
@cladmi
newlib.mk: use addprefix to declare all variables
7c69cb2 
Taken out of RIOT-OS#9415
cladmi pushed a commit to cladmi/RIOT that referenced this pull request Aug 2, 2018
@cladmi
newlib.mk: use addprefix to declare all variables
06b72cc 
Taken out of RIOT-OS#9415
cladmi pushed a commit to cladmi/RIOT that referenced this pull request Oct 17, 2018
@cladmi
newlib.mk: use addprefix to declare all variables
f5e859f 
Taken out of RIOT-OS#9415
@stale
Copy link
Copy Markdown

stale bot commented Aug 10, 2019

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. If you want me to ignore this issue, please mark it with the "State: don't stale" label. Thank you for your contributions.

@stale stale bot added the State: stale State: The issue / PR has no activity for >185 days label Aug 10, 2019
@stale stale bot closed this Sep 10, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@cgundogan cgundogan cgundogan approved these changes

+1 more reviewer

@cladmi cladmi cladmi requested changes

Reviewers whose approvals may not affect merge requirements

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 Process: API change Integration Process: PR contains or issue proposes an API change. Should be handled with care. State: stale State: The issue / PR has no activity for >185 days Type: enhancement The issue suggests enhanceable parts / The PR enhances parts of the codebase / documentation

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@toonst @cgundogan @smlng @cladmi