Bug 688659 - Review Request: thunarx-python - Python bindings for the Thunar Extension Framework
Review Request: thunarx-python - Python bindings for the Thunar Extension Fra...
Product: Fedora
Classification: Fedora
Component: Package Review (Show other bugs)
All Linux
medium Severity medium
: ---
: ---
Assigned To: Kevin Fenzi
Fedora Extras Quality Assurance
Depends On:
  Show dependency treegraph
Reported: 2011-03-17 12:29 EDT by Balaji G
Modified: 2013-10-19 10:42 EDT (History)
4 users (show)

See Also:
Fixed In Version: thunarx-python-0.2.3-3.fc14.1
Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of:
Last Closed: 2011-03-28 02:12:18 EDT
Type: ---
Regression: ---
Mount Type: ---
Documentation: ---
Verified Versions:
Category: ---
oVirt Team: ---
RHEL 7.3 requirements from Atomic Host:
Cloudforms Team: ---
kevin: fedora‑review+
kevin: fedora‑cvs+

Attachments (Terms of Use)

  None (edit)
Description Balaji G 2011-03-17 12:29:09 EDT
http://balajig8.fedorapeople.org/packages/thunarx-python.spec -Python bindings for the Thunar Extension Framework

Python bindings for the Thunar Extension Framework.
Comment 1 Kevin Fenzi 2011-03-17 12:30:57 EDT
I'll go ahead and review this when I get a chance here.
Comment 2 Kevin Fenzi 2011-03-19 14:34:01 EDT
Setting flag and starting in on a review. Look for a review later today. :)
Comment 3 Kevin Fenzi 2011-03-19 16:38:17 EDT
OK - Package meets naming and packaging guidelines
OK - Spec file matches base package name. 
OK - Spec has consistant macro usage. 
OK - Meets Packaging Guidelines. 
See below - License
See below- License field in spec matches
See below- License file included in package
OK - Spec in American English
OK - Spec is legible.
OK - Sources match upstream md5sum:
6154df9ab701ec3aba6de251011a3d00  thunarx-python-0.2.3.tar.bz2
6154df9ab701ec3aba6de251011a3d00  thunarx-python-0.2.3.tar.bz2.rpm

See below - BuildRequires correct
OK - Package has %defattr and permissions on files is good. 
OK - Package has a correct %clean section. 
OK - Package has correct buildroot
OK - Package is code or permissible content. 
OK - Packages %doc files don't affect runtime. 
OK - Package has rm -rf RPM_BUILD_ROOT at top of %install

See below - .la files are removed. 

OK - Package compiles and builds on at least one arch. 
OK - Package has no duplicate files in %files. 
OK - Package doesn't own any directories other packages own. 
OK - Package owns all the directories it creates. 
OK - Package obey's FHS standard (except for 2 exceptions)
See below - No rpmlint output. 
See below - final provides and requires are sane.


OK - Should build in mock. 
OK - Should build on all supported archs
OK - Should function as described. 
OK - Should have sane scriptlets. 
OK - Should have subpackages require base package with fully versioned depend. 
OK - Should have dist tag
OK - Should package latest version
OK - Should not use file requires outside of /etc, /bin, /sbin, /usr/bin, or /usr/sbin


1. The license doesn't seem right in the spec file. 
Look at the head of the source files in the src/ 

2. You have no %doc files. Would be good to add AUTHORS ChangeLog docs/ COPYING NEWS README

3. There shouldn't be any need for the 
Requires: Thunar pygtk2 pygobject2 gnome-python2
line. When something is linked against things, rpm is smart enough to add the requires
in for you. ;) 

4. You might want to build the gtk-doc's here. 
checking whether to build gtk-doc documentation... no
checking for gtkdoc-check... no
(Should be BuildRequires: gtk-doc and adding produced files)

