[coreboot] flashrom Makefile improvements

Carl-Daniel Hailfinger c-d.hailfinger.devel.2006 at gmx.net
Tue May 12 10:59:28 CEST 2009


Hi Christian,

thanks for your patch.

A detailed review follows. Don't let the review discourage you,
Makefiles are always controversial. It is entirely possible someone will
post a different review which tells you all changes are good.

On 09.05.2009 04:32, Christian Ruppert wrote:
> Hey guys,
>
> I wrote a patch which includes some Makefile improvements so please review.
>
> Signed-off-by: Christian Ruppert <spooky85 at gmail.com>
>
> Index: Makefile
> ===================================================================
> --- Makefile	(revision 483)
> +++ Makefile	(working copy)
> @@ -8,12 +8,16 @@
>  
>  CC     ?= gcc
>  STRIP	= strip
> -INSTALL = /usr/bin/install
> -PREFIX  = /usr/local
> +INSTALL = install
> +PREFIX  ?= /usr/local
> +DESTDIR =
>   

Any reason to clear DESTDIR?

>  #CFLAGS  = -O2 -g -Wall -Werror
> -CFLAGS  = -Os -Wall -Werror
> -LDFLAGS = 
> +CFLAGS  ?= -Os -Wall -Werror
>   

Hm. Not completely sure about making CFLAGS conditional. I like it, though.
Could you kill the commented out CFLAGS line?

>  
> +prefix = $(DESTDIR)$(PREFIX)
> +man8dir = $(prefix)/share/man/man8
> +sbindir = $(prefix)/sbin
> +
>  OS_ARCH	= $(shell uname)
>  ifneq ($(OS_ARCH), SunOS)
>  STRIP_ARGS = -s
> @@ -27,7 +31,7 @@
>  LDFLAGS += -L/usr/local/lib
>  endif
>  
> -LDFLAGS += -lpci -lz
> +LIBS += -lpci
>   

OK.

>  
>  OBJS = chipset_enable.o board_enable.o udelay.o jedec.o stm50flw0x0x.o \
>  	sst28sf040.o am29f040b.o mx29f002.o sst39sf020.o m29f400bt.o \
> @@ -45,11 +49,11 @@
>            | sed -e "s/.*://" -e "s/\([0-9]*\).*/\1/")"'
>  
>  $(PROGRAM): $(OBJS)
> -	$(CC) -o $(PROGRAM) $(OBJS) $(LDFLAGS)
> +	$(CC) $(LDFLAGS) -o $(PROGRAM) $(OBJS) $(LIBS)
>  	$(STRIP) $(STRIP_ARGS) $(PROGRAM)
>   

While you're touching this code, can you move the stripping to a
separate rule so it is possible to create unstripped binaries?
Distributions want that.

>  
>  flashrom.o: flashrom.c
> -	$(CC) -c $(CFLAGS) $(SVNDEF) $(CPPFLAGS) $< -o $@
> +	$(CC) -c $(CFLAGS) $(SVNDEF) $< -o $@
>   

Any reason to remove the CPPFLAGS (C preprocessor flags) from the
flashrom rule?

>  
>  clean:
>  	rm -f $(PROGRAM) *.o
> @@ -58,25 +62,26 @@
>  	rm -f .dependencies
>  
>  dep:
> -	@$(CC) $(SVNDEF) -MM *.c > .dependencies
> +	@$(CC) $(CFLAGS) $(SVNDEF) -MM *.c > .dependencies
>   

In theory, CFLAGS should not be needed here and CPPFLAGS be used instead.

>  
>  pciutils:
> -	@echo; printf "Checking for pciutils and zlib... "
> +	@echo; printf "Checking for pciutils... "
>  	@$(shell ( echo "#include <pci/pci.h>";		   \
>  		   echo "struct pci_access *pacc;";	   \
>  		   echo "int main(int argc, char **argv)"; \
>  		   echo "{ pacc = pci_alloc(); return 0; }"; ) > .test.c )
> -	@$(CC) $(CFLAGS) .test.c -o .test $(LDFLAGS) >/dev/null 2>&1 &&	\
> +	@$(CC) $(CFLAGS) $(LDFLAGS) .test.c -o .test $(LIBS) >/dev/null 2>&1 &&	\
>  		echo "found." || ( echo "not found."; echo;		\
> -		echo "Please install pciutils-devel and zlib-devel.";	\
> +		echo "Please install pciutils-devel.";	\
>  		echo "See README for more information."; echo;		\
>  		rm -f .test.c .test; exit 1)
>  	@rm -f .test.c .test
>   

There are pciutils versions out there where compiling any file with
pci/pci.h also requires zlib headers (but not zlib itself). This has to
stay unchanged. Sorry. Think of it as a workaround for old broken
software releases still present on soe of today's systems.

>  
>  install: $(PROGRAM)
> -	$(INSTALL) $(PROGRAM) $(PREFIX)/sbin
> -	mkdir -p $(PREFIX)/share/man/man8
> -	$(INSTALL) $(PROGRAM).8 $(PREFIX)/share/man/man8
> +	$(INSTALL) -d $(sbindir)
> +	$(INSTALL) -d $(man8dir)
>   

The -d switch for install is not supported everywhere, but mkdir -p
works everywhere AFAIK.

> +	$(INSTALL) -m 0755 $(PROGRAM) $(sbindir)
> +	$(INSTALL) -m 0644 $(PROGRAM).8 $(man8dir)
>   

Not sure about -m, but it seems to be supported in all systems I checked.

>  
>  .PHONY: all clean distclean dep pciutils
>  
>   

Overall, this is a patch in the right direction.

Regards,
Carl-Daniel

-- 
http://www.hailfinger.org/





More information about the coreboot mailing list