Build: makerpms.sh generates Python 2 & 3 packages at the same time #272

pspacek · 2016-11-24T16:50:16Z

Petr Viktorin recommended me to copy the whole build directory and run
configure twice, with different values for PYTHON variable.

After thinking a bit about that, it seems as cleanest approach.
Building for two versions of Python at the same time should be
temporary state so I decided not to complicate Autotools build system
with conditional spagetti for two versions of Python.

For proper Python2/3 distiction in the two separate builds, I added
find/grep/sed combo which replaces shebangs with system-wide Python
interpreter as necessary. This is workaround for the fact that FreeIPA
does not use setuptools properly. Honza told me that proper use of
setuptools is not trivial so we decided to go with this for now.

https://fedorahosted.org/freeipa/ticket/157

tiran · 2016-11-24T16:55:13Z

AFAIK the build won't run pylint twice with the correct Python version. You could replace the configure option for pylint and the pylint command with:

$(PYTHON) -m pylint

tiran · 2016-11-24T17:12:38Z

freeipa.spec.in

+Requires: %{name}-server-common = %{version}-%{release}
+Requires: %{name}-common = %{version}-%{release}
+Requires: python3-ipaclient = %{version}-%{release}
+Requires: python-pyldap >= 2.4.15


python3-pyldap

Good catch, fixed.

pspacek · 2016-11-24T17:29:48Z

It does not automatically run pylint at all. Isn't --with-pylint option for configure good enough?

MartinBasti · 2016-11-28T13:23:18Z

Shouldn't be there python3 in BuildRequires as well? At least with python3-pylint we need python3 dependencies to be able do pylint3 validation

pspacek · 2016-11-30T13:04:38Z

Fixed. Now with_pylint section contains nested section with_python3.

pspacek · 2016-11-30T13:07:40Z

@mbasti-rh @jcholast @tiran
If you want I can replace the --with-pytlint option with --enable-pylint option (without parameters) and use cheimes's trick with $(PYTHON) -m pylint so the Pylint always follows the Python version you used for particular build. Up to you.

(Just keep in mind that build needs to be done under Python 2 till samba-python bindings are ported to Python 3.)

tiran · 2016-11-30T18:30:32Z

+1 for my trick

Since I disabled the import warnings for samba bindings in fef6f18, pylint is passing under Python 3, too.

MartinBasti · 2016-12-07T08:59:39Z

I would like rather explicit pylint version than autodetection

tiran · 2016-12-07T14:28:37Z

It makes more sense to follow the principal test what you build, build what you test.

MartinBasti · 2016-12-07T15:01:18Z

But we build both 2/3 versions at once

MartinBasti · 2016-12-07T15:12:44Z

Or we can run both pylints as far as we wants py2/3 compatible versions

MartinBasti · 2016-12-07T15:29:55Z

I had discussion with Petr, and currently we cannot run both pylints in build system and it is not easy to add it there.

So we have to manually override pylint versions in travis tests, so I would stay with the current version of commits

tiran · 2016-12-07T15:34:53Z

It's easily possible with my proposal, just saying:

make pylint PYTHON=python3
make pylint PYTHON=python2

tiran · 2016-12-07T15:39:29Z

PS: I'd rather not run both linters in parallel. We use pylint in parallel mode, which runs as many workers as CPU cores. make pylint already uses up 90-100% CPU cycles on all cores.

MartinBasti · 2016-12-07T15:42:31Z

Ok if Petr agree we can go with your proposal

pspacek · 2016-12-08T09:48:58Z

I'm fine with make pylint PYTHON=python3 as long as you can agree on it :-)

pspacek · 2016-12-08T10:36:39Z

I've implemented tiran's proposal and rebased the patchset.

tiran · 2016-12-08T11:28:39Z

freeipa.spec.in

+#BuildRequires:  python3-samba
+BuildRequires:  python3-setuptools
+# 0.6: serialization.load_pem_private_key, load_pem_public_key
+BuildRequires:  python3-cryptography >= 0.6


tiran · 2016-12-08T11:29:01Z

