buildrom bug

[Peter Stuge]

I configured v2 and TINT, then:

$ make
/home/stuge/co/buildrom/buildrom-devel/packages/grub2/grub2.mk:28:
*** To build GRUB2, you need to install 'ruby'.  Stop.

I don't want to build GRUB2.

//Peter

[Uwe Hermann]

On Thu, Apr 24, 2008 at 06:32:17AM +0200, Peter Stuge wrote:
> I configured v2 and TINT, then:

Cannot reproduce using ALIX.1C target in buildrom. Which target did you
use? Do you have some .config* .tmpconfig* .kconfig* files around?
Try a fresh checkout, just in case.
> $ make
> /home/stuge/co/buildrom/buildrom-devel/packages/grub2/grub2.mk:28:
> *** To build GRUB2, you need to install 'ruby'.  Stop.
>
> I don't want to build GRUB2.

Uwe.

[Peter Stuge]

On Thu, Apr 24, 2008 at 11:21:41AM +0200, Uwe Hermann wrote:
> On Thu, Apr 24, 2008 at 06:32:17AM +0200, Peter Stuge wrote:
> > I configured v2 and TINT, then:
>
> Cannot reproduce using ALIX.1C target in buildrom.

Maybe you have ruby installed.
> Which target did you use?

I thought I picked 1C but it seems I was using DB800.
> Do you have some .config* .tmpconfig* .kconfig* files around?
> Try a fresh checkout, just in case.

It was a fresh checkout. Am retrying.
> > $ make
> > /home/stuge/co/buildrom/buildrom-devel/packages/grub2/grub2.mk:28:
> > *** To build GRUB2, you need to install 'ruby'.  Stop.

Still the same.

--8<-- grub2.mk
HAVE_RUBY:=$(call find-tool,ruby)

ifeq ($(HAVE_RUBY),n)
$(error To build GRUB2, you need to install 'ruby')
endif
-->8--

The problem is that this is executed even when a different payload
has been selected.

//Peter

[Ward Vandewege]

On Thu, Apr 24, 2008 at 12:54:20PM +0200, Peter Stuge wrote:
> On Thu, Apr 24, 2008 at 11:21:41AM +0200, Uwe Hermann wrote:
> > On Thu, Apr 24, 2008 at 06:32:17AM +0200, Peter Stuge wrote:
> > > I configured v2 and TINT, then:
> >
> > Cannot reproduce using ALIX.1C target in buildrom.
>
> Maybe you have ruby installed.
> > Which target did you use?
>
> I thought I picked 1C but it seems I was using DB800.
> > Do you have some .config* .tmpconfig* .kconfig* files around?
> > Try a fresh checkout, just in case.
>
> It was a fresh checkout. Am retrying.
> > > $ make
> > > /home/stuge/co/buildrom/buildrom-devel/packages/grub2/grub2.mk:28:
> > > *** To build GRUB2, you need to install 'ruby'.  Stop.
>
> Still the same.

It's a bug. This is a workaround (I'm still working on a proper fix, 
make should not include all payload makefiles all the time):

-------------------------------------
Index: packages/grub2/grub2.mk
===================================================================
--- packages/grub2/grub2.mk     (revision 155)
+++ packages/grub2/grub2.mk     (working copy)
@@ -24,9 +24,11 @@

HAVE_RUBY:=$(call find-tool,ruby)

+ifeq ($(PAYLOAD),grub2)
 ifeq ($(HAVE_RUBY),n)
 $(error To build GRUB2, you need to install 'ruby')
 endif
+endif

$(SOURCE_DIR)/$(GRUB2_TAR):
        @ mkdir -p $(SOURCE_DIR)
-------------------------------------

Thanks,
Ward.

[Jordan Crouse]

On 24/04/08 09:54 -0400, Ward Vandewege wrote:
> On Thu, Apr 24, 2008 at 12:54:20PM +0200, Peter Stuge wrote:
> > On Thu, Apr 24, 2008 at 11:21:41AM +0200, Uwe Hermann wrote:
> > > On Thu, Apr 24, 2008 at 06:32:17AM +0200, Peter Stuge wrote:
> > > > I configured v2 and TINT, then:
> > >
> > > Cannot reproduce using ALIX.1C target in buildrom.
> >
> > Maybe you have ruby installed.
> > > Which target did you use?
> >
> > I thought I picked 1C but it seems I was using DB800.
> > > Do you have some .config* .tmpconfig* .kconfig* files around?
> > > Try a fresh checkout, just in case.
> >
> > It was a fresh checkout. Am retrying.
> > > > $ make
> > > > /home/stuge/co/buildrom/buildrom-devel/packages/grub2/grub2.mk:28:
> > > > *** To build GRUB2, you need to install 'ruby'.  Stop.
> >
> > Still the same.
>
> It's a bug. This is a workaround (I'm still working on a proper fix, 
> make should not include all payload makefiles all the time):

Thanks - I ack this if we need to, but ++ on fixing the includes
properly.
> Index: packages/grub2/grub2.mk
>
> --- packages/grub2/grub2.mk     (revision 155)
> +++ packages/grub2/grub2.mk     (working copy)
> @@ -24,9 +24,11 @@
>
> HAVE_RUBY:=$(call find-tool,ruby)
>
> +ifeq ($(PAYLOAD),grub2)
>  ifeq ($(HAVE_RUBY),n)
>  $(error To build GRUB2, you need to install 'ruby')
>  endif
> +endif
>
> $(SOURCE_DIR)/$(GRUB2_TAR):
>         @ mkdir -p $(SOURCE_DIR)
>
> Thanks,
> Ward.

[Myles Watson]

On Thu, Apr 24, 2008 at 8:38 AM, Jordan Crouse  wrote:
> On 24/04/08 09:54 -0400, Ward Vandewege wrote:
> > On Thu, Apr 24, 2008 at 12:54:20PM +0200, Peter Stuge wrote:
> > > On Thu, Apr 24, 2008 at 11:21:41AM +0200, Uwe Hermann wrote:
> > > > On Thu, Apr 24, 2008 at 06:32:17AM +0200, Peter Stuge wrote:
> > > > > I configured v2 and TINT, then:
> > > >
> > > > Cannot reproduce using ALIX.1C target in buildrom.
> > >
> > > Maybe you have ruby installed.
> > > > Which target did you use?
> > >
> > > I thought I picked 1C but it seems I was using DB800.
> > > > Do you have some .config* .tmpconfig* .kconfig* files around?
> > > > Try a fresh checkout, just in case.
> > >
> > > It was a fresh checkout. Am retrying.
> > > > > $ make
> > > > > /home/stuge/co/buildrom/buildrom-devel/packages/grub2/grub2.mk:28:
> > > > > *** To build GRUB2, you need to install 'ruby'.  Stop.
> > >
> > > Still the same.
> >
> > It's a bug. This is a workaround (I'm still working on a proper fix,
> > make should not include all payload makefiles all the time):

