[Buildroot] [PATCH 1/3] package/Makefile.in: Use '-isystem' instead of '-I' in HOST_CFLAGS globally

[Buildroot] [PATCH 1/3] package/Makefile.in: Use '-isystem' instead of '-I' in HOST_CFLAGS globally

[David De Grave]

 As the $(HOST_DIR) is the buildroot's system install directory, and that
all the builds should use that one in priority (it's even mandatory for
uboot and linux), it make sense to use '-isystem' to ensure it is searched
right before the system directories and still let the possibility for the
packages to use specific ones using '-I'.

 Now, changing '-I' into '-isystem' carries the risk that a package will add
'-I/usr/include' to CFLAGS and thus give priority to system headers over
host headers. However, this turns out not to be the case, because gcc will
remove the '-I' options that add system includes:

  $ gcc -v -c -isystem $PWD/output/host/include -I/usr/include \
    support/kconfig/expr.c
  ...
  ignoring duplicate directory "/usr/include" as it is a non-system \
    directory that duplicates a system directory
  #include "..." search starts here:
  #include <...> search starts here:
   {output}/host/include
   /usr/lib/gcc/x86_64-linux-gnu/8/include
   /usr/local/include
   /usr/lib/gcc/x86_64-linux-gnu/8/include-fixed
   /usr/include/x86_64-linux-gnu
   /usr/include
  End of search list.
  ...

Signed-off-by: David De Grave (Essensium/Mind) <david.degrave@mind.be>
---
 package/Makefile.in | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/package/Makefile.in b/package/Makefile.in
index 91b3e8f936..5ed70f0a65 100644
--- a/package/Makefile.in
+++ b/package/Makefile.in
@@ -223,7 +223,7 @@ UNZIP := $(shell which unzip || type -p unzip) -q
 
 APPLY_PATCHES = PATH=$(HOST_DIR)/bin:$$PATH support/scripts/apply-patches.sh $(if $(QUIET),-s)
 
-HOST_CPPFLAGS  = -I$(HOST_DIR)/include
+HOST_CPPFLAGS  = -isystem $(HOST_DIR)/include
 HOST_CFLAGS   ?= -O2
 HOST_CFLAGS   += $(HOST_CPPFLAGS)
 HOST_CXXFLAGS += $(HOST_CFLAGS)
-- 
2.13.6

[Buildroot] [PATCH 2/3] Revert "linux/linux.mk: Use '-isystem' instead of '-I' in HOST_CFLAGS"

[David De Grave]

Since we made the use of '-isystem' global in a previous commit for
HOST_CFLAGS, that change is no longer needed.

This reverts commit 14a842315c12983eb492dfa310a9c358b7336da6.

Signed-off-by: David De Grave (Essensium/Mind) <david.degrave@mind.be>
---
 linux/linux.mk | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/linux/linux.mk b/linux/linux.mk
index a31617ca22..675e7906a8 100644
--- a/linux/linux.mk
+++ b/linux/linux.mk
@@ -115,7 +115,7 @@ LINUX_EXTRA_DOWNLOADS += $(ARCH_XTENSA_OVERLAY_URL)
 endif
 
 LINUX_MAKE_FLAGS = \
-	HOSTCC="$(HOSTCC) $(patsubst -I%,-isystem %,$(HOST_CFLAGS)) $(HOST_LDFLAGS)" \
+	HOSTCC="$(HOSTCC) $(HOST_CFLAGS) $(HOST_LDFLAGS)" \
 	ARCH=$(KERNEL_ARCH) \
 	INSTALL_MOD_PATH=$(TARGET_DIR) \
 	CROSS_COMPILE="$(TARGET_CROSS)" \
-- 
2.13.6

[Buildroot] [PATCH 3/3] boot/uboot/uboot.mk: substitution for '-I/' to '-isystem /' no longer needed

[David De Grave]

As the '-isystem' is now used globally, the uboot's substitution to define
HOSTCC is no longer needed too.

This reverts commit 0bf80e4bcd5277e3ad935e03c632deba4c7316f2
         and commit baae5156ce37e8b2775f04710f7d1c8e97e4114c

Signed-off-by: David De Grave (Essensium/Mind) <david.degrave@mind.be>
---
 boot/uboot/uboot.mk | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/boot/uboot/uboot.mk b/boot/uboot/uboot.mk
index bddafe234d..7cb9859565 100644
--- a/boot/uboot/uboot.mk
+++ b/boot/uboot/uboot.mk
@@ -132,7 +132,7 @@ endif
 UBOOT_MAKE_OPTS += \
 	CROSS_COMPILE="$(TARGET_CROSS)" \
 	ARCH=$(UBOOT_ARCH) \
