make: provide support for listing supported and blacklisting toolchains by miri64 · Pull Request #9730 · RIOT-OS/RIOT

miri64 · 2018-08-07T14:40:13Z

Contribution description

This PR introduces two things:

The ability of a board/cpu-family to define the toolchains they support via the TOOLCHAINS_SUPPORTED macro
The ability to blacklist certain toolchains for modules, applications, or pkg that does not support that particular toolchain via the TOOLCHAINS_BLACKLIST (similar to BOARD_BLACKLIST)

As a consequence of those to introductions this PR also contains:

A make target to list all supported toolchains (excluding blacklisted ones) for a specific (board, application)-tuple
~~Blacklisting of llvm for pkgs we encountered hard to fix problems with in combination with LLVM/clang in murdock: also compile with LLVM/clang #9398~~ (moved to pkg: blacklist selected pkgs for LLVM/clang #9734)

Things that might be worth an inclusion into this:

A global make target that lists all boards that support a given toolchain (e.g. info-%-supporting or something)

Issues/PRs references

Errors for packages that caused blacklisting were detected in #9398.

#9734 applies the concept of blacklisting toolchains for several packets that have problems with LLVM/clang.

cladmi

Some review before testing remarks:

I like the idea and implementation.

I would like to also have a short documentation in https://github.com/RIOT-OS/RIOT/blob/master/makefiles/vars.inc.mk
As they do not need to be exported they can be explained in a comment like BUILDDEPS.

Some nitpicks inline.

Comparing to FEATURES, there is currently no need for a TOOLCHAINS_REQUIRED but it could be added later when/if necessary.

I did not tested the test and changes yet but will do it now.

cladmi · 2018-08-07T14:43:05Z

pkg/micro-ecc/Makefile.include

 INCLUDES += -I$(PKGDIRBASE)/micro-ecc
+
+# LLVM/clang can't handle the inline assembler instructions in this package
+TOOLCHAINS_BLACKLIST := llvm


This should be +=.

cladmi · 2018-08-07T14:44:16Z

cpu/cortexm_common/Makefile.include

  LINKFLAGS += $(LINKFLAGPREFIX)--defsym=_ram_length=$(RAM_LEN)
 endif
+
+TOOLCHAINS_SUPPORTED := gnu llvm


You can just use = here there is no reason to do :=.

cladmi · 2018-08-07T14:44:22Z

cpu/native/Makefile.include

 # include common peripheral initialization
 USEMODULE += periph_common
+
+TOOLCHAINS_SUPPORTED := gnu llvm


You can just use = here there is no reason to do :=.

cladmi · 2018-08-07T14:51:13Z

Makefile.include

 INCLUDES += -I$(RIOTCPU)/$(CPU)/include
 include $(RIOTCPU)/$(CPU)/Makefile.include

+TOOLCHAINS_SUPPORTED ?= gnu


Maybe a small comment on top.

miri64 · 2018-08-07T15:03:20Z

Addressed comments

cladmi · 2018-08-07T15:13:39Z

pkg/libcose/Makefile.include

+
+# There is a problem with the LLVM assembler and the assembly part of this
+# package
+TOOLCHAINS_BLACKLIST += llvm


I do not get errors for BOARD=iotlab-m3 or BOARD=native using TOOLCHAIN=llvm for unittests…

Mhhh.... I do get it. Are you in the right branch ;-)?

This PR branch yes.

I of course get the warning for the BLACKLISTED toolchain.

Ah! Mine are exactly as the murdock ones

cladmi · 2018-08-07T15:17:53Z

pkg/openthread/Makefile.include

  DIRS += $(OPENTHREAD_DIR)/contrib/netdev
 endif
+
+# There is a problem with ranlib and LLVM/clang in this package


Maybe also add problem with unused '-mcpu' arguments and ranlib

But those are only warnings.

Which I think may be the root cause of the ranlib error.

