buildsystem: add python toolchain #2277

thoradia · 2017-11-29T19:18:46Z

This adds the python2 toolchain to the amazing buildsystem rework by @InuSasha
This toolchain is not auto-detected, optional and allows to easily build bundled pinned Python packages.
cffi is provided as a sample, see my tree for others. This could be used to build TUF or other Python dependencies.
I hope it will make it this time.

InuSasha · 2017-11-29T20:25:57Z

I had think about this, too. but i have no knowledges about python.

but i see three points:

please add the license header to the cffi package
is it possible to split the build and the install steps into two parts (make_x and makeinstall_x)?
in the make:host step, the "echo Executing" is missing.

thanks,
Sascha

thoradia · 2017-11-29T20:50:08Z

Thank you for the feedback

cffi is a sample to test the script, that i intended to remove it the PR is approved
for host I can split build/install into make/makeinstall
for target such a split is difficult because easy_install does both build and install. This is why it is in makeinstall. Using the manual toolchain is appropriate if something needs to be done between build and install, which is fortunately not the case for the 60+ Python packages i build with my tree.
I will add echo to each step.

Waiting for further comments to update the PR

vpeter4 · 2017-11-30T08:03:22Z

Seems both package.mk files are missing header?

thoradia · 2017-11-30T08:41:33Z

@vpeter4
supplied packages are samples to demonstrate how it works. I will remove them before the PR is merged.

I have refined a bit:

added echo
split host build/install in make/makeinstall (this is not possible for target)
target packages are installed in $INSTALL/usr/lib/$PKG_PYTHON_VERSION/site-packages

thoradia · 2017-11-30T21:42:22Z

This now compresses packages that can be and removes .py and .exe files, to save space.
It works in my build and I consider it done.
If this is approved I would gladly rework TUF.

MilhouseVH · 2017-11-30T23:38:06Z

scripts/build

+    "python2:host")
+      echo "Executing (host): python setup.py build" | tr -s " "
+      export LDSHARED="$CC -shared $LDSHARED"
+      python setup.py build --build-base .$HOST_NAME


Why not python2 instead of python? Same for down below.

To leave room for python3

Not really sure I follow... I'm referring to the fact you are using the unversioned python sym-link rather than the versioned python2 sym-link - since this is a python2-specific change using the python2 sym-link seems more logical/appropriate/correct. Also, you could/should use the full $TOOLCHAIN/bin/python2 path and avoid using whatever version is found on $PATH.

maybe a python as a synonym of python2?

Thank you for reviewing @MilhouseVH
I fixed to use the full $TOOLCHAIN/bin/python2 path

echo lines include python2 now

I actually meant to include the $TOOLCHAIN/bin/ part too, otherwise the command being echoed is not the same as the command being executed.

Edit: Not sure how others feel about this, maybe it's unnecessary as we don't do this for cmake/ninja/make (although maybe we should - but that's a different issue).

I was not aware that these outputs were important :)

Can be, if trying to reproduce or understand a particular failure - having the exact command that failed available in the log can help enormously. That's why these echo statements are there. :)

I propose to leave this for another pull request: the other echo statements need to be updated and I do not know what level of detail you expect

thoradia · 2017-12-02T03:48:01Z

@Raybuntu TUF is back, pinned at the current releases

thoradia · 2017-12-02T08:00:42Z

To use in an add-on, copy the generated site-packages sub-directory in $ADDON_DIR/lib/$PKG_PYTHON_VERSION and run PYTHONUSERBASE=$ADDON_DIR python

thoradia · 2017-12-02T17:05:10Z

The tools.python-tools add-on provides frequently required Python packages such as lxml, pyOpenSSL and setuptools (setuptools is used by many Python packages to validate dependencies).
To use these packages from shell use PYTHONUSERBASE="/storage/.kodi/addons/tools.python-tools" python ...

thoradia · 2017-12-04T07:27:45Z

cffi is used by many platform specific Python packages and can not be easy_installed (see le_tuf) . It would therefore be profitable to add it to /usr, in order to avoid having to add it to every package/add-on that requires it. This would only add ~600k to the image. How should I add cffi to the image @MilhouseVH?

MilhouseVH · 2017-12-04T07:35:42Z

I really don't know anything about cffi or how or where it needs to be installed. If you're looking for a dependency then I guess adding it to Python2 as a PKG_TARGET_DEPENDS might work.

thoradia · 2017-12-04T07:42:27Z

$INSTALL would need to be added as is to the image root. Would adding cffi to Python2 as a PKG_TARGET_DEPENDS achieve this @MilhouseVH?

