#58 Proper Python 3 Packaging
Merged 5 years ago by frantisekz. Opened 5 years ago by frantisekz.
frantisekz/testcloud dev  into  dev

file modified
+2
@@ -70,6 +70,8 @@ 

  mockbuild: mocksrpm

  	mock -r $(BUILDTARGET) --no-clean --rebuild $(NVR).$(TARGETDIST).src.rpm

  	cp /var/lib/mock/$(BUILDTARGET)/result/$(NVR).$(TARGETDIST).noarch.rpm .

+ 	cp /var/lib/mock/$(BUILDTARGET)/result/python2-$(NVR).$(TARGETDIST).noarch.rpm .

+ 	cp /var/lib/mock/$(BUILDTARGET)/result/python3-$(NVR).$(TARGETDIST).noarch.rpm .

  

  .PHONY: nvr

  nvr:

file modified
+1 -1
@@ -1,4 +1,4 @@ 

- #! /usr/bin/python

+ #! /usr/bin/python3

  from testcloud import cli

  

  cli.main()

file modified
-1
@@ -1,4 +1,3 @@ 

- #!/usr/bin/env python

  # -*- coding: utf-8 -*-

  # Copyright 2015, Red Hat, Inc.

  # License: GPL-2.0+ <http://spdx.org/licenses/GPL-2.0+>

file modified
-1
@@ -1,4 +1,3 @@ 

- #!/usr/bin/env python

  # -*- coding: utf-8 -*-

  # Copyright 2015, Red Hat, Inc.

  # License: GPL-2.0+ <http://spdx.org/licenses/GPL-2.0+>

file modified
-1
@@ -1,4 +1,3 @@ 

- #!/usr/bin/env python

  # -*- coding: utf-8 -*-

  # Copyright 2015, Red Hat, Inc.

  # License: GPL-2.0+ <http://spdx.org/licenses/GPL-2.0+>

file modified
-1
@@ -1,4 +1,3 @@ 

- #!/usr/bin/env python

  # -*- coding: utf-8 -*-

  # Copyright 2015, Red Hat, Inc.

  # License: GPL-2.0+ <http://spdx.org/licenses/GPL-2.0+>

file modified
+65 -33
@@ -1,10 +1,14 @@ 

+ # Avoid warnings when bytecompiling settings.py in /etc

+ %global __python %{__python3}

+ 

  # sitelib for noarch packages, sitearch for others (remove the unneeded one)

  %{!?python_sitelib: %global python_sitelib %(%{__python} -c "from distutils.sysconfig import get_python_lib; print(get_python_lib())")}

  

+ 

  Name:           testcloud

  # Update also version in testcloud/__init__.py when changing this!

  Version:        0.1.18

- Release:        1%{?dist}

+ Release:        2%{?dist}

  Summary:        Tool for running cloud images locally

  

  License:        GPLv2+
@@ -16,29 +20,27 @@ 

  # Ensure we can create the testcloud group

  Requires(pre):  shadow-utils

  

- # testcloud integrates with libvirt

  Requires:       libvirt

- %if 0%{?fedora} <= 26

- Requires:       libvirt-python

- %else

- Requires:       python2-libvirt

- %endif

- 

- # This is used to manipulate images on disk

+ Requires:       polkit

  Requires:       libguestfs

  Requires:       libguestfs-tools

  

- # Used to download images from valid URLs

- Requires:       python2-requests

+ Requires:       python3-%{name} = %{version}-%{release}

  

- Requires:       polkit

- Requires:       python2-jinja2

+ # Install python2 interface on stable Fedora Releases

+ %if 0%{?fedora} <= 28

+ Requires:       python2-%{name} = %{version}-%{release}

+ %endif

  

- %if 0%{?fedora} <= 26

- BuildRequires:  libvirt-python

- %else

+ %description

+ testcloud is a relatively simple system which is capable of booting images

+ designed for cloud systems on a local system with minimal configuration.

+ testcloud is designed to be (and remain) somewhat simple, trading fancy cloud

+ system features for ease of use and sanity in development.

+ 

+ %package -n python2-%{name}

