Merge pull request #12262 from cladmi/pr/cflags/fix_spaces_and_rebuild

makefiles: do not remove defines from CFLAGS

Makefile.include

@@ -783,13 +783,11 @@ $(RIOTBUILD_CONFIG_HEADER_C): FORCE
 	$(Q)'$(RIOTTOOLS)/genconfigheader/genconfigheader.sh' $(CFLAGS_WITH_MACROS) \
 		| '$(LAZYSPONGE)' $(LAZYSPONGE_FLAGS) '$@'
 
-# Immediate evaluation but keep CLAGS_WITH_MACROS deferred
-_CFLAGS := $(CFLAGS)
-CFLAGS_WITH_MACROS = $(_CFLAGS)
+CFLAGS_WITH_MACROS += $(CFLAGS)
 CFLAGS_WITH_MACROS += -DRIOT_VERSION=\"$(RIOT_VERSION)\"
+# MODULE_NAME defines. Declared in 'makefiles/modules.inc.mk'
+CFLAGS_WITH_MACROS += $(EXTDEFINES)
 
-CFLAGS := $(patsubst -D%,,$(CFLAGS))
-CFLAGS := $(patsubst -U%,,$(CFLAGS))
 CFLAGS += -include '$(RIOTBUILD_CONFIG_HEADER_C)'
 
 # include mcuboot support

dist/tools/cmake/generate-xcompile-toolchain.sh

@@ -1,17 +1,31 @@
 #!/usr/bin/env sh
-echo "SET(CMAKE_SYSTEM_NAME Generic)"
-echo "SET(CMAKE_SYSTEM_VERSION 1)"
+
+# When setting variables, use the 'bracket argument' format to allow having \"
+# inside the string. Which is not supported by quoted arguments
+# https://cmake.org/cmake/help/latest/manual/cmake-language.7.html#bracket-argument
+#
+#     bracket_argument ::=  bracket_open bracket_content bracket_close
+#     bracket_open     ::=  '[' '='* '['
+#     bracket_content  ::=  <any text not containing a bracket_close with
+#                            the same number of '=' as the bracket_open>
+#                            bracket_close    ::=  ']' '='* ']'
+#
+# I will use [==[value]==] in this file, to not maybe match a case with one '='
+# I used it for all variables even ones that currently do not need it.
+
+printf 'SET(CMAKE_SYSTEM_NAME Generic)\n'
+printf 'SET(CMAKE_SYSTEM_VERSION 1)\n'
 # specify the cross compiler"
-echo "SET(CMAKE_C_COMPILER \"${CC}\" CACHE STRING \"\")"
-echo "SET(CMAKE_CXX_COMPILER \"${CXX}\" CACHE STRING \"\")"
-echo "SET(CMAKE_LINKER \"${LINK}\" CACHE STRING \"\")"
-echo "SET(CMAKE_RANLIB \"${RANLIB}\" CACHE STRING \"\")"
+printf 'SET(CMAKE_C_COMPILER [==[%s]==] CACHE STRING "")\n' "${CC}"
+printf 'SET(CMAKE_CXX_COMPILER [==[%s]==] CACHE STRING "")\n' "${CXX}"
+printf 'SET(CMAKE_LINKER [==[%s]==] CACHE STRING "")\n' "${LINK}"
+printf 'SET(CMAKE_RANLIB [==[%s]==] CACHE STRING "")\n' "${RANLIB}"
 # disable linker test
-echo "SET(CMAKE_TRY_COMPILE_TARGET_TYPE STATIC_LIBRARY)"
-echo "SET(CMAKE_C_FLAGS \"${CFLAGS}\" CACHE STRING \"\")"
-echo "SET(CMAKE_EXE_LINKER_FLAGS \"${LFLAGS}\" CACHE STRING \"\")"
+printf 'SET(CMAKE_TRY_COMPILE_TARGET_TYPE STATIC_LIBRARY)\n'
+printf 'SET(CMAKE_C_FLAGS [==[%s]==] CACHE STRING "")\n' "${CFLAGS}"
+printf 'SET(CMAKE_EXE_LINKER_FLAGS [==[%s]==] CACHE STRING "")\n' "${LFLAGS}"
 # search for programs in the build host directories