This patch changes the Makefile and the config/payloads/*.conf files so that
only the payload .mk files that are needed are included.  It still includes
kernel.mk, roms.mk and geodevsa.mk every time, but that seemed like a separate
change.

Thanks,
Myles

Signed-off-by: Myles Watson

[Ward Vandewege]

Hi Myles,

On Fri, Apr 25, 2008 at 08:49:00AM -0600, Myles Watson wrote:
> This patch changes the Makefile and the config/payloads/*.conf files so that
> only the payload .mk files that are needed are included.  It still includes
> kernel.mk, roms.mk and geodevsa.mk every time, but that seemed like a separate
> change.

Great start.

Don't we need that PAYLOAD_AND_DEP_MK change for every payload?

Also; looks like we need one extra small change (see below).
> Index: config/payloads/etherboot.conf
>
> --- config/payloads/etherboot.conf	(revision 160)
> +++ config/payloads/etherboot.conf	(working copy)
> @@ -8,3 +8,4 @@
>  PAYLOAD_COMPRESSED=$(OUTPUT_DIR)/etherboot-payload.elf.lzma
>
> PAYLOAD-y=etherboot
> +PAYLOAD_AND_DEP_MK=$(PACKAGE_DIR)/etherboot/etherboot.mk
> Index: config/payloads/ofw.conf
> ===================================================================
> --- config/payloads/ofw.conf	(revision 160)
> +++ config/payloads/ofw.conf	(working copy)
> @@ -12,3 +12,4 @@
>
> PAYLOAD-y=ofw
>  #HOSTTOOLS-y=crc32sum
> +PAYLOAD_AND_DEP_MK=$(PACKAGE_DIR)/ofw/ofw.mk
> Index: config/payloads/memtest.conf
> ===================================================================
> --- config/payloads/memtest.conf	(revision 160)
> +++ config/payloads/memtest.conf	(working copy)
> @@ -1,4 +1,4 @@
> -# Configuration file for the etherboot payload
> +# Configuration file for the memtest payload
>
> # Common configuration options
>
> @@ -8,3 +8,5 @@
>  PAYLOAD_COMPRESSED=$(OUTPUT_DIR)/memtest-payload.elf.lzma
>
> PAYLOAD-y=memtest
>
> +PAYLOAD_AND_DEP_MK=$(PACKAGE_DIR)/memtest/memtest.mk
> Index: config/payloads/tint.conf
> ===================================================================
> --- config/payloads/tint.conf	(revision 160)
> +++ config/payloads/tint.conf	(working copy)
> @@ -10,5 +10,7 @@
>  PAYLOAD-y=tint
>  PAYLOAD=tint
>
> +PAYLOAD_AND_DEP_MK=$(PACKAGE_DIR)/libpayload/libpayload.mk $(PACKAGE_DIR)/tint/tint.mk
>
> # Add libpayload as a dependency
>  DEPENDS-y=libpayload
> Index: config/payloads/grub2.conf
> ===================================================================
> --- config/payloads/grub2.conf	(revision 160)
> +++ config/payloads/grub2.conf	(working copy)
> @@ -9,3 +9,5 @@
>
> PAYLOAD-y=grub2
>  PAYLOAD=grub2
>
> +PAYLOAD_AND_DEP_MK=$(PACKAGE_DIR)/grub2/grub2.mk
> Index: config/payloads/coreinfo.conf
> ===================================================================
> --- config/payloads/coreinfo.conf	(revision 160)
> +++ config/payloads/coreinfo.conf	(working copy)
> @@ -11,4 +11,5 @@
>  PAYLOAD=coreinfo
>
> # Add libpayload as a dependency
> +PAYLOAD_AND_DEP_MK=$(PACKAGE_DIR)/coreinfo/coreinfo.mk $(PACKAGE_DIR)/libpayload/libpayload.mk
>  DEPENDS-y=libpayload
> Index: config/payloads/filo.conf
> ===================================================================
> --- config/payloads/filo.conf	(revision 160)
> +++ config/payloads/filo.conf	(working copy)
> @@ -8,6 +8,7 @@
>  PAYLOAD_COMPRESSED=$(OUTPUT_DIR)/filo-payload.elf.lzma
>
> PAYLOAD-y=filo
> +PAYLOAD_AND_DEP_MK=$(PACKAGE_DIR)/filo/filo.mk
>
> PAYLOAD=filo
> Index: Makefile
>
> --- Makefile	(revision 160)
> +++ Makefile	(working copy)
> @@ -117,9 +117,10 @@
>  # The following code gets all the make targets, but filters out the kernel
>  # targets which are implicitly set by the platform configuration
>
> -MKTARGETS:= $(shell ls $(PACKAGE_DIR)/*/*.mk)
> +include $(PAYLOAD_AND_DEP_MK)
>
> -include $(filter-out $(PACKAGE_DIR)/kernel/% $(PACKAGE_DIR)/coreboot-v2/% $(PACKAGE_DIR)/coreboot-v3/%,$(MKTARGETS))
> +include $(PACKAGE_DIR)/geodevsa/geodevsa.mk
> +include $(PACKAGE_DIR)/roms/roms.mk

I needed to add

+include $(PACKAGE_DIR)/utils/nrv2b.mk

to get make to behave.

This also brings up the question why nrv2b.mk is in the 'utils' directory,
instead of the $(PACKAGE_DIR)/nrv2b/ directory?

Thanks,
Ward.

[Myles Watson]

> Hi Myles,
>
> On Fri, Apr 25, 2008 at 08:49:00AM -0600, Myles Watson wrote:
> > This patch changes the Makefile and the config/payloads/*.conf files so
>
> that
> > only the payload .mk files that are needed are included.  It still
>
> includes
> > kernel.mk, roms.mk and geodevsa.mk every time, but that seemed like a
>
> separate
> > change.
>
> Great start.
>
> Don't we need that PAYLOAD_AND_DEP_MK change for every payload?

Did I miss one?  Most of the payloads don't have dependencies.  The
libpayload ones were the only ones I saw.
> Also; looks like we need one extra small change (see below).
> > Index: Makefile
> >
> > --- Makefile	(revision 160)
> > +++ Makefile	(working copy)
> > @@ -117,9 +117,10 @@
> >  # The following code gets all the make targets, but filters out the
>
> kernel
> > # targets which are implicitly set by the platform configuration
> >
> > -MKTARGETS:= $(shell ls $(PACKAGE_DIR)/*/*.mk)
> > +include $(PAYLOAD_AND_DEP_MK)
> >
> > -include $(filter-out $(PACKAGE_DIR)/kernel/% $(PACKAGE_DIR)/coreboot-
>
> v2/% $(PACKAGE_DIR)/coreboot-v3/%,$(MKTARGETS))
> > +include $(PACKAGE_DIR)/geodevsa/geodevsa.mk
> > +include $(PACKAGE_DIR)/roms/roms.mk
>
> I needed to add
>
> +include $(PACKAGE_DIR)/utils/nrv2b.mk

Yep, I missed that one.  Fixed.
> This also brings up the question why nrv2b.mk is in the 'utils' directory,
> instead of the $(PACKAGE_DIR)/nrv2b/ directory?

Because it doesn't get included in the ROM?

Thanks,
Myles

[Ward Vandewege]

On Fri, Apr 25, 2008 at 09:20:23AM -0600, Myles Watson wrote:
> > Don't we need that PAYLOAD_AND_DEP_MK change for every payload?
>
> Did I miss one?  Most of the payloads don't have dependencies.  The
> libpayload ones were the only ones I saw.

The LAB and kernel payloads?
> > +include $(PACKAGE_DIR)/utils/nrv2b.mk
>
> Yep, I missed that one.  Fixed.

Great.
> > This also brings up the question why nrv2b.mk is in the 'utils' directory,
> > instead of the $(PACKAGE_DIR)/nrv2b/ directory?
>
> Because it doesn't get included in the ROM?

Hmm, ok, yes.

Thanks,
Ward.

[Jordan Crouse]

On 25/04/08 11:37 -0400, Ward Vandewege wrote:
> On Fri, Apr 25, 2008 at 09:20:23AM -0600, Myles Watson wrote:
> > > Don't we need that PAYLOAD_AND_DEP_MK change for every payload?
> >
> > Did I miss one?  Most of the payloads don't have dependencies.  The
> > libpayload ones were the only ones I saw.
>
> The LAB and kernel payloads?
> > > +include $(PACKAGE_DIR)/utils/nrv2b.mk
> >
> > Yep, I missed that one.  Fixed.
>
> Great.
> > > This also brings up the question why nrv2b.mk is in the 'utils' directory,
> > > instead of the $(PACKAGE_DIR)/nrv2b/ directory?
> >
> > Because it doesn't get included in the ROM?
>
> Hmm, ok, yes.

I guess it all depends on how many directories we want to look at,
and trade that off with how complex the scripts should be.  I don't
really have an opinion one way or the other on that.

Jordan

[Myles Watson]

> -----Original Message-----
> From: [e-mail] [mailto:[e-mail]]
> On Behalf Of Jordan Crouse
> Sent: Friday, April 25, 2008 9:40 AM
> To: Myles Watson; [e-mail]
> Subject: Re: [coreboot] buildrom bug
>
> On 25/04/08 11:37 -0400, Ward Vandewege wrote:
> > On Fri, Apr 25, 2008 at 09:20:23AM -0600, Myles Watson wrote:
> > > > Don't we need that PAYLOAD_AND_DEP_MK change for every payload?
> > >
> > > Did I miss one?  Most of the payloads don't have dependencies.  The
> > > libpayload ones were the only ones I saw.
> >
> > The LAB and kernel payloads?
> > > > +include $(PACKAGE_DIR)/utils/nrv2b.mk
> > >
> > > Yep, I missed that one.  Fixed.
> >
> > Great.
> > > > This also brings up the question why nrv2b.mk is in the 'utils'
>
> directory,
> > > > instead of the $(PACKAGE_DIR)/nrv2b/ directory?
> > >
> > > Because it doesn't get included in the ROM?
> >
> > Hmm, ok, yes.
>
> I guess it all depends on how many directories we want to look at,
> and trade that off with how complex the scripts should be.  I don't
> really have an opinion one way or the other on that.

I'm happy to move it.  I was just speculating on the original reason it was
there.  Unfortunately, lzma is in $(PACKAGE_DIR)/lzma/, which shoots down my
theory.  I think that we should avoid automagically including make files.
There are few enough of them that we should only include specific ones to
avoid problems.

