From e42b4cc75f90af2b8ad338a0c113634408551e88 Mon Sep 17 00:00:00 2001 From: cladmi Date: Fri, 27 Sep 2019 18:50:48 +0200 Subject: [PATCH 1/4] dist/tools/cmake: handle strings with \" CMake quoted strings do not accept having \ or " inside. So use the "bracket argument" format. I migrated all variables to use this format. Migrate to 'printf' to not rely on having \" inside the string everywhere. This prepares for having macros defined in the CFLAGS again. --- .../cmake/generate-xcompile-toolchain.sh | 38 +++++++++++++------ 1 file changed, 26 insertions(+), 12 deletions(-) diff --git a/dist/tools/cmake/generate-xcompile-toolchain.sh b/dist/tools/cmake/generate-xcompile-toolchain.sh index 55f4af8c45..e723b8205d 100755 --- a/dist/tools/cmake/generate-xcompile-toolchain.sh +++ b/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 ::= Date: Fri, 27 Sep 2019 18:07:35 +0200 Subject: [PATCH 2/4] pkg/relic: pass COMP through environment export COMP by using the environment insteal of through the shell to prevnet issues with `\"` being defined when keeping macros in CFLAGS. Another solution was to use COMP='...' but could there could still have issues with single quotes in CFLAGS. --- pkg/relic/Makefile | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/pkg/relic/Makefile b/pkg/relic/Makefile index 7e2466c9dc..6a5d2983e2 100644 --- a/pkg/relic/Makefile +++ b/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) . From 41d10cf005bb3ee9d1901d08679144bc6767a060 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ga=C3=ABtan=20Harter?= Date: Tue, 17 Sep 2019 15:39:26 +0200 Subject: [PATCH 3/4] makefiles: do not remove defines from CFLAGS Do not remove the '-D' and '-U' values from CFLAGS. This prevents issues where a '-D' could contain a space. Some values way be duplicated from the 'riotbuild.h' header and the command line but with the same value so without conflict. To not put too many things in the command line, the -DMODULE_NAME are only put in CFLAGS_WITH_MACROS. Also, as now, the deferred value of CFLAGS is used for 'riotbuild.h', macros set after the inclusion of `Makefile.include` will be taken into account. --- Makefile.include | 8 +++----- makefiles/modules.inc.mk | 2 +- 2 files changed, 4 insertions(+), 6 deletions(-) diff --git a/Makefile.include b/Makefile.include index 8df7939bfe..96a3381aff 100644 --- a/Makefile.include +++ b/Makefile.include @@ -774,13 +774,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 diff --git a/makefiles/modules.inc.mk b/makefiles/modules.inc.mk index 7aa1e78ec6..e337750439 100644 --- a/makefiles/modules.inc.mk +++ b/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)) From d6b109f7209a8822937d2b643b64d0ffb750ec8c Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ga=C3=ABtan=20Harter?= Date: Wed, 18 Sep 2019 11:34:27 +0200 Subject: [PATCH 4/4] tests/build_system_cflags_spaces: test CFLAGS macros handling This tests passing CFLAGS with spaces to an application and also that even if the CFLAGS are defined after Makefile.include, they trigger a rebuild when modified. This includes an example how to pass macros with spaces to a docker build. The test as both an automated part for the CFLAGS with spaces, and a manual part for the two other features. --- tests/build_system_cflags_spaces/Makefile | 14 ++++++ tests/build_system_cflags_spaces/README.md | 43 +++++++++++++++++++ tests/build_system_cflags_spaces/main.c | 37 ++++++++++++++++ .../tests/01-run.py | 38 ++++++++++++++++ 4 files changed, 132 insertions(+) create mode 100644 tests/build_system_cflags_spaces/Makefile create mode 100644 tests/build_system_cflags_spaces/README.md create mode 100644 tests/build_system_cflags_spaces/main.c create mode 100755 tests/build_system_cflags_spaces/tests/01-run.py diff --git a/tests/build_system_cflags_spaces/Makefile b/tests/build_system_cflags_spaces/Makefile new file mode 100644 index 0000000000..c620d2640a --- /dev/null +++ b/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 ?= diff --git a/tests/build_system_cflags_spaces/README.md b/tests/build_system_cflags_spaces/README.md new file mode 100644 index 0000000000..efa45e535e --- /dev/null +++ b/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" +``` diff --git a/tests/build_system_cflags_spaces/main.c b/tests/build_system_cflags_spaces/main.c new file mode 100644 index 0000000000..62cc2efb49 --- /dev/null +++ b/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 + * + * @} + */ + +#include + +/* 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; +} diff --git a/tests/build_system_cflags_spaces/tests/01-run.py b/tests/build_system_cflags_spaces/tests/01-run.py new file mode 100755 index 0000000000..a29ad95403 --- /dev/null +++ b/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))