MilhouseVH · 2017-12-04T07:51:16Z

If you build cffi as a target and place the cffi install files in $INSTALL then I believe those files will be copied into the image, yes. If you add cffi to the PKG_TARGET_DEPENDS of Python2 then cffi will be built before Python2 - or do you want cffi built after Python2? If so then look at how pycryptodome is added in packages/virtual/mediacenter/package.mk.

thoradia · 2017-12-04T08:01:17Z

Thanks. I try building an image that includes cffi.

Ray-future · 2017-12-04T22:10:56Z

scripts/build

@@ -213,6 +213,11 @@ if [ "$PKG_IS_KERNEL_PKG" = "yes" ]; then
  fi
 fi

+if [ "$PKG_TOOLCHAIN" = "python" ]; then
+  PKG_DEPENDS_HOST="toolchain distutilscross:host $PKG_DEPENDS_HOST"
+  PKG_DEPENDS_TARGET="toolchain distutilscross:host Python2 $PKG_DEPENDS_TARGET"


This should still be python2 only

Would be nice to have it working with python3 too.

What do you mean? build for both Python2 and Python3 at the same time?

I mean your package will depend on Python2 only so if I have a python package that is Python3 for the Target it will depend on Python2 which is wrong.

Ray-future · 2017-12-04T22:16:25Z

scripts/build

+      _pythonpath="$_prefix/lib/$PKG_PYTHON_VERSION/site-packages"
+      mkdir -p "$_pythonpath"
+      PYTHONPATH="$PYTHONPATH:$_pythonpath" \
+      $TOOLCHAIN/bin/python setup.py easy_install \


easy_install will automatically download dependencies right? So this should be changed to "install"

Do you really intend to keep checking dependencies manually or at runtime?

By the way, the current make_host/make_install_host automatically downloads, builds and installs dependencies too: the cffi:host of #2132 would build fine without pycparser:host. So you might want to correct that.

I see no added value in keeping the current toolchain, except for problematic Python packages.

I noticed that too for host installs and I'm considering it a bug actually cause it's a side effect that can lead to weird error's.

MilhouseVH · 2017-12-05T04:41:48Z

Updated to coherently use $TOOLCHAIN/bin/python and $PKG_PYTHON_VERSION. It should therefore also work for python3.

Where is PKG_PYTHON_VERSION being set?

+      $TOOLCHAIN/bin/python setup.py easy_install \

For this to work with Pyhon3 aren't you assuming that python will be a symbolic link to python3? I'm not really sure that will be true - for the forseeable future we will have to continue building Python2 in the toolchain (for Samba) even if/when Kodi makes the switch to Python3, so python will almost always refer to python2 and never python3.

You should have kept it the way you had it - if you want to build with Python2, use $TOOLCHAIN/bin/python2. If you now want to build with Python3, then use $TOOLCHAIN/bin/python3. Don't leave it to random chance (which is basically what the unversioned sym link will be).

Since you have $PKG_PYTHON_VERSION why not use the appropriate binary? For example:

      [[ $PKG_PYTHON_VERSION =~ ^3.* ]] && _python_bin=$TOOLCHAIN/bin/python3 || _python_bin=$TOOLCHAIN/bin/python2
      $_python_bin setup.py easy_install \

Or something. But don't use the unversioned python if you expect this to work reliably with both Python2 and Python3,

thoradia · 2017-12-05T07:44:48Z

@MilhouseVH

PKG_PYTHON_VERSION=python2.7 is set in config/functions since #2083 .
I assumed that this indicated the target Python version.

If this is the case, I will

call $TOOLCHAIN/bin/$PKG_PYTHON_VERSION
update PKG_DEPENDS_HOST/TARGET according to $PKG_PYTHON_VERSION

@Raybuntu

I will add two toolchains, so we all profit:

python_simple for usual python build/install
python_easy for python easy_install

What do you say?

fixed

MilhouseVH · 2017-12-05T08:16:30Z

PKG_PYTHON_VERSION=python2.7 is set in config/functions since #2083 .
I assumed that this indicated the target Python version.

Ah yes, I'd forgotten about that variable! Yes, it indicates the specific python version to be used by the package - conceivably in future this might change to python3.x as the default, and then overridden by individual packages such as samba which (currently) has a hard dependency on python2.7 for the host.

Your proposed changes sound OK, thanks.

Ray-future · 2017-12-05T08:18:23Z