BTW I also added the include for lzma in the same place as the nrvb.mk file.

Myles

[Jordan Crouse]

On 25/04/08 10:13 -0600, Myles Watson wrote:
> > -----Original Message-----
> > From: [e-mail] [mailto:[e-mail]]
> > On Behalf Of Jordan Crouse
> > Sent: Friday, April 25, 2008 9:40 AM
> > To: Myles Watson; [e-mail]
> > Subject: Re: [coreboot] buildrom bug
> >
> > On 25/04/08 11:37 -0400, Ward Vandewege wrote:
> > > On Fri, Apr 25, 2008 at 09:20:23AM -0600, Myles Watson wrote:
> > > > > Don't we need that PAYLOAD_AND_DEP_MK change for every payload?
> > > >
> > > > Did I miss one?  Most of the payloads don't have dependencies.  The
> > > > libpayload ones were the only ones I saw.
> > >
> > > The LAB and kernel payloads?
> > > > > +include $(PACKAGE_DIR)/utils/nrv2b.mk
> > > >
> > > > Yep, I missed that one.  Fixed.
> > >
> > > Great.
> > > > > This also brings up the question why nrv2b.mk is in the 'utils'
> >
> > directory,
> > > > > instead of the $(PACKAGE_DIR)/nrv2b/ directory?
> > > >
> > > > Because it doesn't get included in the ROM?
> > >
> > > Hmm, ok, yes.
> >
> > I guess it all depends on how many directories we want to look at,
> > and trade that off with how complex the scripts should be.  I don't
> > really have an opinion one way or the other on that.
>
> I'm happy to move it.  I was just speculating on the original reason it was
> there.  Unfortunately, lzma is in $(PACKAGE_DIR)/lzma/, which shoots down my
> theory.  I think that we should avoid automagically including make files.
> There are few enough of them that we should only include specific ones to
> avoid problems.

Your mistake is that you assumed that we (I) followed any specific plan. :)

Yes, go ahead and move them where ever you think they work the best,
and ack on the including make files comment.

Jordan

[Myles Watson]

On Fri, Apr 25, 2008 at 10:54 AM, Jordan Crouse  wrote:
> On 25/04/08 10:13 -0600, Myles Watson wrote:
> > > -----Original Message-----
> > > From: [e-mail] [mailto:[e-mail]]
> > > On Behalf Of Jordan Crouse
> > > Sent: Friday, April 25, 2008 9:40 AM
> > > To: Myles Watson; [e-mail]
> > > Subject: Re: [coreboot] buildrom bug
> > >
> > > On 25/04/08 11:37 -0400, Ward Vandewege wrote:
> > > > On Fri, Apr 25, 2008 at 09:20:23AM -0600, Myles Watson wrote:
> > > > > > Don't we need that PAYLOAD_AND_DEP_MK change for every payload?
> > > > >
> > > > > Did I miss one?  Most of the payloads don't have dependencies.  The
> > > > > libpayload ones were the only ones I saw.
> > > >
> > > > The LAB and kernel payloads?

You're right.  I've included them in this patch.
> > > > > > +include $(PACKAGE_DIR)/utils/nrv2b.mk
> > > > >
> > > > > Yep, I missed that one.  Fixed.
> > > >
> > > > Great.
> > > > > > This also brings up the question why nrv2b.mk is in the 'utils'
> > >
> > > directory,
> > > > > > instead of the $(PACKAGE_DIR)/nrv2b/ directory?
> > > > >
> > > > > Because it doesn't get included in the ROM?
> > > >
> > > > Hmm, ok, yes.
> > >
> > > I guess it all depends on how many directories we want to look at,
> > > and trade that off with how complex the scripts should be.  I don't
> > > really have an opinion one way or the other on that.
> >
> > I'm happy to move it.  I was just speculating on the original reason it was
> > there.  Unfortunately, lzma is in $(PACKAGE_DIR)/lzma/, which shoots down my
> > theory.  I think that we should avoid automagically including make files.
> > There are few enough of them that we should only include specific ones to
> > avoid problems.
>
> Your mistake is that you assumed that we (I) followed any specific plan. :)

If we could only see the future... design would be so much easier.
> Yes, go ahead and move them where ever you think they work the best,
>  and ack on the including make files comment.

Here's the updated patch.  It needs to be preceded by
svn mv packages/utils packages/nrv2b

Signed-off-by: Myles Watson

Thanks,
Myles

[Myles Watson]

> Here's the updated patch.  It needs to be preceded by
>  svn mv packages/utils packages/nrv2b

Sorry.  Here's the new patch.

Signed-off-by: Myles Watson

Thanks,
Myles

[Jordan Crouse]

On 25/04/08 11:45 -0600, Myles Watson wrote:
> > Here's the updated patch.  It needs to be preceded by
> >  svn mv packages/utils packages/nrv2b
>
> Sorry.  Here's the new patch.

So here is the million dollar question.  If all the .mk
files are packages/<package>/<package>.mk, is there anything
stopping us from constructing PAYLOAD_AND_DEP_MK like so:

PAYLOAD_AND_DEP_MK = $(patsubst %,$(PACKAGE_DIR)/packages/%/%.mk,
$(DEPENDS-y) $(PAYLOAD-y) $(HOSTTOOLS-y))

That will have the side effect of including the kernel packages, but
we could filter those out.  I'm keen on the psuedo-automagic way that
buildrom just works when you add a new package.

