Fix feature detection when fdti requires libusb for linking
see attached patch. patch is too small to be copyrightable... ciao Joerg -- Joerg Mayer <jmayer@loplof.de> We are stuck with technology when what we really want is just stuff that works. Some say that should read Microsoft instead of technology.
Hi Jörg, thanks for your patch. Review follows. On 23.08.2009 07:52, Joerg Mayer wrote:
Index: Makefile =================================================================== --- Makefile (revision 696) +++ Makefile (working copy) @@ -75,12 +75,13 @@ endif endif
+FTDILIBS = $(shell pkg-config --libs libftdi)
AFAICS this breaks if pkg-config is not installed. Maybe use -lftdi as fallback? Besides that, on my machine it looks like this: # pkg-config --libs libftdi -L/usr/local/lib -lftdi -lusb However, compiling and linking with -lftdi (and no other linker parameter) works fine here. I'm not familiar enough with pkg-config to know why this is the case. Wouldn't we need # pkg-config --cflags libftdi for FEATURE_CFLAGS as well?
FEATURE_CFLAGS += $(shell LC_ALL=C grep -q "FTDISUPPORT := yes" .features && printf "%s" "-D'FT2232_SPI_SUPPORT=1'")
-FEATURE_LIBS += $(shell LC_ALL=C grep -q "FTDISUPPORT := yes" .features && printf "%s" "-lftdi") +FEATURE_LIBS += $(shell LC_ALL=C grep -q "FTDISUPPORT := yes" .features && echo $(FTDILIBS))
$(PROGRAM): $(OBJS) - $(CC) $(LDFLAGS) -o $(PROGRAM) $(OBJS) $(LIBS) $(FEATURE_LIBS) + $(CC) $(LDFLAGS) -o $(PROGRAM) $(OBJS) $(FEATURE_LIBS) $(LIBS)
Is this one just cosmetic or am I missing some deep meaning?
# TAROPTIONS reduces information leakage from the packager's system. # If other tar programs support command line arguments for setting uid/gid of @@ -133,7 +134,7 @@ echo "struct ftdi_context *ftdic = NULL;"; \ echo "int main(int argc, char **argv)"; \ echo "{ return ftdi_init(ftdic); }"; ) > .featuretest.c ) - @$(CC) $(CFLAGS) $(LDFLAGS) .featuretest.c -o .featuretest $(LIBS) -lftdi >/dev/null 2>&1 && \ + @$(CC) $(CFLAGS) $(LDFLAGS) .featuretest.c -o .featuretest $(FTDILIBS) $(LIBS)>/dev/null 2>&1 && \
OK.
( echo "found."; echo "FTDISUPPORT := yes" >> .features.tmp ) || \ ( echo "not found."; echo "FTDISUPPORT := no" >> .features.tmp ) @$(DIFF) -q .features.tmp .features >/dev/null 2>&1 && rm .features.tmp || mv .features.tmp .features
When you resubmit (or simply answer the questions above), please add a Signed-off-by statement as well. Details of this procedure are explained at http://www.coreboot.org/Development_Guidelines#Sign-off_Procedure Regards, Carl-Daniel -- http://www.hailfinger.org/
On 8/24/09 4:05 AM, Carl-Daniel Hailfinger wrote:
Hi Jörg,
thanks for your patch. Review follows.
On 23.08.2009 07:52, Joerg Mayer wrote:
Index: Makefile =================================================================== --- Makefile (revision 696) +++ Makefile (working copy) @@ -75,12 +75,13 @@ endif endif
+FTDILIBS = $(shell pkg-config --libs libftdi)
AFAICS this breaks if pkg-config is not installed.
Maybe we should make pkg-config a requirement? The same problem happens with libpci and libz. Or is there a better way to solve this? Stefan -- coresystems GmbH • Brahmsstr. 16 • D-79104 Freiburg i. Br. Tel.: +49 761 7668825 • Fax: +49 761 7664613 Email: info@coresystems.de • http://www.coresystems.de/ Registergericht: Amtsgericht Freiburg • HRB 7656 Geschäftsführer: Stefan Reinauer • Ust-IdNr.: DE245674866
On Mon, Aug 24, 2009 at 04:05:37AM +0200, Carl-Daniel Hailfinger wrote:
On 23.08.2009 07:52, Joerg Mayer wrote:
Index: Makefile =================================================================== --- Makefile (revision 696) +++ Makefile (working copy) @@ -75,12 +75,13 @@ endif endif
+FTDILIBS = $(shell pkg-config --libs libftdi)
AFAICS this breaks if pkg-config is not installed. Maybe use -lftdi as fallback? Besides that, on my machine it looks like this: # pkg-config --libs libftdi -L/usr/local/lib -lftdi -lusb
You are right, for this to work properly we'd need to check for pkg-config first, with the downside of making the Makefile more and more unreadable.
However, compiling and linking with -lftdi (and no other linker parameter) works fine here. I'm not familiar enough with pkg-config to know why this is the case.
The reason has nothing at all to do with pkg-config, it has to do with the way libftdi is built on my system: It has usb support and thus reqiures that whenever I link against libftdi, I need to link against libusb as well.
Wouldn't we need # pkg-config --cflags libftdi for FEATURE_CFLAGS as well?
Yes, that would be the proper way - I didn't do it for the same reasons that I left out the pkg-config check: It isn't needed on my system :-) and because it makes the Makefile more complex.
FEATURE_CFLAGS += $(shell LC_ALL=C grep -q "FTDISUPPORT := yes" .features && printf "%s" "-D'FT2232_SPI_SUPPORT=1'")
-FEATURE_LIBS += $(shell LC_ALL=C grep -q "FTDISUPPORT := yes" .features && printf "%s" "-lftdi") +FEATURE_LIBS += $(shell LC_ALL=C grep -q "FTDISUPPORT := yes" .features && echo $(FTDILIBS))
$(PROGRAM): $(OBJS) - $(CC) $(LDFLAGS) -o $(PROGRAM) $(OBJS) $(LIBS) $(FEATURE_LIBS) + $(CC) $(LDFLAGS) -o $(PROGRAM) $(OBJS) $(FEATURE_LIBS) $(LIBS)
Is this one just cosmetic or am I missing some deep meaning?
IIRC, some systems or some linker flags require that the libs are listed in dependency order - so lets call that cosmetic.
# TAROPTIONS reduces information leakage from the packager's system. # If other tar programs support command line arguments for setting uid/gid of @@ -133,7 +134,7 @@ echo "struct ftdi_context *ftdic = NULL;"; \ echo "int main(int argc, char **argv)"; \ echo "{ return ftdi_init(ftdic); }"; ) > .featuretest.c ) - @$(CC) $(CFLAGS) $(LDFLAGS) .featuretest.c -o .featuretest $(LIBS) -lftdi >/dev/null 2>&1 && \ + @$(CC) $(CFLAGS) $(LDFLAGS) .featuretest.c -o .featuretest $(FTDILIBS) $(LIBS)>/dev/null 2>&1 && \
OK.
( echo "found."; echo "FTDISUPPORT := yes" >> .features.tmp ) || \ ( echo "not found."; echo "FTDISUPPORT := no" >> .features.tmp ) @$(DIFF) -q .features.tmp .features >/dev/null 2>&1 && rm .features.tmp || mv .features.tmp .features
Given all the details above: Change things as you like and feel free to add a Signed-off-by: Joerg Mayer <jmayer@loplof.de> line. As an alternative, I offer to write a CMakefile for flashrom. While I'm no expert working with CMake, I've worked with it a bit and am confident that I could handle the rather trivial dependencies this project has. Thas is to say getting rid of "make features" and handling that stuff via CMake. The downside is that flashrom requires a new build tool and the project maintainers need to know something about it. Also, *right now* it still is a bit of an overkill. The good thing is, that the project is bound to acquire new features (no, I'm not talking about sending email ;) and it looks like other platforms might like to pick up flashrom as well - and CMake is multiplatform including Windows. Ciao Joerg -- Joerg Mayer <jmayer@loplof.de> We are stuck with technology when what we really want is just stuff that works. Some say that should read Microsoft instead of technology.
Retrieve the proper linker flags for libftdi via pkg-config and fall back if pkg-config isn't available or if it doesn't know libftdi. Fix $LIBS and $FEATURE_LIBS to honor dependency order. The original patch is from Jörg, it has been updated to work on the current tree and to have a fallback in case pkg-config is not available or not working. Signed-off-by: Jörg Mayer <jmayer@loplof.de> Acked-by: Carl-Daniel Hailfinger <c-d.hailfinger.devel.2006@gmx.net> Index: flashrom-libftdi_pkgconfig/Makefile =================================================================== --- flashrom-libftdi_pkgconfig/Makefile (Revision 761) +++ flashrom-libftdi_pkgconfig/Makefile (Arbeitskopie) @@ -120,10 +120,11 @@ OBJS += satasii.o endif +FTDILIBS := $(shell pkg-config --libs libftdi 2>/dev/null || printf "%s" "-lftdi -lusb") ifeq ($(CONFIG_FT2232SPI), yes) # This is a totally ugly hack. FEATURE_CFLAGS += $(shell LC_ALL=C grep -q "FTDISUPPORT := yes" .features && printf "%s" "-D'FT2232_SPI_SUPPORT=1'") -FEATURE_LIBS += $(shell LC_ALL=C grep -q "FTDISUPPORT := yes" .features && printf "%s" "-lftdi") +FEATURE_LIBS += $(shell LC_ALL=C grep -q "FTDISUPPORT := yes" .features && printf "%s" "$(FTDILIBS)") OBJS += ft2232_spi.o endif @@ -146,7 +147,7 @@ FEATURE_LIBS += $(shell LC_ALL=C grep -q "NEEDLIBZ := yes" .libdeps && printf "%s" "-lz") $(PROGRAM): $(OBJS) - $(CC) $(LDFLAGS) -o $(PROGRAM) $(OBJS) $(LIBS) $(FEATURE_LIBS) + $(CC) $(LDFLAGS) -o $(PROGRAM) $(OBJS) $(FEATURE_LIBS) $(LIBS) # TAROPTIONS reduces information leakage from the packager's system. # If other tar programs support command line arguments for setting uid/gid of @@ -218,7 +219,7 @@ echo "struct ftdi_context *ftdic = NULL;"; \ echo "int main(int argc, char **argv)"; \ echo "{ return ftdi_init(ftdic); }"; ) > .featuretest.c ) - @$(CC) $(CFLAGS) $(LDFLAGS) .featuretest.c -o .featuretest $(LIBS) -lftdi >/dev/null 2>&1 && \ + @$(CC) $(CFLAGS) $(LDFLAGS) .featuretest.c -o .featuretest $(FTDILIBS) $(LIBS) >/dev/null 2>&1 && \ ( echo "found."; echo "FTDISUPPORT := yes" >> .features.tmp ) || \ ( echo "not found."; echo "FTDISUPPORT := no" >> .features.tmp ) @$(DIFF) -q .features.tmp .features >/dev/null 2>&1 && rm .features.tmp || mv .features.tmp .features -- Developer quote of the week: "We are juggling too many chainsaws and flaming arrows and tigers."
Jörg checked it, and told me to go ahead. Committed in r762. Regards, Carl-Daniel -- Developer quote of the week: "We are juggling too many chainsaws and flaming arrows and tigers."
participants (3)
-
Carl-Daniel Hailfinger -
Joerg Mayer -
Stefan Reinauer