Note: This is a beta release of Red Hat Bugzilla 5.0. The data contained within is a snapshot of the live data so any changes you make will not be reflected in the production Bugzilla. Also email is disabled so feel free to test any aspect of the site that you want. File any problems you find or give feedback here.
Bug 1366878 - Review Request: python-attrs - Python attributes without boilerplate
Summary: Review Request: python-attrs - Python attributes without boilerplate
Keywords:
Status: CLOSED ERRATA
Alias: None
Product: Fedora
Classification: Fedora
Component: Package Review
Version: rawhide
Hardware: All
OS: Linux
medium
medium
Target Milestone: ---
Assignee: Igor Gnatenko
QA Contact: Fedora Extras Quality Assurance
URL:
Whiteboard:
Depends On:
Blocks:
TreeView+ depends on / blocked
 
Reported: 2016-08-14 00:42 UTC by Eric Smith
Modified: 2016-09-11 18:18 UTC (History)
2 users (show)

Fixed In Version:
Doc Type: If docs needed, set a value
Doc Text:
Clone Of:
Environment:
Last Closed: 2016-09-10 20:55:47 UTC
ignatenko: fedora-review+


Attachments (Terms of Use)

Description Eric Smith 2016-08-14 00:42:20 UTC
Spec URL: https://fedorapeople.org/~brouhaha/python-attrs/python-attrs.spec
SRPM URL: https://fedorapeople.org/~brouhaha/python-attrs/python-attrs-16.0.0-1.fc24.src.rpm
Description: 
attrs is an MIT-licensed Python package with class decorators that
ease the chores of implementing the most common attribute-related
object protocols.
Fedora Account System Username: brouhaha

Comment 1 Igor Gnatenko 2016-08-14 07:20:12 UTC
* Add %global modname attrs in the beginning

> %if 0%{?rhel} && 0%{?rhel} <= 7
> %{!?__python2: %global __python2 /usr/bin/python2}
> %{!?python2_sitelib: %global python2_sitelib %(%{__python2} -c "from distutils.sysconfig import get_python_lib; print(get_python_lib())")}
> %{!?python2_sitearch: %global python2_sitearch %(%{__python2} -c "from distutils.sysconfig import get_python_lib; print(get_python_lib(1))")}
> %endif
> 
> %if 0%{?fedora} > 12 || 0%{?rhel} > 7
> %global with_python3 1
> %endif
Drop all this useless stuff

> Group:          Development/Libraries
drop it

> Source0:        https://github.com/hynek/attrs/archive/16.0.0.tar.gz
Source0:        https://github.com/hynek/attrs/archive/%{version}/%{modname}-%{version}.tar.gz

> BuildRequires:  python2-devel python-setuptools python2-hypothesis
replace python-setuptools with python2-setuptools and put under subpackage

> %package -n python3-attrs
replace attrs with %{modname}

> %if 0%{?with_python3}
> rm -rf %{py3dir}
> cp -a . %{py3dir}
> %endif # with_python3
I guess it's not needed in your case as you don't modify python3 sources

> %{__python3} setup.py build
%py3_build

> %{__python2} setup.py build
%py2_build

> %{__python3} setup.py install --skip-build --root %{buildroot}
%py3_install

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

* Also change for python2 being first