Jordan
> Index: config/payloads/etherboot.conf
>
> --- config/payloads/etherboot.conf	(revision 167)
> +++ config/payloads/etherboot.conf	(working copy)
> @@ -8,3 +8,4 @@
>  PAYLOAD_COMPRESSED=$(OUTPUT_DIR)/etherboot-payload.elf.lzma
>
> PAYLOAD-y=etherboot
> +PAYLOAD_AND_DEP_MK=$(PACKAGE_DIR)/etherboot/etherboot.mk
> Index: config/payloads/ofw.conf
> ===================================================================
> --- config/payloads/ofw.conf	(revision 167)
> +++ config/payloads/ofw.conf	(working copy)
> @@ -12,3 +12,4 @@
>
> PAYLOAD-y=ofw
>  #HOSTTOOLS-y=crc32sum
> +PAYLOAD_AND_DEP_MK=$(PACKAGE_DIR)/ofw/ofw.mk
> Index: config/payloads/lab.conf
> ===================================================================
> --- config/payloads/lab.conf	(revision 167)
> +++ config/payloads/lab.conf	(working copy)
> @@ -33,3 +33,19 @@
>  PAYLOAD=lab
>
> HOSTTOOLS-y = mkelfimage unifdef
> +PAYLOAD_AND_DEP_MK=$(PACKAGE_DIR)/mkelfimage/mkelfimage.mk
> +PAYLOAD_AND_DEP_MK+=$(PACKAGE_DIR)/uclibc/uclibc.mk
> +PAYLOAD_AND_DEP_MK+=$(PACKAGE_DIR)/unifdef/unifdef.mk
>
> +ifeq ($(CONFIG_KBL),y)
> PAYLOAD_AND_DEP_MK+=$(PACKAGE_DIR)/kexec-boot-loader/kexec-boot-loader.mk
>
> +endif
> +ifeq ($(CONFIG_BUSYBOX),y)
> PAYLOAD_AND_DEP_MK+=$(PACKAGE_DIR)/busybox/busybox.mk
>
> +endif
> +ifeq ($(CONFIG_BOOTMENU),y)
> PAYLOAD_AND_DEP_MK+=$(PACKAGE_DIR)/bootmenu/bootmenu.mk
>
> +endif
> +ifeq ($(CONFIG_OLPCFLASH),y)
> PAYLOAD_AND_DEP_MK+=$(PACKAGE_DIR)/olpcflash/olpcflash.mk
>
> +endif
> Index: config/payloads/memtest.conf
> ===================================================================
> --- config/payloads/memtest.conf	(revision 167)
> +++ config/payloads/memtest.conf	(working copy)
> @@ -1,4 +1,4 @@
> -# Configuration file for the etherboot payload
> +# Configuration file for the memtest payload
>
> # Common configuration options
>
> @@ -8,3 +8,5 @@
>  PAYLOAD_COMPRESSED=$(OUTPUT_DIR)/memtest-payload.elf.lzma
>
> PAYLOAD-y=memtest
>
> +PAYLOAD_AND_DEP_MK=$(PACKAGE_DIR)/memtest/memtest.mk
> Index: config/payloads/tint.conf
> ===================================================================
> --- config/payloads/tint.conf	(revision 167)
> +++ config/payloads/tint.conf	(working copy)
> @@ -10,5 +10,7 @@
>  PAYLOAD-y=tint
>  PAYLOAD=tint
>
> +PAYLOAD_AND_DEP_MK=$(PACKAGE_DIR)/libpayload/libpayload.mk $(PACKAGE_DIR)/tint/tint.mk
>
> # Add libpayload as a dependency
>  DEPENDS-y=libpayload
> Index: config/payloads/kernel.conf
> ===================================================================
> --- config/payloads/kernel.conf	(revision 167)
> +++ config/payloads/kernel.conf	(working copy)
> @@ -19,3 +19,4 @@
>  PAYLOAD=kernel
>
> HOSTTOOLS-y = mkelfimage
> +PAYLOAD_AND_DEP_MK=$(PACKAGE_DIR)/mkelfimage/mkelfimage.mk
> Index: config/payloads/grub2.conf
> ===================================================================
> --- config/payloads/grub2.conf	(revision 167)
> +++ config/payloads/grub2.conf	(working copy)
> @@ -9,3 +9,5 @@
>
> PAYLOAD-y=grub2
>  PAYLOAD=grub2
>
> +PAYLOAD_AND_DEP_MK=$(PACKAGE_DIR)/grub2/grub2.mk
> Index: config/payloads/coreinfo.conf
> ===================================================================
> --- config/payloads/coreinfo.conf	(revision 167)
> +++ config/payloads/coreinfo.conf	(working copy)
> @@ -11,4 +11,5 @@
>  PAYLOAD=coreinfo
>
> # Add libpayload as a dependency
> +PAYLOAD_AND_DEP_MK=$(PACKAGE_DIR)/coreinfo/coreinfo.mk $(PACKAGE_DIR)/libpayload/libpayload.mk
>  DEPENDS-y=libpayload
> Index: config/payloads/filo.conf
> ===================================================================
> --- config/payloads/filo.conf	(revision 167)
> +++ config/payloads/filo.conf	(working copy)
> @@ -8,6 +8,7 @@
>  PAYLOAD_COMPRESSED=$(OUTPUT_DIR)/filo-payload.elf.lzma
>
> PAYLOAD-y=filo
> +PAYLOAD_AND_DEP_MK=$(PACKAGE_DIR)/filo/filo.mk
>
> PAYLOAD=filo
> Index: Makefile
>
> --- Makefile	(revision 167)
> +++ Makefile	(working copy)
> @@ -115,9 +115,14 @@
>  # The following code gets all the make targets, but filters out the kernel
>  # targets which are implicitly set by the platform configuration
>
> -MKTARGETS:= $(shell ls $(PACKAGE_DIR)/*/*.mk)
> +ifneq ($(PAYLOAD_AND_DEP_MK),)
> +include $(PAYLOAD_AND_DEP_MK)
> +endif
>
> -include $(filter-out $(PACKAGE_DIR)/kernel/% $(PACKAGE_DIR)/coreboot-v2/% $(PACKAGE_DIR)/coreboot-v3/%,$(MKTARGETS))
> +include $(PACKAGE_DIR)/nrv2b/nrv2b.mk
> +include $(PACKAGE_DIR)/lzma/lzma.mk
> +include $(PACKAGE_DIR)/geodevsa/geodevsa.mk
> +include $(PACKAGE_DIR)/roms/roms.mk
>
> include $(KERNEL_MK)

[Peter Stuge]

On Fri, Apr 25, 2008 at 11:50:22AM -0600, Jordan Crouse wrote:
> PAYLOAD_AND_DEP_MK = $(patsubst %,$(PACKAGE_DIR)/packages/%/%.mk,
> $(DEPENDS-y) $(PAYLOAD-y) $(HOSTTOOLS-y))

Nice.

//Peter

[Myles Watson]

> On 25/04/08 11:45 -0600, Myles Watson wrote:
> > > Here's the updated patch.  It needs to be preceded by
> > >  svn mv packages/utils packages/nrv2b
> >
> > Sorry.  Here's the new patch.
>
> So here is the million dollar question.  If all the .mk
> files are packages/<package>/<package>.mk, is there anything
> stopping us from constructing PAYLOAD_AND_DEP_MK like so:
>
> PAYLOAD_AND_DEP_MK = $(patsubst %,$(PACKAGE_DIR)/packages/%/%.mk,
> $(DEPENDS-y) $(PAYLOAD-y) $(HOSTTOOLS-y))

Doesn't that put you back where you were with the "You need ruby to compile
grub2"?  The point of the PAYLOAD_AND_DEP_MK variable was supposed to be
that it got set (purposefully) in the config/payloads/<payload>.conf file.
I think it's reasonable to ask a developer to set the variable when he adds
the .conf file for a new package.
> That will have the side effect of including the kernel packages, but
> we could filter those out.  I'm keen on the psuedo-automagic way that
> buildrom just works when you add a new package.

If my explanation above didn't address this, can you give me an example of a
new package that would get included without a .conf file?

Thanks,
Myles

[Peter Stuge]

On Fri, Apr 25, 2008 at 12:01:58PM -0600, Myles Watson wrote:
> > PAYLOAD_AND_DEP_MK = $(patsubst %,$(PACKAGE_DIR)/packages/%/%.mk,
> > $(DEPENDS-y) $(PAYLOAD-y) $(HOSTTOOLS-y))
>
> Doesn't that put you back where you were with the "You need ruby to
> compile grub2"?

I don't know how pathsubst works exactly but I assumed that
$(PAYLOAD-y) would be used somehow to only apply the substitution on
the selected payload?

//Peter

[Jordan Crouse]

On 25/04/08 12:01 -0600, Myles Watson wrote:
> > On 25/04/08 11:45 -0600, Myles Watson wrote:
> > > > Here's the updated patch.  It needs to be preceded by
> > > >  svn mv packages/utils packages/nrv2b
> > >
> > > Sorry.  Here's the new patch.
> >
> > So here is the million dollar question.  If all the .mk
> > files are packages/<package>/<package>.mk, is there anything
> > stopping us from constructing PAYLOAD_AND_DEP_MK like so:
> >
> > PAYLOAD_AND_DEP_MK = $(patsubst %,$(PACKAGE_DIR)/packages/%/%.mk,
> > $(DEPENDS-y) $(PAYLOAD-y) $(HOSTTOOLS-y))
>
> Doesn't that put you back where you were with the "You need ruby to compile
> grub2"?  The point of the PAYLOAD_AND_DEP_MK variable was supposed to be
> that it got set (purposefully) in the config/payloads/<payload>.conf file.
> I think it's reasonable to ask a developer to set the variable when he adds
> the .conf file for a new package.

DEPENDS-y PAYLOAD-y and HOSTTOOLS-y are the dependencies
for what you are actually building.

Our original problem was that we were doing a $(wildcard */*/.mk), which
was including the kitchen sink.  So unless your package is somehow listing
grub2 as one of its dependencies, then packages/grub2/grub2.mk won't make
it into the list, and you won't have a problem.

I think it is pretty unreasonable to ask the developer to add the 
variable - it includes magic that he shouldn't have to think about,
namely how packages are referenced and included.

And lets look at it this way - 99.99% of the time, its going to be 
packages/<package>/<package>.mk - we're just automating the process.
If we want, we can make PAYLOAD_AND_DEP_MK ?= so that the developer
can override it if they want, but I think most people won't want to.

Jordan

[Myles Watson]

> -----Original Message-----
> From: Jordan Crouse [mailto:[e-mail]]
> Sent: Friday, April 25, 2008 12:12 PM
> To: Myles Watson
> Cc: [e-mail]
> Subject: Re: buildrom bug
>
> On 25/04/08 12:01 -0600, Myles Watson wrote:
> > > On 25/04/08 11:45 -0600, Myles Watson wrote:
> > > > > Here's the updated patch.  It needs to be preceded by
> > > > >  svn mv packages/utils packages/nrv2b
> > > >
> > > > Sorry.  Here's the new patch.
> > >
> > > So here is the million dollar question.  If all the .mk
> > > files are packages/<package>/<package>.mk, is there anything
> > > stopping us from constructing PAYLOAD_AND_DEP_MK like so:
> > >
> > > PAYLOAD_AND_DEP_MK = $(patsubst %,$(PACKAGE_DIR)/packages/%/%.mk,
> > > $(DEPENDS-y) $(PAYLOAD-y) $(HOSTTOOLS-y))
> >
> > Doesn't that put you back where you were with the "You need ruby to
>
> compile
> > grub2"?  The point of the PAYLOAD_AND_DEP_MK variable was supposed to be
> > that it got set (purposefully) in the config/payloads/<payload>.conf
>
> file.
> > I think it's reasonable to ask a developer to set the variable when he
>
> adds
> > the .conf file for a new package.
>
> DEPENDS-y PAYLOAD-y and HOSTTOOLS-y are the dependencies
> for what you are actually building.

