makefiles/feature_precheck: check libc availability prior deciding usage by kfessel · Pull Request #15993 · RIOT-OS/RIOT

kfessel · 2021-02-12T12:22:56Z

Contribution description

Add a feature pre-check for picolibc and newlibc into the make process

Testing procedure

make things with either having or not having picolibc or newlib installed

Issues/PRs references

Fixes #15325

@maribu , @nmeum , @benpicco : might what to have a look at it

maribu

this looks much less scary that I had imagined :-)

maribu · 2021-02-12T12:29:06Z

makefiles/features_precheck.inc.mk

@@ -0,0 +1,17 @@
+# Check if provided features are available
+
+ifneq (,$(filter picolibc,$(FEATURES_PROVIDED)) $(filter newlib,$(FEATURES_PROVIDED)))


The outer if can be dropped, can't it?

No it can (i wanted to avoid the rest of the check if not nesecary but the shell checks a only done if the feature is provided).

I will remove it

maribu · 2021-02-12T12:31:26Z

makefiles/features_precheck.inc.mk

+# Check if provided features are available
+
+ifneq (,$(filter picolibc,$(FEATURES_PROVIDED)) $(filter newlib,$(FEATURES_PROVIDED)))
+  ifneq (,$(filter picolibc,$(FEATURES_PROVIDED)))


You need to white-list this file in dist/tools/buildsystem_sanity_check/check.sh:75. Please also update the comment in the line above.

You need to white-list this file in dist/tools/buildsystem_sanity_check/check.sh:75. Please also update the comment in the line above.

Huh? This completely bypasses the intention of the check...

No, this was added when FEAUTES_OPTIONAL was added to replace the paddern

ifneq (,$(filter foo,$(FEAUTRES_PROVIDES))) USEMODULE += optional_submodule endif

The intention was to enforce the use of FEATURES_OPTIONAL instead. But this here is unrelated.

No, this was added when FEAUTES_OPTIONAL was added to replace the paddern

ifneq (,$(filter foo,$(FEAUTRES_PROVIDES))) USEMODULE += optional_submodule endif

The intention was to enforce the use of FEATURES_OPTIONAL instead. But this here is unrelated.

I'm not sure I understand, FEATURES_OPTIONAL should not be checked either, only FEATURES_USED. The buildsystem check is there so that there are no conditionals against FEATURES_OPTIONAL, FEATURES_PROVIDED, FETURES_REQUIRED, see original or #10179 and

RIOT/dist/tools/buildsystem_sanity_check/check.sh

Lines 56 to 75 in 3a9f8d5

# Modules should not check the content of FEATURES_PROVIDED/_REQUIRED/OPTIONAL

# Handling specific behaviors/dependencies should by checking the content of:

# * `USEMODULE`

# * maybe `FEATURES_USED` if it is not a module (== not a periph_)