-	HOSTCC="$(HOSTCC) $(subst -I/,-isystem /,$(subst -I /,-isystem /,$(HOST_CFLAGS)))" \
+	HOSTCC="$(HOSTCC) $(HOST_CFLAGS)" \
 	HOSTLDFLAGS="$(HOST_LDFLAGS)"
 
 ifeq ($(BR2_TARGET_UBOOT_NEEDS_ATF_BL31),y)
-- 
2.13.6

[Buildroot] [PATCH 1/3] package/Makefile.in: Use '-isystem' instead of '-I' in HOST_CFLAGS globally

[Thomas Petazzoni]

Hello David,

On Fri, 31 Aug 2018 16:20:22 +0200, David De Grave (Essensium/Mind)
wrote:
>  As the $(HOST_DIR) is the buildroot's system install directory, and that
> all the builds should use that one in priority (it's even mandatory for
> uboot and linux), it make sense to use '-isystem' to ensure it is searched
> right before the system directories and still let the possibility for the
> packages to use specific ones using '-I'.

Could you give a few more details about the motivation for this change.
Is it just because it is "cleaner" to refer to Buildroot
$(HOST_DIR)/include using -isystem, or does this fix some actual
problem/build issue ?

Doing such a change is pretty invasive, it could potentially cause
some build failures, so I'd like to understand the motivation.

Thanks!

Thomas
-- 
Thomas Petazzoni, CTO, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com

[Buildroot] [PATCH 1/3] package/Makefile.in: Use '-isystem' instead of '-I' in HOST_CFLAGS globally

[Arnout Vandecappelle]

On 09/09/2018 15:32, Thomas Petazzoni wrote:
> Hello David,
> 
> On Fri, 31 Aug 2018 16:20:22 +0200, David De Grave (Essensium/Mind)
> wrote:
>>  As the $(HOST_DIR) is the buildroot's system install directory, and that
>> all the builds should use that one in priority (it's even mandatory for
>> uboot and linux), it make sense to use '-isystem' to ensure it is searched
>> right before the system directories and still let the possibility for the
>> packages to use specific ones using '-I'.
> 
> Could you give a few more details about the motivation for this change.
> Is it just because it is "cleaner" to refer to Buildroot
> $(HOST_DIR)/include using -isystem, or does this fix some actual
> problem/build issue ?

 It fixes the existing problem for u-boot and linux in a more generic way. Also,
the same issue might exist for any host package that we have. The exported
headers are often in an 'include' directory that is accessed with a -I flag. If
that -I flag is added after the CFLAGS passed in by us, then the compiler will
first find the host-installed headers rather than the local headers.
Autobuilders will usually not find such an issue, because they have a rather
minimal set of devel packages installed on the system.

 It's true that these issues are rare: normally the local -I flags are added
*before* the CFLAGS passed in by the user. However, I'm pretty sure that
packages do exist that don't do this (e.g. if the Makefile just does 'CFLAGS +=
...'). It's pretty hard to detect such cases, because *usually* it doesn't give
an error (because the system headers are probably compatible with the local
headers).

 Regards,
 Arnout

> 
> Doing such a change is pretty invasive, it could potentially cause
> some build failures, so I'd like to understand the motivation.
> 
> Thanks!
> 
> Thomas
> 

-- 
Arnout Vandecappelle                          arnout at mind be
Senior Embedded Software Architect            +32-16-286500
Essensium/Mind                                http://www.mind.be
G.Geenslaan 9, 3001 Leuven, Belgium           BE 872 984 063 RPR Leuven
LinkedIn profile: http://www.linkedin.com/in/arnoutvandecappelle
GPG fingerprint:  7493 020B C7E3 8618 8DEC 222C 82EB F404 F9AC 0DDF

[Buildroot] [PATCH 1/3] package/Makefile.in: Use '-isystem' instead of '-I' in HOST_CFLAGS globally

[David De Grave]

Hello Thomas, Arnout,

On Mon, Sep 10, 2018 at 11:51 AM, Arnout Vandecappelle <arnout@mind.be>
wrote:


> > Could you give a few more details about the motivation for this change.
> > Is it just because it is "cleaner" to refer to Buildroot
> > $(HOST_DIR)/include using -isystem, or does this fix some actual
> > problem/build issue ?
>

More than making things cleaner/logic, it fixes a build issue I've with the
beagle bone
black's Linux kernel: https://github.com/beagleboard/linux.git

They implemented a driver who handle DTBs overlays and what they call
"capes"
(PCB extensions to the bbb mother board). Since the update of the DTC
package few
weeks ago, my build is throwing compilation errors in linux when it starts
to compile
their own version of the dtc. Due to the way they include their headers,
some of them
were taken from the host while some others are taken from local dir.
Moreover, their
dtc version use different names in their defines (a missing underscore) and
this why
at a certain point, it throws a "function redefinition error". Maybe other
kernels are
suffering from the same problem, I don't know...

I first fixed the issue by adding a patch to my project to change the
involved sources
in the Kernel tree...

