Added uwsgi to packages

Hi,
please find attached a patch to add the uwsgi (
https://uwsgi-docs.readthedocs.org/en/latest/) package to buildroot

Best regards

Dear Andrea Cappelli,

Thanks for your contribution!

On Tue, 29 Dec 2015 16:07:11 +0100, Andrea Cappelli wrote:

> please find attached a patch to add the uwsgi (
> https://uwsgi-docs.readthedocs.org/en/latest/) package to buildroot

Thanks. However, you posted your patch as an attachment, which is quite
impractical to review. Could you send it inline, by using the "git
send-email" tool, as explained in the Buildroot documentation at
https://buildroot.org/downloads/manual/manual.html#submitting-patches ?

I am nonetheless trying to give you some review below.

> From 28572c6adc48fd64d69f12f89312be587d991fbc Mon Sep 17 00:00:00 2001
> From: Andrea Cappelli <a.cappelli@gmail.com>
> Date: Tue, 29 Dec 2015 16:02:51 +0100
> Subject: [PATCH 1/1] Added uwsgi package

The title should be:

	uwsgi: new package

> 
> 
> Signed-off-by: Andrea Cappelli <a.cappelli@gmail.com>
> ---
>  package/Config.in                                  |    1 +
>  ...ent-variable-to-support-cross-compilation.patch |   17 ++++++++++
>  ...ent-variable-to-support-cross-compilation.patch |   33 ++++++++++++++++++++
>  package/uwsgi/Config.in                            |    8 +++++
>  package/uwsgi/uwsgi.mk                             |   18 +++++++++++
>  5 files changed, 77 insertions(+)
>  create mode 100644 package/uwsgi/0001-Added-environment-variable-to-support-cross-compilation.patch
>  create mode 100644 package/uwsgi/0002-Added-environment-variable-to-support-cross-compilation.patch
>  create mode 100644 package/uwsgi/Config.in
>  create mode 100644 package/uwsgi/uwsgi.mk
> 
> diff --git a/package/Config.in b/package/Config.in
> index a024d3d..e2a9b0c 100644
> --- a/package/Config.in
> +++ b/package/Config.in
> @@ -1450,6 +1450,7 @@ endif
>  	source "package/ulogd/Config.in"
>  	source "package/ushare/Config.in"
>  	source "package/ussp-push/Config.in"
> +	source "package/uwsgi/Config.in"
>  	source "package/vde2/Config.in"
>  	source "package/vnstat/Config.in"
>  	source "package/vpnc/Config.in"
> diff --git a/package/uwsgi/0001-Added-environment-variable-to-support-cross-compilation.patch b/package/uwsgi/0001-Added-environment-variable-to-support-cross-compilation.patch
> new file mode 100644
> index 0000000..3216cc7
> --- /dev/null
> +++ b/package/uwsgi/0001-Added-environment-variable-to-support-cross-compilation.patch

This patch and the second patch should be merged, they really are the
same thing.

> @@ -0,0 +1,17 @@
> +Added environment variable to support cross-compilation (1)
> +
> +diff --git a/uwsgiconfig.py b/uwsgiconfig.py
> +index 29051d5..e8cb2d2 100644
> +--- a/uwsgiconfig.py
> ++++ b/uwsgiconfig.py
> +@@ -120,6 +120,9 @@ def binarize(name):
> +     return name.replace('/', '_').replace('.','_').replace('-','_')
> + 
> + def spcall(cmd):
> ++    if 'UWSGI_CROSS_COMPILE' in os.environ and not os.path.isabs(cmd):
> ++        cmd = os.path.join(os.environ['UWSGI_CROSS_COMPILE'],
> ++                           'usr', 'bin', cmd)

This looks wrong: you are pointing UWSGI_CROSS_COMPILE to
$(STAGING_DIR), which contains things built for the target. And then...

> +     p = subprocess.Popen(cmd, shell=True, stdout=subprocess.PIPE,stderr=open('uwsgibuild.log','w'))

You run on the build machine a command installed in STAGING_DIR.
Doesn't look good. Can you explain what you're trying to do here?

> + 
> +     if p.wait() == 0:
> +
> diff --git a/package/uwsgi/0002-Added-environment-variable-to-support-cross-compilation.patch b/package/uwsgi/0002-Added-environment-variable-to-support-cross-compilation.patch
> new file mode 100644
> index 0000000..6230132
> --- /dev/null
> +++ b/package/uwsgi/0002-Added-environment-variable-to-support-cross-compilation.patch
> @@ -0,0 +1,33 @@
> +Added environment variable to support cross-compilation (2)
> +
> +diff --git a/plugins/python/uwsgiplugin.py b/plugins/python/uwsgiplugin.py
> +index f9b4ddb..c5d3e2b 100644
> +--- a/plugins/python/uwsgiplugin.py
> ++++ b/plugins/python/uwsgiplugin.py
> +@@ -51,11 +51,21 @@ if not 'UWSGI_PYTHON_NOLIB' in os.environ:
> +             LIBS.append('-lutil')
> +     else:
> +         try:
> +-            LDFLAGS.append("-L%s" % sysconfig.get_config_var('LIBDIR'))
> +-            os.environ['LD_RUN_PATH'] = "%s" % (sysconfig.get_config_var('LIBDIR'))

Hum, I don't remember if our sysconfig is not supposed to return
correct values, provided the appropriate _python_sysroot environment
variable is defined. But I'm not sure anymore, this it would be worth checking.

> ++            if not 'UWSGI_CROSS_COMPILE' in os.environ:
> ++                path_ = sysconfig.get_config_var('LIBDIR')
> ++            else:
> ++                path_ = os.path.join(os.environ['UWSGI_CROSS_COMPILE'],
> ++                                            'usr', 'lib')

I think UWSGI_CROSS_COMPILE is not the best choice. Something like
'SYSROOT' would be better for example. CROSS_COMPILE variables
typically point to the cross-compilation tools, which is not what
you're doing here.

> ++            LDFLAGS.append("-L%s" % path_)
> ++            os.environ['LD_RUN_PATH'] = path_
> +         except:
> +-            LDFLAGS.append("-L%s/lib" % sysconfig.PREFIX)
> +-            os.environ['LD_RUN_PATH'] = "%s/lib" % sysconfig.PREFIX
> ++            if not 'UWSGI_CROSS_COMPILE' in os.environ:
> ++                path_ = "%s/lib" % sysconfig.PREFIX
> ++            else:
> ++                path_ = os.path.join(os.environ['UWSGI_CROSS_COMPILE'],
> ++                                            'usr', 'lib')
> ++            LDFLAGS.append("-L%s" % path_)
> ++            os.environ['LD_RUN_PATH'] = path_

Adding directories pointing to cross-compiled libraries in LD_RUN_PATH
seems wrong. LD_RUN_PATH will be used by the dynamic linker on your
build machine... so it will not do anything good if pointed to
cross-compiled libraries.

> + 
> +         LIBS.append('-lpython%s' % get_python_version())
> + else:
> +
> diff --git a/package/uwsgi/Config.in b/package/uwsgi/Config.in
> new file mode 100644
> index 0000000..bdeb60b
> --- /dev/null
> +++ b/package/uwsgi/Config.in
> @@ -0,0 +1,8 @@
> +config BR2_PACKAGE_UWSGI
> +	bool "uwsgi"
> +        depends on BR2_PACKAGE_PYTHON

Indentation should be done via a tab.

Also, why do you depend on python here? It seems like uwsgi can be used
in many other languages than python: ruby, etc. Do we really need
python on the target to build uwsgi ?

> +	select BR2_PACKAGE_LIBXML2
> +	help
> +	  The uWSGI project aims at developing a full stack for building hosting services.
> +          https://uwsgi-docs.readthedocs.org/en/latest/
> +	  https://pypi.python.org/pypi/uWSGI

Only the former URL should be kept I believe, and it must be separated
from the description by an empty new line.

Also, the description should be wrapped to max 72 characters in width.

> diff --git a/package/uwsgi/uwsgi.mk b/package/uwsgi/uwsgi.mk
> new file mode 100644
> index 0000000..6ff6019
> --- /dev/null
> +++ b/package/uwsgi/uwsgi.mk
> @@ -0,0 +1,18 @@
> +################################################################################
> +#
> +# uwsgi
> +#
> +################################################################################
> +
> +UWSGI_VERSION = 2.0.11.2
> +UWSGI_SOURCE = uwsgi-$(UWSGI_VERSION).tar.gz
> +UWSGI_SITE = https://pypi.python.org/packages/source/u/uWSGI
> +UWSGI_SETUP_TYPE = setuptools
> +UWSGI_LICENSE = GPLv2
> +UWSGI_LICENSE_FILES = LICENSE
> +
> +UWSGI_DEPENDENCIES = libxml2
> +
> +UWSGI_ENV += UWSGI_CROSS_COMPILE="$(STAGING_DIR)"

> +
> +$(eval $(python-package))

You could also use the Makefile, to not use the python-package
infrastructure, and therefore not make this package depend on python
being available for the target.

Thanks!

Thomas

2015-12-29 18:32 GMT+01:00 Thomas Petazzoni <
thomas.petazzoni@free-electrons.com>:

> Dear Andrea Cappelli,
>
>
Hi Thomas



> Thanks. However, you posted your patch as an attachment, which is quite
> impractical to review. Could you send it inline, by using the "git
> send-email" tool, as explained in the Buildroot documentation at
> https://buildroot.org/downloads/manual/manual.html#submitting-patches ?
>

Ok, i'll configure git with my smtp account



>
> [...]
>
> This patch and the second patch should be merged, they really are the
> same thing.
>

I split in 2 separate patch because I read somewhere that each patch should
refer only to a single source file (but maybe this was a rule chosen in a
specific project, I assumed that was a common practice)



>
> [...]
> You run on the build machine a command installed in STAGING_DIR.
> Doesn't look good. Can you explain what you're trying to do here?
>
>
uwsgi was available on PyPi (pypi.python.org) as package, you can download
and recompile it. The build system is created with a setup.py which in turn
invoke some function inside uwsgiconfig.py

One of the function use the os.system command to invoke external tools,
among which are pcre-config and xml2-config, which are used to get path to
be passed to the compiler

The problem is that instead of using the buildroot ones (which I found on
STAGING_DIR/usr/bin) the ones from PATH are chosen, which leads to cross
compilation error (/urs/lib of host was not good for the purpose)

I asked in the ML of the project if someone has built for buildroot and
found an old thread in which a guy had success with a dirty hack: he
changed each call pcre-config using an absolute path to buildroot ones

The project maintainer said that the right way was to add an ENV var which
tells to build system a cross compiling is in place and so adjust path
accordingly

I can't simply add STAGING_DIR to path because if I do so other tools get
chosen from buildroot instead of host, so having the opposite problem



> > +
> > +     if p.wait() == 0:
> > +
> > diff --git
> a/package/uwsgi/0002-Added-environment-variable-to-support-cross-compilation.patch
> b/package/uwsgi/0002-Added-environment-variable-to-support-cross-compilation.patch
> > new file mode 100644
> > index 0000000..6230132
> > --- /dev/null
> > +++
> b/package/uwsgi/0002-Added-environment-variable-to-support-cross-compilation.patch
> > @@ -0,0 +1,33 @@
> > +Added environment variable to support cross-compilation (2)
> > +
> > +diff --git a/plugins/python/uwsgiplugin.py
> b/plugins/python/uwsgiplugin.py
> > +index f9b4ddb..c5d3e2b 100644
> > +--- a/plugins/python/uwsgiplugin.py
> > ++++ b/plugins/python/uwsgiplugin.py
> > +@@ -51,11 +51,21 @@ if not 'UWSGI_PYTHON_NOLIB' in os.environ:
> > +             LIBS.append('-lutil')
> > +     else:
> > +         try:
> > +-            LDFLAGS.append("-L%s" % sysconfig.get_config_var('LIBDIR'))
> > +-            os.environ['LD_RUN_PATH'] = "%s" %
> (sysconfig.get_config_var('LIBDIR'))
>
> Hum, I don't remember if our sysconfig is not supposed to return
> correct values, provided the appropriate _python_sysroot environment
> variable is defined. But I'm not sure anymore, this it would be worth
> checking.
>


Before sending the patch I created an image for a project I'm working on,
building from scratch (a fresh clone of 2015.11.1), in my environment this
setup is working well




> I think UWSGI_CROSS_COMPILE is not the best choice. Something like
> 'SYSROOT' would be better for example. CROSS_COMPILE variables
> typically point to the cross-compilation tools, which is not what
> you're doing here.
>

Uhm, ok



>
> > ++            LDFLAGS.append("-L%s" % path_)
> > ++            os.environ['LD_RUN_PATH'] = path_
> > +         except:
> > +-            LDFLAGS.append("-L%s/lib" % sysconfig.PREFIX)
> > +-            os.environ['LD_RUN_PATH'] = "%s/lib" % sysconfig.PREFIX
> > ++            if not 'UWSGI_CROSS_COMPILE' in os.environ:
> > ++                path_ = "%s/lib" % sysconfig.PREFIX
> > ++            else:
> > ++                path_ = os.path.join(os.environ['UWSGI_CROSS_COMPILE'],
> > ++                                            'usr', 'lib')
> > ++            LDFLAGS.append("-L%s" % path_)
> > ++            os.environ['LD_RUN_PATH'] = path_
>
> Adding directories pointing to cross-compiled libraries in LD_RUN_PATH
> seems wrong. LD_RUN_PATH will be used by the dynamic linker on your
> build machine... so it will not do anything good if pointed to
> cross-compiled libraries.
>
> > +
> > +         LIBS.append('-lpython%s' % get_python_version())
> > + else:
> > +
> > diff --git a/package/uwsgi/Config.in b/package/uwsgi/Config.in
> > new file mode 100644
> > index 0000000..bdeb60b
> > --- /dev/null
> > +++ b/package/uwsgi/Config.in
> > @@ -0,0 +1,8 @@
> > +config BR2_PACKAGE_UWSGI
> > +     bool "uwsgi"
> > +        depends on BR2_PACKAGE_PYTHON
>
> Indentation should be done via a tab.
>
> Also, why do you depend on python here?


Because python is required for building and I read in manual to do so

"In their Config.in file, they should depend on BR2_PACKAGE_PYTHON so that
when Buildroot will enable Python 3 usage
for modules, we will be able to enable Python modules progressively on
Python 3."

17.8.2

But maybe I didn't understand well



> You could also use the Makefile, to not use the python-package
> infrastructure, and therefore not make this package depend on python
> being available for the target.
>

I usually install it using pip in a virtualenv (which i can't do on
buildoroot due to lack of gcc on target), I didn't compile it directly, so
using python-package seems to me the simplest way to accomplish the same
task

I can try to change this if it's problem

Best regards

Andrea,

On Tue, 29 Dec 2015 19:20:02 +0100, Andrea Cappelli wrote:

> > Thanks. However, you posted your patch as an attachment, which is quite
> > impractical to review. Could you send it inline, by using the "git
> > send-email" tool, as explained in the Buildroot documentation at
> > https://buildroot.org/downloads/manual/manual.html#submitting-patches ?
> 
> Ok, i'll configure git with my smtp account

Great, thanks!

> > This patch and the second patch should be merged, they really are the
> > same thing.
> 
> I split in 2 separate patch because I read somewhere that each patch should
> refer only to a single source file (but maybe this was a rule chosen in a
> specific project, I assumed that was a common practice)

I don't know which project has this rule, but it seems like a silly
rule. Instead, modifications should be separated into different patches
by "logical changes". See
http://lxr.free-electrons.com/source/Documentation/SubmittingPatches#L201.

> > You run on the build machine a command installed in STAGING_DIR.
> > Doesn't look good. Can you explain what you're trying to do here?
> >
> >
> uwsgi was available on PyPi (pypi.python.org) as package, you can download
> and recompile it. The build system is created with a setup.py which in turn
> invoke some function inside uwsgiconfig.py

Right. But you can also use the Makefile, which also invokes
uwsgiconfig.py.

> One of the function use the os.system command to invoke external tools,
> among which are pcre-config and xml2-config, which are used to get path to
> be passed to the compiler
> 
> The problem is that instead of using the buildroot ones (which I found on
> STAGING_DIR/usr/bin) the ones from PATH are chosen, which leads to cross
> compilation error (/urs/lib of host was not good for the purpose)
> 
> I asked in the ML of the project if someone has built for buildroot and
> found an old thread in which a guy had success with a dirty hack: he
> changed each call pcre-config using an absolute path to buildroot ones
> 
> The project maintainer said that the right way was to add an ENV var which
> tells to build system a cross compiling is in place and so adjust path
> accordingly
> 
> I can't simply add STAGING_DIR to path because if I do so other tools get
> chosen from buildroot instead of host, so having the opposite problem

Absolutely. In this case, what I'd prefer to see if to have two
environment variables called PCRE_CONFIG and XML2_CONFIG, which allow
to override the path to the pcre-config and xml2-config binaries.

Note that there is an on-going discussion in the Buildroot project to
find a better mechanism to handle such *-config scripts, and make sure
they are naturally available in the PATH when building target packages.
However, this discussion has not matured yet, so we cannot count on
that right now.

> > Also, why do you depend on python here?
> 
> 
> Because python is required for building and I read in manual to do so

As you say: "python is required for building". So you need host-python,
i.e python installed on the build machine. But most likely not python
on the target, *except* if you intend to use uwsgi for Python
applications. Apparently, uwsgi is mainly a C library, no ?

> "In their Config.in file, they should depend on BR2_PACKAGE_PYTHON so that
> when Buildroot will enable Python 3 usage
> for modules, we will be able to enable Python modules progressively on
> Python 3."

Right, for packages that need Python on the target.

> > You could also use the Makefile, to not use the python-package
> > infrastructure, and therefore not make this package depend on python
> > being available for the target.
> 
> I usually install it using pip in a virtualenv (which i can't do on
> buildoroot due to lack of gcc on target), I didn't compile it directly, so
> using python-package seems to me the simplest way to accomplish the same
> task
> 
> I can try to change this if it's problem

I don't really like the fact that it's a python-package if really it
isn't one. Can you try making it a generic-package, with
<pkg>_BUILD_CMDS calling $(MAKE) -C $(@D), and adding host-python to
the <pkg>_DEPENDENCIES variable ?

Thanks!

Thomas

2015-12-29 20:42 GMT+01:00 Thomas Petazzoni <
thomas.petazzoni@free-electrons.com>:

> Andrea,
>
> [...]
> I don't really like the fact that it's a python-package if really it
> isn't one. Can you try making it a generic-package, with
> <pkg>_BUILD_CMDS calling $(MAKE) -C $(@D), and adding host-python to
> the <pkg>_DEPENDENCIES variable ?
>

Hi Thomas,
as soon as I have some spare time I'll rework the patch as you suggested,
try in on my dev board and send back

Thank you for your review