Skip to content

LLVM toolchain: set CFLAGS for pkgs#6752

Merged
smlng merged 3 commits intoRIOT-OS:masterfrom
smlng:llvm/pkgs_set_cflags
Mar 23, 2017
Merged

LLVM toolchain: set CFLAGS for pkgs#6752
smlng merged 3 commits intoRIOT-OS:masterfrom
smlng:llvm/pkgs_set_cflags

Conversation

@smlng
Copy link
Copy Markdown
Member

@smlng smlng commented Mar 16, 2017

set CFLAGS for packages oonf_api and tinydtls to work around compiler errors when using llvm toolchain on Linux. Is also required by #6600 ...

@smlng smlng added Type: enhancement The issue suggests enhanceable parts / The PR enhances parts of the codebase / documentation Area: build system Area: Build system Area: pkg Area: External package ports labels Mar 16, 2017
@smlng smlng self-assigned this Mar 16, 2017
@smlng smlng mentioned this pull request Mar 16, 2017
Closed
@jnohlgard
Copy link
Copy Markdown
Member

jnohlgard commented Mar 16, 2017

btw, we should probably remove the ifeq Darwin stuff and just change the default on Darwin to TOOLCHAIN=llvm, because Apple Xcode is clang/LLVM

jnohlgard
jnohlgard reviewed Mar 16, 2017
View reviewed changes
pkg/tinydtls/Makefile.include Outdated
ifeq ($(shell uname -s),Darwin)
CFLAGS += -Wno-gnu-zero-variadic-macro-arguments -Wno-unused-function
endif
ifneq (,$(filter $(TOOLCHAIN),llvm clang))
Copy link
Copy Markdown
Member

@jnohlgard jnohlgard Mar 16, 2017

Choose a reason for hiding this comment

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

you don't need to filter, there's an override in Makefile.include which replaces TOOLCHAIN=clang with TOOLCHAIN=llvm if the user sets it.

ifeq ($(TOOLCHAIN),llvm)
...
endif

pkg/oonf_api/Makefile.include Outdated
ifeq ($(shell uname -s),Darwin)
CFLAGS += -Wno-reserved-id-macro -Wno-keyword-macro
endif
ifneq (,$(filter $(TOOLCHAIN),llvm clang))
Copy link
Copy Markdown
Member

@jnohlgard jnohlgard Mar 16, 2017

Choose a reason for hiding this comment

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

ifeq ($(TOOLCHAIN),llvm)
...
endif

@smlng smlng force-pushed the llvm/pkgs_set_cflags branch 2 times, most recently from a500d1a to a326648 Compare March 16, 2017 19:26
@smlng
Copy link
Copy Markdown
Member Author

smlng commented Mar 16, 2017

@gebart addressed your comments, and took the liberty to pick up your suggestions and set TOOLCHAIN=llvm for macOS by default.

jnohlgard
jnohlgard requested changes Mar 16, 2017
View reviewed changes
pkg/tinydtls/Makefile.include Outdated
INCLUDES += -I$(PKG_BUILDDIR)

ifeq ($(shell uname -s),Darwin)
ifneq (,$(filter $(TOOLCHAIN),llvm))
Copy link
Copy Markdown
Member

@jnohlgard jnohlgard Mar 16, 2017

Choose a reason for hiding this comment

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

Could you replace this with
ifeq ($(TOOLCHAIN), llvm)
You don't need filter since it's not a list.

@smlng smlng force-pushed the llvm/pkgs_set_cflags branch from a326648 to a6bec45 Compare March 17, 2017 08:09
@smlng
Copy link
Copy Markdown
Member Author

smlng commented Mar 17, 2017

@gebart comments addressed.

jnohlgard
jnohlgard reviewed Mar 20, 2017
View reviewed changes
pkg/oonf_api/Makefile.include Outdated
ifeq ($(shell uname -s),Darwin)
CFLAGS += -Wno-reserved-id-macro -Wno-keyword-macro
ifeq ($(TOOLCHAIN), llvm)
CFLAGS += -Wno-reserved-id-macro -Wno-keyword-macro -Wno-parentheses-equality
Copy link
Copy Markdown
Member

@jnohlgard jnohlgard Mar 20, 2017

Choose a reason for hiding this comment

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

What is the error you get if you don't have -Wno-parentheses-equality and -Wno-reserved-id-macro?
I only need -Wno-keyword-macro to build this with Clang (clang version 4.0.0 (tags/RELEASE_400/final))

Copy link
Copy Markdown
Member Author

@smlng smlng Mar 20, 2017

Choose a reason for hiding this comment

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

@gebart I think -Wno-reserved-id-macro is no needed, but -Wno-parentheses-equality is required, see jenkins error log here for LLVM toolchain

@jnohlgard jnohlgard assigned jnohlgard and unassigned smlng Mar 20, 2017
@jnohlgard jnohlgard added this to the Release 2017.04 milestone Mar 20, 2017
@smlng
Copy link
Copy Markdown
Member Author

smlng commented Mar 20, 2017 •
edited
Loading

@gebart btw. there is a problem when setting TOOLCHAIN=llvm for macOS in general, when used with boards. Because then RIOT make tries to use LLVM for these as well, though there is support for target arm-none-eabi and even avr in LLVM, it fails on macOS

[edit]: hence I think we have to revert this and fix later.

@jnohlgard
Copy link
Copy Markdown
Member

jnohlgard commented Mar 20, 2017

Regarding MacOS, add a check for BOARD=native as well

@smlng smlng force-pushed the llvm/pkgs_set_cflags branch from a6bec45 to 72e90ae Compare March 20, 2017 15:09
jnohlgard
jnohlgard requested changes Mar 22, 2017
View reviewed changes
Makefile.include Outdated
# Use TOOLCHAIN environment variable to select the toolchain to use.
# Default: gnu
# Default for macOS: llvm; for other OS: gnu
ifeq ($(BOARD)$(OS),nativeDarwin)
Copy link
Copy Markdown
Member

@jnohlgard jnohlgard Mar 22, 2017

Choose a reason for hiding this comment

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

please do this in two steps instead:

ifeq ($(BOARD),native)
ifeq ($(OS),Darwin)
TOOLCHAIN ?= llvm
endif
endif
TOOLCHAIN ?= gnu

Since we set TOOLCHAIN with ?= we don't need to do it in an else statement, it will not overwrite the value set inside the if block.

smlng added 3 commits March 22, 2017 23:07
@smlng
pkg: oonf_api, disable compiler errors if toolchain llvm
60bb3ab 
    - set no-keyword-macro
    - set no-parentheses-equality
@smlng
pkg: tinydtls, disable compiler errors if toolchain llvm
36ae072 
    - set no-gnu-zero-variadic-macro-arguments
    - set no-unused-function
@smlng
make: set default toolchain for board native on macOS to llvm
53b0dd1
@smlng smlng force-pushed the llvm/pkgs_set_cflags branch from 72e90ae to 53b0dd1 Compare March 22, 2017 22:07
@jnohlgard jnohlgard added the CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR label Mar 23, 2017
@smlng smlng merged commit a4cc6ad into RIOT-OS:master Mar 23, 2017
@smlng smlng deleted the llvm/pkgs_set_cflags branch March 23, 2017 08:56
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

Assignees

@jnohlgard jnohlgard

Labels

Area: build system Area: Build system Area: pkg Area: External package ports 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

Projects

None yet

Milestone

Release 2017.04

Development

Successfully merging this pull request may close these issues.

2 participants

@smlng @jnohlgard