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 <[email protected]> > > 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/ -- coreboot mailing list: [email protected] http://www.coreboot.org/mailman/listinfo/coreboot
