Bug 518636

Summary: Review Request: django-reversion - Django extension that provides version control capabilities
Product: [Fedora] Fedora Reporter: Luca Botti <luca.botti>
Component: Package ReviewAssignee: Michel Alexandre Salim <michel>
Status: CLOSED NEXTRELEASE QA Contact: Fedora Extras Quality Assurance <extras-qa>
Severity: medium Docs Contact:
Priority: low    
Version: rawhideCC: fedora-package-review, michel, notting, tim
Target Milestone: ---Flags: michel: fedora‑review+
tibbs: fedora‑cvs+
Target Release: ---   
Hardware: All   
OS: Linux   
Whiteboard:
Fixed In Version: Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of: Environment:
Last Closed: 2010-11-16 02:18:49 EST Type: ---
Regression: --- Mount Type: ---
Documentation: --- CRM:
Verified Versions: Category: ---
oVirt Team: --- RHEL 7.3 requirements from Atomic Host:
Cloudforms Team: ---

Description Luca Botti 2009-08-21 08:05:26 EDT
Spec URL: http://lucabotti.fedorapeople.org/packages/django-reversion.spec
SRPM URL: http://lucabotti.fedorapeople.org/packages/django-reversion-1.1.2-1.fc11.src.rpm
Description: Reversion is an extension to the Django web framework that provides
comprehensive version control facilities. 

Features:
* Roll back to any point in a model's history - an unlimited undo facility!
* Recover deleted models - never lose data again!
* Admin integration for maximum usability.
* Group related changes into revisions that can be rolled back in a single
  transaction.
* Automatically save a new version whenever your model changes using Django's
  flexible signalling framework.
* Automate your revision management with easy-to-use middleware. 

Reversion can be easily added to your existing Django project with a minimum
of code changes. 
See http://code.google.com/p/django-reversion/
Comment 1 Michel Alexandre Salim 2009-09-28 15:56:29 EDT
Hello -- welcome to the world of Fedora packagers!

Some preliminary suggestions -- full review to follow, after which I can sponsor you. If you could also provide links to reviews you have done, or other packaging work (preferably RPM-based), even if these are not part of any distribution, please do so.

- for the Source0: field, you want to include the full URL. e.g.
http://django-reversion.googlecode.com/files/%{name}-%{version}.tar.gz

- URL at the end of %description is redundant; rpm -qi will show it

- %setup -q is enough; it defaults to -n %{name}-%{version}

- include release number in the Changelog. If you use Emacs, the easiest way to
  generate a changelog entry is to use M-x rpm-add-changelog-entry (you can
  tab-complete after rpm-add)

Use rpmlint on the source and binary RPMs after each update to make sure there are no issues -- and let me know if there is anything unclear. e.g. using your
spec, I get this:

$ rpmlint ../RPMS/noarch/django-reversion-1.1.2-1.fc12.noarch.rpm 
django-reversion.noarch: W: incoherent-version-in-changelog 1.1.2 ['1.1.2-1.fc12', '1.1.2-1']

==> missing revision number (you can skip the %{?dist} part)

django-reversion.noarch: W: no-documentation
==> %files should have a %doc section. In this case, it appears the author
    did not provide any documentation at all, so you'd want to ask him to bundle
    a license file with a tarball. For now, you can package the PKG-INFO file,
    since it comes from upstream and at least mentions the BSD license (though
    it also says LICENSE UNKNOWN).
Comment 2 Luca Botti 2009-09-29 06:11:04 EDT
Hi Michel,

thanks for suggestions. Here the new links:

http://lucabotti.fedorapeople.org/packages/django-reversion-1.1.2-2.fc11.src.rpm
http://lucabotti.fedorapeople.org/packages/django-reversion.spec

Updated to -2 given the fixes in SPEC file.

Thanks again and regards.
Comment 3 Michel Alexandre Salim 2009-09-29 13:44:37 EDT
Hullo!

Package looks good enough to normally pass review (though see the one recommended fix in the SHOULD section). Now I just need to see some more evidence of packaging knowledge (esp. since the initial package comes from Tim! Didn't know he does Python packages too, I thought it's only Lua).

You cannot do full reviews, since you are not sponsored yet, but you can pre-review other packages -- see