@thoradia I guess we need 2 toolchains. One for python2 and one for python3 packages. I know we don't have python3 in the image right now but it will probably change in the future.
As for the easy_install auto dependency egg installs for the target I'm skeptical and I'd like to get some feedback from @lrusak or @MilhouseVH.

thoradia · 2017-12-05T08:55:06Z

@Raybuntu
If scripts/build consistently uses $PKG_PYTHON_VERSION then

the default python version is set in config/functions
a package.mk my override PKG_PYTHON_VERSION in pre_make_target

MilhouseVH · 2017-12-05T09:29:41Z

Just an aside, but we will almost certainly need to modify the way in which PKG_PYTHON_VERSION is currently being set to avoid hard-coding a specific version in multiple packages (which would all break if/when the Python3 version is bumped, eg. from 3.6 to 3.7). The current solution works because almost everything is using python2.7 (which will never be bumped), but to be honest we can cross this bridge when we come to it - I'd favour a new variable PKG_PYTHON_LEGACY=yes|no (default yes, for now) which is then used to set PKG_PYTHON_VERSION=2.7 (yes) or PKG_PYTHON_VERSION=3.x (no) in config/functions after the package.mk has been sourced but there may be better solutions.

However, whatever we decide should have no impact on this PR as by the time this code is executed PKG_PYTHON_VERSION should be set to a usable value.

MilhouseVH · 2017-12-05T10:34:54Z

As for the easy_install auto dependency egg installs for the target I'm skeptical and I'd like to get some feedback from @lrusak or @MilhouseVH.

If eggs are a better way to install Python packages then maybe this has merit.

This is overkill when considering our only Python package, pycryptodome, but may make more sense when considering all the extra packages required by TUF.