> %{python2_sitelib}/*
is too generic.

Comment 2 Eric Smith 2016-08-15 00:57:22 UTC
Spec URL: https://fedorapeople.org/~brouhaha/python-attrs/python-attrs.spec
SRPM URL: https://fedorapeople.org/~brouhaha/python-attrs/python-attrs-16.0.0-2.fc24.src.rpm

Thanks for the review! I made most of the requested changes, except:

"Drop all this useless stuff": the useless stuff is needed to build for EPEL7. In fact, I discovered that I actually had to add a little bit more of the useless stuff to get it to build for EPEL7 in Koji.  Unfortunately I can't yet get it to build for python-3.5 in EPEL7, but it does build for python-2.x.

"is too generic" - is actually *exactly* per the example in the Fedora Packaging Guidelines for Python.  I had specific files listed in another package review some time back, and the reviewer asked me to change to the more generic approach to match the guidelines.  If I did make this one more generic, it would have to have both 
   %{python2_sitelib}/attr
because it doesn't have the trailing "s" of %{modname}, and
   %{python2_sitelib}/%{modname}-%{version}-py%{some_macro_to_get_specific_python_version_like_2.7}.egg-info

I think the "too generic" version is simpler, more obvious, doesn't cause problems, and is compliant with the packaging guidelines.

Let me know if you still want it changed.

Comment 3 Igor Gnatenko 2016-08-15 06:27:33 UTC
> # python2 macros needed for EPEL7
> %{!?__python2: %global __python2 /usr/bin/python2} 
> %{!?python2_sitelib: %global python2_sitelib %(%{__python2} -c "from distutils.sysconfig import get_python_lib; print(get_python_lib())")} 
> %{!?py2_build: %global py2_build %{expand: CFLAGS="%{optflags}" %{__python2} setup.py %{?py_setup_args} build --executable="%{__python2} -s"}}
> %{!?py2_install: %global py2_install %{expand: CFLAGS="%{optflags}" %{__python2} setup.py %{?py_setup_args} install -O1 --skip-build --root %{buildroot}}} 
all those macros are in EPEL7.

> %if 0%{?fedora} > 12 || 0%{?rhel} > 7
> %global with_python3 1
> %endif
%if 0%{?rhel} && 0%{?rhel} <= 7
%bcond_with python3
%else
%bcond_without python3
%endif

And replace 0%{?with_python3} with %{with python3}

> %global sum Python attributes without boilerplate
Define it in Summary tag and then use as %{summary}

> I think the "too generic" version is simpler, more obvious, doesn't cause problems, and is compliant with the packaging guidelines.
for example, if upstream will start (e.g. by accident) some another module you will not notice which will cause problems. But many packages do this, so it's only my preference.

Comment 4 Eric Smith 2016-08-16 02:52:28 UTC
Spec URL: https://fedorapeople.org/~brouhaha/python-attrs/python-attrs.spec
SRPM URL: https://fedorapeople.org/~brouhaha/python-attrs/python-attrs-16.0.0-3.fc24.src.rpm

Made the requested changes, except files section which I'm still considering. Didn't know about some of these RPM variables etc., so it's great to learn about them.

Builds in Koji for F24.

> all those macros are in EPEL7.

I'm not entirely convinced. After having removed the macros, this version of the spec fails to build in Koji for EPEL7, and it looks like it's due to missing or wrong macros for Python2:
    http://koji.fedoraproject.org/koji/taskinfo?taskID=15269733

Comment 5 Igor Gnatenko 2016-08-16 05:46:48 UTC
(In reply to Eric Smith from comment #4)
> Spec URL: https://fedorapeople.org/~brouhaha/python-attrs/python-attrs.spec
> SRPM URL:
> https://fedorapeople.org/~brouhaha/python-attrs/python-attrs-16.0.0-3.fc24.
> src.rpm
> 
> Made the requested changes, except files section which I'm still
> considering. Didn't know about some of these RPM variables etc., so it's
> great to learn about them.
> 
> Builds in Koji for F24.
> 
> > all those macros are in EPEL7.
> 
> I'm not entirely convinced. After having removed the macros, this version of
> the spec fails to build in Koji for EPEL7, and it looks like it's due to
> missing or wrong macros for Python2:
>     http://koji.fedoraproject.org/koji/taskinfo?taskID=15269733E
rror: No Package found for python2-setuptools

It's not problem with macros.

Comment 6 Eric Smith 2016-08-16 06:30:12 UTC
Spec URL: https://fedorapeople.org/~brouhaha/python-attrs/python-attrs.spec
SRPM URL: https://fedorapeople.org/~brouhaha/python-attrs/python-attrs-16.0.0-4.fc24.src.rpm

> It's not problem with macros.

You're right, that wasn't the problem I had encountered previously, and I apologize for wasting your time on it. That broke with a more recent change I made to the conditionals for building for EPEL7, and I didn't notice that the failure cause had changed..  I've now fixed that, and I see that there are still a few lines in the build log that had previously misled me about the macros:

sh: /usr/bin/python2: No such file or directory

However, the build completes despite that, while before it was failing, apparently due to an unrelated problem that is now fixed.

Comment 7 Igor Gnatenko 2016-08-17 13:54:19 UTC
don't know why you are planning to build it for EL7, but nevermind.

Though you don't run full test-suite.

I think to run full test suite you need:
* pythonX-pytest
* pythonX-zope-interface
* pythonX-Pympler

and run tests using:
py.test-X -v

in %check section.

Please fix this during import.

Comment 8 Eric Smith 2016-08-18 17:17:17 UTC
Spec URL: https://fedorapeople.org/~brouhaha/python-attrs/python-attrs.spec
SRPM URL: https://fedorapeople.org/~brouhaha/python-attrs/python-attrs-16.0.0-5.fc24.src.rpm

Fixed the check section to run the full test suite per your advice.  Two tests failed, reported upstream. Upstream reports that it's a known problem, fixed in github and will be in next release. In mean time, I've patched to skip those two tests. Details in comments in the spec file.

Planning to build for EL7 because I use attrs in Python code that I intend to run on EL7.

Comment 9 Gwyn Ciesla 2016-08-18 18:34:10 UTC
Package request has been approved: https://admin.fedoraproject.org/pkgdb/package/rpms/python-attrs

Comment 10 Fedora Update System 2016-08-19 05:09:50 UTC
python-attrs-16.0.0-5.fc25 has been submitted as an update to Fedora 25. https://bodhi.fedoraproject.org/updates/FEDORA-2016-67830f0533

Comment 11 Fedora Update System 2016-08-19 05:16:59 UTC
python-attrs-16.0.0-5.fc24 has been submitted as an update to Fedora 24. https://bodhi.fedoraproject.org/updates/FEDORA-2016-94a198c866

Comment 12 Fedora Update System 2016-08-19 06:18:18 UTC
python-attrs-16.0.0-6.el7 has been submitted as an update to Fedora EPEL 7. https://bodhi.fedoraproject.org/updates/FEDORA-EPEL-2016-f2d9bbaef6

Comment 13 Fedora Update System 2016-08-19 16:50:35 UTC
python-attrs-16.0.0-5.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-67830f0533

Comment 14 Fedora Update System 2016-08-19 23:21:40 UTC
python-attrs-16.0.0-5.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-94a198c866

Comment 15 Fedora Update System 2016-08-20 02:47:31 UTC
python-attrs-16.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-f2d9bbaef6

Comment 16 Fedora Update System 2016-09-10 20:55:44 UTC
python-attrs-16.0.0-5.fc24 has been pushed to the Fedora 24 stable repository. If problems still persist, please make note of it in this bug report.

Comment 17 Fedora Update System 2016-09-10 21:00:24 UTC
python-attrs-16.0.0-5.fc25 has been pushed to the Fedora 25 stable repository. If problems still persist, please make note of it in this bug report.

Comment 18 Fedora Update System 2016-09-11 18:18:49 UTC
python-attrs-16.0.0-6.el7 has been pushed to the Fedora EPEL 7 stable repository. If problems still persist, please make note of it in this bug report.


Note You need to log in before you can comment on or make changes to this bug.