makefiles: do not remove defines from CFLAGS #12262
Conversation
|
Cool! Works for me when defining a string like this: |
|
Still not completely working with the example from #12219 I will see with 'genconfigheader.sh' too. |
|
Passing the value from environment variable is tricky, even without I need help with shell definition and escaping here :/ When used in a standard build, I managed to have the definition working from environment when doing: But I currently did not manage to escape it enough for defining it with |
|
This works for the #12219 case with The And the |
|
I will add an automated test for this. It would only cover the ones defined inside |
|
Automated and manual test added to verify the 3 things being fixed. |
5bbbeb9 to
2d70e23
Compare
|
(forced push to fix the last commit signature) |
|
@cladmi, I briefly looked at this PR and must say that I like it. I already had issues with What is status here ? Is it ready for review now (e.g. are all issues mentioned above fixed) ? If yes, I'll trigger Murdock. |
|
Yes all issues fixed. They need manual testing for some of the things but they are described in the README. |
There was a problem hiding this comment.
I followed the README of the test application and everything went fine.
Note that this PR is also fixing the DEFAULT_CHANNEL variable not taken into account after a previous build, which is already pretty awesome.
I couldn't find any obvious mistake or unjustified change in this PR.
One last word: ACK
|
I triggered Murdock and it is already reporting issues, probably with packages. Can you have a look @cladmi ? |
|
Looks like the ones depending on |
|
I currently do not correctly understand how both packages are building. If I have this: diff --git a/dist/tools/cmake/generate-xcompile-toolchain.sh b/dist/tools/cmake/generate-xcompile-toolchain.sh
index 55f4af8c4..705f34578 100755
--- a/dist/tools/cmake/generate-xcompile-toolchain.sh
+++ b/dist/tools/cmake/generate-xcompile-toolchain.sh
@@ -8,7 +8,7 @@ echo "SET(CMAKE_LINKER \"${LINK}\" CACHE STRING \"\")"
echo "SET(CMAKE_RANLIB \"${RANLIB}\" CACHE STRING \"\")"
# disable linker test
echo "SET(CMAKE_TRY_COMPILE_TARGET_TYPE STATIC_LIBRARY)"
-echo "SET(CMAKE_C_FLAGS \"${CFLAGS}\" CACHE STRING \"\")"
+printf "SET(CMAKE_C_FLAGS '%s' CACHE STRING "'"")\n' "${CFLAGS}"
echo "SET(CMAKE_EXE_LINKER_FLAGS \"${LFLAGS}\" CACHE STRING \"\")"
# search for programs in the build host directories
echo "SET(CMAKE_FIND_ROOT_PATH_MODE_PROGRAM NEVER)"it does not complain about the parentheses. diff --git a/pkg/relic/Makefile b/pkg/relic/Makefile
index 7e2466c9d..cb5b72ded 100644
--- a/pkg/relic/Makefile
+++ b/pkg/relic/Makefile
@@ -18,7 +18,7 @@ all: $(PKG_BUILDDIR)/Makefile
$(PKG_BUILDDIR)/Makefile: $(TOOLCHAIN_FILE)
cd $(PKG_BUILDDIR) && \
- COMP="$(filter-out -Werror -Werror=old-style-definition -Werror=strict-prototypes -std=gnu99, $(CFLAGS) ) " \
+ 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) .
EDIT: passing it through a target specific 'export COMP =' may even remove the need for escaping anything. Relic does not fail about having two different values for the macros. But I do not understand then why I do not have |
|
I had to change the But currently, it puts
Need more debugging and I have no idea about how |
|
https://cmake.org/cmake/help/v3.0/command/set.html?highlight=set
Looks like a reason at least. |
|
I am only supposed to define a string with It may be a thing to use the bracket arguments https://cmake.org/cmake/help/latest/manual/cmake-language.7.html#bracket-argument |
|
I updated the file generating the cmake configuration. At the same time I migrated it to use |
|
Note as I checked, the bracket arguments are only supported since |
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.
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.
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.
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.
00a8924 to
d6b109f
Compare
|
I put the commit in order. It would be good to re-run the test suit too. |
|
It seems you had a hard time with this one ;) |
|
Took me a while to already get I still do not understand the |
|
Error in murdock seemed unrelated. |
|
Thank you for the review! |
|
This causes an absolute path to be part of riotbuild.h (e.g., "-include /home/kaspar/src/riot.tmp/examples/hello-world/bin/samr21-xpro/riotbuild/riotbuild.h"), which unfortunately makes caching less effective. Can we remove at least that define? |
Some data: These are the ccache stats after two consecutive builds of all samr21-xpro configurations, of this PR's merge commit: This is from the merge commit before: |
|
The To get more information on the caching: If the same path is used when building and the same application, there should not be any reason to have changes on caching except that there are more characters in a comment than before. How were the measures done? Real issueThe real issue is that the comment in there is useless, it is just there to trigger creating a new file when Question, is there any real reason to rely, to passing macros with this file? |
Well, Murdock builds in different paths (per worker thread).
I used this script. It basically runs a murdock build twice, but limited to one board and one worker. Also, before the first run, the ccache is emptied and the statistics zeroed. I run this script in a loop over the latest merge commits.
Isn't lazysponge supposed to make sure of that? Change the file stamp on update, I mean?
That's a good one. 😉 I'd be happy to remove the file altogether. Anyhow, for now I propose we either remove the comment in that file, or hack removing the |
The main reason we introduced this (in #5097) seems to be shorter build command lines. |
Ok so it is a murdock build and not a normal local one. It was not clear with your I still find it bad that a different build path is used, as docker can mount the volume anywhere, and so could reproduce the However, I agree the issue here must still be fixed as affects inter applications caching.
I do not see this as a good reason.
|
Docker can do this, but it would require a fresh container for each build (or use of "docker exec"), and both take >1s. Compare that to the total build time of ~2s on average or ~0.5s for the smaller applications. |
|
And a fix in #12348
If each worker has a dedicated directory on the host, and not each job, this one could be updated and still be mapped to the same one inside the container. |
|
Is there some additional quoting needed for strings that include special characters like |
It is likely. If you define them inside a Do you have an example ? |
|
|
|
(This is from within a Makefile.) |
|
What does work is to use |
Contribution description
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.includewill be taken intoaccount.
Testing procedure
I added a test for all the related issues.
They are documented in the
README.md.Test CFLAGS with spaces
make -C tests/build_system_cflags_spaces flash testRebuild on a CFLAGS change set after `Makefile.include`
CONFIGURATION_VALUE=1 make -C tests/build_system_cflags_spaces flash testPassing a CFLAGS macro with space to a DOCKER build
I needed to escape the space in the CFLAGS when defining it from command line.
There may be another solution but did not find it.
DOCKER_ENVIRONMENT_CMDLINE=$'-e CFLAGS=-DSTRING_FROM_DOCKER=\'\\\"with\ space\\\"\''The impact in the build will be that some macros may be repeated between
riotbuild.hand the command line but with the same value.Issues/PRs references
Fixes #5776
Fixes #9589
Fixes #12219