Thanks for the clarification.  I looked at you code and saw the original
wildcard.  Sorry.
> Our original problem was that we were doing a $(wildcard */*/.mk), which
> was including the kitchen sink.  So unless your package is somehow listing
> grub2 as one of its dependencies, then packages/grub2/grub2.mk won't make
> it into the list, and you won't have a problem.
>
> I think it is pretty unreasonable to ask the developer to add the
> variable - it includes magic that he shouldn't have to think about,
> namely how packages are referenced and included.
>
> And lets look at it this way - 99.99% of the time, its going to be
> packages/<package>/<package>.mk - we're just automating the process.
> If we want, we can make PAYLOAD_AND_DEP_MK ?= so that the developer
> can override it if they want, but I think most people won't want to.

I agree.  Do you want to create the patch or send me a patch against mine?

Thanks,
Myles

[Jordan Crouse]

On 25/04/08 12:15 -0600, Myles Watson wrote:
> > -----Original Message-----
> > From: Jordan Crouse [mailto:[e-mail]]
> > Sent: Friday, April 25, 2008 12:12 PM
> > To: Myles Watson
> > Cc: [e-mail]
> > Subject: Re: buildrom bug
> >
> > On 25/04/08 12:01 -0600, Myles Watson wrote:
> > > > On 25/04/08 11:45 -0600, Myles Watson wrote:
> > > > > > Here's the updated patch.  It needs to be preceded by
> > > > > >  svn mv packages/utils packages/nrv2b
> > > > >
> > > > > Sorry.  Here's the new patch.
> > > >
> > > > So here is the million dollar question.  If all the .mk
> > > > files are packages/<package>/<package>.mk, is there anything
> > > > stopping us from constructing PAYLOAD_AND_DEP_MK like so:
> > > >
> > > > PAYLOAD_AND_DEP_MK = $(patsubst %,$(PACKAGE_DIR)/packages/%/%.mk,
> > > > $(DEPENDS-y) $(PAYLOAD-y) $(HOSTTOOLS-y))
> > >
> > > Doesn't that put you back where you were with the "You need ruby to
> >
> > compile
> > > grub2"?  The point of the PAYLOAD_AND_DEP_MK variable was supposed to be
> > > that it got set (purposefully) in the config/payloads/<payload>.conf
> >
> > file.
> > > I think it's reasonable to ask a developer to set the variable when he
> >
> > adds
> > > the .conf file for a new package.
> >
> > DEPENDS-y PAYLOAD-y and HOSTTOOLS-y are the dependencies
> > for what you are actually building.
>
> Thanks for the clarification.  I looked at you code and saw the original
> wildcard.  Sorry.
> > Our original problem was that we were doing a $(wildcard */*/.mk), which
> > was including the kitchen sink.  So unless your package is somehow listing
> > grub2 as one of its dependencies, then packages/grub2/grub2.mk won't make
> > it into the list, and you won't have a problem.
> >
> > I think it is pretty unreasonable to ask the developer to add the
> > variable - it includes magic that he shouldn't have to think about,
> > namely how packages are referenced and included.
> >
> > And lets look at it this way - 99.99% of the time, its going to be
> > packages/<package>/<package>.mk - we're just automating the process.
> > If we want, we can make PAYLOAD_AND_DEP_MK ?= so that the developer
> > can override it if they want, but I think most people won't want to.
>
> I agree.  Do you want to create the patch or send me a patch against mine?

I'll make a patch based on yours.

Jordan

[Marc Karasek]

I am assuming a update / co from svn will not build until this patch is 
in place?  =-O

It doesn't for me..  :(

*********************
Marc Karasek
MTS
Sun Microsystems
mailto:[e-mail]
ph:770.360.6415
*********************

Jordan Crouse wrote:
> On 25/04/08 12:15 -0600, Myles Watson wrote:
> > > -----Original Message-----
> > > From: Jordan Crouse [mailto:[e-mail]]
> > > Sent: Friday, April 25, 2008 12:12 PM
> > > To: Myles Watson
> > > Cc: [e-mail]
> > > Subject: Re: buildrom bug
> > >
> > > On 25/04/08 12:01 -0600, Myles Watson wrote:
> > > > > On 25/04/08 11:45 -0600, Myles Watson wrote:
> > > > > > >  Here's the updated patch.  It needs to be preceded by
> > > > > > >  svn mv packages/utils packages/nrv2b
> > > > > > >               
> > > > > > Sorry.  Here's the new patch.
> > > > >
> > > > > So here is the million dollar question.  If all the .mk
> > > > > files are packages/<package>/<package>.mk, is there anything
> > > > > stopping us from constructing PAYLOAD_AND_DEP_MK like so:
> > > > >
> > > > > PAYLOAD_AND_DEP_MK = $(patsubst %,$(PACKAGE_DIR)/packages/%/%.mk,
> > > > > $(DEPENDS-y) $(PAYLOAD-y) $(HOSTTOOLS-y))
> > > >
> > > > Doesn't that put you back where you were with the "You need ruby to
> > >
> > > compile
> > > > grub2"?  The point of the PAYLOAD_AND_DEP_MK variable was supposed to be
> > > > that it got set (purposefully) in the config/payloads/<payload>.conf
> > >
> > > file.
> > > > I think it's reasonable to ask a developer to set the variable when he
> > >
> > > adds
> > > > the .conf file for a new package.
> > >
> > > DEPENDS-y PAYLOAD-y and HOSTTOOLS-y are the dependencies
> > > for what you are actually building.
> >
> > Thanks for the clarification.  I looked at you code and saw the original
> > wildcard.  Sorry.
> > > Our original problem was that we were doing a $(wildcard */*/.mk), which
> > > was including the kitchen sink.  So unless your package is somehow listing
> > > grub2 as one of its dependencies, then packages/grub2/grub2.mk won't make
> > > it into the list, and you won't have a problem.
> > >
> > > I think it is pretty unreasonable to ask the developer to add the
> > > variable - it includes magic that he shouldn't have to think about,
> > > namely how packages are referenced and included.
> > >
> > > And lets look at it this way - 99.99% of the time, its going to be
> > > packages/<package>/<package>.mk - we're just automating the process.
> > > If we want, we can make PAYLOAD_AND_DEP_MK ?= so that the developer
> > > can override it if they want, but I think most people won't want to.
> >
> > I agree.  Do you want to create the patch or send me a patch against mine?
>
> I'll make a patch based on yours.
>
> Jordan

[Jordan Crouse]

On 25/04/08 16:06 -0400, Marc Karasek wrote:
> I am assuming a update / co from svn will not build until this patch is in 
> place?  =-O
>
> It doesn't for me..  :(

Um - no.  As far as I know, it should work.  Myles?
> Marc Karasek
> MTS
> Sun Microsystems
> mailto:[e-mail]
> ph:770.360.6415
>
> Jordan Crouse wrote:
> > On 25/04/08 12:15 -0600, Myles Watson wrote:
> > > > -----Original Message-----
> > > > From: Jordan Crouse [mailto:[e-mail]]
> > > > Sent: Friday, April 25, 2008 12:12 PM
> > > > To: Myles Watson
> > > > Cc: [e-mail]
> > > > Subject: Re: buildrom bug
> > > >
> > > > On 25/04/08 12:01 -0600, Myles Watson wrote:
> > > > > > On 25/04/08 11:45 -0600, Myles Watson wrote:
> > > > > >
> > > > > > >>  Here's the updated patch.  It needs to be preceded by
> > > > > > >>  svn mv packages/utils packages/nrv2b
> > > > > > >>               
> > > > > > > Sorry.  Here's the new patch.
> > > > > > >             
> > > > > > So here is the million dollar question.  If all the .mk
> > > > > > files are packages/<package>/<package>.mk, is there anything
> > > > > > stopping us from constructing PAYLOAD_AND_DEP_MK like so:
> > > > > >
> > > > > > PAYLOAD_AND_DEP_MK = $(patsubst %,$(PACKAGE_DIR)/packages/%/%.mk,
> > > > > > $(DEPENDS-y) $(PAYLOAD-y) $(HOSTTOOLS-y))
> > > > >
> > > > > Doesn't that put you back where you were with the "You need ruby to
> > > >
> > > > compile
> > > > > grub2"?  The point of the PAYLOAD_AND_DEP_MK variable was supposed to 
> > > > > be
> > > > > that it got set (purposefully) in the config/payloads/<payload>.conf
> > > >
> > > > file.
> > > > > I think it's reasonable to ask a developer to set the variable when he
> > > >
> > > > adds
> > > > > the .conf file for a new package.
> > > >
> > > > DEPENDS-y PAYLOAD-y and HOSTTOOLS-y are the dependencies
> > > > for what you are actually building.
> > >
> > > Thanks for the clarification.  I looked at you code and saw the original
> > > wildcard.  Sorry.
> > > > Our original problem was that we were doing a $(wildcard */*/.mk), which
> > > > was including the kitchen sink.  So unless your package is somehow 
> > > > listing
> > > > grub2 as one of its dependencies, then packages/grub2/grub2.mk won't 
> > > > make
> > > > it into the list, and you won't have a problem.
> > > >
> > > > I think it is pretty unreasonable to ask the developer to add the
> > > > variable - it includes magic that he shouldn't have to think about,
> > > > namely how packages are referenced and included.
> > > >
> > > > And lets look at it this way - 99.99% of the time, its going to be
> > > > packages/<package>/<package>.mk - we're just automating the process.
> > > > If we want, we can make PAYLOAD_AND_DEP_MK ?= so that the developer
> > > > can override it if they want, but I think most people won't want to.
> > >
> > > I agree.  Do you want to create the patch or send me a patch against 
> > > mine?
> >
> > I'll make a patch based on yours.
> >
> > Jordan

[Myles Watson]

> -----Original Message-----
> From: [e-mail] [mailto:[e-mail]]
> Sent: Friday, April 25, 2008 2:07 PM
> To: Jordan Crouse
> Cc: Myles Watson; [e-mail]
> Subject: Re: [coreboot] buildrom bug
>
> I am assuming a update / co from svn will not build until this patch is
> in place?  =-O
>
> It doesn't for me..  :(

Sorry.  I forgot the svn mv that went with the patch.  It should work now.

Myles

[Jordan Crouse]

On 25/04/08 12:15 -0600, Myles Watson wrote:
> I agree.  Do you want to create the patch or send me a patch against mine?

We can further clean this up by moving the platform specific 
ifdefs for kernel and coreboot-v2 into packages/kernel/kernel.mk and
packages/coreboot-v2/coreboot-v2.mk respectively.  Then we can avoid
any filtering at the main level.

Thoughts?
Jordan

[Peter Stuge]

On Fri, Apr 25, 2008 at 01:36:08PM -0600, Jordan Crouse wrote:
> Then we can avoid any filtering at the main level.
>
> Thoughts?

Sounds good.

//Peter

[Jordan Crouse]

On 25/04/08 12:15 -0600, Myles Watson wrote:
> I agree.  Do you want to create the patch or send me a patch against mine?

Okay - so here we go.  This patch uses black make magic to construct
the list of .mk files to be included.  But wait, thats not all - it
consolidates all of the platform specific coreboot .mk and kernel .mk
includes into generic toplevel packages/coreboot-v2/coreboot-v2.mk and
packages/kernel/kernel.mk files so that we don't have to specially include
files from within the toplevel Makefile.   Finally, to avoid automagically
including roms.mk and geodevsa.mk, I revamped how geodevsa works.  I'm not
super happy with it, but its at least reasonable and gets rid of some
ifdefs.  We'll need to address this in more detail when we revamp the
lar UI and have other platforms that need option ROMs.

Please test and report back - this isn't a really scary patch, but it touches
lots of files and lots of platforms.

Jordan

[Peter Stuge]

On Fri, Apr 25, 2008 at 03:52:43PM -0600, Jordan Crouse wrote:
> b=`echo $$file | sed -e s:$(ROM_DIR)\/*::`; \

Should the expression include ^ somehow so as to not fail on subdirs
with matching names?

Or just use basename?
> +# FIXME:  Get rid of this ifeq somehow
>
> ifeq ($(CONFIG_COREBOOT_V2),y)
> -include $(CBV2_MK)
> +INCMK += $(PACKAGE_DIR)/coreboot-v2/coreboot-v2.mk
>  else
> -include $(PACKAGE_DIR)/coreboot-v3/coreboot-v3.mk
> +INCMK += $(PACKAGE_DIR)/coreboot-v3/coreboot-v3.mk
>  endif

This will have to exist _somewhere_ though.
> +# Platform specific dependencies
> +DEPENDS-$(CONFIG_PLATFORM_GEODE) += geodevsa

I like it!
> ===================================================================
> --- /dev/null	1970-01-01 00:00:00.000000000 +0000
> +++ buildrom-devel/packages/coreboot-v2/coreboot-v2.mk	2008-04-25 15:28:59.000000000 -0600
> @@ -0,0 +1,24 @@
> +# "toplevel" coreboot-v2.mk - this is where we decide
> +# which of the platform specific files to actually
> +# include
>
> +CBV2MK-y=$(PACKAGE_DIR)/coreboot-v2/generic.mk
> +CBV2MK-$(CONFIG_PLATFORM_NORWICH) = $(PACKAGE_DIR)/coreboot-v2/geodelx.mk
> +CBV2MK-$(CONFIG_PLATFORM_MSM800SEV) = $(PACKAGE_DIR)/coreboot-v2/geodelx.mk
> +CBV2MK-$(CONFIG_PLATFORM_ALIX1C) = $(PACKAGE_DIR)/coreboot-v2/geodelx.mk
> +CBV2MK-$(CONFIG_PLATFORM_DB800) = $(PACKAGE_DIR)/coreboot-v2/geodelx.mk
> +CBV2MK-$(CONFIG_PLATFORM_DBE61) = $(PACKAGE_DIR)/coreboot-v2/geodelx.mk

Why is this mapping needed here? Coreboot already knows that they are
lx boards? Sorry, I am a total buildrom noob so will ask many
questions and try to point out not obvious things.
> +CBV2MK-$(CONFIG_PLATFORM_TYAN_S2881) = $(PACKAGE_DIR)/coreboot-v2/tyan-s2881.mk
> +CBV2MK-$(CONFIG_PLATFORM_TYAN_S2882) = $(PACKAGE_DIR)/coreboot-v2/tyan-s2882.mk
> +CBV2MK-$(CONFIG_PLATFORM_TYAN_S2891) = $(PACKAGE_DIR)/coreboot-v2/tyan-s2891.mk
> +CBV2MK-$(CONFIG_PLATFORM_TYAN_S2892) = $(PACKAGE_DIR)/coreboot-v2/tyan-generic.mk
> +CBV2MK-$(CONFIG_PLATFORM_TYAN_S2895) = $(PACKAGE_DIR)/coreboot-v2/tyan-generic.mk

Err.. When is tyan-generic good and when is tyan-<$newboard> good?
> @@ -31,13 +35,19 @@
>  $(GEODE_PADDED_VSA): $(GEODE_COMPRESSED_VSA)
>    @ cp $< $@
>    @ (size=`stat -c %s $<`; count=`expr $(GEODE_VSA_SIZE) - $$size`; \
> @ dd if=/dev/zero bs=1 count=$$count  >> $@ 2> /dev/null)
> dd if=/dev/zero bs=1 count=$$count  >> $@ 2> /dev/null)

On purpose?
> +ifeq($(KERNELMK-y),)
> +$(error "You do not have a kernel .mk file defined for this platform")
> +endif

Awesome!

So many removed lines of code must make a good patch! :)

Acked-by: Peter Stuge

..but please wait for another.

//Peter

[Jordan Crouse]

On 26/04/08 00:05 +0200, Peter Stuge wrote:
> On Fri, Apr 25, 2008 at 03:52:43PM -0600, Jordan Crouse wrote:
> > b=`echo $$file | sed -e s:$(ROM_DIR)\/*::`; \
>
> Should the expression include ^ somehow so as to not fail on subdirs
> with matching names?
>
> Or just use basename?

Its not just basename - there are also sub directories in there,
i.e. $(ROM_DIR)/blob/vsa which will end up as blob/vsa in the
LAR.  The ^ is a good idea though.
> > +# FIXME:  Get rid of this ifeq somehow
> >
> > ifeq ($(CONFIG_COREBOOT_V2),y)
> > -include $(CBV2_MK)
> > +INCMK += $(PACKAGE_DIR)/coreboot-v2/coreboot-v2.mk
> >  else
> > -include $(PACKAGE_DIR)/coreboot-v3/coreboot-v3.mk
> > +INCMK += $(PACKAGE_DIR)/coreboot-v3/coreboot-v3.mk
> >  endif
>
> This will have to exist _somewhere_ though.

