[Buildroot] [PATCH] Add packages sip and PyQt

[Arnout Vandecappelle <arnout@mind.be>]

  Hi Sergey,

  Thank you for your contribution! I have quite a few comments on your 
patch, though, so hopefully you can modify it and resend.

On 17/10/13 13:27, Sergey Kostanbaev wrote:
> Add support for sip-4.15.3 and pyqt-4.9.6 to work with qt-4.8.x. Tested on arm platform.

  Two packages should come in two separate patches: the first one adding 
sip, the second one adding pyqt. Such patches normally have a subject like:

sip: new package

  Also, we prefix python-related packages with 'python-'. So the packages 
should be called python-sip and python-pyqt.

>
> Signed-off-by: Sergey Kostanbaev <sergey.kostanbaev@gmail.com>
> ---
>   Config.in                         |    5
>   pyqt/Config.in                    |    6
>   pyqt/pyqt-4.9.6-configure.patch   |  924 ++++++++++++++++++++++++++++++++++++
>   pyqt/pyqt-4.9.6-full.patch        |  949 ++++++++++++++++++++++++++++++++++++++

  Patches shouldn't have a version number, but should be numbered. So

python-pyqt-0001-configure.patch
python-pyqt-0002-full.patch

>   pyqt/pyqt.mk                      |   63 ++
>   sip/Config.in                     |    5
>   sip/sip-4.15.3-configure.py.patch |   38 +
>   sip/sip.mk                        |   71 ++
>   8 files changed, 2061 insertions(+)
>
> diff --git a/package/Config.in b/package/Config.in
> index a94cb62..9c9ee19 100644
> --- a/package/Config.in
> +++ b/package/Config.in
> @@ -157,6 +157,11 @@ source "package/qextserialport/Config.in"
>   source "package/qjson/Config.in"
>   source "package/qtuio/Config.in"
>   source "package/qwt/Config.in"
> +
> +comment "PyQT libraries"

  I don't think this comment is needed.

> +source "package/sip/Config.in"
> +source "package/pyqt/Config.in"
> +
>   endif
>
>   source "package/qt5/Config.in"
> diff --git a/package/sip/Config.in b/package/sip/Config.in
> new file mode 100644
> index 0000000..bbdb943
> --- /dev/null
> +++ b/package/sip/Config.in
> @@ -0,0 +1,5 @@
> +config BR2_PACKAGE_SIP
> +	bool "sip"
> +	depends on BR2_PACKAGE_PYTHON
> +	help
> +	    SIP for PyQt4
> \ No newline at end of file

  I don't think we really need sip on the target. sip is needed to build 
pyqt, but we're not going to build it on the target. So you also don't 
need a Config.in for it then.

> diff --git a/package/sip/sip-4.15.3-configure.py.patch b/package/sip/sip-4.15.3-configure.py.patch
> new file mode 100644
> index 0000000..06591c0
> --- /dev/null
> +++ b/package/sip/sip-4.15.3-configure.py.patch

  Patches should have a description, similar to a commit message. They 
should also have a Signed-off-by tag. See
http://buildroot.net/downloads/manual/manual.html#_format_and_licensing_of_the_package_patches

  However, this patch is probably not needed if cross-compilation is not 
needed.