freeipa.spec.in

+BuildRequires:  python3-setuptools
+# 0.6: serialization.load_pem_private_key, load_pem_public_key
+BuildRequires:  python3-cryptography >= 0.6
+BuildRequires:  python3-gssapi


tiran · 2016-12-08T11:29:21Z

freeipa.spec.in

+Requires: python3-ipaclient = %{version}-%{release}
+Requires: python3-pyldap >= 2.4.15
+Requires: python3-lxml
+Requires: python3-gssapi >= 1.1.2


tiran · 2016-12-08T11:44:00Z

CI is failing: 12-08 10:53 ipadocker.cli ERROR Command echo Secret123 | kinit admin && ipa ping failed (exit code 1). I have kicked Travis. Let's see if the problem persists.
please sync the version requirements of python3-cryptography and python3-gssapi with Python 2 versions.
Regarding your workaround and setuptools comment: I have been meaning to move all scripts to setuptools' entry points for a while. Setuptools only supports one script directory, which defaults to PREFIX/bin. I need to come up with a workaround. But that's a topic for another PR.

pspacek · 2016-12-08T12:13:30Z

I've synchronized python-cryptography and python-gssapi versions. Thank you for noticing. Let's see if CI tests pass or not.

MartinBasti · 2016-12-09T10:01:28Z

ACK, but travis runs tests under PY3 and should not, I tried locally and tests works for me (ipa-run-tests is still py2 by default), so there is an issue with travis tests. I prefer to fix travis first before pushing this to not blocking tests for other PRs.

pspacek · 2016-12-09T11:11:42Z

@martbab Can I do something in the build system to make your CI implementation easier?

MartinBasti · 2016-12-09T12:54:29Z

make install put #!/usr/bin/python into ipa-run-tests and it causes that on F25 our tests are running by default under PY3

Probably travis tests uses make install (haven't looked)

martbab · 2016-12-09T13:01:26Z

Travis installs the built RPMs and then runs server install and outoftree test suite. It does not use make install.

BTW does this mean that Travis is actually running tests using Py3? Sorry but I am just trying to get up to speed.

pspacek · 2016-12-09T13:03:27Z

make install will install whatever is auto-detected during configure (or overriden by $PYTHON variable while calling make install).

So, how are you caling the stuff? If you want to always run Python 2 tests, specify PYTHON=/usr/bin/python2.

pspacek · 2016-12-09T13:06:08Z

Relevant parts of SPEC file are here:
684f4f5#diff-021f8f69d8c0598dcead3341b439283aR778

MartinBasti · 2016-12-09T13:07:04Z

@martbab Yes for this PR travis run tests under PY3, see tracebacks they are from python3.5 site-packages

pspacek · 2016-12-09T13:42:11Z

To be sure I re-built RPMs from this PR using makerpms.sh script. My findings are:

/usr/bin/ipa-run-tests is a symlink to /usr/bin/ipa-run-tests-2.7
/usr/bin/ipa-run-tests-2.7 has shebang #!/usr/bin/python2

I have no idea why it should fail in Travis.

MartinBasti · 2016-12-09T13:46:41Z

Have you tried on F25? it might be related to it

martbab · 2016-12-09T13:47:19Z

I have re-built RPMs in the F25 image and found out that the /usr/bin/ipa command has the following shebang:

#!/usr/bin/python3

so this one is the culprit. ipa-run-tests has python2 shebang. Keep in mind that before running tests ipa ping is called to a) test the functionality of the installation and b) to prepare ~/.ipa directory for tests. I can remove this call if needed but IMHO it is a bug.

martbab · 2016-12-09T13:53:06Z

Yep, running ipa ping on Fedora 25 confirms this:

# ipa ping
ipa: ERROR: AttributeError: 'str' object has no attribute 'decode'
Traceback (most recent call last):
  File "/usr/lib/python3.5/site-packages/ipaclient/remote_plugins/__init__.py", line 109, in get_package
    plugins = api._remote_plugins
AttributeError: 'API' object has no attribute '_remote_plugins'

