[coreboot] [PATCH] Use a tag for SeaBIOS stable checkouts.

[Stefan Reinauer]

* Peter Stuge <peter at stuge.se> [110124 00:35]:
> > I neither want to check the changes I make to the config file in to
> > the local repository
> 
> Can we please not use perl for this? sed can do it just fine.
 
Sure. Not all sed versions support in-place changes though, so the code
will have to handle that. Just send the patch, I'll ack.

> Can I ask why you don't want to create a commit for the changes?

It needs more of git setup in order to use the system (such as the user
name and email), so I am hesitating to put this constraint on the users
for something that should not require a commit in the first place. It's
only a configuration change.
 
> > nor do I want to undo/redo the changes for every build,
> 
> Well, but every build can (outside abuild) switch to a different
> branch. And we need to handle that. (Ie. a user first building with
> master, then building with stable.)

I believe that my original code did that, at least it worked in my
tests.

> > nor do I want to re-checkout for every target.
> 
> Why not? Note we are talking git checkout now, not git clone.

No, actually I was talking about git clone. Sorry, I am not quite used
to all the git nomenclature yet. In fact git checkout simply failed
for me with varying errors.

> > +TAG-$(CONFIG_SEABIOS_MASTER)=origin/master
> >  TAG-$(CONFIG_SEABIOS_STABLE)=rel-0.6.1.3
> ..
> >  checkout:
> >         echo "Checking out SeaBIOS $(TAG-y)"
> > -       test -d seabios && ( cd seabios; git pull ) || \
> > +       test -d seabios && ( cd seabios; git fetch ) || \
> 
> I agree that git fetch should be here, certainly not git pull.

A fetch alone does not seem to make much sense. It will not update the
tree when working with the master tag, as far as I can tell. The idea of
master is that you will always get the latest and greatest SeaBIOS, so 
the tree should be updated accordingly, eh, fetched and merged.

> But we need to consider what we want to happen if someone has made
> local changes in this repository. And we also need to consider *our*
> local changes that are done in this repository. The fact that they
> are not so easily distinguishable is a problem.

Let's start with the simple case of us just building a plain image from
the SeaBIOS git repository without the user changing stuff on top of
that. Remember, this is just the "simple path". If you want to develop
SeaBIOS and change it, you can always specify it as external ELF payload
instead of using this automatism.

> > -       cd seabios; git checkout $(TAG-y)
> > +       cd seabios; git checkout -m $(TAG-y)
> 
> I don't really like trying to do a merge of local changes here. It
> will quickly conflict with later development from upstream.

Well, unless SeaBIOS switches over to a non-checked-in, auto generated
config.h we will probably have to do this. Do we want to switch SeaBIOS
over to Kconfig? That would certainly solve the issue, and allow to
specify a default config, as well as user or coreboot specific changes
to the configuration.

> We need to weigh two objectives against each other:
> 
> 1. someone has local changes in payloads/external/SeaBIOS/seabios
> that we don't want to erase

I think that's out of focus, as described above. However, we need to
configure SeaBIOS to actually work on top of coreboot, and that requires
automatically doing "local changes" due to the way the code works currently.

> I think merging is always the wrong thing to do, because it will make
> a mess of the repo and what the user has configured is exclusively
> one of the tags we offer, so that is what they should always get.

I think it's quite incredible that we can not update the version of
SeaBIOS automatically because we enabled it to build with coreboot and
serial console support. We need to fix this constraint.

> Subject: [PATCH 1/2] Allow initial COMMONCFLAGS to be set from the make command line

The patch will probably allow CFLAGS like -O2 be overwritten by -Os?

I think COMMONCFLAGS is not something the user should set. The commonly
used notion would suggest the user set CFLAGS instead.
 
>  Makefile |    2 +-
>  1 files changed, 1 insertions(+), 1 deletions(-)
> 
> diff --git a/Makefile b/Makefile
> index 384bf79..ccdf96a 100644
> --- a/Makefile
> +++ b/Makefile
> @@ -27,7 +27,7 @@ cc-option = $(shell if test -z "`$(1) $(2) -S -o /dev/null -xc \
>                /dev/null 2>&1`"; then echo "$(2)"; else echo "$(3)"; fi ;)
>  
>  # Default compiler flags
> -COMMONCFLAGS = -Os -MD -Wall -Wno-strict-aliasing -Wold-style-definition \
> +COMMONCFLAGS += -Os -MD -Wall -Wno-strict-aliasing -Wold-style-definition \
>                 $(call cc-option,$(CC),-Wtype-limits,) \
>                 -m32 -march=i386 -mregparm=3 -mpreferred-stack-boundary=2 \
>                 -mrtd -minline-all-stringops \
 

