[coreboot] flashrom Makefile improvements
Christian Ruppert
spooky85 at gmail.com
Tue May 12 19:17:06 CEST 2009
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 <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?
>
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 <spooky85 at gmail.com>
Regards,
Christian Ruppert
-------------- next part --------------
A non-text attachment was scrubbed...
Name: Makefile.patch
Type: text/x-patch
Size: 2311 bytes
Desc: not available
URL: <http://www.coreboot.org/pipermail/coreboot/attachments/20090512/5f46b0f0/attachment.patch>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 198 bytes
Desc: This is a digitally signed message part.
URL: <http://www.coreboot.org/pipermail/coreboot/attachments/20090512/5f46b0f0/attachment.sig>
More information about the coreboot
mailing list