makefiles/cflags.inc.mk: add -Wmissing-include-dirs flag #9243
makefiles/cflags.inc.mk: add -Wmissing-include-dirs flag #9243cladmi merged 6 commits intoRIOT-OS:masterfrom
Conversation
cladmi
added
Impact: minor
cladmi
added
the
State: WIP
|
There are many missing directories even |
|
@cladmi Do you think using INCLUDES += $(wildcard $(RIOTBOARD)/board/include)would be a viable workaround for missing board dirs? |
|
@gebart I was thinking about exactly the same thing. |
cladmi
force-pushed
the
pr/warning/missing_include_dirs
branch
3 times, most recently
from
15487f0 to
36887b8
Compare
|
@gebart I needed to adapt it a bit more as specifying the I fixed the broken packages and murdock should now be happy. |
jnohlgard
left a comment
jnohlgard
left a comment
There was a problem hiding this comment.
This looks simple enough. Should be fine if Murdock agrees
Makefile.include
Outdated
| INCLUDES += -I$(RIOTBASE)/core/include -I$(RIOTBASE)/drivers/include -I$(RIOTBASE)/sys/include | ||
| INCLUDES += -I$(RIOTCPU)/$(CPU)/include | ||
| INCLUDES += -I$(RIOTBOARD)/$(BOARD)/include | ||
| INCLUDES += $(patsubst %,-I%,$(wildcard $(RIOTBOARD)/$(BOARD)/include)) |
There was a problem hiding this comment.
use $(addprefix -I, instead of $(patsubst %,-I%
There was a problem hiding this comment.
I updated and already squashed the fix as it easy to verify.
Some boards do not have a boards/BOARD/include directory and rely on a board/common one.
In this Makefile, 'CURDIR' is RIOTBASE/pkg/tiny-asn1 directory which does not exist and not PKGDIRBASE/tiny-asn1. The correct include directory is set by pkg/tiny-asn1/Makefile.include.
The include/crypto path should include 'sys' to be vavil But all source files are already using '#include "crypto/HEADER.h"' so it does not need fixing.
Warn if a user-supplied include directory does not exist.
cladmi
force-pushed
the
pr/warning/missing_include_dirs
branch
from
36887b8 to
08ff1b7
Compare
jnohlgard
left a comment
jnohlgard
left a comment
There was a problem hiding this comment.
Is this still WIP? it looks like Murdock is happy now.
Changes look good 👍
cladmi
removed
the
State: WIP
|
Forgot to remove the WIP, I think I got all of them. |
cladmi
added
CI: ready for build
|
This broke git HEAD for me on a debian sid system. When compiling any example for a platform which uses newlib nano e.g. #9216 seems to fix this. |
|
Could you give me the output of and Also, if you build with I would like to see if the warning is showing a problem where newlib-nano would not be used even if requested or if I should just temporarily revert the warning. |
Yes. BTW: According to @pyropeter the same issue seems to exist on Arch Linux. |
|
just FYI: on my arch system current master builds fine: |
|
Thank you. As in With this in mind, I would not say that this PR breaks your compilation but it actually shows it is not working as intended and that your GCC version is currently not supported by the RIOT build system and so the fix is to fix With this consideration, I would prefer not revert this PR and fix |
|
I have the same build issue here. Running ubuntu 18.04 with gcc 7.2 from ARM Embedded (latest release), so saying
seems weird to me. I installed my toolchain manually by downloading it and updating my PATH appropriately.
Ok for the first part but not for the second part. For some users, the build is broken with not obvious reasons. |
|
If the directory chosen to enable The problem is not that there is now a warning/error for this, but that the wrong directory is selected. |
I understand this. But as an end-user, the result is just that the build is failing which is not acceptable |
I agree. I am trying as much as I can to fix this, or narrow down the issue and give a proper error message. Can you try using this patch and tell if you get a warning ? diff --git a/dist/tools/genconfigheader/genconfigheader.sh b/dist/tools/genconfigheader/genconfigheader.sh
index bf17aef..72d2e99 100755
--- a/dist/tools/genconfigheader/genconfigheader.sh
+++ b/dist/tools/genconfigheader/genconfigheader.sh
@@ -71,6 +71,14 @@ for arg in "$@"; do
esac
done
+# Warn if newlib_nano is requested but not actually used
+echo '#include <newlib.h>' >> "${TMPFILE}"
+echo '#ifdef MODULE_NEWLIB_NANO' >> "${TMPFILE}"
+echo '#ifndef _NANO_FORMATTED_IO' >> "${TMPFILE}"
+echo '#error newlib-nano requested but not included by the build system' >> "${TMPFILE}"
+echo '#endif' >> "${TMPFILE}"
+echo '#endif' >> "${TMPFILE}"
+
# Only replace old file if the new file differs. This allows make to check the
# date of the config header for dependency calculations.
NEWMD5=$(${MD5SUM} ${TMPFILE} | cut -c -32)It adds a check that newlib nano is correctly used when requested. I put it in the riotbuild.h as I had no idea where to put it otherwise. |
|
Ok, sorry for my previous message, I was a bit frustrated with the broken build. I managed to fix my setup by uninstalling system wide newlib (apt remove newlib...). Now everything works and I checked, newlib is shipped with the ARM embedded toolchain and correctly included (checked with info-build target). |
|
Do you by chance have still the previous |
|
Yes, I have it :) |
Because of RIOT-OS#9243 failing to find the proper newlib-nano include directory triggered an error as the given newlib-nano include directory was not existing. This PR does: * only add the NEWLIB_NANO_INCLUDE_DIR to INCLUDES if it exists * show a warning if the found directory does not exist but still keeps going as it was the previous behaviour.
Because of RIOT-OS#9243, failing to find the proper newlib-nano include directory triggered an error as the given newlib-nano include directory was not existing. This PR does: * only add the NEWLIB_NANO_INCLUDE_DIR to INCLUDES if it exists * show a warning if the directory does not exist but still keeps going as it was the previous behavior.
Because of RIOT-OS#9243, failing to find the proper newlib-nano include directory triggered an error as the given newlib-nano include directory does not exist. This PR does: * only add the NEWLIB_NANO_INCLUDE_DIR to INCLUDES if it exists * show a warning if the directory does not exist but still keeps going as it was the previous behavior.
Because of RIOT-OS#9243, failing to find the proper newlib-nano include directory triggered an error as the given newlib-nano include directory does not exist. This PR does: * only add the NEWLIB_NANO_INCLUDE_DIR to INCLUDES if it exists * show a warning if the directory does not exist but still keeps going as it was the previous behavior.
Because of RIOT-OS#9243, failing to find the proper newlib-nano include directory triggered an error as the given newlib-nano include directory does not exist. This PR does: * only add the NEWLIB_NANO_INCLUDE_DIR to INCLUDES if it exists * show a warning if the directory does not exist but still keeps going as it was the previous behavior.
5 participants
Contribution description
Warn if a user-supplied include directory does not exist.
Issues/PRs references
None