Yes, probably.
> > ===================================================================
> > --- /dev/null	1970-01-01 00:00:00.000000000 +0000
> > +++ buildrom-devel/packages/coreboot-v2/coreboot-v2.mk	2008-04-25 15:28:59.000000000 -0600
> > @@ -0,0 +1,24 @@
> > +# "toplevel" coreboot-v2.mk - this is where we decide
> > +# which of the platform specific files to actually
> > +# include
> >
> > +CBV2MK-y=$(PACKAGE_DIR)/coreboot-v2/generic.mk
> > +CBV2MK-$(CONFIG_PLATFORM_NORWICH) = $(PACKAGE_DIR)/coreboot-v2/geodelx.mk
> > +CBV2MK-$(CONFIG_PLATFORM_MSM800SEV) = $(PACKAGE_DIR)/coreboot-v2/geodelx.mk
> > +CBV2MK-$(CONFIG_PLATFORM_ALIX1C) = $(PACKAGE_DIR)/coreboot-v2/geodelx.mk
> > +CBV2MK-$(CONFIG_PLATFORM_DB800) = $(PACKAGE_DIR)/coreboot-v2/geodelx.mk
> > +CBV2MK-$(CONFIG_PLATFORM_DBE61) = $(PACKAGE_DIR)/coreboot-v2/geodelx.mk
>
> Why is this mapping needed here? Coreboot already knows that they are
> lx boards? Sorry, I am a total buildrom noob so will ask many
> questions and try to point out not obvious things.

No problem.   These are for platform specific magic that we need to 
do around the coreboot-v2 build, such as add different Config.lb files,
etc, etc.  All of the LX boards have the same platform specific magic
(add VSA), so they share a similar target.
> > +CBV2MK-$(CONFIG_PLATFORM_TYAN_S2881) = $(PACKAGE_DIR)/coreboot-v2/tyan-s2881.mk
> > +CBV2MK-$(CONFIG_PLATFORM_TYAN_S2882) = $(PACKAGE_DIR)/coreboot-v2/tyan-s2882.mk
> > +CBV2MK-$(CONFIG_PLATFORM_TYAN_S2891) = $(PACKAGE_DIR)/coreboot-v2/tyan-s2891.mk
> > +CBV2MK-$(CONFIG_PLATFORM_TYAN_S2892) = $(PACKAGE_DIR)/coreboot-v2/tyan-generic.mk
> > +CBV2MK-$(CONFIG_PLATFORM_TYAN_S2895) = $(PACKAGE_DIR)/coreboot-v2/tyan-generic.mk
>
> Err.. When is tyan-generic good and when is tyan-<$newboard> good?

Thats a question for Myles.
> > @@ -31,13 +35,19 @@
> >  $(GEODE_PADDED_VSA): $(GEODE_COMPRESSED_VSA)
> >    @ cp $< $@
> >    @ (size=`stat -c %s $<`; count=`expr $(GEODE_VSA_SIZE) - $$size`; \
> > @ dd if=/dev/zero bs=1 count=$$count  >> $@ 2> /dev/null)
> > dd if=/dev/zero bs=1 count=$$count  >> $@ 2> /dev/null)
>
> On purpose?

yes - the @ is a makeism - the second @ was in the shell and freaking
out sh.

Jordan

[Peter Stuge]

On Fri, Apr 25, 2008 at 04:32:34PM -0600, Jordan Crouse wrote:
> > > +CBV2MK-y=$(PACKAGE_DIR)/coreboot-v2/generic.mk
> > > +CBV2MK-$(CONFIG_PLATFORM_NORWICH) = $(PACKAGE_DIR)/coreboot-v2/geodelx.mk
> > > +CBV2MK-$(CONFIG_PLATFORM_MSM800SEV) = $(PACKAGE_DIR)/coreboot-v2/geodelx.mk
> > > +CBV2MK-$(CONFIG_PLATFORM_ALIX1C) = $(PACKAGE_DIR)/coreboot-v2/geodelx.mk
> > > +CBV2MK-$(CONFIG_PLATFORM_DB800) = $(PACKAGE_DIR)/coreboot-v2/geodelx.mk
> > > +CBV2MK-$(CONFIG_PLATFORM_DBE61) = $(PACKAGE_DIR)/coreboot-v2/geodelx.mk
> >
> > Why is this mapping needed here? Coreboot already knows that they are
> > lx boards? Sorry, I am a total buildrom noob so will ask many
> > questions and try to point out not obvious things.
>
> No problem.   These are for platform specific magic that we need to 
> do around the coreboot-v2 build, such as add different Config.lb files,
> etc, etc.  All of the LX boards have the same platform specific magic
> (add VSA), so they share a similar target.

My point is that buildrom is integrated fairly tightly with coreboot
already and it should not need to duplicate this mapping between e.g.
ALIX1C and geodelx.

For v3 read the dts. For v2, hm, maybe look for "cpu/amd/model_lx" in
mainboard/v/b/Config.lb.

Or no?
> > > @ (size=`stat -c %s $<`; count=`expr $(GEODE_VSA_SIZE) - $$size`; \
> > > @ dd if=/dev/zero bs=1 count=$$count  >> $@ 2> /dev/null)
> > > dd if=/dev/zero bs=1 count=$$count  >> $@ 2> /dev/null)
> >
> > On purpose?
>
> yes - the @ is a makeism - the second @ was in the shell and
> freaking out sh.

Oh yes - good one.

//Peter

[Jordan Crouse]

On 26/04/08 00:47 +0200, Peter Stuge wrote:
> On Fri, Apr 25, 2008 at 04:32:34PM -0600, Jordan Crouse wrote:
> > > > +CBV2MK-y=$(PACKAGE_DIR)/coreboot-v2/generic.mk
> > > > +CBV2MK-$(CONFIG_PLATFORM_NORWICH) = $(PACKAGE_DIR)/coreboot-v2/geodelx.mk
> > > > +CBV2MK-$(CONFIG_PLATFORM_MSM800SEV) = $(PACKAGE_DIR)/coreboot-v2/geodelx.mk
> > > > +CBV2MK-$(CONFIG_PLATFORM_ALIX1C) = $(PACKAGE_DIR)/coreboot-v2/geodelx.mk
> > > > +CBV2MK-$(CONFIG_PLATFORM_DB800) = $(PACKAGE_DIR)/coreboot-v2/geodelx.mk
> > > > +CBV2MK-$(CONFIG_PLATFORM_DBE61) = $(PACKAGE_DIR)/coreboot-v2/geodelx.mk
> > >
> > > Why is this mapping needed here? Coreboot already knows that they are
> > > lx boards? Sorry, I am a total buildrom noob so will ask many
> > > questions and try to point out not obvious things.
> >
> > No problem.   These are for platform specific magic that we need to 
> > do around the coreboot-v2 build, such as add different Config.lb files,
> > etc, etc.  All of the LX boards have the same platform specific magic
> > (add VSA), so they share a similar target.
>
> My point is that buildrom is integrated fairly tightly with coreboot
> already and it should not need to duplicate this mapping between e.g.
> ALIX1C and geodelx.
>
> For v3 read the dts. For v2, hm, maybe look for "cpu/amd/model_lx" in
> mainboard/v/b/Config.lb.

No - this is actually stuff external to coreboot itself.  In this
case its the code that appends VSA to the ROM.

it is named geodelx.mk because thats a handy name, not because there is
any sort of mapping.

[Peter Stuge]

On Fri, Apr 25, 2008 at 04:51:12PM -0600, Jordan Crouse wrote:
> > My point is that buildrom is integrated fairly tightly with coreboot
> > already and it should not need to duplicate this mapping between e.g.
> > ALIX1C and geodelx.
> >
> > For v3 read the dts. For v2, hm, maybe look for "cpu/amd/model_lx" in
> > mainboard/v/b/Config.lb.
>
> No - this is actually stuff external to coreboot itself.

I know.
> In this case its the code that appends VSA to the ROM.

Yes.
> it is named geodelx.mk because thats a handy name, not because
> there is any sort of mapping.

All boards in coreboot using cpu/amd/model_lx need VSA, so rather
than teaching buildrom one coreboot target at a time, I am suggesting
to once and for all teach buildrom that all coreboot targets using
cpu/amd/model_lx need VSA, by reusing a mapping (between $target_name
and lx) that can be found in coreboot instead of duplicating the
mapping in buildrom.

This can be extrapolated to work for other things as well, but right
now I don't think there are any other uses.

Put another way:
buildrom should not need it's own list of boards that use model_lx
when coreboot already has one. (Though the index is backwards.)

//Peter

[Myles Watson]

> > > +CBV2MK-$(CONFIG_PLATFORM_TYAN_S2881) = $(PACKAGE_DIR)/coreboot-
>
> v2/tyan-s2881.mk
> > > +CBV2MK-$(CONFIG_PLATFORM_TYAN_S2882) = $(PACKAGE_DIR)/coreboot-
>
> v2/tyan-s2882.mk
> > > +CBV2MK-$(CONFIG_PLATFORM_TYAN_S2891) = $(PACKAGE_DIR)/coreboot-
>
> v2/tyan-s2891.mk
> > > +CBV2MK-$(CONFIG_PLATFORM_TYAN_S2892) = $(PACKAGE_DIR)/coreboot-
>
> v2/tyan-generic.mk
> > > +CBV2MK-$(CONFIG_PLATFORM_TYAN_S2895) = $(PACKAGE_DIR)/coreboot-
>
> v2/tyan-generic.mk
> > Err.. When is tyan-generic good and when is tyan-<$newboard> good?
>
> Thats a question for Myles.