During handling of the above exception, another exception occurred:

Traceback (most recent call last):
  File "/usr/lib/python3.5/site-packages/ipalib/cli.py", line 1348, in run
    api.finalize()
  File "/usr/lib/python3.5/site-packages/ipalib/plugable.py", line 714, in finalize
    self.__do_if_not_done('load_plugins')
  File "/usr/lib/python3.5/site-packages/ipalib/plugable.py", line 421, in __do_if_not_done
    getattr(self, name)()
  File "/usr/lib/python3.5/site-packages/ipalib/plugable.py", line 592, in load_plugins
    for package in self.packages:
  File "/usr/lib/python3.5/site-packages/ipalib/__init__.py", line 919, in packages
    ipaclient.remote_plugins.get_package(self),
  File "/usr/lib/python3.5/site-packages/ipaclient/remote_plugins/__init__.py", line 111, in get_package
    server_info = ServerInfo(api)
  File "/usr/lib/python3.5/site-packages/ipaclient/remote_plugins/__init__.py", line 25, in __init__
    hostname = DNSName(api.env.server).ToASCII()
  File "/usr/lib/python3.5/site-packages/ipapython/dnsutil.py", line 74, in ToASCII
    return self.to_text().decode('ascii')  # must be unicode string
AttributeError: 'str' object has no attribute 'decode'
ipa: ERROR: an internal error has occurred

Petr Viktorin recommended me to copy the whole build directory and run configure twice, with different values for PYTHON variable. After thinking a bit about that, it seems as cleanest approach. Building for two versions of Python at the same time should be temporary state so I decided not to complicate Autotools build system with conditional spagetti for two versions of Python. For proper Python2/3 distiction in the two separate builds, I added find/grep/sed combo which replaces shebangs with system-wide Python interpreter as necessary. This is workaround for the fact that FreeIPA does not use setuptools properly. Honza told me that proper use of setuptools is not trivial so we decided to go with this for now. https://fedorahosted.org/freeipa/ticket/157

python3-samba is intentionally ommited because it is not in Fedora repos. Pylint somehow magically ignores this missing package. Keep in mind that server will not work until this this solved. https://fedorahosted.org/freeipa/ticket/157

configure option --with/without-pylint was replaced by --enable/disable-pylint. Pylint is always called as $(PYTHON) -m python. If you need to override Pylint version, use command "make pylint PYTHON=xxx". https://fedorahosted.org/freeipa/ticket/157

pspacek · 2016-12-12T16:09:32Z

I've found the root cause - incorrect order of operations in freeipa.spec.in. Now it should work.

MartinBasti · 2016-12-13T13:36:44Z

Fixed upstream
master:
https://fedorahosted.org/freeipa/changeset/b7d70baee73c64898de91e2fa59b3f9f417c8e01
https://fedorahosted.org/freeipa/changeset/21a0987601dc4fa9de3fe63a18a604337a5edb7b
https://fedorahosted.org/freeipa/changeset/9c87c39e65547004031284080749bc66dab1a8f9

tiran reviewed Nov 24, 2016

View reviewed changes

pspacek force-pushed the build-python3 branch from 7ab796b to bb1ac49 Compare November 24, 2016 17:27

pspacek mentioned this pull request Nov 24, 2016

Build system refactoring phase 3 #213

Closed

3 tasks

pspacek force-pushed the build-python3 branch from bb1ac49 to de0f80a Compare November 30, 2016 12:49

pspacek force-pushed the build-python3 branch from de0f80a to 6d08735 Compare December 8, 2016 10:30

tiran reviewed Dec 8, 2016

View reviewed changes

pspacek force-pushed the build-python3 branch from 6d08735 to 37af778 Compare December 8, 2016 12:12

pspacek added 3 commits December 12, 2016 17:08

Build: specify BuildRequires for Python 3 pylint

8156233

python3-samba is intentionally ommited because it is not in Fedora repos. Pylint somehow magically ignores this missing package. Keep in mind that server will not work until this this solved. https://fedorahosted.org/freeipa/ticket/157

