Skip to content

makefile/..picolibc: make missing picolibc fail louder#16008

Merged
benpicco merged 2 commits intoRIOT-OS:masterfrom
kfessel:p-make-picolib-failure
Feb 23, 2021
Merged

makefile/..picolibc: make missing picolibc fail louder#16008
benpicco merged 2 commits intoRIOT-OS:masterfrom
kfessel:p-make-picolib-failure

Conversation

@kfessel
Copy link
Contributor

@kfessel kfessel commented Feb 15, 2021

Contribution description

missing picolibc auto adaptation needs changes in make-system and is therefor on hold this PR make the build fail early and loud and needs basically no modification of unrelated make-system parts.

Testing procedure

Build something that prefers or requires picolibc on a system that does not have it (installed) (e.g.: current Ubuntu LTS 20.04)
e.g.: RIOT/examples/gnrc_minimal for a cortex-m Board

before u get a failure somthing like:

"make" -C <Riot>/sys/picolibc_syscalls_default
<Riot>/sys/picolibc_syscalls_default/syscalls.c:236:5: error: implicit declaration of function 'FDEV_SETUP_STREAM' [-Werror=implicit-function-declaration]
  236 |     FDEV_SETUP_STREAM(picolibc_put, picolibc_get, picolibc_flush, _FDEV_SETUP_RW);
      |     ^~~~~~~~~~~~~~~~~
<Riot>/sys/picolibc_syscalls_default/syscalls.c:236:67: error: '_FDEV_SETUP_RW' undeclared here (not in a function)
  236 |     FDEV_SETUP_STREAM(picolibc_put, picolibc_get, picolibc_flush, _FDEV_SETUP_RW);
      |                                                                   ^~~~~~~~~~~~~~
cc1: all warnings being treated as errors

somewhere in the build process

after the patch you get:

<Riot>/makefiles/libc/picolibc.mk:10: picolib was selected to be build but no picolib.spec could be found
<Riot>/makefiles/libc/picolibc.mk:11: *** check your installation or build configuration.. Stop.

right at the start

Issues/PRs references

PR.: to fix the selection process (need make-system reordering): #15993
Issue about failed selection process: #15325
Issue about the criticality of order of inclusion: #9913

@maribu, @fjmolinas might want to have a look at this

@maribu
Copy link
Member

maribu commented Feb 15, 2021

Maybe a more concrete help message? In the end, it comes down to these choices:

  1. install picolibc for $(TARGET_ARCH)
  2. add FEATURES_REQUIRED += newlib to the Makefile
  3. Run export FEATURES_REQUIRED=newlibc

@kfessel
Copy link
Contributor Author

kfessel commented Feb 15, 2021

I am all in for improvements to the wording of the message. The current message is short and technical but maybe less helpful.

But the preference may have been done at so many places that i wasn't sure how to word an instant help like remove 'Feature_Optional += picolib ' from every module in use or Force newlib by using Feature Required

adding FEATURES_REQUIRED += newlib to the Makefile disables native build

installing picolibc is impossible for Ubuntu LTS (20.04) (the reason why i had to dist-upgrade the other day since i wanted to check my other PR)

adding FEATURES_REQUIRED += newlib to the Makefile/export still does not work for the gnrc_minimal example

but you get an really beautyfull warning message from #15973 (not realy but ii dont know better):

The following features may conflict: newlib picolibc
Rationale: Only one standard C library can be used

EXPECT undesired behaviour!

@maribu
Copy link
Member

maribu commented Feb 15, 2021

An alternative approach would be to go for FEATURES_BLACKLIST += picolibc instead. But I think the build system shouldn't consider optional features that would end up in a conflict for usage. A fix is PR'ed here: #16012

@kfessel
Copy link
Contributor Author

kfessel commented Feb 15, 2021

i knew i saw some PR by you regarding that dependency issue

@kfessel
Copy link
Contributor Author

kfessel commented Feb 15, 2021

I will add that suggestion to the PR

@kfessel kfessel force-pushed the p-make-picolib-failure branch from ebef4b2 to b9226d5 Compare February 15, 2021 13:09
@kfessel
Copy link
Contributor Author

kfessel commented Feb 15, 2021

added sugestions and rebased on current master to make them work as expected.

current warning words are:

<Riot>/makefiles/libc/picolibc.mk:10: picolibc was selected to be build but no picolibc.spec could be found
<Riot>/makefiles/libc/picolibc.mk:11: you might want to install "picolibc" for "arm-none-eabi"
<Riot>/makefiles/libc/picolibc.mk:12: or add "FEATURES_BLACKLIST += picolibc" to Makefile
<Riot>/makefiles/libc/picolibc.mk:13: *** check your installation or build configuration.. Stop.

i think this is ready for ci

@kfessel kfessel force-pushed the p-make-picolib-failure branch from b9226d5 to b0e3d08 Compare February 15, 2021 13:31
@kfessel kfessel force-pushed the p-make-picolib-failure branch from b0e3d08 to f7a8e08 Compare February 15, 2021 13:40
@kfessel
Copy link
Contributor Author

kfessel commented Feb 15, 2021

I force pushed some "c"s to the picolibc

@kfessel kfessel changed the title makefile/..picolib: make missing picolib fail louder makefile/..picolibc: make missing picolibc fail louder Feb 15, 2021
@benpicco benpicco 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 Type: enhancement The issue suggests enhanceable parts / The PR enhances parts of the codebase / documentation labels Feb 23, 2021
@benpicco benpicco merged commit 34d7d23 into RIOT-OS:master Feb 23, 2021
@kfessel kfessel deleted the p-make-picolib-failure branch February 25, 2021 13:33
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 Impact: minor The PR is small in size and might only require a quick look of a knowledgeable reviewer Type: enhancement The issue suggests enhanceable parts / The PR enhances parts of the codebase / documentation

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants