Bug 1381935

Summary:	Review Request: python-distro - Linux Distribution - a Linux OS platform information API
Product:	[Fedora] Fedora	Reporter:	Miroslav Suchý <msuchy>
Component:	Package Review	Assignee:	Neal Gompa <ngompa13>
Status:	CLOSED ERRATA	QA Contact:	Fedora Extras Quality Assurance <extras-qa>
Severity:	medium	Docs Contact:
Priority:	medium
Version:	rawhide	CC:	ngompa13, package-review, riehecky
Target Milestone:	---	Keywords:	Reopened
Target Release:	---	Flags:	ngompa13: fedora-review+
Hardware:	All
OS:	Linux
Whiteboard:
Fixed In Version:		Doc Type:	If docs needed, set a value
Doc Text:		Story Points:	---
Clone Of:		Environment:
Last Closed:	2017-01-17 22:51:18 UTC	Type:	---
Regression:	---	Mount Type:	---
Documentation:	---	CRM:
Verified Versions:		Category:	---
oVirt Team:	---	RHEL 7.3 requirements from Atomic Host:
Cloudforms Team:	---	Target Upstream Version:
Embargoed:

Description Miroslav Suchý 2016-10-05 11:47:33 UTC

Spec URL: http://miroslav.suchy.cz/fedora/python-distro/python-distro.spec
SRPM URL: http://miroslav.suchy.cz/fedora/python-distro/python-distro-1.0.0-2.fc24.src.rpm
Description: 
The distro (for: Linux Distribution) package provides information about the
Linux distribution it runs on, such as a reliable machine-readable ID, or
version information.

It is a renewed alternative implementation for Python's original
platform.linux_distribution function, but it also provides much more
functionality. An alternative implementation became necessary because
Python 3.5 deprecated this function, and Python 3.7 is expected to remove it
altogether. Its predecessor function platform.dist was already deprecated since
Python 2.6 and is also expected to be removed in Python 3.7. Still, there are
many cases in which access to that information is needed. See Python issue 1322
for more information.

Fedora Account System Username: msuchy

Comment 1 Neal Gompa 2016-10-05 12:05:03 UTC

Taking this review.

Comment 2 Neal Gompa 2016-10-05 12:23:26 UTC

* Naming is correct.
* License is correct.
* Python 2 and Python 3 subpackages are being correctly generated for Fedora.

However, I see a few issues:
* Starting with Fedora 23, the default implementation of commands should be Python 3 instead of Python 2. Please switch /usr/bin/distro to Python 3.
* You are hard-requiring /bin/lsb_release, which upstream says is not necessary. Please downgrade it to Recommends, as the lsb package has a large web of dependencies that people may not necessarily want.
* If you intend to package this for EPEL, you'll need to do a little bit more work to support Python 3 in EPEL 6/7. For reference, here's a couple of examples[1][2].


[1]: https://apps.fedoraproject.org/packages/python-nose2/sources/spec/
[2]: https://apps.fedoraproject.org/packages/python-pika/sources/spec/

Comment 3 Neal Gompa 2016-10-05 13:42:08 UTC

Oh, also, your description in the python3 subpackage says it's the python 2 version, which it obviously is not.

You may choose to wrap the common description in a macro and reuse it for the description sections, too.

Comment 4 Miroslav Suchý 2016-10-05 16:48:54 UTC

(In reply to Neal Gompa from comment #2)
> * You are hard-requiring /bin/lsb_release, which upstream says is not
> necessary. Please downgrade it to Recommends, as the lsb package has a large
> web of dependencies that people may not necessarily want.

There is a traceback when the binary is not available. So for now, I want to have there hard requires. I will flip it to Recommends once resolved.

> * If you intend to package this for EPEL, you'll need to do a little bit
> more work to support Python 3 in EPEL 6/7. For reference, here's a couple of
> examples[1][2].

I plan to create only python2 subpackage for el6 and el7.

Comment 5 Miroslav Suchý 2016-10-05 17:22:48 UTC

Updated:
Spec URL: http://miroslav.suchy.cz/fedora/python-distro/python-distro.spec
SRPM URL: http://miroslav.suchy.cz/fedora/python-distro/python-distro-1.0.0-5.fc24.src.rpm

I am now generating just python2 subpackage on EL6,7.

/usr/bin/distro now use python3 on fedoras.

I chosen to not use macro for description. This way it more readable and maintainable for me. I hope this is not a problem.

Comment 6 Neal Gompa 2016-10-05 20:33:55 UTC

Comments and issues:

* A simpler way to do the %install phase is to simply re-order it so that %py2_install occurs first, then %py3_install next. That should properly overwrite /usr/bin/distro. After all, the only case where you have /usr/bin/distro point to Python 2 is when the Python 3 subpackage doesn't exist.

* The conditional inside the Python 3 subpackage conditional for the files list is redundant. You're already going to have /usr/bin/distro in the python3 subpackage if it's being built.

* Your rpm changelog is in the wrong format (lacks email address) and has a duplicate entry rather than unique entries.

* It's fine with not using a macro for the description. That's purely a preference. You've fixed the description in each subpackage, which is enough.

Comment 7 Miroslav Suchý 2016-10-06 10:59:33 UTC

> A simpler way to do the %install phase is to simply re-order it ...

Done.

> The conditional inside the Python 3 subpackage conditional for the files list is redundant.

Fixed.

> * Your rpm changelog is in the wrong format (lacks email address)

a) I have misconfigured Tito (which create that entries for me). It have email now.
b) The email address is not mandatory. It is actually optional. Albeit most maintainers use it.

> and has a duplicate entry rather than unique entries.

I actually done so many releases. Only after that I found some problem so I created another release. So it really reflect what I have been doing. I prefer it over keeping the same release number for all intermittent releases.

Updated:
Spec URL: http://miroslav.suchy.cz/fedora/python-distro/python-distro.spec
SRPM URL: http://miroslav.suchy.cz/fedora/python-distro/python-distro-1.0.0-6.fc24.src.rpm

Comment 8 Neal Gompa 2016-10-08 02:36:43 UTC

It looks good to me. Please follow up with upstream on the requirement for /usr/bin/lsb_release. It should not be mandatory on systems with os-release(5).

But otherwise...

APPROVED.

Comment 9 Miroslav Suchý 2016-10-10 07:03:51 UTC

Thank you.

Comment 10 Patrick Uiterwijk 2016-10-11 12:24:37 UTC

Package request has been approved: https://admin.fedoraproject.org/pkgdb/package/rpms/python-distro

Comment 11 Fedora Update System 2016-10-12 00:24:54 UTC

python-distro-1.0.0-6.fc23 has been pushed to the Fedora 23 testing repository. If problems still persist, please make note of it in this bug report.
See https://fedoraproject.org/wiki/QA:Updates_Testing for
instructions on how to install test updates.
You can provide feedback for this update here: https://bodhi.fedoraproject.org/updates/FEDORA-2016-7f7fa72af4

Comment 12 Fedora Update System 2016-10-12 00:55:45 UTC

python-distro-1.0.0-6.fc24 has been pushed to the Fedora 24 testing repository. If problems still persist, please make note of it in this bug report.
See https://fedoraproject.org/wiki/QA:Updates_Testing for
instructions on how to install test updates.
You can provide feedback for this update here: https://bodhi.fedoraproject.org/updates/FEDORA-2016-0295334433

Comment 13 Fedora Update System 2016-10-12 01:53:12 UTC

python-distro-1.0.0-6.fc25 has been pushed to the Fedora 25 testing repository. If problems still persist, please make note of it in this bug report.
See https://fedoraproject.org/wiki/QA:Updates_Testing for
instructions on how to install test updates.
You can provide feedback for this update here: https://bodhi.fedoraproject.org/updates/FEDORA-2016-879d7c7f3e

Comment 14 Fedora Update System 2016-10-12 03:17:04 UTC

python-distro-1.0.0-6.el7 has been pushed to the Fedora EPEL 7 testing repository. If problems still persist, please make note of it in this bug report.
See https://fedoraproject.org/wiki/QA:Updates_Testing for
instructions on how to install test updates.
You can provide feedback for this update here: https://bodhi.fedoraproject.org/updates/FEDORA-EPEL-2016-8579a792ba

Comment 15 Fedora Update System 2016-10-12 03:17:29 UTC

python-distro-1.0.0-6.el6 has been pushed to the Fedora EPEL 6 testing repository. If problems still persist, please make note of it in this bug report.
See https://fedoraproject.org/wiki/QA:Updates_Testing for
instructions on how to install test updates.
You can provide feedback for this update here: https://bodhi.fedoraproject.org/updates/FEDORA-EPEL-2016-42f9ffc3e9

Comment 16 Fedora Update System 2016-10-18 11:31:20 UTC

python-distro-1.0.0-6.fc25 has been pushed to the Fedora 25 stable repository. If problems still persist, please make note of it in this bug report.

Comment 17 Fedora Update System 2016-10-19 14:20:22 UTC

python-distro-1.0.0-6.fc24 has been pushed to the Fedora 24 stable repository. If problems still persist, please make note of it in this bug report.

Comment 18 Fedora Update System 2016-10-19 17:21:05 UTC

python-distro-1.0.0-6.fc23 has been pushed to the Fedora 23 stable repository. If problems still persist, please make note of it in this bug report.

Comment 19 Fedora Update System 2016-10-26 23:47:51 UTC

python-distro-1.0.0-6.el6 has been pushed to the Fedora EPEL 6 stable repository. If problems still persist, please make note of it in this bug report.

Comment 20 Fedora Update System 2017-01-03 14:59:17 UTC

python-distro-1.0.1-2.el7 has been submitted as an update to Fedora EPEL 7. https://bodhi.fedoraproject.org/updates/FEDORA-EPEL-2017-2d4b482b6f

Comment 21 Fedora Update System 2017-01-03 21:48:38 UTC

python-distro-1.0.1-2.el7 has been pushed to the Fedora EPEL 7 testing repository. If problems still persist, please make note of it in this bug report.
See https://fedoraproject.org/wiki/QA:Updates_Testing for
instructions on how to install test updates.
You can provide feedback for this update here: https://bodhi.fedoraproject.org/updates/FEDORA-EPEL-2017-2d4b482b6f

Comment 22 Fedora Update System 2017-01-17 22:51:18 UTC

python-distro-1.0.1-2.el7 has been pushed to the Fedora EPEL 7 stable repository. If problems still persist, please make note of it in this bug report.