http://fedoraproject.org/wiki/PackageMaintainers/ReviewRequests

for a list of unassigned reviews. Make sure you clearly state that it's a pre-review, and not assign the review to yourself (preventing an authorized reviewer from seeing it).

Here's my review for this, as a reference: It's a bit more verbose than normal,
I normally take out the irrelevant Not Applicable parts, but in case you review
(or package) different types of software, they might come in handy.

MUST

OK rpmlint
$ rpmlint ../SRPMS/django-reversion-1.1.2-2.fc12.src.rpm ../RPMS/noarch/django-reversion-1.1.2-2.fc12.noarch.rpm 
2 packages and 0 specfiles checked; 0 errors, 0 warnings.

OK package name
OK spec file name
OK package guideline-compliant
OK license complies with guidelines
?  license field accurate
   upstream is unclear about this. Project page and one mention in PKG-INFO
   states BSD, but there is no license file and PKG-INFO also lists license as
   UNKNOWN
NA license file not deleted
   not included by upstream. See SHOULD section below
OK spec in US English
OK spec legible
OK source matches upstream
$ sha1sum django-reversion-1.1.2.tar.gz ../SOURCES/django-reversion-1.1.2.tar.gz 
8ff80fb027dc8f98d21f479b19ef0b450b266811  django-reversion-1.1.2.tar.gz
8ff80fb027dc8f98d21f479b19ef0b450b266811  ../SOURCES/django-reversion-1.1.2.tar.gz

OK builds under >= 1 archs, others excluded
   built using Koji: http://koji.fedoraproject.org/koji/taskinfo?taskID=1717143
OK build dependencies complete
NA locales handled using %find_lang, no %{_datadir}/locale
NA library -> ldconfig
NA relocatable: give reason
OK own all directories
OK no dupes in %files
OK permission
OK %clean RPM_BUILD_ROOT
OK macros used consistently
OK Package contains code
NA large docs => -doc
OK doc not runtime dependent
NA headers in -devel
NA static in -static
NA if contains *.pc, req pkgconfig
NA if libfiles are suffixed, the non-suffixed goes to devel
NA devel requires versioned base package
NA desktop file uses desktop-file-install
OK clean buildroot before install
OK filenames UTF-8

SHOULD
FIX if license text missing, ask upstream to include it
    perhaps post a bug at the upstream tracker and then put a comment in the
    spec above the %doc PKG-INFO line? that way you can package the correct
    license file once an fixed update comes out
NA  desc and summary contain translations if available
    well, nice to have, but I've only seen one package that does this (and
    the upstream author added the translation).
OK  package build in mock on all architectures
?   package functioned as described
OK  scriplets are sane
NA  other subpackages should require versioned base
NA  if main pkg is development-wise, pkgconfig can go in main package
OK  require package not files
Comment 4 Luca Botti 2009-09-30 10:12:31 EDT
Hi Michel,


For the SHOULD part, I opened the issue with the author, which is going to add the documents for next point release, due in a couple of weeks.

