[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