Broken include paths

[Myles Watson]

There are a couple of boards that don't compile for me with crossgcc.
Here's a representative error:

src/mainboard/asus/a8v-e_se/mptable.c:23:54: error:
src/include/../../../southbridge/via/vt8237r/vt8237r.h: Permission
denied

The problem is that the compiler is looking for the file
../../../southbridge...  starting from src/include, since it was
specified with <>

There are many ways to fix it, but I'm not sure which one is the
correct (most future-proof) way.

Here are three of the possible fixes:

1. Use "" so the path is relative to the file.

Index: svn/src/mainboard/asus/a8v-e_se/mptable.c
===================================================================
--- svn/src/mainboard/asus/a8v-e_se/mptable.c	(revision 5667)
+++ svn/src/mainboard/asus/a8v-e_se/mptable.c	(working copy)
@@ -20,8 +20,8 @@
 #include <string.h>
 #include <stdint.h>
 #include <arch/smp/mpspec.h>
-#include <../../../southbridge/via/vt8237r/vt8237r.h>
-#include <../../../southbridge/via/k8t890/k8t890.h>
+#include "../../../southbridge/via/vt8237r/vt8237r.h"
+#include "../../../southbridge/via/k8t890/k8t890.h"

static void *smp_write_config_table(void *v)
 {

2. Make the path valid from src/include

Index: svn/src/mainboard/asus/a8v-e_se/mptable.c
===================================================================
--- svn/src/mainboard/asus/a8v-e_se/mptable.c	(revision 5667)
+++ svn/src/mainboard/asus/a8v-e_se/mptable.c	(working copy)
@@ -20,8 +20,8 @@
 #include <string.h>
 #include <stdint.h>
 #include <arch/smp/mpspec.h>
-#include <../../../southbridge/via/vt8237r/vt8237r.h>
-#include <../../../southbridge/via/k8t890/k8t890.h>
+#include <../southbridge/via/vt8237r/vt8237r.h>
+#include <../southbridge/via/k8t890/k8t890.h>

static void *smp_write_config_table(void *v)
 {

3. Just use the path

Index: svn/src/mainboard/asus/a8v-e_se/mptable.c
===================================================================
--- svn.orig/src/mainboard/asus/a8v-e_se/mptable.c
+++ svn/src/mainboard/asus/a8v-e_se/mptable.c
@@ -20,8 +20,8 @@
 #include <string.h>
 #include <stdint.h>
 #include <arch/smp/mpspec.h>
-#include <../../../southbridge/via/vt8237r/vt8237r.h>
-#include <../../../southbridge/via/k8t890/k8t890.h>
+#include <southbridge/via/vt8237r/vt8237r.h>
+#include <southbridge/via/k8t890/k8t890.h>

static void *smp_write_config_table(void *v)
 {

They all compile for me.

Thanks,
Myles

[Peter Stuge]

Myles Watson wrote:
> There are many ways to fix it, but I'm not sure which one is the
> correct (most future-proof) way.

..
> Just use the path

I think this is *by far* the cleanest approach!

//Peter

[Myles Watson]

On Mon, Jul 26, 2010 at 11:09 AM, Peter Stuge  wrote:
> Myles Watson wrote:
> > There are many ways to fix it, but I'm not sure which one is the
> > correct (most future-proof) way.
>
> ..
> > Just use the path
>
> I think this is *by far* the cleanest approach!

I agree that it looks the best.  I'm worried that it introduces ambiguity.

#include <path/file.h>

Could look in src/path/file.h or src/include/path/file.h and others

Is that what we want?  Should we remove -I$(src) from the command line
in the long term?

from src/arch/i386/Makefile.bootblock.inc:

$(CC) -MMD -x assembler-with-cpp -DASSEMBLY -E -I$(src)/include
-I$(src)/arch/i386/include -I$(obj) -I$(obj)/bootblock -include
$(obj)/config.h -I. -I$(src) $< -o $@

It seems like it could be simpler.  I also don't understand the order.

Thanks,
Myles

[Peter Stuge]

Myles Watson wrote:
> > > Just use the path
> >
> > I think this is *by far* the cleanest approach!
>
> I agree that it looks the best.  I'm worried that it introduces
> ambiguity.
>
> #include <path/file.h>
>
> Could look in src/path/file.h or src/include/path/file.h and others
>
> Is that what we want?  Should we remove -I$(src) from the command line
> in the long term?

I'm not sure that I feel good about .h files outside include/ being
referenced from other parts of the code. They should probably be
moved to include/ if they are needed in more than one place..
> from src/arch/i386/Makefile.bootblock.inc:
>
> $(CC) -MMD -x assembler-with-cpp -DASSEMBLY -E -I$(src)/include
> -I$(src)/arch/i386/include -I$(obj) -I$(obj)/bootblock -include
> $(obj)/config.h -I. -I$(src) $< -o $@
>
> It seems like it could be simpler.

I think simplifying the codebase will be a continuous effort.
> I also don't understand the order.

Me neither. And again, why are there include files in
src/arch/i386/include instead of include/arch-i386 or something?

//Peter

[Patrick Georgi]

Am 26.07.2010 20:01, schrieb Peter Stuge:
> I'm not sure that I feel good about .h files outside include/ being
> referenced from other parts of the code. They should probably be
> moved to include/ if they are needed in more than one place..

Mainboards need to include chipset specific information, and I'd like to
avoid moving all that into include/ if possible.
> > I also don't understand the order.
>
> Me neither. And again, why are there include files in
> src/arch/i386/include instead of include/arch-i386 or something?

It was partially inherited from newconfig, partially "what works". And I
guess that was how it was handled throughout newconfig's lifecycle, too.

Patrick

[Myles Watson]

On Mon, Jul 26, 2010 at 12:57 PM, Patrick Georgi  wrote:
> Am 26.07.2010 20:01, schrieb Peter Stuge:
> > I'm not sure that I feel good about .h files outside include/ being
> > referenced from other parts of the code. They should probably be
> > moved to include/ if they are needed in more than one place..
>
> Mainboards need to include chipset specific information, and I'd like to
> avoid moving all that into include/ if possible.

I think that's a reasonable exception to the rule.  Some of that code
could move to the chipsets, but not all of it.

Thanks,
Myles

[Myles Watson]

On Mon, Jul 26, 2010 at 12:01 PM, Peter Stuge  wrote:
> Myles Watson wrote:
> > > > Just use the path
> > >
> > > I think this is *by far* the cleanest approach!
> >
> > I agree that it looks the best.  I'm worried that it introduces
> > ambiguity.
> >
> > #include <path/file.h>
> >
> > Could look in src/path/file.h or src/include/path/file.h and others
> >
> > Is that what we want?  Should we remove -I$(src) from the command line
> > in the long term?

It won't be, but I think it should only be used for including .c files
for romcc.
> I'm not sure that I feel good about .h files outside include/ being
> referenced from other parts of the code. They should probably be
> moved to include/ if they are needed in more than one place..

So I guess option 1 is the best.  It makes it obvious (and ugly) when
that rule is ignored.
> > from src/arch/i386/Makefile.bootblock.inc:
> >
> > $(CC) -MMD -x assembler-with-cpp -DASSEMBLY -E -I$(src)/include
> > -I$(src)/arch/i386/include -I$(obj) -I$(obj)/bootblock -include
> > $(obj)/config.h -I. -I$(src) $< -o $@

I forgot about all of our included .c files.  That's the reason for -I$(src).
> And again, why are there include files in
> src/arch/i386/include instead of include/arch-i386 or something?

Linux does it that way.  It keeps all of the architecture-specific
code and includes under arch/

include_path.diff: fix the ones that are broken for me.
include_path2.diff: fix the ones that look identical but work anyway.
include_path3.diff: fix <../path/file.h> to be "../../../path/file.h"
to make it obvious that they're not in src/include

Abuild tested.

Signed-off-by: Myles Watson

Thanks,
Myles

[Peter Stuge]

Patrick Georgi wrote:
> > .h files outside include/
> > They should probably be moved to include/
>
> Mainboards need to include chipset specific information, and I'd
> like to avoid moving all that into include/ if possible.

Why not do it?

Myles Watson wrote:
> I think that's a reasonable exception to the rule.

Why have an exception at all?
> Some of that code could move to the chipsets, but not all of it.

Hmm, can you clarify?

//Peter

[Myles Watson]

On Mon, Jul 26, 2010 at 1:22 PM, Peter Stuge  wrote:
> Patrick Georgi wrote:
> > > .h files outside include/
> > > They should probably be moved to include/
> >
> > Mainboards need to include chipset specific information, and I'd
> > like to avoid moving all that into include/ if possible.
>
> Why not do it?

It's used in multiple places, but only two or three usually.
> Myles Watson wrote:
> > I think that's a reasonable exception to the rule.
>
> Why have an exception at all?

So src/include/ doesn't get cluttered.
> > Some of that code could move to the chipsets, but not all of it.
>
> Hmm, can you clarify?

I was thinking of some of the ACPI code, that is not
mainboard-dependent but chipset-dependent.  That's been slowly moving
to the chipset directories.

Some of the mainboard initialization code calls functions depending on
what's on the board.  That won't be moved.

Thanks,
Myles

[Stefan Reinauer]

On 7/26/10 9:28 PM, Myles Watson wrote:
> I was thinking of some of the ACPI code, that is not
> mainboard-dependent but chipset-dependent.  That's been slowly moving
> to the chipset directories.

If we can get rid of exceptions by cleaning more code up in this way we
should certainly do it.

Stefan