+ Summary:        Python 2 interface to testcloud

  BuildRequires:  python2-libvirt

- %endif

  BuildRequires:  python2-devel

  BuildRequires:  python2-jinja2

  BuildRequires:  python2-mock
@@ -46,14 +48,29 @@ 

  BuildRequires:  python2-pytest-cov

  BuildRequires:  python2-requests

  BuildRequires:  python2-setuptools

+ Requires:       python2-requests

+ Requires:       python2-libvirt

+ Requires:       python2-jinja2

  

- # Provides:       python2-testcloud

- 

- %description

- testcloud is a relatively simple system which is capable of booting images

- designed for cloud systems on a local system with minimal configuration.

- testcloud is designed to be (and remain) somewhat simple, trading fancy cloud

- system features for ease of use and sanity in development.

+ %description -n python2-%{name}

+ Python 2 interface to testcloud.

+ 

+ %package -n python3-%{name}

+ Summary:        Python 3 interface to testcloud

+ BuildRequires:  python3-libvirt

+ BuildRequires:  python3-devel

+ BuildRequires:  python3-jinja2

+ BuildRequires:  python3-mock

+ BuildRequires:  python3-pytest

+ BuildRequires:  python3-pytest-cov

+ BuildRequires:  python3-requests

+ BuildRequires:  python3-setuptools

+ Requires:       python3-requests

+ Requires:       python3-libvirt

+ Requires:       python3-jinja2

+ 

+ %description -n python3-%{name}

+ Python 3 interface to testcloud.

  

  # Create the testcloud group

  %pre
@@ -62,16 +79,13 @@ 

  %prep

  %setup -q -n %{name}-%{version}

  

- %check

- %{__python2} setup.py test

- # Remove compiled .py files from /etc after running unittests