> @@ -0,0 +1,38 @@
> +--- a/configure.py	2013-04-24 12:41:35.382000017 +0400
> ++++ b/configure.py	2013-04-24 13:09:30.000000000 +0400
> +@@ -273,9 +273,9 @@
> +         "default_mod_dir":  plat_py_site_dir,
> +         "default_sip_dir":  opts.sipsipdir,
> +         "py_version":       py_version,
> +-        "py_inc_dir":       plat_py_inc_dir,
> +-        "py_conf_inc_dir":  plat_py_conf_inc_dir,
> +-        "py_lib_dir":       plat_py_lib_dir,
> ++        "py_inc_dir":       opts.py_inc_dir,
> ++        "py_conf_inc_dir":  opts.py_conf_inc_dir,
> ++        "py_lib_dir":       opts.py_lib_dir,
> +         "universal":        opts.universal,
> +         "arch":             opts.arch,
> +         "deployment_target":    opts.deployment_target
> +@@ -399,6 +399,22 @@
> +             "macros")
> +     p.add_option_group(g)
> +
> ++    # Python configuration
> ++    g = optparse.OptionGroup(p, title="Python include path")
> ++    g.add_option("-i", "--py_inc_dir", action="callback",
> ++	    default=plat_py_inc_dir, type="string", metavar="DIR",
> ++	    dest="py_inc_dir", callback=store_abspath, help="where the Python "
> ++	    "include directory located [default: %s]" % plat_py_inc_dir)
> ++    g.add_option("-c", "--py_conf_inc_dir", action="callback",
> ++	    default=plat_py_conf_inc_dir, type="string", metavar="DIR",
> ++	    dest="py_conf_inc_dir", callback=store_abspath, help="where the Python "
> ++	    "Configuration include directory located [default: %s]" % plat_py_conf_inc_dir)
> ++    g.add_option("-l", "--py_lib_dir", action="callback",
> ++	    default=plat_py_conf_inc_dir, type="string", metavar="DIR",
> ++	    dest="py_lib_dir", callback=store_abspath, help="where the Python "
> ++	    "library located [default: %s]" % plat_py_lib_dir)
> ++
> ++
> +     # Installation.
> +     g = optparse.OptionGroup(p, title="Installation")
> +     g.add_option("-b", "--bindir", action="callback",
> diff --git a/package/sip/sip.mk b/package/sip/sip.mk
> new file mode 100644
> index 0000000..2a4254c
> --- /dev/null
> +++ b/package/sip/sip.mk
> @@ -0,0 +1,71 @@
> +#############################################################
> +#
> +# sip
> +#
> +#############################################################

  That should be 80 #'s

> +SIP_VERSION = 4.15.3
> +SIP_SOURCE = sip-$(SIP_VERSION).tar.gz
> +SIP_SITE = http://sourceforge.net/projects/pyqt/files/sip/sip-$(SIP_VERSION)

  We use the direct download links on sourceforge:

http://downloads.sourceforge.net/project/pyqt/sip/sip-$(SIP_VERSION)

> +
> +################################################
> +# HOST

  This is not needed.

> +
> +HOST_SIP_DEPENDENCIES = host-python
> +define HOST_SIP_CONFIGURE_CMDS
> +    (cd $(@D); \
> +	echo "Host Configuring DIR=$(PWD)"; \

  This echo is not needed.

> +	LD_LIBRARY_PATH=$(HOST_DIR)/lib $(HOST_DIR)/usr/bin/python configure.py; \

  You should also pass HOST_CONFIGURE_OPTS in the environment. And this 
already contains (a better) LD_LIBRARY_PATH definition.

> +    )
> +endef
> +
[snip target definitions which are not needed IMO]
> +$(eval $(host-autotools-package))

  This is not an autotools package, so you shouldn't use the autotools 
rules. That does some additional things that may not apply here.

> diff --git a/package/pyqt/Config.in b/package/pyqt/Config.in
> new file mode 100644
> index 0000000..4e4ff07
> --- /dev/null
> +++ b/package/pyqt/Config.in
> @@ -0,0 +1,6 @@
> +config BR2_PACKAGE_PYQT
> +	bool "pyqt"
> +	depends on BR2_PACKAGE_QT

  It's already in an if BR2_PACKAGE_QT, so this is not needed.

> +	depends on BR2_PACKAGE_SIP

  This is not needed IMO. It requires the host sip, but not the target sip.

> +	help
> +	    PyQT4 for Qt Embedded 4

  There should be an upstream URL, i.e.
http://www.riverbankcomputing.com/software/pyqt/

  Also, help text indentation should be one tab + two spaces, not four 
spaces.

> \ No newline at end of file

  There should be a newline at the end of the file.

[snip 2 patches]

  These patches are suspiciously large. They lack a description, but I 
guess they are needed for cross-compilation. Upstream says that 
configure-ng will support cross-compilation in the future [1] so I think 
you should probably start with that script. And check in upstream's 
mercurial repository what is already possible. Or perhaps even ask 
upstream for help.

[1] http://pyqt.sourceforge.net/Docs/PyQt4/installation.html


  If you do need such large patches, try to split them up a little more 
and certainly give a good comment, so we can try to review them. Also, if 
possible, try to send them upstream. Upstream can give much more valuable 
comments on the patches.


> diff --git a/package/pyqt/pyqt.mk b/package/pyqt/pyqt.mk
> new file mode 100644
> index 0000000..6c3ccfb
> --- /dev/null
> +++ b/package/pyqt/pyqt.mk
> @@ -0,0 +1,63 @@
> +#############################################################

  80 #'s

> +#
> +# PyQT Open Source Edition for Qt Embedded (QWS)

  Just

# python-pyqt

> +#
> +#############################################################
> +PYQT_VERSION = 4.9.6
> +PYQT_SOURCE = PyQt-x11-gpl-$(PYQT_VERSION).tar.gz
> +PYQT_SITE = http://sourceforge.net/projects/pyqt/files/PyQt4/PyQt-$(PYQT_VERSION)

  Use downloads.sourceforge.net

> +
> +################################################
> +# TARGET
> +
> +PYQT_DEPENDENCIES = sip qt

  host-sip

> +
> +define PYQT_CONFIGURE_CMDS
> +    echo "Construting qtdirs"

  echo is not needed.

> +    echo $(STAGING_DIR)/usr             > $(@D)/qtdirs.out
> +    echo $(STAGING_DIR)/usr/include    >> $(@D)/qtdirs.out
> +    echo $(STAGING_DIR)/usr/lib        >> $(@D)/qtdirs.out
> +    echo $(HOST_DIR)/usr/bin           >> $(@D)/qtdirs.out
> +    echo $(STAGING_DIR)/usr            >> $(@D)/qtdirs.out
> +    echo $(STAGING_DIR)/usr/plugins    >> $(@D)/qtdirs.out
> +
> +    echo 264196                >> $(@D)/qtdirs.out
> +    echo 8                     >> $(@D)/qtdirs.out
> +    echo Open Source           >> $(@D)/qtdirs.out
> +    echo shared                >> $(@D)/qtdirs.out
> +    echo PyQt_Accessibility    >> $(@D)/qtdirs.out
> +    echo PyQt_SessionManager   >> $(@D)/qtdirs.out
> +    echo PyQt_qreal_double     >> $(@D)/qtdirs.out
> +    echo PyQt_OpenSSL          >> $(@D)/qtdirs.out
> +    echo PyQt_Shortcut         >> $(@D)/qtdirs.out
> +    echo PyQt_ButtonGroup      >> $(@D)/qtdirs.out
> +    echo PyQt_RawFont          >> $(@D)/qtdirs.out
> +    echo WS_MACX               >> $(@D)/qtdirs.out
> +    echo WS_WIN                >> $(@D)/qtdirs.out

  This really needs some comments to explain what it does.

  Also, it's nicer to keep a template in package/python-pyqt and sed-ing 
it to the build directory.

> +
> +    ( cd $(@D); \
> +	echo "TARGET_DIR=$(TARGET_DIR)" \
> +	echo "HOST_DIR=$(HOST_DIR)" \
> +	echo "BUILD_DIR=$(BUILD_DIR)" \
> +	echo "PyQT Configuring Target DIR=`pwd`"; \

  That's a pretty weird echo... And anyway not needed.

> +	PATH=$$PATH:$(HOST_DIR)/usr/bin \

  Use TARGET_CONFIGURE_OPTS.

> +	CROSS_SIPCONFIG=$(BUILD_DIR)/sip-$(SIP_VERSION)/ \

  Hm, we don't like referring to another build dir. Is it possible to 
install whatever is needed somewhere in $(HOST_DIR) and refer to that?

> +	QMAKESPEC=$(BUILD_DIR)/qt-$(QT_VERSION)/mkspecs/qws/linux-$(QT_EMB_PLATFORM)-g++ \
> +	LD_LIBRARY_PATH=$(HOST_DIR)/lib $(HOST_DIR)/usr/bin/python configure.py \

  Is LD_LIBRARY_PATH really needed?

> +		-b $(TARGET_DIR)/usr/bin \
> +		-d $(TARGET_DIR)/usr/lib/python$(PYTHON_VERSION_MAJOR)/site-packages \
> +		-l $(STAGING_DIR)/usr/include/python$(PYTHON_VERSION_MAJOR)  \
> +		-m $(STAGING_DIR)/usr/lib/python$(PYTHON_VERSION_MAJOR)/config \
> +		-q $(HOST_DIR)/usr/bin/qmake \
> +		-v $(TARGET_DIR)/usr/share/sip/PyQt4 \
> +		-w --confirm-license \
> +    )
> +endef
> +
> +define PYQT_INSTALL_TARGET_CMDS
> +# TODO copy only needed files  DESTDIR=

  Don't leave comments from your drafts lingering around. If there are 
todos, mention them in your commit message.

> +    PATH="$(PATH):$(HOST_DIR)/usr/bin"  $(MAKE1) install -C $(@D)

  Use the same environment as above.

> +    touch $(TARGET_DIR)/usr/lib/python$(PYTHON_VERSION_MAJOR)/site-packages/PyQt4/__init__.py

  Why is this needed?

> +endef
> +
> +$(eval $(autotools-package))

  Again, it's not really an autotools package.


  Regards,
  Arnout

> --
> 1.8.3.2
> _______________________________________________
> buildroot mailing list
> buildroot at busybox.net
> http://lists.busybox.net/mailman/listinfo/buildroot
>
>


-- 
Arnout Vandecappelle                          arnout at mind be
Senior Embedded Software Architect            +32-16-286500
Essensium/Mind                                http://www.mind.be
G.Geenslaan 9, 3001 Leuven, Belgium           BE 872 984 063 RPR Leuven
LinkedIn profile: http://www.linkedin.com/in/arnoutvandecappelle
GPG fingerprint:  7CB5 E4CC 6C2E EFD4 6E3D A754 F963 ECAB 2450 2F1F