check_not_parsing_features() {

local patterns=()

local pathspec=()

patterns+=(-e 'if.*filter.*FEATURES_PROVIDED')

patterns+=(-e 'if.*filter.*FEATURES_REQUIRED')

patterns+=(-e 'if.*filter.*FEATURES_OPTIONAL')

# Pathspec with exclude should start by an inclusive pathspec in git 2.7.4

pathspec+=('*')

# Ignore this file when matching as it self matches

pathspec+=(":!${SCRIPT_PATH}")

# These two files contain sanity checks using FEATURES_ so are allowed

pathspec+=(':!Makefile.include' ':!makefiles/info-global.inc.mk')

, am I looking at the wrong line?

maribu · 2021-02-12T12:35:38Z

makefiles/features_precheck.inc.mk

+  ifneq (,$(filter picolibc,$(FEATURES_PROVIDED)))
+    # Test if picolibc.specs is available
+    ifneq ($(shell $(LINK) -specs=picolibc.specs -E - 2>/dev/null >/dev/null </dev/null ; echo $$?),0)
+      FEATURES_PROVIDED := $(filter-out picolibc,$(FEATURES_PROVIDED))


Maybe just use FEATURES_BLACKLIST? That way it would be more obvious that the feature is not provided due to the setup, rather than due to missing upstream support.

seem reasonable i test if it keeps working and change

BLACKLISTING does not work as intended

as i just saw you also fixed the Blacklist problem i had in #15973

fjmolinas

NACK, didn't look at the other changes yet, but I'm against moving the include order.

fjmolinas · 2021-02-12T13:23:57Z

Makefile.include

+# Include Board and CPU configuration
+INCLUDES += $(addprefix -I,$(wildcard $(BOARDDIR)/include))
+include $(BOARDDIR)/Makefile.include
+INCLUDES += -I$(RIOTCPU)/$(CPU)/include
+include $(RIOTCPU)/$(CPU)/Makefile.include
+
+# Import all toolchain settings
+include $(RIOTMAKE)/toolchain/$(TOOLCHAIN).inc.mk


This is reverting a long effort of changing the order of inclusion: #9913

we can also have a error message telling the user that he has to blacklist the provided but not exiting toolchain that the dependency solver prefered for a reason unknown to the user (maybe it was listed first somewhere, or it is alphanumerical first)

fjmolinas · 2021-02-12T13:25:03Z

makefiles/features_precheck.inc.mk

+  ifneq (,$(filter picolibc,$(FEATURES_PROVIDED)))
+    # Test if picolibc.specs is available
+    ifneq ($(shell $(LINK) -specs=picolibc.specs -E - 2>/dev/null >/dev/null </dev/null ; echo $$?),0)
+      FEATURES_PROVIDED := $(filter-out picolibc,$(FEATURES_PROVIDED))


This is not declarative, FEATURES so far are declarative.

I think: I fixed that with blacklisting as @maribu suggested

fjmolinas · 2021-02-12T13:27:46Z

NACK, didn't look at the other changes yet, but I'm against moving the include order.

@kfessel can you explain the reasoning for this change better?

fjmolinas · 2021-02-12T13:30:41Z

Makefile.include

 # provide this macro
 TOOLCHAINS_SUPPORTED ?= gnu
-# Import all toolchain settings
-include $(RIOTMAKE)/toolchain/$(TOOLCHAIN).inc.mk


Isn't this is the only thing that needs to be moved?

No, we need access to the specific toolchain therefore the cpu prefix definition has to move up
(can't ask x86 gcc if picolibc for arm is available)

fjmolinas · 2021-02-12T15:51:12Z

makefiles/features_precheck.inc.mk

+ifneq (,$(filter picolibc,$(FEATURES_PROVIDED)))
+  # Test if picolibc.specs is available
+  ifneq ($(shell $(LINK) -specs=picolibc.specs -E - 2>/dev/null >/dev/null </dev/null ; echo $$?),0)
+    FEATURES_BLACKLIST += picolibc
+ endif
+endif


Why is there a need to check if the FEATURE is provided? Can't you simply blacklist if not present?

as you guessed it is to avoid the shell call, if not need, and why should we auto blacklist something no one is talking about.

This is what I mean BTW:

RIOT/Makefile.include

Lines 176 to 179 in a5b9136

# UNAME is always needed so use simple variable expansion so only evaluated once

UNAME := $(shell uname -m -s)

OS = $(word 1, $(UNAME))

OS_ARCH = $(word 2, $(UNAME))

, just assign it once, if we want to move towards enabling picolibc most of the time anyway, we will want to call this at least once.

fjmolinas · 2021-02-12T15:54:02Z

makefiles/features_precheck.inc.mk

+ifneq (,$(filter picolibc,$(FEATURES_PROVIDED)))
+  # Test if picolibc.specs is available
+  ifneq ($(shell $(LINK) -specs=picolibc.specs -E - 2>/dev/null >/dev/null </dev/null ; echo $$?),0)
+    FEATURES_BLACKLIST += picolibc
+ endif
+endif


Suggested change

ifneq (,$(filter picolibc,$(FEATURES_PROVIDED)))

# Test if picolibc.specs is available

ifneq ($(shell $(LINK) -specs=picolibc.specs -E - 2>/dev/null >/dev/null </dev/null ; echo $$?),0)

FEATURES_BLACKLIST += picolibc

endif

endif

# Test if picolibc.specs is available

ifneq ($(shell $(LINK) -specs=picolibc.specs -E - 2>/dev/null >/dev/null </dev/null ; echo $$?),0)

FEATURES_BLACKLIST += picolibc

endif

Is it as it is now to avoid the shell call? In that case, I would assign it to a variable once and the check that variable, similar to what is done with OS.

maybe we should but i do not see a huge performance penalty atm.
I will have a look at OS.

So this can be used to opportunistically enable picolibc and fall back to newlib if it's not installed?

Shouldn't the first step be testing it as the default libc in the ci?

kfessel · 2021-02-23T16:24:16Z

will rebase to solve for conflicting change in master

stale · 2022-03-02T13:18:33Z

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.

benpicco · 2022-03-02T15:17:50Z

So this can be used to opportunistically enable picolibc and fall back to newlib if it's not installed?

kfessel · 2022-03-02T17:00:07Z

So this can be used to opportunistically enable picolibc and fall back to newlib if it's not installed?

yes this was the intention

the stall:bot is digging deep :nice:

without picolibc-arm... on the system:

examples/gnrc_minimal$ BOARD=nucleo-f767zi make
Building application "gnrc_minimal" for "nucleo-f767zi" with MCU "stm32".

picolibc was selected to be build but no picolibc.spec could be found
you might want to install picolibc for arm-none-eabi
or add FEATURES_BLACKLIST += picolibc to Makefile)
check your installation or build configuration.
make: *** [../RIOT-master/makefiles/libc/picolibc.mk:21: _missing-picolibc] Error 1

since #16008 got merged is at least failing loud providing a kinda fix

with this PR it would just fallback to use newlibc

benpicco · 2022-03-02T17:28:59Z

Can you give it a rebase?

fjmolinas · 2022-03-02T20:01:27Z

My change requests still stand here.

fjmolinas · 2022-03-02T20:42:28Z

Thinking about this again I'm still against it, if we change to have picolibc enabled by default then picolibc would become a dependency of RIOT, we don't blacklist native because libcan is not there, either install it or don't use it.

If we move to picolibc as default then what we can do is check if it's installed and if not add a warning that it should be installed or FEATURES_REQUIRED += newlib added to their application Makefile or RIOT_MAKEFILES_GLOBAL_PRE. If you don't want to install everything there is always the docker path.

Features so far are all declarative, I do not see the need to change because of picolibc, neither do I think its a good idea.

stale · 2022-09-21T05:51:18Z

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.

maribu reviewed Feb 12, 2021

View reviewed changes

maribu requested review from aabadie, benpicco and fjmolinas February 12, 2021 12:32

maribu added Area: build system Area: Build system Type: enhancement The issue suggests enhanceable parts / The PR enhances parts of the codebase / documentation labels Feb 12, 2021

maribu reviewed Feb 12, 2021

View reviewed changes

fjmolinas requested changes Feb 12, 2021

View reviewed changes

fjmolinas reviewed Feb 12, 2021

View reviewed changes

maribu mentioned this pull request Feb 12, 2021

treewide: clean up architecture specific toolchain setup #15996

Closed

This was referenced Feb 15, 2021

makefiles: FEATURE_OPTIONAL picolib defaults picolib even if that is not installed -> examples/gnrc_minimal: fail building for cortex without picolib #15325

Closed

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

Merged

kfessel force-pushed the p-feature-precheck branch from 1d9b00b to 94bb72d Compare February 23, 2021 16:48

MrKevinWeiss added this to the Release 2021.07 milestone Jun 21, 2021

MrKevinWeiss removed this from the Release 2021.07 milestone Jul 15, 2021

stale bot added the State: stale State: The issue / PR has no activity for >185 days label Mar 2, 2022

stale bot removed the State: stale State: The issue / PR has no activity for >185 days label Mar 2, 2022

stale bot added the State: stale State: The issue / PR has no activity for >185 days label Sep 21, 2022

kfessel added State: don't stale State: Tell state-bot to ignore this issue and removed State: stale State: The issue / PR has no activity for >185 days labels Sep 23, 2022

kfessel added 2 commits September 23, 2022 14:52

makefiles/feature_precheck: check libc availability prior deciding usage

1b0c69b

Blacklist non available, Fix: select optional features only from usable

29d5dfe

kfessel force-pushed the p-feature-precheck branch from 94bb72d to 29d5dfe Compare September 23, 2022 12:53

github-actions bot added the Area: tools Area: Supplementary tools label Sep 23, 2022

		@@ -0,0 +1,17 @@
		# Check if provided features are available

		ifneq (,$(filter picolibc,$(FEATURES_PROVIDED)) $(filter newlib,$(FEATURES_PROVIDED)))

	# Modules should not check the content of FEATURES_PROVIDED/_REQUIRED/OPTIONAL
	# Handling specific behaviors/dependencies should by checking the content of:
	# * `USEMODULE`
	# * maybe `FEATURES_USED` if it is not a module (== not a periph_)
	check_not_parsing_features() {
	local patterns=()
	local pathspec=()

	patterns+=(-e 'if.filter.FEATURES_PROVIDED')
	patterns+=(-e 'if.filter.FEATURES_REQUIRED')
	patterns+=(-e 'if.filter.FEATURES_OPTIONAL')

	# Pathspec with exclude should start by an inclusive pathspec in git 2.7.4
	pathspec+=('*')

	# Ignore this file when matching as it self matches
	pathspec+=(":!${SCRIPT_PATH}")

	# These two files contain sanity checks using FEATURES_ so are allowed
	pathspec+=(':!Makefile.include' ':!makefiles/info-global.inc.mk')

	# UNAME is always needed so use simple variable expansion so only evaluated once
	UNAME := $(shell uname -m -s)
	OS = $(word 1, $(UNAME))
	OS_ARCH = $(word 2, $(UNAME))

Conversation

kfessel commented Feb 12, 2021

Contribution description

Testing procedure

Issues/PRs references

Uh oh!

maribu left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kfessel Feb 12, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

fjmolinas left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kfessel Feb 12, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kfessel Feb 12, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

fjmolinas commented Feb 12, 2021

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kfessel Feb 12, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kfessel Feb 12, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kfessel commented Feb 23, 2021

Uh oh!

stale bot commented Mar 2, 2022

Uh oh!

benpicco commented Mar 2, 2022

Uh oh!

kfessel commented Mar 2, 2022

Uh oh!

kfessel Feb 12, 2021 •

edited

Loading

kfessel Feb 12, 2021 •

edited

Loading

kfessel Feb 12, 2021 •

edited

Loading

kfessel Feb 12, 2021 •

edited

Loading

kfessel Feb 12, 2021 •

edited

Loading