[Buildroot] [PATCH v2] wiringpi: new package
Peter Seiderer
ps.report at gmx.net
Thu Dec 3 21:06:27 UTC 2015
Hello Thomas,
On Mon, 30 Nov 2015 22:10:30 +0100, Thomas Petazzoni <thomas.petazzoni at free-electrons.com> wrote:
> Dear Peter Seiderer,
>
> On Tue, 24 Nov 2015 22:36:31 +0100, Peter Seiderer wrote:
>
> > diff --git a/package/Config.in b/package/Config.in
> > index bdc3063..9d273e7 100644
> > --- a/package/Config.in
> > +++ b/package/Config.in
> > @@ -438,6 +438,7 @@ endif
> > source "package/w_scan/Config.in"
> > source "package/wf111/Config.in"
> > source "package/wipe/Config.in"
> > + source "package/wiringpi/Config.in"
>
> Isn't wiringpi mainly a library ? Maybe it should go under Libraries ->
> Hardware handling.
O.k., will fix it...
>
> > +diff --git a/devLib/Makefile b/devLib/Makefile
> > +index 0fb0033..f956abe 100644
> > +--- a/devLib/Makefile
> > ++++ b/devLib/Makefile
> > +@@ -37,7 +37,7 @@ DYNAMIC=libwiringPiDev.so.$(VERSION)
> > + #DEBUG = -g -O0
> > + DEBUG = -O2
> > + CC = gcc
> > +-INCLUDE = -I.
> > ++INCLUDE = -I$(DESTDIR)$(PREFIX)/wiringPi -I$(DESTDIR)$(PREFIX)/devLib
> > + DEFS = -D_GNU_SOURCE
> > + CFLAGS = $(DEBUG) $(DEFS) -Wformat=2 -Wall -Winline $(INCLUDE) -pipe -fPIC
> > +
> > +diff --git a/gpio/Makefile b/gpio/Makefile
> > +index 7dcd090..f5b588a 100644
> > +--- a/gpio/Makefile
> > ++++ b/gpio/Makefile
> > +@@ -33,7 +33,7 @@ endif
> > + #DEBUG = -g -O0
> > + DEBUG = -O2
> > + CC = gcc
> > +-INCLUDE = -I$(DESTDIR)$(PREFIX)/include
> > ++INCLUDE = -I$(DESTDIR)$(PREFIX)/wiringPi -I$(DESTDIR)$(PREFIX)/devLib
>
> This patch is not correct I believe. It makes the assumption that
> wiringPi is already installed in $(DESTDIR)$(PREFIX), which it is not
> when you are building it.
Intention was to set DESTDIR/PREFIX values to the build directory to avoid
the install step before linking gpio (as the original build script does)...
>
> > diff --git a/package/wiringpi/wiringpi.mk b/package/wiringpi/wiringpi.mk
> > new file mode 100644
> > index 0000000..258bb25
> > --- /dev/null
> > +++ b/package/wiringpi/wiringpi.mk
> > @@ -0,0 +1,29 @@
>
> Missing comment header.
O.k., will fix (should have marked this patch as early draft ;-) )
>
> > +WIRINGPI_VERSION = 2.29
> > +WIRINGPI_SITE = git://git.drogon.net/wiringPi
> > +WIRINGPI_INSTALL_STAGING = YES
>
> Missing license + license file information.
O.k., will fix...
>
> > +
> > +define WIRINGPI_BUILD_CMDS
> > + $(MAKE) -C $(@D)/wiringPi CC=$(TARGET_CC)
>
> It would be a lot better to use $(TARGET_CONFIGURE_OPTS), and
> $(TARGET_MAKE_ENV), i.e:
>
> $(TARGET_MAKE_ENV) $(TARGET_CONFIGURE_OPTS) $(MAKE) -C $(@D)/wiringPi
>
> the CC=$(TARGET_CC) is already part of TARGET_CONFIGURE_OPTS. Also you
Did not work, because the original Makefile used 'CC=gcc' which is only
overridden by command line options, not by environment variables...
> will probably have to adjust the Makefile to turn CFLAGS = into CFLAGS
> +=.
>
> > + $(INSTALL) -D -m 0755 $(@D)/wiringPi/libwiringPi.so.2.29 $(STAGING_DIR)/usr/lib/libwiringPi.so.2.29
> > + ln -sf $(STAGING_DIR)/usr/lib/libwiringPi.so.2.29 $(STAGING_DIR)/usr/lib/libwiringPi.so
>
> Why are you doing installation within the <pkg>_BUILDS_CMDS ? This is not good ?
Because I tried to not patch the original build system which assumes:
- build wiringPi, install wiringPi
- build devLib, install devLib
- build gpio, install gpio
But will complete rework my patch...
>
> > + $(MAKE) -C $(@D)/devLib CC=$(TARGET_CC) PREFIX=$(@D) DESTDIR=
>
> Why is DESTDIR= needed here ?
>
> > + $(INSTALL) -D -m 0755 $(@D)/devLib/libwiringPiDev.so.2.29 $(STAGING_DIR)/usr/lib/libwiringPiDev.so.2.29
> > + ln -sf $(STAGING_DIR)/usr/lib/libwiringPiDev.so.2.29 $(STAGING_DIR)/usr/lib/libwiringPiDev.so
>
> Ditto installation.
Will be changed in next version...
>
> > + $(MAKE) -C $(@D)/gpio CC=$(TARGET_CC) PREFIX=$(@D) DESTDIR=
> > +endef
> > +
> > +define WIRINGPI_INSTALL_STAGING_CMDS
> > + $(INSTALL) -D -m 0644 $(@D)/wiringPi/*.h $(STAGING_DIR)/usr/include
> > + $(INSTALL) -D -m 0644 $(@D)/devLib/*.h $(STAGING_DIR)/usr/include
>
> This is not good because $(INSTALL) -D expect a full destination path.
> I guess the easiest is just:
O.k, will fix...
>
> cp -dpfr $(@D)/wiringPi/*.h $(STAGING_DIR)/usr/include
>
> > +define WIRINGPI_INSTALL_TARGET_CMDS
> > + $(INSTALL) -D -m 0755 $(@D)/wiringPi/libwiringPi.so* $(TARGET_DIR)/usr/lib
> > + ln -sf libwiringPi.so.2.29 $(TARGET_DIR)/usr/lib/libwiringPi.so
> > + $(INSTALL) -D -m 0755 $(@D)/devLib/libwiringPiDev.so* $(TARGET_DIR)/usr/lib
> > + ln -sf libwiringPiDev.so.2.29 $(TARGET_DIR)/usr/lib/libwiringPiDev.so
> > + $(INSTALL) -D -m 0755 $(@D)/gpio/gpio $(TARGET_DIR)/usr/bin
> > + $(INSTALL) -D -m 0755 $(@D)/gpio/pintest $(TARGET_DIR)/usr/bin
>
> Same comments here: $(INSTALL) -D requires a full destination path.
> Otherwise, if $(TARGET_DIR)/usr/bin doesn't exist as a directory, you
> will have you "pintest" program installed as "bin" in $(TARGET_DIR)/usr.
>
> Moreover, the upstream project has some "install" and "install-static"
> targets. You should use them instead of doing manual installation.
>
O.k., will be in the next version...
> Finally, if you're installing unconditionally shared libraries, then it
> means that they are always being built. So your package should depend
> on !BR2_STATIC_LIBS.
>
> Can you fix those comments and send an updated version ?
Yes, will do...
Thanks for review...
Regards,
Peter
>
> Thanks!
>
> Thomas
More information about the buildroot
mailing list