-echo "SET(CMAKE_FIND_ROOT_PATH_MODE_PROGRAM NEVER)"
+printf 'SET(CMAKE_FIND_ROOT_PATH_MODE_PROGRAM NEVER)\n'
 # for libraries and headers in the target directories
-echo "SET(CMAKE_FIND_ROOT_PATH_MODE_LIBRARY ONLY)"
-echo "SET(CMAKE_FIND_ROOT_PATH_MODE_INCLUDE ONLY)"
+printf 'SET(CMAKE_FIND_ROOT_PATH_MODE_LIBRARY ONLY)\n'
+printf 'SET(CMAKE_FIND_ROOT_PATH_MODE_INCLUDE ONLY)\n'

makefiles/modules.inc.mk

@@ -2,8 +2,8 @@ _ALLMODULES = $(sort $(USEMODULE) $(USEPKG))
 
 # Define MODULE_MODULE_NAME preprocessor macros for all modules.
 ED = $(addprefix MODULE_,$(_ALLMODULES))
+# EXTDEFINES will be put in CFLAGS_WITH_MACROS
 EXTDEFINES = $(addprefix -D,$(shell echo '$(ED)' | tr 'a-z-' 'A-Z_'))
-CFLAGS += $(EXTDEFINES)
 
 # filter "pseudomodules" from "real modules", but not "no_pseudomodules"
 REALMODULES += $(filter-out $(PSEUDOMODULES), $(_ALLMODULES))

pkg/relic/Makefile

@@ -16,9 +16,13 @@ all: $(PKG_BUILDDIR)/Makefile
 	$(MAKE) -C $(PKG_BUILDDIR) && \
 	cp $(PKG_BUILDDIR)/lib/librelic_s.a $(BINDIR)/$(PKG_NAME).a
 
+# Pass 'COMP' with a target specific export to not have issues with the shell
+# escaping evaluation.
+COMP = $(filter-out -Werror -Werror=old-style-definition -Werror=strict-prototypes -std=gnu99,$(CFLAGS))
+$(PKG_BUILDDIR)/Makefile: export COMP ?=
+
 $(PKG_BUILDDIR)/Makefile: $(TOOLCHAIN_FILE)
 	cd $(PKG_BUILDDIR) && \
-	COMP="$(filter-out -Werror -Werror=old-style-definition -Werror=strict-prototypes -std=gnu99, $(CFLAGS) ) " \
 	cmake -DCMAKE_TOOLCHAIN_FILE=$(TOOLCHAIN_FILE) \
 		  -DCHECK=off -DTESTS=0 -DBENCH=0 -DSHLIB=off -Wno-dev $(RELIC_CONFIG_FLAGS) .
 

tests/build_system_cflags_spaces/Makefile

@@ -0,0 +1,14 @@
+APPLICATION = cflags_with_spaces
+BOARD ?= native
+RIOTBASE ?= $(CURDIR)/../..
+
+CFLAGS += -DSUPER_STRING='"I love sentences with spaces"'
+
+include $(RIOTBASE)/Makefile.include
+
+# Changing this value should trigger a rebuild even if defined after
+# Makefile.include
+CONFIGURATION_VALUE ?= 0
+CFLAGS += -DDEFINED_AFTER_MAKEFILE_INCLUDE=$(CONFIGURATION_VALUE)
+# Exported to be available in the automated test
+test: export CONFIGURATION_VALUE ?=

tests/build_system_cflags_spaces/README.md