pspacek force-pushed the build-python3 branch from 37af778 to f09181e Compare December 12, 2016 16:08

MartinBasti added ack Pull Request approved, can be merged pushed Pull Request has already been pushed labels Dec 13, 2016

MartinBasti closed this Dec 13, 2016

Build: makerpms.sh generates Python 2 & 3 packages at the same time #272

Build: makerpms.sh generates Python 2 & 3 packages at the same time #272

pspacek commented Nov 24, 2016 •

edited by MartinBasti

tiran commented Nov 24, 2016

tiran Nov 24, 2016

pspacek Nov 24, 2016

pspacek commented Nov 24, 2016

MartinBasti commented Nov 28, 2016

pspacek commented Nov 30, 2016

pspacek commented Nov 30, 2016

tiran commented Nov 30, 2016

MartinBasti commented Dec 7, 2016

tiran commented Dec 7, 2016

MartinBasti commented Dec 7, 2016

MartinBasti commented Dec 7, 2016

MartinBasti commented Dec 7, 2016

tiran commented Dec 7, 2016

tiran commented Dec 7, 2016

MartinBasti commented Dec 7, 2016

pspacek commented Dec 8, 2016

pspacek commented Dec 8, 2016

tiran Dec 8, 2016

tiran Dec 8, 2016

tiran Dec 8, 2016

tiran commented Dec 8, 2016

pspacek commented Dec 8, 2016

MartinBasti commented Dec 9, 2016 •

edited

pspacek commented Dec 9, 2016

MartinBasti commented Dec 9, 2016

martbab commented Dec 9, 2016

pspacek commented Dec 9, 2016

pspacek commented Dec 9, 2016

MartinBasti commented Dec 9, 2016 •

edited

pspacek commented Dec 9, 2016

MartinBasti commented Dec 9, 2016

martbab commented Dec 9, 2016

martbab commented Dec 9, 2016

pspacek commented Dec 12, 2016

MartinBasti commented Dec 13, 2016

Build: makerpms.sh generates Python 2 & 3 packages at the same time #272

Build: makerpms.sh generates Python 2 & 3 packages at the same time #272

Conversation

pspacek commented Nov 24, 2016 • edited by MartinBasti

tiran commented Nov 24, 2016

tiran Nov 24, 2016

Choose a reason for hiding this comment

pspacek Nov 24, 2016

Choose a reason for hiding this comment

pspacek commented Nov 24, 2016

MartinBasti commented Nov 28, 2016

pspacek commented Nov 30, 2016

pspacek commented Nov 30, 2016

tiran commented Nov 30, 2016

MartinBasti commented Dec 7, 2016

tiran commented Dec 7, 2016

MartinBasti commented Dec 7, 2016

MartinBasti commented Dec 7, 2016

MartinBasti commented Dec 7, 2016

tiran commented Dec 7, 2016

tiran commented Dec 7, 2016

MartinBasti commented Dec 7, 2016

pspacek commented Dec 8, 2016

pspacek commented Dec 8, 2016

tiran Dec 8, 2016

Choose a reason for hiding this comment

tiran Dec 8, 2016

Choose a reason for hiding this comment

tiran Dec 8, 2016

Choose a reason for hiding this comment

tiran commented Dec 8, 2016

pspacek commented Dec 8, 2016

MartinBasti commented Dec 9, 2016 • edited

pspacek commented Dec 9, 2016

MartinBasti commented Dec 9, 2016

martbab commented Dec 9, 2016

pspacek commented Dec 9, 2016

pspacek commented Dec 9, 2016

MartinBasti commented Dec 9, 2016 • edited

pspacek commented Dec 9, 2016

MartinBasti commented Dec 9, 2016

martbab commented Dec 9, 2016

martbab commented Dec 9, 2016

pspacek commented Dec 12, 2016

MartinBasti commented Dec 13, 2016

pspacek commented Nov 24, 2016 •

edited by MartinBasti

MartinBasti commented Dec 9, 2016 •

edited

MartinBasti commented Dec 9, 2016 •

edited