> From: Peter Stuge <peter at stuge.se>
> Date: Mon, 24 Jan 2011 00:20:18 +0100
> Subject: [PATCH 2/2] Allow override of CONFIG_COREBOOT CONFIG_DEBUG_SERIAL CONFIG_VGAHOOKS
> 
> This allows the coreboot build system control over these parameters
> without having to modify src/config.h.
> ---
>  src/config.h |    8 ++++++++
>  1 files changed, 8 insertions(+), 0 deletions(-)
> 
> diff --git a/src/config.h b/src/config.h
> index db3e578..f0cba10 100644
> --- a/src/config.h
> +++ b/src/config.h
> @@ -13,12 +13,18 @@
>  #define CONFIG_APPNAME4 "BXPC"
>  
>  // Configure as a coreboot payload.
> +#ifndef CONFIG_COREBOOT
>  #define CONFIG_COREBOOT 0
> +#endif
>  
>  // Control how verbose debug output is.
> +#ifndef CONFIG_DEBUG_LEVEL
>  #define CONFIG_DEBUG_LEVEL 1
> +#endif
>  // Send debugging information to serial port
> +#ifndef CONFIG_DEBUG_SERIAL
>  #define CONFIG_DEBUG_SERIAL 0
> +#endif
>  // Screen writes are also sent to debug ports.
>  #define CONFIG_SCREEN_AND_DEBUG 1
>  
> @@ -120,7 +126,9 @@
>  // Support generation of ACPI tables (for emulators)
>  #define CONFIG_ACPI 1
>  // Support bios callbacks specific to via vgabios.
> +#ifndef CONFIG_VGAHOOKS
>  #define CONFIG_VGAHOOKS 0
> +#endif
>  // Support S3 resume handler.
>  #define CONFIG_S3_RESUME 1
>  // Run the vga rom during S3 resume.
>

Should other flags be exported as well? All of them? 

I like the approach, and it is the right thing to do, not changing the
code. But it also adds "logic" to the config file, which is a bit eerie.

> Index: payloads/external/SeaBIOS/Makefile.inc
> ===================================================================
> --- payloads/external/SeaBIOS/Makefile.inc	(revision 6292)
> +++ payloads/external/SeaBIOS/Makefile.inc	(working copy)
> @@ -5,20 +5,12 @@
>  
>  all: seabios
>  
> -seabios: patch
> -	cd seabios; $(MAKE) CC="$(CC)" LD="$(LD)"
> +seabios: checkout
> +	cd seabios; $(MAKE) CC="$(CC)" LD="$(LD)" COMMONCFLAGS="-DCONFIG_COREBOOT=1 -DCONFIG_DEBUG_SERIAL=1 -DCONFIG_VGAHOOKS=1"
>  
> -patch: checkout
> -	test -r seabios/.patched || \
> -	perl -pi -e "s,#define CONFIG_COREBOOT 0,#define CONFIG_COREBOOT 1,;" \
> -		 -e "s,#define CONFIG_DEBUG_SERIAL 0,#define CONFIG_DEBUG_SERIAL 1,;" \
> -		 -e "s,#define CONFIG_VGAHOOKS 0,#define CONFIG_VGAHOOKS 1,;" \
> -		 seabios/src/config.h
> -	touch seabios/.patched
> -
>  checkout:
>  	echo "Checking out SeaBIOS $(TAG-y)"
> -	test -d seabios && ( cd seabios; git pull ) || \
> +	test -d seabios && ( cd seabios; git fetch ) || \

will this actually make sure I am running the latest version when
selecting master?

If so, I think we should go with this until we have a better solution.
It will require the above config.h change to be checked into a new
stable release first though, and our stable release version in
payloads/external/SeaBIOS/Makefile.inc should be pushed to that version
then.

Stefan