cladmi · 2018-08-07T15:20:01Z

pkg/micro-ecc/Makefile.include

 INCLUDES += -I$(PKGDIRBASE)/micro-ecc
+
+# LLVM/clang can't handle the inline assembler instructions in this package
+TOOLCHAINS_BLACKLIST += llvm


Same here, I do not get any errors.

make -C tests/pkg_micro-ecc --no-print-directory BOARD=iotlab-m3 clean all TOOLCHAIN=llvm

Do you have a failing murdock output maybe ?

Here you go: https://ci.riot-os.org/RIOT-OS/RIOT/9398/b30bc6725d6f9ff4307275a26e754dde5904c27d/output/compile/tests/pkg_micro-ecc/samr21-xpro:llvm.txt

Ok it fails with samr21-xpro but not iotlab-m3 in docker and on my computer.

Ok... I'll take the blacklisting out of this PR for now.

miri64 · 2018-08-07T15:43:06Z

Addressed more comments

miri64 · 2018-08-07T16:01:03Z

Ok... I'll take the blacklisting out of this PR for now.

Done

miri64 · 2018-08-07T16:07:05Z

Removed the milestone. This isn't part of the release (it just popped up during the testing procedures).

miri64 · 2018-08-07T16:07:36Z

Ok... I'll take the blacklisting out of this PR for now.

Done

See #9734 for the usage of the blacklisting.

cladmi · 2018-08-07T16:22:51Z

makefiles/info.inc.mk

 	@echo $($*)
+
+info-toolchains-supported:
+	@echo $(filter-out $(TOOLCHAINS_BLACKLIST),$(TOOLCHAINS_SUPPORTED))


I would also like to have something like info-toolchain-is-supported that produces an error if not

info-toolchain-is-supported: @exit $(if $(filter $(TOOLCHAIN), $(filter-out $(TOOLCHAINS_BLACKLIST),$(TOOLCHAINS_SUPPORTED))),1,0)

Or maybe not info- but more on assert/test/check.

So it can be used in https://github.com/RIOT-OS/RIOT/blob/master/makefiles/buildtests.inc.mk to skip the compilation in buildtest.

Done (see below)

cladmi · 2018-08-07T17:04:51Z

makefiles/buildtests.inc.mk

-			RESULT=false ; \
-		fi ; \
-		$(MAKE) clean-intermediates >/dev/null 2>&1 || true; \
+		if $(MAKE) check-toolchain-supported; then


Needs a terminating \ and also silencing stdout and stderr. Missing BOARD=$${board}.

Suggestion, it could be replace by an ! condition and using continue to remove one indentation.

cladmi · 2018-08-07T17:04:58Z

makefiles/buildtests.inc.mk

+				RESULT=false ; \
+			fi ; \
+			$(MAKE) clean-intermediates >/dev/null 2>&1 || true; \
+		fi


Same here for \.

cladmi · 2018-08-07T17:07:41Z

makefiles/info.inc.mk

 	@echo $(filter-out $(TOOLCHAINS_BLACKLIST),$(TOOLCHAINS_SUPPORTED))
+
+check-toolchain-supported:
+	@exit $(if $(filter $(TOOLCHAIN), $(filter-out $(TOOLCHAINS_BLACKLIST),$(TOOLCHAINS_SUPPORTED))),1,0)


This is not failing:

make -C examples/hello-world/ check-toolchain-supported TOOLCHAIN=llvm QUIET=0 BOARD=hifive1 make: Entering directory '/home/harter/work/git/RIOT/examples/hello-world' make: Leaving directory '/home/harter/work/git/RIOT/examples/hello-world'

I reversed the 1 and the 0 while writing the code… my bad

It should also be added to the .PHONY targets.

miri64 · 2018-08-07T17:20:07Z

