Hey Carl, On Tuesday 12 May 2009 10:59:28 Carl-Daniel Hailfinger wrote: > 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? > Well, no need to re-define 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? > It is useful if you have *FLAGS in your environment instead of passing all *FLAGS directly to the Makefile. The commented out stuff was already there so i just kept it. > > +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. > Of course. > > 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? > Oops, I read it as CXXFLAGS %) > > 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. > I wasn't sure if they're really needed there. > > 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. > I'm not familiar with other architectures etc. so i re-added mkdir. > > + $(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
Thanks! Signed-off-by: Christian Ruppert <[email protected]> Regards, Christian Ruppert
Index: Makefile
===================================================================
--- Makefile (revision 483)
+++ Makefile (working copy)
@@ -8,12 +8,14 @@
CC ?= gcc
STRIP = strip
-INSTALL = /usr/bin/install
-PREFIX = /usr/local
-#CFLAGS = -O2 -g -Wall -Werror
-CFLAGS = -Os -Wall -Werror
-LDFLAGS =
+INSTALL = install
+PREFIX ?= /usr/local
+CFLAGS ?= -Os -Wall -Werror
+prefix = $(DESTDIR)$(PREFIX)
+man8dir = $(prefix)/share/man/man8
+sbindir = $(prefix)/sbin
+
OS_ARCH = $(shell uname)
ifneq ($(OS_ARCH), SunOS)
STRIP_ARGS = -s
@@ -27,7 +29,7 @@
LDFLAGS += -L/usr/local/lib
endif
-LDFLAGS += -lpci -lz
+LIBS += -lpci
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 +47,10 @@
| sed -e "s/.*://" -e "s/\([0-9]*\).*/\1/")"'
$(PROGRAM): $(OBJS)
- $(CC) -o $(PROGRAM) $(OBJS) $(LDFLAGS)
- $(STRIP) $(STRIP_ARGS) $(PROGRAM)
+ $(CC) $(LDFLAGS) -o $(PROGRAM) $(OBJS) $(LIBS)
flashrom.o: flashrom.c
- $(CC) -c $(CFLAGS) $(SVNDEF) $(CPPFLAGS) $< -o $@
+ $(CC) -c $(CFLAGS) $(CPPFLAGS) $(SVNDEF) $< -o $@
clean:
rm -f $(PROGRAM) *.o
@@ -58,15 +59,18 @@
rm -f .dependencies
dep:
- @$(CC) $(SVNDEF) -MM *.c > .dependencies
+ @$(CC) $(CPPFLAGS) $(SVNDEF) -MM *.c > .dependencies
+strip: $(PROGRAM)
+ $(STRIP) $(STRIP_ARGS) $(PROGRAM)
+
pciutils:
@echo; printf "Checking for pciutils and zlib... "
@$(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 "See README for more information."; echo; \
@@ -74,9 +78,9 @@
@rm -f .test.c .test
install: $(PROGRAM)
- $(INSTALL) $(PROGRAM) $(PREFIX)/sbin
- mkdir -p $(PREFIX)/share/man/man8
- $(INSTALL) $(PROGRAM).8 $(PREFIX)/share/man/man8
+ mkdir -p $(sbindir) $(man8dir)
+ $(INSTALL) -m 0755 $(PROGRAM) $(sbindir)
+ $(INSTALL) -m 0644 $(PROGRAM).8 $(man8dir)
.PHONY: all clean distclean dep pciutils
signature.asc
Description: This is a digitally signed message part.
-- coreboot mailing list: [email protected] http://www.coreboot.org/mailman/listinfo/coreboot
