Re: [ptxdist] My first Contribution! libpphidget rules :D

[Juergen Beisert <jbe@pengutronix.de>]

Hi Mohtadi,

barhoumi mohtadi wrote:
> Well, libphidget provides a set of "plug and play" building blocks for low
> cost USB sensing and control from your PC. I've needed it to recognize and
> work with an acceleromter i'm testing. Just providing it in case someone
> else need it :D
>
> Thanks for your remarks and suggestions!

Okay, lets start the review process...

> libphiget.make
>
> # -*-makefile-*-
> #
> # Copyright (C) 2012 by Mohtadi Barhoumi <mohtadi.barhoumi@univ-reims.fr>
> #
> # See CREDITS for details about who has contributed to this project.
> #
> # For further information about the PTXdist project and license conditions
> # see the README file.
> #
>
> #
> # We provide this package
> #
> PACKAGES-$(PTXCONF_LIBPHIDGET) += libphidget
>
> #
> # Paths and names
> #
> LIBPHIDGET_VERSION    := 2.1.8.20120514
> LIBPHIDGET_MD5        :=

Please add the MD5 sum here.

> LIBPHIDGET        := libphidget_$(LIBPHIDGET_VERSION)
> LIBPHIDGET_SUFFIX    := tar.gz
> LIBPHIDGET_URL        := http://www.phidgets.com/downloads/libraries/$(LIBPHIDGET).$(LIBPHIDGET_SUFFIX) LIBPHIDGET_SOURCE    := $(SRCDIR)/$(LIBPHIDGET).
$(LIBPHIDGET_SUFFIX)
> LIBPHIDGET_DIR        := $(BUILDDIR)/$(LIBPHIDGET)
> LIBPHIDGET_LICENSE    := unknown

Please add the license of this package here.

> # ----------------------------------------------------------------------------
> # Get 
> # ----------------------------------------------------------------------------
>
> #$(LIBPHIDGET_SOURCE):
> #    @$(call targetinfo)
> #    @$(call get, LIBPHIDGET)

Its completely commented out. Why not remove it?

> # ----------------------------------------------------------------------------
> # Prepare 
> # ----------------------------------------------------------------------------
>
> #LIBPHIDGET_CONF_ENV    := $(CROSS_ENV)
>
> #
> # autoconf
> #
> LIBPHIDGET_CONF_TOOL    := autoconf
> #LIBPHIDGET_CONF_OPT    := $(CROSS_AUTOCONF_USR)
>
> #$(STATEDIR)/libphidget.prepare:
> #    @$(call targetinfo)
> #    @$(call clean, $(LIBPHIDGET_DIR)/config.cache)
> #    cd $(LIBPHIDGET_DIR) && \
> #        $(LIBPHIDGET_PATH) $(LIBPHIDGET_ENV) \
> #        ./configure $(LIBPHIDGET_CONF_OPT)
> #    @$(call touch)

Are you really happy with the default configure parameters? Maybe there is
something this package is depending on, and if you do not enable or disable
it here it depends on the build order if some smart configure scripts guess if
they should compile in component X or Y...
To get a reliable result, you should check the external dependencies and add
them here or in the corresponding *.in file

A quick look to the output of "configure --help" shows at least these parameters
you should handle with care:

  --enable-jni         Compile in Java support
  --enable-debug       Turn on debugging
  --enable-zeroconf    Turn on zeroconf, choose avahi or bonjour
  --enable-zeroconf-lookup    Turn on zeroconf lookup
  --enable-labview     Turn on Labview support
  --enable-oldlibusb   Use libusb-0.1 instead of 1.0

> # ----------------------------------------------------------------------------
> # Compile 
> # ----------------------------------------------------------------------------
>
> #$(STATEDIR)/libphidget.compile:
> #    @$(call targetinfo)
> #    @$(call world/compile, LIBPHIDGET)
> #    @$(call touch)

Its completely commented out. Why not remove it?

> # ----------------------------------------------------------------------------
> # Install 
> # ----------------------------------------------------------------------------
>
> #$(STATEDIR)/libphidget.install:
> #    @$(call targetinfo)
> #    @$(call world/install, LIBPHIDGET)
> #    @$(call touch)

Same here.

> # ----------------------------------------------------------------------------
> # Target-Install 
> # ----------------------------------------------------------------------------
>
> $(STATEDIR)/libphidget.targetinstall:
>     @$(call targetinfo)
>
>     @$(call install_init, libphidget)
>     @$(call install_fixup, libphidget,PRIORITY,optional)
>     @$(call install_fixup, libphidget,SECTION,base)
>     @$(call install_fixup, libphidget,AUTHOR,"Mohtadi Barhoumi <mohtadi.barhoumi@univ-reims.fr>")
>     @$(call install_fixup, libphidget,DESCRIPTION,)
>
>     @$(call install_lib, libphidget, 0, 0, 0644, libphidget21)
>
>     @$(call install_finish, libphidget)
>
>     @$(call touch)
>
> # ----------------------------------------------------------------------------
> # Clean 
> # ----------------------------------------------------------------------------
>
> #$(STATEDIR)/libphidget.clean:
> #    @$(call targetinfo)
> #    @$(call clean_pkg, LIBPHIDGET)

Same here.

> # vim: syntax=make
>
> libphidget.in
>
> ## SECTION=system_libraries
>
> config LIBPHIDGET
>     tristate
>     prompt "libphidget"
>     help
>       Phidgets are a set of plug and play building blocks for low
>       cost USB sensing and control from your PC.

The "--enable-oldlibusb" configure parameter presents at least a dependency to
the LIBUSB. Or the LIBUSB_COMPAT, depending on the parameter itself.

Without considering this dependency, your package may fail at build-time. You
must tell PTXdist to build the libusb/libusbcompat prior your package.

Using "readelf -d libphidget21.so.0.0.0" also presents additional dependencies
to:

 - libpthread
 - libm
 - libdl

Without considering these dependencies, your package may fail at run-time.

Regards,
Juergen

-- 
Pengutronix e.K.                              | Juergen Beisert             |
Linux Solutions for Science and Industry      | http://www.pengutronix.de/  |

-- 
ptxdist mailing list
ptxdist@pengutronix.de