Addressed comments (was already working on it, while testing it with #9734 ;-))

cladmi · 2018-08-08T09:59:25Z

The normal TOOLCHAINS_SUPPORTED case works correctly for me here:

BOARDS="native hifive1 samr21-xpro" TOOLCHAIN=llvm make --no-print-directory -C examples/hello-world/ buildtest
Building for native ... success.
Building for samr21-xpro ... success.

When using #9734 I also correctly get the TOOLCHAINS_BLACKLISTED

BOARDS="native hifive1 samr21-xpro iotlab-m3" TOOLCHAIN=llvm make --no-print-directory -C tests/pkg_micro-ecc buildtest
Building for native ... success.
Building for iotlab-m3 ... success.

cladmi

Only missing a short documentation in https://github.com/RIOT-OS/RIOT/blob/master/makefiles/vars.inc.mk
As they do not need to be exported they can be explained in a comment like BUILDDEPS.

I could propose something if it can help, but feel free to adapt/change it:

# TOOLCHAINS_SUPPORTED  List of supported toolchains by a cpu (gnu/llvm/…).
# TOOLCHAINS_BLACKLISTED List of unspported toolchains for a module or an example.

miri64 · 2018-08-12T10:24:39Z

Mh, BOARD_BLACKLIST and BOARD_WHITELIST aren't documented there as well. Should I open an issue or PR for that?

miri64 · 2018-08-12T10:25:24Z

(TOOLCHAIN isn't either ;-))

miri64 · 2018-08-12T10:27:47Z

Rebased (please check if I didn't mess up the change from bfbc9c1) and addressed comments.

miri64 · 2018-08-12T10:34:22Z

And another rebase due to #9351 being merged ;-)

cladmi · 2018-08-16T14:30:47Z

This looks good to me, please rebase/squash, I will retest and review the commits individually after.

Assuming `TOOLCHAIN_SUPPORTED` is provided by the board and `TOOLCHAIN_BLACKLIST` by a module or `pkg`, this outputs the fact that a toolchain is not supported or blacklisted in a similar manner as (un-)supported features and boards are.

miri64 · 2018-08-16T14:43:07Z

Squashed

cladmi

ACK Test from #9730 (comment) work.

Note, this is not doing any murdock handling as it does not compile with llvm and that everything is currently compatible with gnu. (Tested in murdock integration #9398)

cladmi · 2018-08-16T15:11:22Z

I was adding CI: run tests but it is not necessary as it not changing anything for murdock.

miri64 added Type: enhancement The issue suggests enhanceable parts / The PR enhances parts of the codebase / documentation Area: build system Area: Build system Area: toolchain Area: toolchains; everything related to compilation, libc, linking, … labels Aug 7, 2018

miri64 added this to the Release 2018.07 milestone Aug 7, 2018

miri64 requested a review from cladmi August 7, 2018 14:40

miri64 added a commit to miri64/RIOT that referenced this pull request Aug 7, 2018

Merge 'make/enh/toolchain-supported-list' (RIOT-OS#9730)

f08cbf6

miri64 mentioned this pull request Aug 7, 2018

murdock: also compile with LLVM/clang #9398

Merged

cladmi reviewed Aug 7, 2018

View reviewed changes

miri64 force-pushed the make/enh/toolchain-supported-list branch from e43d9be to 6a598b2 Compare August 7, 2018 16:00

miri64 mentioned this pull request Aug 7, 2018

pkg: blacklist selected pkgs for LLVM/clang #9734

Merged

miri64 removed this from the Release 2018.07 milestone Aug 7, 2018

cladmi reviewed Aug 7, 2018

View reviewed changes

miri64 added a commit to miri64/RIOT that referenced this pull request Aug 7, 2018

Merge branch 'make/enh/toolchain-supported-list' (RIOT-OS#9730)

0ca2315

cladmi suggested changes Aug 7, 2018

View reviewed changes

miri64 added a commit to miri64/RIOT that referenced this pull request Aug 7, 2018

Merge branch 'make/enh/toolchain-supported-list' (RIOT-OS#9730)

377972b

miri64 added a commit to miri64/RIOT that referenced this pull request Aug 8, 2018

Merge 'make/enh/toolchain-supported-list' (RIOT-OS#9730)

956e73b

cladmi suggested changes Aug 8, 2018

View reviewed changes

miri64 mentioned this pull request Aug 8, 2018

lpc2387: non-ARM statement in naked function #9739

Closed

miri64 force-pushed the make/enh/toolchain-supported-list branch from e2434ef to 2f27f11 Compare August 12, 2018 10:27

miri64 added a commit to miri64/RIOT that referenced this pull request Aug 12, 2018

Merge branch 'make/enh/toolchain-supported-list' (RIOT-OS#9730)

1e6f76a

miri64 force-pushed the make/enh/toolchain-supported-list branch from 2f27f11 to 7943cb0 Compare August 12, 2018 10:34

miri64 added a commit to miri64/RIOT that referenced this pull request Aug 12, 2018

Merge branch 'make/enh/toolchain-supported-list' (RIOT-OS#9730)

9b9a687

miri64 added a commit to miri64/RIOT that referenced this pull request Aug 12, 2018

Merge 'make/enh/toolchain-supported-list' (RIOT-OS#9730)

95f8be9

miri64 added a commit to miri64/RIOT that referenced this pull request Aug 13, 2018

Merge 'make/enh/toolchain-supported-list' (RIOT-OS#9730)

cbb8f44

miri64 added a commit to miri64/RIOT that referenced this pull request Aug 14, 2018

Merge 'make/enh/toolchain-supported-list' (RIOT-OS#9730)

e4c2c83

miri64 and others added 6 commits August 16, 2018 16:41

cortexm_common: Mark llvm and gnu as supported toolchains

b305ee4

native: Mark llvm and gnu as supported toolchains

c0a7da3

makefiles/info.inc.mk: provide info targets about toolchain support

ac9311a

make: provide toolchain support for buildtests

0972082

makefiles/vars.inc.mk: add doc on TOOLCHAIN lists

6309df6

miri64 force-pushed the make/enh/toolchain-supported-list branch from 7943cb0 to 6309df6 Compare August 16, 2018 14:43

miri64 added the CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR label Aug 16, 2018

miri64 added a commit to miri64/RIOT that referenced this pull request Aug 16, 2018

Merge branch 'make/enh/toolchain-supported-list' (RIOT-OS#9730)

2d2a8c7

cladmi added CI: run tests If set, CI server will run tests on hardware for the labeled PR and removed CI: run tests If set, CI server will run tests on hardware for the labeled PR labels Aug 16, 2018

cladmi approved these changes Aug 16, 2018

View reviewed changes

cladmi merged commit 623e3e1 into RIOT-OS:master Aug 16, 2018

miri64 deleted the make/enh/toolchain-supported-list branch August 16, 2018 15:15

Conversation

miri64 commented Aug 7, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Contribution description

Issues/PRs references

Uh oh!

cladmi 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!

miri64 commented Aug 7, 2018

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!

Choose a reason for hiding this comment

Uh oh!

miri64 commented Aug 7, 2018

Uh oh!

miri64 commented Aug 7, 2018

Uh oh!

miri64 commented Aug 7, 2018

Uh oh!

miri64 commented Aug 7, 2018

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!

miri64 commented Aug 7, 2018

Uh oh!

cladmi commented Aug 8, 2018

Uh oh!

cladmi left a comment

Choose a reason for hiding this comment

Uh oh!

miri64 commented Aug 12, 2018

Uh oh!

miri64 commented Aug 12, 2018

Uh oh!

miri64 commented Aug 12, 2018

Uh oh!

miri64 commented Aug 12, 2018

Uh oh!

cladmi commented Aug 16, 2018

Uh oh!

miri64 commented Aug 16, 2018

Uh oh!

miri64 commented Aug 7, 2018 •

edited

Loading