This will need much testing (in conjunction with #2288) before proceeding.

Is python_simple/ python_easy necessary - what was the thinking behind this again? I'd rather avoid unnecessary over-complication if possible.

Ray-future · 2017-12-05T11:19:03Z

Then +1 from my side.

chewitt · 2017-12-05T11:41:17Z

I lost track of all the different python suggestions that have been floated recently, but I have a simple requirement. We need predicable (reproducible) output from the build system, which I assume requires explicit version control of all packages and no dynamic "get latest" logic which could result (over time) in different things being built. Can someone confirm this is the case?

thoradia · 2017-12-05T18:23:15Z

Thank you all for your feedback.
I will try to come back with something meaningfull by next week.

Food for thoughts (1), from the setuptools doc:
The setuptools install command is basically a shortcut to run the
easy_install command on the current project.

Food for thoughts (2):
The current makeinstall_host for Python will auto install unsatisfied requirements because it installs in PYTHONPATH, whereas makeinstall_target does not because it installs outside of PYTHONPATH. I will try to fix that.

thoradia · 2017-12-11T16:35:58Z

I have rebased this:

host uses build/install
target uses build/easy_install (to zip files if possible)
both rely on PKG_PYTHON version (2 unless specified), none rely on PKG_PYTHON_VERSION
setting PKG_PYTHON_DEPS can be used to easily determine install requirements, eg to create a new package or to update an existing package

Example:

PKG_NAME="cffi"
PKG_VERSION="1.11.2"
PKG_SHA256="ab87dd91c0c4073758d07334c1e5f712ce8fe48f007b86f8238773963ee700a6"
PKG_LICENSE="MIT"
PKG_SITE="http://cffi.readthedocs.io/"
PKG_URL="https://files.pythonhosted.org/packages/source/${PKG_NAME:0:1}/$PKG_NAME/$PKG_NAME-$PKG_VERSION.tar.gz"
PKG_DEPENDS_HOST="libffi:host"
PKG_DEPENDS_TARGET="cffi:host libffi"
PKG_LONGDESC="Foreign Function Interface for Python calling C code"

PKG_TOOLCHAIN="python"
PKG_PYTHON="3"

HiassofT · 2017-12-11T18:31:53Z

I'm no python expert but if I understand the docs correctly easy_install can download packages automatically from the 'net.

This is something we need to prevent from happening. All external stuff needed to create LE has to be version-controlled via package.mk and downloaded via scripts/get.

thoradia · 2017-12-11T19:39:45Z

@HiassofT
I think I got that point and I have implemented easy_install in such a way that it does not install requirements (unless PKG_PYTHON_DEPS is set

In fact, reviewers here focus on easy_install, but seem completely unaware of the fact that install on host downloads and installs the latest version available. In #2132 for example, cffi will install the lastest version of pycparser, irrespective of the version set in the pycparser package.mk (try PKG_VERSION=2.17 to verify).

I am sincerely trying to help, but I am getting nowehere, and you neither.

Ray-future · 2017-12-11T19:45:06Z

@thoradia your work is very appreciated. The auto install on python host is considered a bug.
Please go on.

HiassofT · 2017-12-11T23:13:20Z

@thoradia I'd also like to thank you for your contribution!

It's fine for me if you add dependency checks via PKG_PYTON_DEPS but we need to make sure that under no circumstances easy_install will download anything on it's own from the net - that would circumvent the dependency mechanism in LE and will also make it rather hard for us to provide the full source code to LE (which is a requirement of the GPL and is currently done via mirroring all packages we download from the net).

If you can configure it so it that downloads are disabled everything's fine from my side.

thoradia · 2018-02-19T21:03:05Z

See thoradia@aa6224f

thoradia force-pushed the 9.0-python2 branch 3 times, most recently from a3f841a to ab43550 Compare November 30, 2017 00:25

thoradia force-pushed the 9.0-python2 branch from ab43550 to e414491 Compare November 30, 2017 08:36

thoradia force-pushed the 9.0-python2 branch 4 times, most recently from 2a74de4 to 5bf9844 Compare November 30, 2017 20:33

MilhouseVH reviewed Nov 30, 2017

View reviewed changes

thoradia force-pushed the 9.0-python2 branch 4 times, most recently from 9f093dd to 93018b0 Compare December 2, 2017 03:41

thoradia force-pushed the 9.0-python2 branch from 93018b0 to a762a99 Compare December 2, 2017 07:27

thoradia force-pushed the 9.0-python2 branch from 7d60d30 to 92763b5 Compare December 4, 2017 07:07

thoradia force-pushed the 9.0-python2 branch 2 times, most recently from f92acbb to 4340cbf Compare December 4, 2017 17:05

Ray-future reviewed Dec 4, 2017

View reviewed changes

thoradia mentioned this pull request Dec 4, 2017

Alternative for TUF support #2288

Closed

thoradia force-pushed the 9.0-python2 branch from 9bfe1ff to ee78ada Compare December 4, 2017 22:59

thoradia force-pushed the 9.0-python2 branch from ee78ada to 466451b Compare December 5, 2017 08:52

thoradia force-pushed the 9.0-python2 branch from 466451b to 119ade8 Compare December 11, 2017 16:12

thoradia force-pushed the 9.0-python2 branch 3 times, most recently from f23a9c0 to 1f5faf6 Compare December 15, 2017 12:39

thoradia added 2 commits December 15, 2017 13:40

Python2: fix easy_install (from https://bugs.python.org/issue23102)

b713b6b

buildsystem: add python toolchain

192f698

thoradia force-pushed the 9.0-python2 branch from 1f5faf6 to 192f698 Compare December 15, 2017 12:41

thoradia closed this Feb 19, 2018

thoradia deleted the 9.0-python2 branch February 19, 2018 21:03

buildsystem: add python toolchain #2277

buildsystem: add python toolchain #2277

Conversation

thoradia commented Nov 29, 2017

InuSasha commented Nov 29, 2017

thoradia commented Nov 29, 2017

vpeter4 commented Nov 30, 2017

thoradia commented Nov 30, 2017

thoradia commented Nov 30, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MilhouseVH Dec 2, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

thoradia commented Dec 2, 2017

thoradia commented Dec 2, 2017

thoradia commented Dec 2, 2017

thoradia commented Dec 4, 2017

MilhouseVH commented Dec 4, 2017

thoradia commented Dec 4, 2017

MilhouseVH commented Dec 4, 2017

thoradia commented Dec 4, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MilhouseVH commented Dec 5, 2017 • edited

thoradia commented Dec 5, 2017 • edited

MilhouseVH commented Dec 5, 2017

Ray-future commented Dec 5, 2017

thoradia commented Dec 5, 2017

MilhouseVH commented Dec 5, 2017

MilhouseVH commented Dec 5, 2017

Ray-future commented Dec 5, 2017

chewitt commented Dec 5, 2017

thoradia commented Dec 5, 2017

thoradia commented Dec 11, 2017

HiassofT commented Dec 11, 2017

thoradia commented Dec 11, 2017

Ray-future commented Dec 11, 2017

HiassofT commented Dec 11, 2017

thoradia commented Feb 19, 2018

MilhouseVH Dec 2, 2017 •

edited

MilhouseVH commented Dec 5, 2017 •

edited

thoradia commented Dec 5, 2017 •

edited