But then I thought that other people may suffers from this problem too and
decided to
make it more generic by patching buildroot itself, so that I could save
them few hours
of investigations. This is why I submitted my first patch who applied the
same kind of
change in the linux.mk file.

Then, I discussed this with Arnout and we thought it would be even more
benefit to
make it global as it's already a requirement for uboot and now linux too.
Moreover it
sounds more logic/cleaner to make it that way, but that was not the main
idea.

So, that's why I submitted this patch series.

> Doing such a change is pretty invasive, it could potentially cause
> > some build failures, so I'd like to understand the motivation.
>

I understand, no problem :-)

Regards,
David.

-- 

*David De Grave*
Senior Embedded Software Developer
Gsm : +32(0)496.364.960 | Tel : +32-16-28.65.00 | Fax : +32-16-28a.65.01
Essensium-Mind <https://www.mind.be/> - Gaston Geenslaan 9, B-3001 Leuven,
Belgium
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.busybox.net/pipermail/buildroot/attachments/20180910/1fdc15a7/attachment.html>

[Buildroot] [PATCH 1/3] package/Makefile.in: Use '-isystem' instead of '-I' in HOST_CFLAGS globally

[Yann E. MORIN]

David, All,

On 2018-09-10 15:00 +0200, David De Grave spake thusly:
> Hello Thomas, Arnout,
> On Mon, Sep 10, 2018 at 11:51 AM, Arnout Vandecappelle < [1]arnout@mind.be> wrote:
> ?
> 
>   > Could you give a few more details about the motivation for this change.
>   > Is it just because it is "cleaner" to refer to Buildroot
>   > $(HOST_DIR)/include using -isystem, or does this fix some actual
>   > problem/build issue ?
> 
> More than making things cleaner/logic, it fixes a build issue I've with the beagle bone
> black's Linux kernel: [2]https://github.com/beagleboard/linux.git
> They implemented a driver who handle DTBs overlays and what they call "capes"
> (PCB extensions to the bbb mother board). Since the update of the DTC package few
> weeks ago, my build is throwing compilation errors in linux when it starts to compile
> their own version of the dtc. Due to the way they include their headers, some of them
> were taken from the host while some others are taken from local dir. Moreover, their
> dtc version use different names in their defines (a missing underscore) and this why
> at a certain point, it throws a "function redefinition error". Maybe other kernels are
> suffering from the same problem, I don't know...
> I first fixed the issue by adding a patch to my project to change the involved sources
> in the Kernel tree...
> But then I thought that other people may suffers from this problem too and decided to
> make it more generic by patching buildroot itself, so that I could save them few hours
> of investigations. This is why I submitted my first patch who applied the same kind of
> change in the [3]linux.mk file.
> Then, I discussed this with Arnout and we thought it would be even more benefit to
> make it global as it's already a requirement for uboot and now linux too. Moreover it
> sounds more logic/cleaner to make it that way, but that was not the main idea.
> So, that's why I submitted this patch series.

The -isystem has already been tried, and lately resurfaced in another
thread, in which it was noticed that it broke at least a few packages,
please see:

    http://lists.busybox.net/pipermail/buildroot/2018-October/232615.html

So, before we reinstate the use of -isystem, a few packages will have to
be fixed first.

Thanks!

Regards,
Yann E. MORIN.

>   > Doing such a change is pretty invasive, it could potentially cause
>   > some build failures, so I'd like to understand the motivation.
> 
> I understand, no problem :-)
> Regards,
> David.
> --
> 
> +--------------------------------------------------------------------------------------------------------+
> |        | David De Grave                                                                                |
> | ? | Senior Embedded Software Developer                                                            |
> |        | Gsm : +32(0)496.364.960 | Tel : +32-16-28.65.00?| Fax?:?+32-16-28a.65.01? |
> |        | [4]Essensium-Mind - Gaston Geenslaan 9, B-3001 Leuven, Belgium                                |
> +--------------------------------------------------------------------------------------------------------+
> 
> Links:
> 1. mailto:arnout at mind.be
> 2. https://github.com/beagleboard/linux.git
> 3. http://linux.mk
> 4. https://www.mind.be/

> _______________________________________________
> buildroot mailing list
> buildroot at busybox.net
> http://lists.busybox.net/mailman/listinfo/buildroot


-- 
.-----------------.--------------------.------------------.--------------------.
|  Yann E. MORIN  | Real-Time Embedded | /"\ ASCII RIBBON | Erics' conspiracy: |
| +33 662 376 056 | Software  Designer | \ / CAMPAIGN     |  ___               |
| +33 223 225 172 `------------.-------:  X  AGAINST      |  \e/  There is no  |
| http://ymorin.is-a-geek.org/ | _/*\_ | / \ HTML MAIL    |   v   conspiracy.  |
'------------------------------^-------^------------------^--------------------'