@@ -0,0 +1,43 @@
+Build system cflags with space test
+===================================
+
+This tests that the build system now handles CFLAGS correctly.
+
+There is an automated test for CFLAGS with spaces when defined in the Makefile.
+
+
+Other manual tests
+------------------
+
+There are also 2 other manual tests that can be run
+
+
+### Modifying CFLAGS defined after including Makefile.include
+
+CFLAGS defined after `Makefile.include` trigger a rebuild when changed.
+
+Running the test once should work and then when changing `CONFIGURATION_VALUE`
+it should pass too.
+
+    make flash test
+    CONFIGURATION_VALUE=1 make flash test
+
+
+### Setting CFLAGS with space for docker
+
+This one is a trickier as CFLAGS are modified in the Makefile, so cannot be
+detected automatically in the docker handling. The solution is to pass it with
+`DOCKER_ENVIRONMENT_CMDLINE`.
+
+When the CFLAGS is defined like this, I did not find another solution than
+escaping the space.
+
+```
+DOCKER_ENVIRONMENT_CMDLINE=$'-e CFLAGS=-DSTRING_FROM_DOCKER=\'\\\"with\ space\\\"\'' \
+  BUILD_IN_DOCKER=1 make
+grep '#define STRING_FROM_DOCKER "with space"' bin/native/riotbuild/riotbuild.h \
+  || { echo 'ERROR CFLAGS not passed correctly' >&2; false; }
+
+...
+#define STRING_FROM_DOCKER "with space"
+```

tests/build_system_cflags_spaces/main.c

@@ -0,0 +1,37 @@
+/*
+ * Copyright (C) 2019 Freie Universität Berlin
+ *
+ * This file is subject to the terms and conditions of the GNU Lesser
+ * General Public License v2.1. See the file LICENSE in the top level
+ * directory for more details.
+ */
+
+/**
+ * @ingroup     tests
+ * @{
+ *
+ * @file
+ * @brief       Test the CFLAGS handling
+ *
+ * @author      Gaëtan Harter <gaetan.harter@fu-berlin.de>
+ *
+ * @}
+ */
+
+#include <stdio.h>
+
+/* Define a CFLAGS string with spaces from outside docker */
+/* DOCKER_ENVIRONMENT_CMDLINE=$'-e CFLAGS=-DSTRING_FROM_DOCKER=\'\\\"with\ space\\\"\''*/
+#ifndef STRING_FROM_DOCKER
+#define STRING_FROM_DOCKER ""
+#endif
+
+
+int main(void)
+{
+    puts("The output of the configuration variables:");
+    printf("SUPER_STRING: %s\n", SUPER_STRING);
+    printf("DEFINED_AFTER_MAKEFILE_INCLUDE: %u\n", DEFINED_AFTER_MAKEFILE_INCLUDE);
+    printf("CFLAGS_STRING_FROM_DOCKER: %s\n", STRING_FROM_DOCKER);
+    return 0;
+}

tests/build_system_cflags_spaces/tests/01-run.py

@@ -0,0 +1,38 @@
+#!/usr/bin/env python3
+
+# Copyright (C) 2019 Freie Universität Berlin
+#
+# This file is subject to the terms and conditions of the GNU Lesser
+# General Public License v2.1. See the file LICENSE in the top level
+# directory for more details.
+
+"""
+Test for passing `CFLAGS` with spaces to the application.
+
+It also tests that even if a `CFLAGS` is set after including Makefile.include,
+changing its value will trigger a rebuild.
+
+There is also a way to test passing additional values with spaces to docker
+documented in the `README.md`.
+"""
+
+import os
+import sys
+from testrunner import run
+
+
+# Verify the macro matches the configuration value
+CONFIGURATION_VALUE = os.environ['CONFIGURATION_VALUE']
+
+
+def testfunc(child):
+    child.expect_exact('The output of the configuration variables:')
+    child.expect_exact('SUPER_STRING: I love sentences with spaces')
+    child.expect_exact('DEFINED_AFTER_MAKEFILE_INCLUDE: %s' %
+                       CONFIGURATION_VALUE)
+    # This one is not tested here, see the output in 'riotbuild.h'
+    child.expect(r'CFLAGS_STRING_FROM_DOCKER: .*')
+
+
+if __name__ == "__main__":
+    sys.exit(run(testfunc))