make: do not locally export compilation variables by cladmi · Pull Request #12098 · RIOT-OS/RIOT

cladmi · 2019-08-27T15:24:14Z

Contribution description

These are already exported by makefiles/vars.inc.mk.
It is a prerequisite to allow handling compilation without global exports.

This currently does not un-export them from makefiles/toolchain.

Testing procedure

All the removed export are exported in makefiles/vars.inc.mk or not needed in sub-execution.
I newly exported CFLAGS_CPU as it is needed by pkg/openthread.

git diff $(git merge-base HEAD master) | grep '^-' | grep -v -e '--- a/' -e '#' \
   | grep -v -e 'CFLAGS ' -e 'OBJCOPY'  -e CCAS -e ASFLAGS -e INCLUDES -e OFLAGS -e OBJDUMPFLAGS -e CXXEXFLAGS -e CXXUWFLAGS -e CFLAGS_CPU
-export CFLAGS_LINK  = -ffunction-sections -fdata-sections -fno-builtin -fshort-enums
-export CFLAGS_DBG  ?= -ggdb -g3
-export CFLAGS_OPT  ?= -Os
-
-export CFLAGS_LINK  = -ffunction-sections -fdata-sections -fno-builtin -fshort-enums
-export CFLAGS_DBG  ?= -ggdb -g3
-export CFLAGS_OPT  ?= -Os
-        export CFLAGS_FPU ?= -mfloat-abi=soft
-                export CFLAGS_FPU ?= -mfloat-abi=hard -mfpu=fpv5-d16
-                export CFLAGS_FPU ?= -mfloat-abi=hard -mfpu=fpv4-sp-d16
-export CFLAGS_LINK  = -ffunction-sections -fno-builtin -fshort-enums -fdata-sections
-export CFLAGS_DBG   = -g3
-export CFLAGS_OPT   = -Os

And the CFLAGS_ variables are only used by included files:

git grep '$(CFLAGS_.*)'  | grep -v -e 'pkg/openthread/Makefile'  -e 'dist/tools/uhcpd/Makefile'
Makefile.include:       $(Q)'$(RIOTTOOLS)/genconfigheader/genconfigheader.sh' $(CFLAGS_WITH_MACROS) \
boards/common/msba2/Makefile.include:CFLAGS += $(CFLAGS_CPU) $(CFLAGS_LINK) $(CFLAGS_DBG) $(CFLAGS_OPT)
boards/common/msba2/Makefile.include:ASFLAGS += $(CFLAGS_CPU) $(CFLAGS_DBG)
boards/common/msba2/Makefile.include:export LINKFLAGS += $(CFLAGS_CPU) $(CFLAGS_DBG) $(CFLAGS_OPT) -static -lgcc -nostartfiles
makefiles/arch/atmega.inc.mk:CFLAGS_CPU   = -mmcu=$(CPU) $(CFLAGS_FPU)
makefiles/arch/atmega.inc.mk:CFLAGS    += $(CFLAGS_CPU) $(CFLAGS_LINK) $(CFLAGS_DBG) $(CFLAGS_OPT)
makefiles/arch/atmega.inc.mk:ASFLAGS   += $(CFLAGS_CPU) $(CFLAGS_DBG)
makefiles/arch/atmega.inc.mk:LINKFLAGS += $(CFLAGS_CPU) $(CFLAGS_DBG) $(CFLAGS_OPT) -static -lgcc -e reset_handler -Wl,--gc-sections
makefiles/arch/cortexm.inc.mk:CFLAGS_CPU   = -mcpu=$(MCPU) -mlittle-endian -mthumb $(CFLAGS_FPU)
makefiles/arch/cortexm.inc.mk:CFLAGS += $(CFLAGS_CPU) $(CFLAGS_LINK) $(CFLAGS_DBG) $(CFLAGS_OPT)
makefiles/arch/cortexm.inc.mk:ASFLAGS += $(CFLAGS_CPU) $(CFLAGS_DBG)
makefiles/arch/cortexm.inc.mk:export LINKFLAGS += $(CFLAGS_CPU) $(CFLAGS_DBG) $(CFLAGS_OPT) -static -lgcc -nostartfiles
makefiles/arch/mips.inc.mk:CFLAGS += $(CFLAGS_CPU) $(CFLAGS_LINK) $(CFLAGS_OPT) $(CFLAGS_DBG)
makefiles/arch/mips.inc.mk:eASFLAGS += $(CFLAGS_CPU) $(CFLAGS_OPT) $(CFLAGS_DBG)
makefiles/arch/mips.inc.mk:export LINKFLAGS += $(CFLAGS_CPU) $(CFLAGS_DBG) $(CFLAGS_OPT)
makefiles/arch/msp430.inc.mk:CFLAGS += $(CFLAGS_CPU) $(CFLAGS_LINK) $(CFLAGS_DBG) $(CFLAGS_OPT)
makefiles/arch/msp430.inc.mk:ASFLAGS += $(CFLAGS_CPU) --defsym $(CPU_MODEL)=1 $(CFLAGS_DBG)
makefiles/arch/msp430.inc.mk:export LINKFLAGS += $(CFLAGS_CPU) $(CFLAGS_DBG) $(CFLAGS_OPT) -Wl,--gc-sections -static -lgcc
makefiles/arch/riscv.inc.mk:CFLAGS += $(CFLAGS_CPU) $(CFLAGS_DBG) $(CFLAGS_OPT) $(CFLAGS_LINK)
makefiles/arch/riscv.inc.mk:ASFLAGS += $(CFLAGS_CPU) $(CFLAGS_DBG)
makefiles/arch/riscv.inc.mk:export LINKFLAGS += $(CFLAGS_CPU) $(CFLAGS_LINK) $(CFLAGS_DBG) $(CFLAGS_OPT) -Wl,--gc-sections -static -lgcc
makefiles/eclipse.inc.mk:       $(Q)printf "%s\n" $(CC) $(CFLAGS_WITH_MACROS) $(INCLUDES) | \
makefiles/toolchain/llvm.inc.mk:    $(shell $(PREFIX)gcc $(CFLAGS_CPU) -v -x $1 -E /dev/null 2>&1 | \
makefiles/toolchain/llvm.inc.mk:  ifeq (,$(CFLAGS_CPU))

And there should not be any other changes than these variables and comments except the first two commits changes and the dist/Makefile comment.

git diff --word-diff $(git merge-base HEAD master) | sed 's/\[-export-\]//g' | sed 's/\[-#.*\]//' | grep -e '{' -e '\[-'
[-CFLAGS += $(CFLAGS_BASIC)-]
{+#INCLUDES+} += -Iapplication_include
{+export CFLAGS_CPU            # CPU architecture specific compiler flags+}

Issues/PRs references

Part of removing exporting everything everywhere #10850

Nothing declares this anymore

smlng · 2019-08-28T08:17:22Z

Looks good to me, only removing export from variables. Also if Murdock is happy this should be good to go.

cladmi · 2019-08-28T10:11:46Z

@smlng What do you think about the CFLAGS_CPU added documentation? I needed to export the variable but not sure if the description is good. 94badbc

smlng

mhm, somehow the usage of CFLAGS_CPU and its addition to CFLAGS is not very consistent. Its mostly used in makefiles/arch/* but also in boards/common/msba2.
Also I don't understand why it needs to be exported, as it is always added to CFLAGS anyway.

makefiles/vars.inc.mk

cladmi · 2019-08-28T10:26:05Z

@smlng as I put in the commit: "CFLAGS_CPU is required for 'pkg/openthread'. So declare it as a global compilation variable."

git grep '$(CFLAGS_CPU)' -- pkg/openthread/
pkg/openthread/Makefile:                CPPFLAGS="$(OPENTHREAD_COMMON_FLAGS) $(CFLAGS_CPU) -D$(CONFIG_FILE)" \
pkg/openthread/Makefile:                CFLAGS="$(OPENTHREAD_COMMON_FLAGS) $(CFLAGS_CPU) " \
pkg/openthread/Makefile:                CXXFLAGS="$(OPENTHREAD_COMMON_FLAGS) $(CFLAGS_CPU) -fno-exceptions -fno-rtti " \
pkg/openthread/Makefile:                LDFLAGS="$(OPENTHREAD_COMMON_FLAGS) $(CFLAGS_CPU) -nostartfiles -specs=nano.specs \

I do not question why this is there, just it is there, so this PR handles exporting the value as I remove the export in the cpu/arch.

cladmi · 2019-08-28T10:29:28Z

mhm, somehow the usage of CFLAGS_CPU and its addition to CFLAGS is not very consistent. Its mostly used in makefiles/arch/* but also in boards/common/msba2.

This is being changes for msba2 to be done in cpu #11882

Unrelated to this PR though.

cladmi · 2019-08-28T13:41:04Z

I found some missing 'export INCLUDES' they were not in my original "grep" command.

smlng · 2019-08-28T19:23:14Z

cpu/mips_pic32mz/Makefile.include

 # the pickit programmer (MPLAB-IPE) wants physical addresses in the hex file!!
-export OBJCOPY = objcopy #use system objcopy as toolchain one is broken.
-export OFLAGS += \
+OBJCOPY = objcopy #use system objcopy as toolchain one is broken.


this also exported in makefiles/toolchain/{llvm,gnu}.inc.mk, don't know if it makes sense to change that here too

As I said in the PR description "This currently does not un-export them from makefiles/toolchain."

The reasoning is that I prefer changing the toolchain in its specific PR.
It is not supposed to be different, but as it would change almost each line, I think it deserve a smaller PR to ease review.

smlng · 2019-08-28T19:26:08Z

apart from OBJCOPY as commented above, I checked changed variables are only exported in vars.inc.mk.

I guess other such as LINKFLAGS need separate careful handling.

smlng · 2019-08-28T19:26:26Z

please squash

cladmi · 2019-08-29T08:35:05Z

I guess other such as LINKFLAGS need separate careful handling.

Yes, LINK, LINKFLAGS and such are use during linking, so supposedly not even required to be exported, but I would like to do a specific package review too for these as LINK is used in dist/tools/cmake/generate-xcompile-toolchain.sh for example (also LFLAGS that is not defined in RIOT).

CFLAGS_CPU is required for 'pkg/openthread'. So declare it as a global compilation variable. The goal is to move the export to `makefiles/vars.inc.mk` and remove the local exports that could be in boards/cpu/arch.

CFLAGS and INCLUDES are already exported by `makefiles/vars.inc.mk`. It is a prerequisite to allow handling compilation without global exports.

These are already exported by `makefiles/vars.inc.mk`. It is a prerequisite to allow handling compilation without global exports.

smlng

re-checked exports only in vars.inc.mk (apart from mentioned exceptions), ACK

cladmi · 2019-08-29T09:57:18Z

CI is green and it executed as many compilation jobs as nightly "41197"

cladmi · 2019-08-29T12:36:01Z

Thank you for the review. More will be coming :)

cpu/arm7_common: remove deprecated CFLAGS_BASIC

3010d1c

Nothing declares this anymore

cladmi force-pushed the pr/export/remove_local_export_compilation_variables branch from 0fa1e9b to 1933b45 Compare August 27, 2019 15:24

cladmi mentioned this pull request Aug 27, 2019

Tracking: remove harmful use of export in make and immediate evaluation #10850

Closed

19 tasks

smlng added 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 Type: cleanup The issue proposes a clean-up / The PR cleans-up parts of the codebase / documentation labels Aug 28, 2019

smlng added the Reviewed: 1-fundamentals The fundamentals of the PR were reviewed according to the maintainer guidelines label Aug 28, 2019

smlng suggested changes Aug 28, 2019

View reviewed changes

makefiles/vars.inc.mk Outdated Show resolved Hide resolved

smlng reviewed Aug 28, 2019

View reviewed changes

cladmi added 5 commits August 29, 2019 10:35

makefiles/vars.inc.mk: export CFLAGS_CPU

a68bf99

CFLAGS_CPU is required for 'pkg/openthread'. So declare it as a global compilation variable. The goal is to move the export to `makefiles/vars.inc.mk` and remove the local exports that could be in boards/cpu/arch.

pkg/oonf_api: remove exporting CFLAGS and INCLUDES

15e9812

CFLAGS and INCLUDES are already exported by `makefiles/vars.inc.mk`. It is a prerequisite to allow handling compilation without global exports.

makefiles: do not locally export compilation variables

34f6924

These are already exported by `makefiles/vars.inc.mk`. It is a prerequisite to allow handling compilation without global exports.

boards: do not locally export compilation variables

ef542ef

These are already exported by `makefiles/vars.inc.mk`. It is a prerequisite to allow handling compilation without global exports.

cpu: do not locally export compilation variables

1be5b7b

These are already exported by `makefiles/vars.inc.mk`. It is a prerequisite to allow handling compilation without global exports.

cladmi force-pushed the pr/export/remove_local_export_compilation_variables branch from 71cf9f3 to 1be5b7b Compare August 29, 2019 08:36

smlng approved these changes Aug 29, 2019

View reviewed changes

MrKevinWeiss merged commit 969e3b3 into RIOT-OS:master Aug 29, 2019

cladmi deleted the pr/export/remove_local_export_compilation_variables branch August 29, 2019 12:36

maribu mentioned this pull request Sep 10, 2019

cpu/arm7_common: Moved compiler flags here #11882

Merged

kb2ma added this to the Release 2019.10 milestone Sep 16, 2019

Conversation

cladmi commented Aug 27, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Contribution description

Testing procedure

Issues/PRs references

Uh oh!

smlng commented Aug 28, 2019

Uh oh!

cladmi commented Aug 28, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

smlng left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

cladmi commented Aug 28, 2019

Uh oh!

cladmi commented Aug 28, 2019

Uh oh!

cladmi commented Aug 28, 2019

Uh oh!

smlng Aug 28, 2019

Choose a reason for hiding this comment

Uh oh!

cladmi Aug 29, 2019

Choose a reason for hiding this comment

Uh oh!

smlng commented Aug 28, 2019

Uh oh!

smlng commented Aug 28, 2019

Uh oh!

cladmi commented Aug 29, 2019

Uh oh!

smlng left a comment

Choose a reason for hiding this comment

Uh oh!

cladmi commented Aug 29, 2019

Uh oh!

cladmi commented Aug 29, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

cladmi commented Aug 27, 2019 •

edited

Loading

cladmi commented Aug 28, 2019 •

edited

Loading