- rm -f %{buildroot}%{_sysconfdir}/testcloud/*.py{c,o}

- 

  %build

- %{__python2} setup.py build

+ %py2_build

+ %py3_build

  

  %install

- %{__python2} setup.py install -O1 --skip-build --root %{buildroot}

+ %py2_install

+ %py3_install

  

  # configuration files

  mkdir -p %{buildroot}%{_sysconfdir}/testcloud/
@@ -93,11 +107,16 @@ 

  mkdir -p %{buildroot}%{_sysconfdir}/polkit-1/rules.d

  install conf/99-testcloud-nonroot-libvirt-access.rules %{buildroot}%{_sysconfdir}/polkit-1/rules.d/99-testcloud-nonroot-libvirt-access.rules

  

+ %check

+ %{__python2} setup.py test

+ %{__python3} setup.py test

+ # Remove compiled .py files from /etc after os_install_post

+ rm -f %{buildroot}%{_sysconfdir}/testcloud/*.py{c,o}

+ rm -rf %{buildroot}%{_sysconfdir}/testcloud/__pycache__

+ 

  %files

  %doc README.rst

  %license LICENSE

- %{python2_sitelib}/testcloud

- %{python2_sitelib}/*.egg-info

  

  %dir %{_sysconfdir}/testcloud

  %dir %attr(0775, qemu, testcloud) %{_sharedstatedir}/testcloud
@@ -110,7 +129,20 @@ 

  %config(noreplace) %{_sysconfdir}/testcloud/settings.py

  %{_bindir}/testcloud

  

+ %files -n python2-%{name}

+ %{python2_sitelib}/testcloud

+ %{python2_sitelib}/*.egg-info

+ 

+ %files -n python3-%{name}

+ %{python3_sitelib}/testcloud

+ %{python3_sitelib}/*.egg-info

+ 

  %changelog

+ * Fri May 25 2018 Frantisek Zatloukal <fzatlouk@redhat.com> - 0.1.18-2

+ - Drop Fedora 26

+ - Use Python 3 by default

+ - Split testcloud into testcloud, python2-testcloud and python3-testcloud

+ 

  * Wed May 02 2018 Frantisek Zatloukal <fzatlouk@redhat.com> - 0.1.18-1

  - Host /dev/random passthrough

  

file modified
-1
@@ -1,4 +1,3 @@ 

- #!/usr/bin/env python

  # -*- coding: utf-8 -*-

  # Copyright 2015, Red Hat, Inc.

  # License: GPL-2.0+ <http://spdx.org/licenses/GPL-2.0+>

file modified
+5
@@ -1,3 +1,8 @@ 

+ # -*- coding: utf-8 -*-

+ # Copyright 2015, Red Hat, Inc.

+ # License: GPL-2.0+ <http://spdx.org/licenses/GPL-2.0+>

+ # See the LICENSE file for more details on Licensing

+ 

  import os

  import imp

  

file modified
-1
@@ -1,4 +1,3 @@ 

- #!/usr/bin/env python

  # -*- coding: utf-8 -*-

  # Copyright 2015, Red Hat, Inc.

  # License: GPL-2.0+ <http://spdx.org/licenses/GPL-2.0+>

file modified
-1
@@ -1,4 +1,3 @@ 

- #!/usr/bin/env python

  # -*- coding: utf-8 -*-

  # Copyright 2015, Red Hat, Inc.

  # License: GPL-2.0+ <http://spdx.org/licenses/GPL-2.0+>

file modified
-1
@@ -1,4 +1,3 @@ 

- #!/usr/bin/env python

  # -*- coding: utf-8 -*-

  # Copyright 2015, Red Hat, Inc.

  # License: GPL-2.0+ <http://spdx.org/licenses/GPL-2.0+>

file modified
-1
@@ -1,4 +1,3 @@ 

- #!/usr/bin/env python

  # -*- coding: utf-8 -*-

  # Copyright 2015, Red Hat, Inc.

  # License: GPL-2.0+ <http://spdx.org/licenses/GPL-2.0+>

Splits testcloud into:
- testcloud # Generic testcloud files and /usr/bin/testcloud
- python2-testcloud # python 2 interface to testcloud, not installed if user didn't ask for it
- python3-testcloud # python 3 interface to testcloud

Python 3 version with just Python 3 interface is installed by default (as per Packaging Guidelines:Python[0]). Everything looks just fine, but it's possible I had overlooked something somewhere.

I'd love to also fix (=remove) /usr/bin/env python usage as per Packaging Guidelines:Shebang_lines[1], probably later in separate PR.

[0] "If a piece of software supports python3, it MUST be packaged for python3" ; https://fedoraproject.org/wiki/Packaging:Python
[1] "env, /bin/env and /usr/bin/env MUST NOT be used"; https://fedoraproject.org/wiki/Packaging:Guidelines#Shebang_lines

This is actually a Release change, not a Version change. You change how the software is packaged, but not the software itself. Or do you intend to create a new testcloud release, containing just this packaging change?

You should mention that testcloud now uses Python3 by default, that's quite an important change.

I'm no packaging expert, but this patch seems OK to me. Have you tested it? Please create a Koji scratch build for F28, and I can test the interactions with libtaskotron.

Which releases do you intend to send this to? F27+? Or just F29+? The switch to using Python3 by default can be potentially disruptive. And also, it will break for people who rely on importing testcloud python2 libs. Maybe you need to add python2-testcloud as a default dependency for stable Fedora releases, so that compatibility is not broken? (So that on F27-F28, you require both python2-testcloud and python3-testcloud, and on F29+, you require just python3-testcloud).

If we can verify that nothing broken with the switch to python3, I think this could be sent to stable releases, provided that py2 libraries are also pulled with an update.

Python 3 version with just Python 3 interface is installed by default (as per Packaging Guidelines:Python[0]).

The guideline doesn't say you need to use py3 by default, just that it must be packaged. But given the nearing py2 EOL, I agree that the switch is reasonable.

Please create a Koji scratch build for F28, and I can test the interactions with libtaskotron.

Yeah, libtaskotron should explicitly depend on either python3-testcloud or python2-testcloud.

The switch to using Python3 by default can be potentially disruptive. And also, it will break for people who rely on importing testcloud python2 libs

Fair point.

So that on F27-F28, you require both python2-testcloud and python3-testcloud, and on F29+, you require just python3-testcloud

Yep, I think we can go with this solution for stable releases.

rebased onto 928a3cf4bc1ffbf0081005da6639f94bd22af593

5 years ago

BTW tabs vs spaces here

the python2 python module requires the tool itself that in fact can be run on python3? this is a weird mutliway dependency. why is it needed?

This at the same time makes the python3 module depend on the python2 module on Fedora <= 28

what shebangs are here? if installed by setuptools, the order matters and this will have python3 shebang even on Fedora <= 28.

/etc/testcloud/settings.py is bytecompiled with /usr/bin/python. I suggest it should be not. Also what Python version is this config file supposed to be on the system?

rebased onto f0053e6d14594ce28fa802b5ff2abf000cadd0a3

5 years ago

@churchyard Thanks for comments, I've fixed some of the issue you' ve pointed out.

The idea was, to have the following:
On stable releases, have testcloud command using Python 3, but also to make 'dnf install testcloud' result in having python 2 interface installed, so nobody will have to change dependencies in their packages. So this is the multiway dependency.

The other thing we can do is to have python2 by default on Fedora <=28. This can be only defined by the order of py2/3 build/install commands if I am not mistaken. But we'' ll either have to have separate specs in F29/28 or do it with if/else which seems like overcomplicating the spec imo.

the multiway dependency will still exist later. It there because the "main" package is required by both subpackages and requires one of them at the same time.

the multiway dependency will still exist later. It there because the "main" package is required by both subpackages and requires one of them at the same time.

Yeah, that doesn't make sense. Libraries should not require /usr/bin/testcloud. We'll drop the requirement.

what shebangs are here? if installed by setuptools, the order matters and this will have python3 shebang even on Fedora <= 28.

That's fine I think, we want to default to Python 3 everywhere (provided it works fine).

/etc/testcloud/settings.py is bytecompiled with /usr/bin/python. I suggest it should be not. Also what Python version is this config file supposed to be on the system?

How can we avoid bytecompiling python files in /etc? Or are you saying we should bytecompile it, but with a specific python version? How can we achieve that? The config file needs to work with both Py2 and Py3.

rebased onto b17981dd4cc19dc2473b06a05c66cbfa9218eea9

5 years ago

the multiway dependency will still exist later. It there because the "main" package is required by both subpackages and requires one of them at the same time.

Should be fixed now.

Oh so this requirement is here for the tool to work...

While this one is for backwards compatibility only. I suggets to add a comment that explains this. It's easy to mix this with "if fedora 28, require py2, else require py3" (a common snippet).

How can we avoid bytecompiling python files in /etc? Or are you saying we should bytecompile it, but with a specific python version? How can we achieve that? The config file needs to work with both Py2 and Py3.

You can set %__python to an explicit python version:

%global __python %{__python3}

To have this bytecompiled with that version.

However as a config file, I think you don't actually want to bytecompile it at all.

Does your tool eval or import the file?

If it imports the file, you better %ghost the bytecompiled files for both pythons, otherwise they will be somehow unowned if the tool is ever executed by root and the files are created.

If it evals the file, then it is never bytecompiled and you should stop the bytecompilation and you don't need to worry about the files being created by the user.

Unfortunately, currently you can only disable bytecompilation as a whole, that means you need to follow https://fedoraproject.org/wiki/Packaging:Python_Appendix#Manual_byte_compilation and bytecompile stuff in pythoX_sitelib manually. We realize it's tedious and we are changing it: https://fedoraproject.org/wiki/Changes/No_more_automagic_Python_bytecompilation

rebased onto 56b0163a0388c327125c573f1c27acac31f958fc

5 years ago

Does your tool eval or import the file?

https://pagure.io/testcloud/blob/421649847a6793c32b59707f600859d6a22ad4fd/f/testcloud/config.py#_74

Too complex for my little brain :-) I'd love to replace it with yaml config in the future, but that would mean losing all existing user settings.

Unfortunately, currently you can only disable bytecompilation as a whole, that means you need to follow https://fedoraproject.org/wiki/Packaging:Python_Appendix#Manual_byte_compilation and bytecompile stuff in pythoX_sitelib manually. We realize it's tedious and we are changing it: https://fedoraproject.org/wiki/Changes/No_more_automagic_Python_bytecompilation

Actually it seems that this section already resolves the issue:

%check
%{__python2} setup.py test
%{__python3} setup.py test
# Remove compiled .py files from /etc after running unittests
rm -f %{buildroot}%{_sysconfdir}/testcloud/*.py{c,o}

The .pyc file is not present in existing rpms.

rebased onto 6f67a18f126414eebb0f14957733d84937bc0d2f

5 years ago

https://pagure.io/testcloud/blob/421649847a6793c32b59707f600859d6a22ad4fd/f/testcloud/config.py#_74

no import, yay!

# Remove compiled .py files from /etc after running unittests
rm -f %{buildroot}%{_sysconfdir}/testcloud/*.py{c,o}

Fix the comment, it's not after unittests but after os_install_post. Unittest should not bytecompile this given the code I've seen on the link above. (Note that this might blow into your face, I was told that checks can be generally disabled during build (probably won't happen in Fedora).)

I don't like the solution, it will still yell at you in the logs and ion taskotron for using /usr/bin/python, but I guess we can live with that and use %global _python_bytecompile_extra 0 once https://fedoraproject.org/wiki/Changes/No_more_automagic_Python_bytecompilation is done.

+ /usr/lib/rpm/brp-python-bytecompile /usr/bin/python 1
Bytecompiling .py files below /builddir/build/BUILDROOT/testcloud-0.1.18-2.fc29.noarch/usr/lib/python2.7 using /usr/bin/python2.7
Bytecompiling .py files below /builddir/build/BUILDROOT/testcloud-0.1.18-2.fc29.noarch/usr/lib/python3.6 using /usr/bin/python3.6
/builddir/build/BUILDROOT/testcloud-0.1.18-2.fc29.noarch/etc/testcloud/settings.py
DEPRECATION WARNING: python2 invoked with /usr/bin/python.
    Use /usr/bin/python3 or /usr/bin/python2
    /usr/bin/python will be removed or switched to Python 3 in the future.
    If you cannot make the switch now, please follow instructions at https://fedoraproject.org/wiki/Changes/Avoid_usr_bin_python_in_RPM_Build#Quick_Opt-Out
DEPRECATION WARNING: python2 invoked with /usr/bin/python.
    Use /usr/bin/python3 or /usr/bin/python2
    /usr/bin/python will be removed or switched to Python 3 in the future.
    If you cannot make the switch now, please follow instructions at https://fedoraproject.org/wiki/Changes/Avoid_usr_bin_python_in_RPM_Build#Quick_Opt-Out

I don't like the solution, it will still yell at you in the logs and ion taskotron for using /usr/bin/python

I don't understand where /usr/bin/python is coming from? We don't have a shebang in settings.py.

but I guess we can live with that and use %global _python_bytecompile_extra 0 once https://fedoraproject.org/wiki/Changes/No_more_automagic_Python_bytecompilation is done.

Sounds OK.

I don't understand where /usr/bin/python is coming from? We don't have a shebang in settings.py.

From %{__python}. See point 2. in https://fedoraproject.org/wiki/Packaging:Python_Appendix#Manual_byte_compilation

rebased onto b8d31e2

5 years ago

OK, another change pushed. Does it look OK?

those will now be compile dwith py3, correct? so I guess you need to rm %{buildroot}%{_sysconfdir}/testcloud/__pycache__

A scratch build will show if I'm correct.

Correct, I had to add:

%check
%{__python2} setup.py test
%{__python3} setup.py test
# Remove compiled .py files from /etc after os_install_post
rm -f %{buildroot}%{_sysconfdir}/testcloud/*.py{c,o}
rm -rf %{buildroot}%{_sysconfdir}/testcloud/__pycache__

@frantisekz , please do the change and update this pull request. Then link here a Rawhide and F28 scratch build for testing, thanks.

1 new commit added

  • Exclude pycache
5 years ago

1 new commit added

  • Remove shebangs
5 years ago

Commit a5c6636 fixes this pull-request

Pull-Request has been merged by frantisekz

5 years ago