Bug 226342 - Merge Review: python
Merge Review: python
Status: ASSIGNED
Product: Fedora
Classification: Fedora
Component: python (Show other bugs)
23
All Linux
medium Severity medium
: ---
: ---
Assigned To: Nobody's working on this, feel free to take it
Fedora Package Reviews List
:
Depends On:
Blocks: F9MergeReviewTarget 526126
  Show dependency treegraph
 
Reported: 2007-01-31 15:46 EST by Nobody's working on this, feel free to take it
Modified: 2016-04-18 06:18 EDT (History)
17 users (show)

See Also:
Fixed In Version:
Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of:
Environment:
Last Closed:
Type: ---
Regression: ---
Mount Type: ---
Documentation: ---
CRM:
Verified Versions:
Category: ---
oVirt Team: ---
RHEL 7.3 requirements from Atomic Host:
kevin: fedora‑cvs+


Attachments (Terms of Use)

  None (edit)
Description Nobody's working on this, feel free to take it 2007-01-31 15:46:14 EST
Fedora Merge Review: python

http://cvs.fedora.redhat.com/viewcvs/devel/python/
Initial Owner: katzj@redhat.com
Comment 1 Till Maas 2007-11-09 10:03:11 EST
The version in the changelog entries does not match the real version of the
package, e.g. for python-2.5-14.fc7 the latest changelog entry is "2.5.3-14"
which is very confusing.
Comment 2 Jon Ciesla 2008-09-18 15:01:19 EDT
Adding current owner.
Comment 3 James Antill 2008-10-03 10:12:29 EDT
Package Change Request
======================
Package Name: python
New Branches:  F-10
Owners: james katzj
Comment 4 Jon Ciesla 2008-10-03 10:18:51 EDT
??? Shouldn't ownership be changed in pkgdb?

https://admin.fedoraproject.org/pkgdb/packages/name/python
Comment 5 James Antill 2008-10-03 12:16:54 EDT
The owner in pkgdb is james (me) ... what's the problem?
Comment 6 Jon Ciesla 2008-10-03 12:30:00 EDT
I saw that.  I misread the request.  I thought it was an ownership change or some stripe, I totally missed the New Branch for F-10.

Ignore. :)
Comment 7 Kevin Fenzi 2008-10-03 12:46:36 EDT
cvs done.
Comment 8 Michal Nowak 2009-04-09 07:32:25 EDT
* mostly missing state of the patches w.r.t. upstream

> Patch0: python-2.6-config.patch
> Patch1: Python-2.2.1-pydocnogui.patch

https://fedoraproject.org/wiki/Packaging:Guidelines#All_patches_should_have_an_upstream_bug_link_or_comment

* inconsistency in sub-packages "Requires" field:

- python-libs : Requires: %{python} = %{version}-%{release}
- python-tools: Requires: %{name} = %{version}-%{release}

in case it's expected to be %{python} -eq %{name} I'd prefer to land on %{name} here.

* inconsistency in patching

> %patch6 -p1 -b .plural
> %patch7 -p1

* old-school exec, back-ticks are BASH specific and non-POSIX, even in Python are now discouraged

topdir=`pwd`  ->  topdir=$(pwd)

* missing _"_ should cover LD_LIBRARY_PATH's value

- LD_LIBRARY_PATH=$topdir $topdir/python Tools/scripts/pathfix.py -i "%{_bindir}/env python%{pybasever}" .

- LD_LIBRARY_PATH=$topdir PATH=$PATH:$topdir make -s OPT="$CFLAGS" %{?_smp_mflags}

- make install DESTDIR=$RPM_BUILD_ROOT

* from what is this preventing? Considering that this is the only occurrence.

[ -d $RPM_BUILD_ROOT ] && rm -fr $RPM_BUILD_ROOT
^^^^^^^^^^^^^^^^^^^^^^

* using path-based BR is discouraged /usr/bin/find, use "findutils" better, but note, that are listed in packages not necessary to be pulled to BR's https://fedoraproject.org/wiki/Packaging:Guidelines#Exceptions and thus always expected to be present.

* BuildRoot is non-standard, use one of this ones: https://fedoraproject.org/wiki/Packaging:Guidelines#BuildRoot_tag

> BuildRoot: %{_tmppath}/%{name}-%{version}-root

* is this hard-coded "/usr" necessary?

mkdir -p $RPM_BUILD_ROOT/usr $RPM_BUILD_ROOT%{_mandir}

  -> 