5. The .la files are still there. You need to change
rm -f $RPM_BUILD_ROOT%{_libdir}/*.la
rm -f $RPM_BUILD_ROOT%{_libdir}/*/*.la
And make sure it's after the install call. 

6. rpmlint says: 
thunarx-python.x86_64: W: incoherent-version-in-changelog 0.1-1 ['0.2.3-1.fc16', '0.2.3-1']
thunarx-python.x86_64: W: no-documentation

The first, your version in the changelog is wrong. You have 0.1-1, but it should be 0.2.3-1
The second will be fixed by adding docs. 

Remember to add a new changelog entry and change version to -2 when you fix the above. 
Comment 4 Balaji G 2011-03-21 13:00:28 EDT

Thanks a lot for your exhaustive review comments and for your patience to look through my work. I have uploaded the SPEC and the SRPM to the location



  - Balaji
Comment 5 Kevin Fenzi 2011-03-21 22:42:00 EDT
Thanks for the fixes! 

1. This still isn't quite right... this has the 'or any later version', so it should be 'LGPLv2+' as the License tag. 

2, 3, 4, 5 all look good and fixed. 

6. rpmlint now says: 
thunarx-python.src: W: invalid-license LGPL
thunarx-python.x86_64: W: invalid-license LGPL
thunarx-python-debuginfo.x86_64: W: invalid-license LGPL

which should be fixed by the License change. ;) 

You forgot to change the Release at the top from 1 to 2. ;) 

Fix those two things and I think everything looks good. You can do those two changes before importing the package. This package is APPROVED. I'll go ahead and sponsor you and you can continue the process from: https://fedoraproject.org/wiki/PackageMaintainers/Join#Add_Package_to_Source_Code_Management_.28SCM.29_system_and_Set_Owner once I've added you to the packager group.

Welcome to the fun!
Comment 6 Balaji G 2011-03-21 23:00:34 EDT

Thanks for the review again and hopefully i believe this should be ok. Have uploaded the spec and SRPMs in the same location as above.

  - Balaji
Comment 7 Balaji G 2011-03-21 23:12:05 EDT
New Package SCM Request
Package Name: thunarx-python
Short Description: Python bindings for the Thunar Extension Framework
Owners: balajig8 kevin
Branches: f14 f15 el6
InitialCC: baz

  - Balaji
Comment 8 Kevin Fenzi 2011-03-21 23:18:36 EDT
Git done (by process-git-requests).
Comment 9 Balaji G 2011-03-22 04:25:32 EDT
Thanks Kevin,Will push the package in.
Comment 10 Fedora Update System 2011-03-22 12:42:29 EDT
thunarx-python-0.2.3-3.fc14.1 has been submitted as an update for Fedora 14.
Comment 11 Fedora Update System 2011-03-22 13:11:14 EDT
thunarx-python-0.2.3-3.fc15 has been submitted as an update for Fedora 15.
Comment 12 Fedora Update System 2011-03-23 00:44:25 EDT
thunarx-python-0.2.3-3.fc15 has been pushed to the Fedora 15 testing repository.
Comment 13 Fedora Update System 2011-03-28 02:12:05 EDT
thunarx-python-0.2.3-3.fc15 has been pushed to the Fedora 15 stable repository.
Comment 14 Christoph Wickert 2011-03-28 20:42:14 EDT
One small issue: The package should not own %{_libdir}/thunarx-2/ but %{_libdir}/thunarx-2/* because the directory is already owned by Thunar.

Not sure if this justifies an update though.
Comment 15 Kevin Fenzi 2011-03-28 22:52:31 EDT
Good catch Christoph. ;) 

Yes, that should be fixed. There is a new upstream release in git, but it's not been pushed out with an announcement or tarball yet. It should be good to fix this when updating to that once it's out.
Comment 16 Fedora Update System 2011-04-05 01:20:40 EDT
thunarx-python-0.2.3-3.fc14.1 has been pushed to the Fedora 14 stable repository.

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