For the experience, i will take care of the pre-review part. For reference on other packaging, see the others packages submitted in bugzilla or go to my blog (http://www.lbotti.net/blog).

Thanks again
Comment 5 Jason Tibbitts 2010-01-21 00:40:29 EST
Anything happening here?
Comment 6 Michel Alexandre Salim 2010-11-11 06:58:37 EST
Apologies; package fell through the crack. Do you still need sponsorship? In which case, since there is now a 1.3.2 release, please do a quick update and I'll verify that everything is (still) in order.

https://github.com/etianen/django-reversion/wiki/getting-started
Comment 7 Luca Botti 2010-11-13 03:30:04 EST
Hi Michel,

this are the links for the latest SPEC and SRPMS

http://lucabotti.fedorapeople.org/packages/django-reversion-1.3.2-1.fc14.src.rpm
http://lucabotti.fedorapeople.org/packages/django-reversion.spec

Thanks
Comment 8 Michel Alexandre Salim 2010-11-13 09:32:18 EST
Hi Luca,

You don't need to define both python_sitelib and python_sitearch -- for noarch packages like this only python_sitelib is used.

Also: license file needs to be included, and you need to handle locale files. See complete review below

* TODO Review [40%]
** DONE Names [2/2]
*** DONE Package name
*** DONE Spec name
** DONE Meets [[http://fedoraproject.org/wiki/Packaging/Guidelines][guidelines]]
** DONE source files match upstream
   e13db17c02693c136be0585d1b32b5a0  django-reversion-1.3.2.tar.gz
   e13db17c02693c136be0585d1b32b5a0  ../SOURCES/django-reversion-1.3.2.tar.gz

** TODO License [2/3]
*** DONE License is Fedora-approved
*** DONE License field accurate
*** FAIL License included iff packaged by upstream
    please include LICENSE
** TODO rpmlint [1/2]
*** DONE on src.rpm
    $ rpmlint django-reversion-1.3.2-1.fc14.src.rpm 
    django-reversion.src: W: spelling-error %description -l en_US
    signalling -> sign aling, signal ling, signal-ling
    django-reversion.src: W: spelling-error %description -l en_US
    middleware -> midd le ware, middle-ware, middleweight
    1 packages and 0 specfiles checked; 0 errors, 2 warnings.

    Warnings are just due to the spellchecker not understanding
    technical jargon

*** FAIL on x86_64.rpm
    $ rpmlint ./django-reversion-1.3.2-1.fc14.noarch.rpm
    django-reversion.noarch: W: spelling-error %description -l en_US
    signalling -> signaling, signal ling, signal-ling
    django-reversion.noarch: W: spelling-error %description -l en_US
    middleware -> middle ware, middle-ware, middleweight

    Those can be ignored

    django-reversion.noarch: W: file-not-in-%lang /usr/lib/python2.7/site-packages/reversion/locale/de/LC_MESSAGES/django.mo
    django-reversion.noarch: W: file-not-in-%lang /usr/lib/python2.7/site-packages/reversion/locale/fr/LC_MESSAGES/django.mo
    django-reversion.noarch: W: file-not-in-%lang /usr/lib/python2.7/site-packages/reversion/locale/he/LC_MESSAGES/django.mo
    django-reversion.noarch: W: file-not-in-%lang /usr/lib/python2.7/site-packages/reversion/locale/it/LC_MESSAGES/django.mo
    django-reversion.noarch: W: file-not-in-%lang /usr/lib/python2.7/site-packages/reversion/locale/pl/LC_MESSAGES/django.mo
    django-reversion.noarch: W: file-not-in-%lang /usr/lib/python2.7/site-packages/reversion/locale/ru/LC_MESSAGES/django.mo
    1 packages and 0 specfiles checked; 0 errors, 8 warnings.

    These, however, need to be handled. see %find_lang section below.

** TODO Language & locale [2/3]
*** DONE Spec in US English
*** DONE Spec legible
*** FAIL Use %find_lang to handle locale files
    See for example how the main Django package handle this:
      
    # This is adapted from the %%find_lang macro, which cannot be directly
    # used since Django locale files are not located in %%{_datadir}
    #
    # The rest of the packaging guideline still apply -- do not list
    # locale files by hand!
    (cd $RPM_BUILD_ROOT && find . -name 'django*.mo') | %{__sed} -e 's|^.||' | %{__sed} -e \
      's:\(.*/locale/\)\([^/_]\+\)\(.*\.mo$\):%lang(\2) \1\2\3:' \
      >> %{name}.lang
      
    In this case you can reuse this without modifcation, since
    reversion's .mo files are also named django.mo
      
** DONE Build [3/3]
*** DONE Koji results
*** DONE BRs complete
*** DONE Directory ownership
** TODO Spec inspection [5/10]
*** WAIT No duplicate files
    note that once you use the modified find_lang script, you need to
    make sure the language files are not listed manually in %files
    anymore. You can NOT use %exclude for this as it would actually
    remove the excluded files, not just avoid re-listing them, so
    instead you'd have to manually list all the other directories:
      
    %files -f %{name}.lang
    ...
    %dir %{python_sitelib}/reversion
    %{python_sitelib}/reversion/*.py*
    %{python_sitelib}/reversion/management
    %{python_sitelib}/reversion/templates
    %{python_sitelib}/reversion/templatetags
      
      
*** DONE File permissions
*** DONE Filenames must be UTF-8
*** DONE no BuildRoot ([[https://fedoraproject.org/wiki/Packaging/Guidelines#BuildRoot_tag][except if targeting EPEL5]])
*** DONE Has %clean section
    (except F-13+:
    https://fedoraproject.org/wiki/Packaging/Guidelines#.25clean)
*** TODO %buildroot cleaned on %install
*** WAIT Macro usage consistent
    several minor fixes:
    - python_sitearch is redundant
    - probably should use %{version} in the source field, that way
      when you update the package you don't need to change the version
      in two places
*** DONE Documentation [2/2]
**** N/A If large docs, separate -doc
**** DONE %doc files are non-essential
Comment 9 Luca Botti 2010-11-14 14:42:15 EST
Hi Michel,

thanks for the review and the suggestions.

You can find new version of spec and src.rpm in 

http://lucabotti.fedorapeople.org/packages/django-reversion.spec

and

http://lucabotti.fedorapeople.org/packages/django-reversion-1.3.2-2.fc14.src.rpm

Regards
Comment 10 Michel Alexandre Salim 2010-11-14 15:33:37 EST
Hi Luca,

Looks good -- with one minor glitch, that you can fix when importing the package: you have *two* %changelog section! Please rpmlint the .spec file when you make changes.

Review is APPROVED, since the fix is trivial enough. And I've sponsored you. Please follow the next step in the review guidelines:

1. Change fedora-cvs flag to ? and create a new SCM request:
   http://fedoraproject.org/wiki/Package_SCM_admin_requests
   you only need to request the stable branches; 'master' targets Rawhide and
   you automatically get that

   For your next several packages, please put me in the InitialCc: list, and
   you're more than welcome to mail me with any packaging question or review
   requests.
 
2. Build package for Rawhide and other stable branches
   http://fedoraproject.org/wiki/Using_Fedora_GIT

3. Apart from el6 (which just went stable), the stable branches all required
   updates to be pushed through Bodhi; please create an update request for this
   package there: https://admin.fedoraproject.org/updates/new

   You can list the built packages for multiple distributions on the same form;
   they will share all properties (description, bugs closed, automatic push
   karma and automatic cancellation karma etc.) but will be partitioned based
   on the targeted release.

Welcome to the packaging club :)
Comment 11 Luca Botti 2010-11-14 16:20:29 EST
New Package SCM Request
=======================
Package Name: django-reversion
Short Description: Reversion is an extension to the Django web framework that provides
comprehensive version control facilities. 
Owners: lucabotti
Branches: f13 f14 el5 el6
InitialCC:
Comment 12 Michel Alexandre Salim 2010-11-14 16:24:39 EST
Cc:ing myself -- and removing new lines from the description :)

New Package SCM Request
=======================
Package Name: django-reversion
Short Description: Reversion is an extension to the Django web framework that provides comprehensive version control facilities. 
Owners: lucabotti
Branches: f13 f14 el5 el6
InitialCC: salimma
Comment 13 Tim Niemueller 2010-11-14 16:42:12 EST
I wonder why you don't use the one-liner from the bug summary, and even that could be shorted to something like "Django extension to provide version control".
Comment 14 Luca Botti 2010-11-14 17:35:21 EST
Just the emotion ;-)


New Package SCM Request
=======================
Package Name: django-reversion
Short Description: django-reversion - Django extension that provides version control capabilities 
Owners: lucabotti
Branches: f13 f14 el5 el6
InitialCC: salimma
Comment 15 Michel Alexandre Salim 2010-11-14 18:08:44 EST
You don't want the package name in the description either though, just the package summary.
Comment 16 Tim Niemueller 2010-11-14 18:39:50 EST
Indeed, the "django-reversion - " part has to go, it should just be "Django ...".
Comment 17 Luca Botti 2010-11-15 00:35:39 EST
That's it - lesson learned :-)



New Package SCM Request
=======================
Package Name: django-reversion
Short Description: Django extension that provides version
control capabilities 
Owners: lucabotti
Branches: f13 f14 el5 el6
InitialCC: salimma
Comment 18 Jason Tibbitts 2010-11-15 09:15:30 EST
Git done (by process-git-requests).