flashrom Makefile improvements

[Christian Ruppert]

Hey guys,

I wrote a patch which includes some Makefile improvements so please review.

Signed-off-by: Christian Ruppert

Regards,
Christian

[FENG Yu Ning]

Christian Ruppert wrote:
> I wrote a patch which includes some Makefile improvements so please review.
>
> Signed-off-by: Christian Ruppert

This is ticket #118. I am not familiar with makefile stuff. I wonder
maybe someone is interested in reviewing this patch?

Sorry if I am making noise here.

yu ning

[Carl-Daniel Hailfinger]

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
> 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

[Christian Ruppert]

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
> > 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

Regards,
Christian Ruppert

[Carl-Daniel Hailfinger]

Before I apply this patch, I'd like to see a test on a non-Linux OS.

On 12.05.2009 19:17, Christian Ruppert wrote:
> Signed-off-by: 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

I'm afraid removing -lz kills compilation on my openSUSE 10.3 because it
only ships libpci.a and not libpci.so.
> 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

Error message follows:
/usr/lib/gcc/i586-suse-linux/4.2.1/../../../libpci.a(names.o): In
function `id_parse_list':
(.text+0x52c): undefined reference to `gzgets'
/usr/lib/gcc/i586-suse-linux/4.2.1/../../../libpci.a(names.o): In
function `id_parse_list':
(.text+0x674): undefined reference to `gzeof'
/usr/lib/gcc/i586-suse-linux/4.2.1/../../../libpci.a(names.o): In
function `pci_load_name_list':
(.text+0xb8d): undefined reference to `gzopen'
/usr/lib/gcc/i586-suse-linux/4.2.1/../../../libpci.a(names.o): In
function `pci_load_name_list':
(.text+0xbe6): undefined reference to `gzclose'
/usr/lib/gcc/i586-suse-linux/4.2.1/../../../libpci.a(names.o): In
function `pci_load_name_list':
(.text+0xca8): undefined reference to `gzopen'
/usr/lib/gcc/i586-suse-linux/4.2.1/../../../libpci.a(names.o): In
function `pci_load_name_list':
(.text+0xccb): undefined reference to `gzerror'
/usr/lib/gcc/i586-suse-linux/4.2.1/../../../libpci.a(names.o): In
function `pci_load_name_list':
(.text+0xce1): undefined reference to `gzclose'
/usr/lib/gcc/i586-suse-linux/4.2.1/../../../libpci.a(names.o): In
function `pci_load_name_list':
(.text+0xcf8): undefined reference to `gzclose'
collect2: ld returned 1 exit status

It should be possible to change .test.c compilation a bit to
automatically detect if we need -lz. Until then, I'd like to keep -lz in
LIBS.
As someone working quite a bit on flashrom, I am interested in keeping
it compilable on my machine ;-)

If you are OK with that change, I'd say
Acked-by: Carl-Daniel Hailfinger

Once I get a report that the patch works on a non-Linux arch and if
you're happy, I'll commit.

Regards,
Carl-Daniel

[Carl-Daniel Hailfinger]

On 14.05.2009 15:36, Carl-Daniel Hailfinger wrote:
> On 12.05.2009 19:17, Christian Ruppert wrote:
> > Signed-off-by: Christian Ruppert
>
> Before I apply this patch, I'd like to see a test on a non-Linux OS.
>
> It should be possible to change .test.c compilation a bit to
> automatically detect if we need -lz. Until then, I'd like to keep -lz in
> LIBS.
> As someone working quite a bit on flashrom, I am interested in keeping
> it compilable on my machine ;-)
>
> If you are OK with that change, I'd say
> Acked-by: Carl-Daniel Hailfinger
>
> Once I get a report that the patch works on a non-Linux arch and if
> you're happy, I'll commit.

Thanks to Idwer Vollering for testing and Christian for accepting the
-lz change.

Committed in r508.

Regards,
Carl-Daniel