mkdir -p $RPM_BUILD_ROOT%{_prefix} $RPM_BUILD_ROOT%{_mandir}

* don't mix variable and macro style (e.g. $RPM_BUILD_ROOT -> %{buildroot})

https://fedoraproject.org/wiki/Packaging:Guidelines#Using_.25.7Bbuildroot.7D_and_.25.7Boptflags.7D_vs_.24RPM_BUILD_ROOT_and_.24RPM_OPT_FLAGS

* old-school defattr

%defattr(-, root, root) -> %defattr(-,root,root,-)

* be consistent with

  %dir %{_libdir}/python%{pybasever}
  %dir %{_libdir}/python%{pybasever}/site-packages

where when %{_prefix} is changed then %{_libdir} is changed too:

"""
%if "%{_lib}" == "lib64"
%attr(0755,root,root) %dir /usr/lib/python%{pybasever}
%attr(0755,root,root) %dir /usr/lib/python%{pybasever}/site-packages
%endif
"""

  ->

"""
%if "%{_lib}" == "lib64"
%dir %{_prefix}/lib/python%{pybasever}
%dir %{_prefix}/lib/python%{pybasever}/site-packages
%endif
"""

* hard-coded includedir

/usr/include/* -> %{_includedir}/%{name}%{pybasever}

* %defattr(-,root,root,755) -> %defattr(-,root,root,-)

* RPMLINT:

- python.x86_64: W: obsolete-not-provided python-elementtree

python-elementtree still present in F-9 and devel

- python.x86_64: E: binary-or-shlib-defines-rpath /usr/lib64/python
2.6/lib-dynload/_sqlite3.so ['/usr/lib64']

https://fedoraproject.org/wiki/Packaging:Guidelines#Beware_of_Rpath

- python.x86_64: E: non-standard-executable-perm /usr/lib64/python2
.6/lib-dynload/_ssl.so 0555

[and a lot of others] should be 0755?

- python.x86_64: E: script-without-shebang /usr/lib64/python2.6/run
py.py

Add shebang, or remove exec perm.

- python-devel.x86_64: W: summary-ended-with-dot The libraries and 
header files needed for Python development.
- python-tools.x86_64: W: summary-ended-with-dot A collection of de
velopment tools included with Python.

Fix, please.

- python-test.x86_64: W: spelling-error-in-description pacakge pack
age

ditto

- python-test.x86_64: W: no-documentation

Ignore, perhaps...

- python-test.x86_64: E: non-executable-script /usr/lib64/python2.6
/test/test_multibytecodec_support.py 0644

Add 0755 perms, or remove shebang.

- python-test.x86_64: E: zero-length /usr/lib64/python2.6/test/null
cert.pem

Is it necessary for the test?

- python-test.x86_64: E: wrong-script-interpreter /usr/lib64/python
2.6/test/test_pep263.py "-*-"

rpmlint is confused here because of the leading _!_, which is hardly necessary there, MacCVS should be fixed, not that file.

- python-test.x86_64: E: wrong-script-end-of-line-encoding /usr/lib
64/python2.6/test/test_pep263.py

- python-tools.x86_64: W: devel-file-in-non-devel-package /usr/lib6
4/python2.6/Demo/embed/demo.c

Probably not necessary to create python-tools-devel for two tests/4 .c files.
Comment 9 Michal Nowak 2009-08-11 09:22:43 EDT
ping
Comment 10 Dave Malcolm 2010-01-26 18:53:18 EST
(In reply to comment #8)

Thanks for your work on this.

I did a big reworking of the devel/python.spec last night; mostly to eliminate embedded copies of library sources; to ensure safety doing this I replaced the "dynfiles" code with an explicit list of the compiled modules in the payload (as a "safety net" to trap if modules went away).

I'm sorry about my lack of response on the review so far; I'm somewhat doomed with another issue right now :(

2.6.4-12.fc13 is building as:
http://koji.fedoraproject.org/koji/taskinfo?taskID=1946933
and contains some fixes as follows:

* Tue Jan 26 2010 David Malcolm <dmalcolm@redhat.com> - 2.6.4-12
- Address some of the issues identified in package review (bug 226342):
  - update libs requirement on base package to use %%{name} for consistency's
sake
  - convert from backticks to $() syntax throughout
  - wrap value of LD_LIBRARY_PATH in quotes
  - convert "/usr/bin/find" requirement to "findutils"
  - remove trailing periods from summaries of -devel and -tools subpackages
  - fix spelling mistake in description of -test subpackage
  - convert usage of $$RPM_BUILD_ROOT to %%{buildroot} throughout, for
stylistic consistency
  - supply dirmode arguments to defattr directives

Further comments inline:

> * mostly missing state of the patches w.r.t. upstream
> 
> > Patch0: python-2.6-config.patch
> > Patch1: Python-2.2.1-pydocnogui.patch
> 
> https://fedoraproject.org/wiki/Packaging:Guidelines#All_patches_should_have_an_upstream_bug_link_or_comment

Agreed; this is the biggest issue for me with the spec as it stands.  I hope to address this when other things calm down.


> * inconsistency in sub-packages "Requires" field:
> 
> - python-libs : Requires: %{python} = %{version}-%{release}
> - python-tools: Requires: %{name} = %{version}-%{release}
> 
> in case it's expected to be %{python} -eq %{name} I'd prefer to land on %{name}
> here.
Fixed in 2.6.4-12.fc13


> * inconsistency in patching
> 
> > %patch6 -p1 -b .plural
> > %patch7 -p1

Still TODO, I'm afraid.  My preference is to use -b for C code, and omit it for .py code (to avoid accidentally shipping the backups in the payload).

> * old-school exec, back-ticks are BASH specific and non-POSIX, even in Python
> are now discouraged
> 
> topdir=`pwd`  ->  topdir=$(pwd)

I've updated these throughout the specfile to use $() in 2.6.4-12.fc13


> * missing _"_ should cover LD_LIBRARY_PATH's value
> 
> - LD_LIBRARY_PATH=$topdir $topdir/python Tools/scripts/pathfix.py -i
> "%{_bindir}/env python%{pybasever}" .
> 
> - LD_LIBRARY_PATH=$topdir PATH=$PATH:$topdir make -s OPT="$CFLAGS"
> %{?_smp_mflags}

I've updated these two so to:
  LD_LIBRARY_PATH="$topdir"
in 2.6.4-12.fc13 - is this correct?


> - make install DESTDIR=$RPM_BUILD_ROOT

What's the issue here?


> * from what is this preventing? Considering that this is the only occurrence.
> 
> [ -d $RPM_BUILD_ROOT ] && rm -fr $RPM_BUILD_ROOT
> ^^^^^^^^^^^^^^^^^^^^^^
Changed to "rm -rf $RPM_BUILD_ROOT" in 2.6.4-12.fc13 as per packaging guidelines.

> 
> * using path-based BR is discouraged /usr/bin/find, use "findutils" better, but
> note, that are listed in packages not necessary to be pulled to BR's
> https://fedoraproject.org/wiki/Packaging:Guidelines#Exceptions and thus always
> expected to be present.
Changed to "findutils" in 2.6.4-12.fc13

> * BuildRoot is non-standard, use one of this ones:
> https://fedoraproject.org/wiki/Packaging:Guidelines#BuildRoot_tag
> 
> > BuildRoot: %{_tmppath}/%{name}-%{version}-root

Already changed to be the 2nd one on the list


> * is this hard-coded "/usr" necessary?
> 
> mkdir -p $RPM_BUILD_ROOT/usr $RPM_BUILD_ROOT%{_mandir}
> 
>   -> 
> 
> mkdir -p $RPM_BUILD_ROOT%{_prefix} $RPM_BUILD_ROOT%{_mandir}
Fixed in 2.6.4-12.fc13

> * don't mix variable and macro style (e.g. $RPM_BUILD_ROOT -> %{buildroot})
> 
> https://fedoraproject.org/wiki/Packaging:Guidelines#Using_.25.7Bbuildroot.7D_and_.25.7Boptflags.7D_vs_.24RPM_BUILD_ROOT_and_.24RPM_OPT_FLAGS
Changed $RPM_BUILD_ROOT -> %{buildroot} in 2.6.4-12.fc13

 
> * old-school defattr
> 
> %defattr(-, root, root) -> %defattr(-,root,root,-)
Fixed in 2.6.4-12.fc13

 
> * be consistent with
> 
>   %dir %{_libdir}/python%{pybasever}
>   %dir %{_libdir}/python%{pybasever}/site-packages
> 
> where when %{_prefix} is changed then %{_libdir} is changed too:
> 
> """
> %if "%{_lib}" == "lib64"
> %attr(0755,root,root) %dir /usr/lib/python%{pybasever}
> %attr(0755,root,root) %dir /usr/lib/python%{pybasever}/site-packages
> %endif
> """
> 
>   ->
> 
> """
> %if "%{_lib}" == "lib64"
> %dir %{_prefix}/lib/python%{pybasever}
> %dir %{_prefix}/lib/python%{pybasever}/site-packages
> %endif
> """
Already fixed


> * hard-coded includedir
> 
> /usr/include/* -> %{_includedir}/%{name}%{pybasever}
Already fixed

 
> * %defattr(-,root,root,755) -> %defattr(-,root,root,-)
Which subpackages?

> * RPMLINT:
> 
> - python.x86_64: W: obsolete-not-provided python-elementtree
> 
> python-elementtree still present in F-9 and devel
> 
> - python.x86_64: E: binary-or-shlib-defines-rpath /usr/lib64/python
> 2.6/lib-dynload/_sqlite3.so ['/usr/lib64']
> 
> https://fedoraproject.org/wiki/Packaging:Guidelines#Beware_of_Rpath
> 
> - python.x86_64: E: non-standard-executable-perm /usr/lib64/python2
> .6/lib-dynload/_ssl.so 0555
> 
> [and a lot of others] should be 0755?
> 
> - python.x86_64: E: script-without-shebang /usr/lib64/python2.6/run
> py.py
> 
> Add shebang, or remove exec perm.
I haven't yet looked into the ones above.  RPath issue is possibly already fixed.


> - python-devel.x86_64: W: summary-ended-with-dot The libraries and 
> header files needed for Python development.
> - python-tools.x86_64: W: summary-ended-with-dot A collection of de
> velopment tools included with Python.
> 
> Fix, please.
Fixed in 2.6.4-12.fc13


> - python-test.x86_64: W: spelling-error-in-description pacakge pack
> age
> 
> ditto
Fixed in 2.6.4-12.fc13

> - python-test.x86_64: W: no-documentation
> 
> Ignore, perhaps...
> 
> - python-test.x86_64: E: non-executable-script /usr/lib64/python2.6
> /test/test_multibytecodec_support.py 0644
> 
> Add 0755 perms, or remove shebang.
> 
> - python-test.x86_64: E: zero-length /usr/lib64/python2.6/test/null
> cert.pem
> 
> Is it necessary for the test?
> 
> - python-test.x86_64: E: wrong-script-interpreter /usr/lib64/python
> 2.6/test/test_pep263.py "-*-"
> 
> rpmlint is confused here because of the leading _!_, which is hardly necessary
> there, MacCVS should be fixed, not that file.
> 
> - python-test.x86_64: E: wrong-script-end-of-line-encoding /usr/lib
> 64/python2.6/test/test_pep263.py
> 
> - python-tools.x86_64: W: devel-file-in-non-devel-package /usr/lib6
> 4/python2.6/Demo/embed/demo.c
> 
> Probably not necessary to create python-tools-devel for two tests/4 .c files.    
I haven't yet looked at the ones above.


Thanks again for your review work; sorry for delay in resolving the issues you identified.
Comment 11 Dave Malcolm 2010-01-29 19:21:30 EST
I've gone through the patches, adding comments explaining what they all do, and upstream status, as far as possible, in tag "python-2_6_4-13_fc13".  I also removed the various commented-out patches.

See:
http://cvs.fedoraproject.org/viewvc/rpms/python/devel/python.spec?r1=1.164&r2=1.165

There appear to be some places where patches need to be upstreamed.
Comment 12 Jon Ciesla 2011-10-18 11:55:29 EDT
Where are we on this?
Comment 13 Cole Robinson 2015-02-11 15:38:42 EST
Mass reassigning all merge reviews to their component. For more details, see this FESCO ticket:

  https://fedorahosted.org/fesco/ticket/1269

If you don't know what merge reviews are about, please see:

  https://fedoraproject.org/wiki/Merge_Reviews

How to handle this bug is left to the discretion of the package maintainer.
Comment 14 Jan Kurik 2015-07-15 11:24:07 EDT
This bug appears to have been reported against 'rawhide' during the Fedora 23 development cycle.
Changing version to '23'.

(As we did not run this process for some time, it could affect also pre-Fedora 23 development
cycle bugs. We are very sorry. It will help us with cleanup during Fedora 23 End Of Life. Thank you.)

More information and reason for this action is here:
https://fedoraproject.org/wiki/BugZappers/HouseKeeping/Fedora23

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