The patch that gets rid of tyan-<$newboard> hasn't been acked yet.

Myles

[Myles Watson]

> > > @@ -31,13 +35,19 @@
> > >  $(GEODE_PADDED_VSA): $(GEODE_COMPRESSED_VSA)
> > >    @ cp $< $@
> > >    @ (size=`stat -c %s $<`; count=`expr $(GEODE_VSA_SIZE) - $$size`; \
> > > @ dd if=/dev/zero bs=1 count=$$count  >> $@ 2> /dev/null)
> > > dd if=/dev/zero bs=1 count=$$count  >> $@ 2> /dev/null)
> >
> > On purpose?
>
> yes - the @ is a makeism - the second @ was in the shell and freaking
> out sh.

Can you indent the dd so that it's obvious that it's part of the previous
line?

Thanks,
Myles

[Jordan Crouse]

On 28/04/08 09:40 -0600, Myles Watson wrote:
> > > > @@ -31,13 +35,19 @@
> > > >  $(GEODE_PADDED_VSA): $(GEODE_COMPRESSED_VSA)
> > > >    @ cp $< $@
> > > >    @ (size=`stat -c %s $<`; count=`expr $(GEODE_VSA_SIZE) - $$size`; \
> > > > @ dd if=/dev/zero bs=1 count=$$count  >> $@ 2> /dev/null)
> > > > dd if=/dev/zero bs=1 count=$$count  >> $@ 2> /dev/null)
> > >
> > > On purpose?
> >
> > yes - the @ is a makeism - the second @ was in the shell and freaking
> > out sh.
>
> Can you indent the dd so that it's obvious that it's part of the previous
> line?

Yep, not a problem.  Isn't shell inside of Make grand? </sarcasm>

[Myles Watson]

> -----Original Message-----
> From: Jordan Crouse [mailto:[e-mail]]
> Sent: Friday, April 25, 2008 3:53 PM
> To: Myles Watson
> Cc: [e-mail]
> Subject: Re: buildrom bug
>
> On 25/04/08 12:15 -0600, Myles Watson wrote:
> > I agree.  Do you want to create the patch or send me a patch against
>
> mine?
>
> Okay - so here we go.  This patch uses black make magic to construct
> the list of .mk files to be included.  But wait, thats not all - it
> consolidates all of the platform specific coreboot .mk and kernel .mk
> includes into generic toplevel packages/coreboot-v2/coreboot-v2.mk and
> packages/kernel/kernel.mk files so that we don't have to specially include
> files from within the toplevel Makefile.   Finally, to avoid automagically
> including roms.mk and geodevsa.mk, I revamped how geodevsa works.  I'm not
> super happy with it, but its at least reasonable and gets rid of some
> ifdefs.  We'll need to address this in more detail when we revamp the
> lar UI and have other platforms that need option ROMs.
>
> Please test and report back - this isn't a really scary patch, but it
> touches
> lots of files and lots of platforms.

Jordan,

Could you send it again from the latest version?  Sorry for the mix up.

Thanks,
Myles

[Jordan Crouse]

On 28/04/08 13:08 -0600, Myles Watson wrote:
> > -----Original Message-----
> > From: Jordan Crouse [mailto:[e-mail]]
> > Sent: Friday, April 25, 2008 3:53 PM
> > To: Myles Watson
> > Cc: [e-mail]
> > Subject: Re: buildrom bug
> >
> > On 25/04/08 12:15 -0600, Myles Watson wrote:
> > > I agree.  Do you want to create the patch or send me a patch against
> >
> > mine?
> >
> > Okay - so here we go.  This patch uses black make magic to construct
> > the list of .mk files to be included.  But wait, thats not all - it
> > consolidates all of the platform specific coreboot .mk and kernel .mk
> > includes into generic toplevel packages/coreboot-v2/coreboot-v2.mk and
> > packages/kernel/kernel.mk files so that we don't have to specially include
> > files from within the toplevel Makefile.   Finally, to avoid automagically
> > including roms.mk and geodevsa.mk, I revamped how geodevsa works.  I'm not
> > super happy with it, but its at least reasonable and gets rid of some
> > ifdefs.  We'll need to address this in more detail when we revamp the
> > lar UI and have other platforms that need option ROMs.
> >
> > Please test and report back - this isn't a really scary patch, but it
> > touches
> > lots of files and lots of platforms.
>
> Jordan,
>
> Could you send it again from the latest version?  Sorry for the mix up.

It should apply against the latest version, I did the nrv2b.mk fixup 
locally in my tree.

Jordan

[Myles Watson]

> -----Original Message-----
> From: Jordan Crouse [mailto:[e-mail]]
> Sent: Monday, April 28, 2008 1:15 PM
> To: Myles Watson
> Cc: [e-mail]
> Subject: Re: buildrom bug
>
> On 28/04/08 13:08 -0600, Myles Watson wrote:
> > > -----Original Message-----
> > > From: Jordan Crouse [mailto:[e-mail]]
> > > Sent: Friday, April 25, 2008 3:53 PM
> > > To: Myles Watson
> > > Cc: [e-mail]
> > > Subject: Re: buildrom bug
> > >
> > > On 25/04/08 12:15 -0600, Myles Watson wrote:
> > > > I agree.  Do you want to create the patch or send me a patch against
> > >
> > > mine?
> > >
> > > Okay - so here we go.  This patch uses black make magic to construct
> > > the list of .mk files to be included.  But wait, thats not all - it
> > > consolidates all of the platform specific coreboot .mk and kernel .mk
> > > includes into generic toplevel packages/coreboot-v2/coreboot-v2.mk and
> > > packages/kernel/kernel.mk files so that we don't have to specially
>
> include
> > > files from within the toplevel Makefile.   Finally, to avoid
>
> automagically
> > > including roms.mk and geodevsa.mk, I revamped how geodevsa works.  I'm
>
> not
> > > super happy with it, but its at least reasonable and gets rid of some
> > > ifdefs.  We'll need to address this in more detail when we revamp the
> > > lar UI and have other platforms that need option ROMs.
> > >
> > > Please test and report back - this isn't a really scary patch, but it
> > > touches
> > > lots of files and lots of platforms.
> >
> > Jordan,
> >
> > Could you send it again from the latest version?  Sorry for the mix up.
>
> It should apply against the latest version, I did the nrv2b.mk fixup
> locally in my tree.

I was actually talking about the simplify-v2 patch.  For example, there is
no more tyan-generic.mk or tyan-s2891.mk.

Thanks,
Myles

[Ward Vandewege]

On Fri, Apr 25, 2008 at 11:43:04AM -0600, Myles Watson wrote:
> > > > > The LAB and kernel payloads?
>
> You're right.  I've included them in this patch.

Hmm, I don't see them in the attached patch?
> Here's the updated patch.  It needs to be preceded by
> svn mv packages/utils packages/nrv2b
>
> Signed-off-by: Myles Watson

With LAB and kernel:

Acked-by: Ward Vandewege

Thanks,
Ward.

[Myles Watson]

> -----Original Message-----
> From: Ward Vandewege [mailto:[e-mail]]
> Sent: Friday, April 25, 2008 11:48 AM
> To: Myles Watson
> Cc: Jordan Crouse; [e-mail]
> Subject: Re: [coreboot] buildrom bug
>
> On Fri, Apr 25, 2008 at 11:43:04AM -0600, Myles Watson wrote:
> > > > > > The LAB and kernel payloads?
> >
> > You're right.  I've included them in this patch.
>
> Hmm, I don't see them in the attached patch?
> > Here's the updated patch.  It needs to be preceded by
> > svn mv packages/utils packages/nrv2b
> >
> > Signed-off-by: Myles Watson
>
> With LAB and kernel:
>
> Acked-by: Ward Vandewege

Rev 168.

Thanks,
Myles

[Jordan Crouse]

On 24/04/08 06:32 +0200, Peter Stuge wrote:
> I configured v2 and TINT, then:
>
> $ make
> /home/stuge/co/buildrom/buildrom-devel/packages/grub2/grub2.mk:28:
> *** To build GRUB2, you need to install 'ruby'.  Stop.
>
> I don't want to build GRUB2.

Oops!  Thats my bad.  Patch forthcoming.

Jordan