Bug 481322 - Review Request: emacs-magit - Emacs interface to the most common Git operations
Summary: Review Request: emacs-magit - Emacs interface to the most common Git operations
Keywords:
Status: CLOSED CURRENTRELEASE
Alias: None
Product: Fedora
Classification: Fedora
Component: Package Review
Version: rawhide
Hardware: All
OS: Linux
medium
medium
Target Milestone: ---
Assignee: Jerry James
QA Contact: Fedora Extras Quality Assurance
URL:
Whiteboard:
Depends On:
Blocks:
TreeView+ depends on / blocked
 
Reported: 2009-01-23 15:38 UTC by Tom Moertel
Modified: 2009-05-13 16:43 UTC (History)
3 users (show)

Fixed In Version:
Doc Type: Bug Fix
Doc Text:
Clone Of:
Environment:
Last Closed: 2009-05-13 16:43:49 UTC
Type: ---
Embargoed:
loganjerry: fedora-review+
kevin: fedora-cvs+


Attachments (Terms of Use)

Description Tom Moertel 2009-01-23 15:38:39 UTC
Spec URL: http://community.moertel.com/rpms/fedora/10/SPECS/emacs-magit.spec
SRPM URL: http://community.moertel.com/rpms/fedora/10/SRPMS/emacs-magit-0.7-1.20090122git.fc10.src.rpm
Description: Magit is an add-on package for GNU Emacs. It is an interface to
the Git source-code management system that aims to make the most
common operations convenient.

Comment 1 Jerry James 2009-01-27 18:41:57 UTC
I'll take this one.  Review coming up in a few minutes.

Comment 2 Jerry James 2009-01-27 19:48:00 UTC
MUST items:
- rpmlint output:
emacs-magit-el.noarch: W: no-documentation
2 packages and 1 specfiles checked; 0 errors, 1 warnings.
- package name: follows Emacs-specific guidelines
- spec file name matches base package name: OK
- packaging guidelines: OK
- approved Fedora license: OK
- License field matches actual license: FAIL
The file magit.el includes the "or later" phrase, so the license is actually GPLv3+, not GPLv3.  Furthermore, the info file contains a declaration that it is licensed under the GNU Free Documentation License, version 1.2 or any later version.  Therefore, the License field should read "GPLv3+ and GFDL+".  The guidelines strongly encourage you to separate the info file into a separate subpackage, so that each can have a single license, but I will not insist on this.
- File containing license should be in %doc: FAIL.  You must add COPYING to the %doc line in the spec file.  Why is that empty, by the way?  I suggest also adding AUTHORS, ChangeLog, NEWS, and README to the %doc line.
- spec file in American English: OK
- spec file is legible: OK
- source URL guidelines: FAIL
Please see https://fedoraproject.org/wiki/Packaging/SourceURL.  In short, you need to add a comment saying how to produce the source tarball.  See the section entitled "Using Revision Control".
- builds on at least one arch: OK
- appropriate use of ExcludeArch: OK
- complete BuildRequires: FAIL
The use of autoreconf triggers an invocation of automake.  Since automake Requires autoconf, you can simply replace "autoconf" with "automake" on the BuildRequires line.
- locale handling: OK
- use of ldconfig: OK
- relocatable package: OK
- own all created directories: OK
- no duplicate %files entries: OK
- proper file permissions: OK
- %clean section: OK
- consistent use of macros: OK
- code or permissible content: OK
- large documentation files: OK
- %doc files not needed at runtime: OK
- header files in -devel: OK
- static libraries in -static: OK
- Requires pkgconfig: OK
- .so files in -devel: OK
- devel packages require base package: OK
- no .la archives: OK
- desktop file: OK
- do not own files/directories created by other packages: OK
- clean build root before installing: OK
- filenames are valid UTF-8: OK

SHOULD items:
- query upstream for license text: the tarball includes the text of GPLv3 in COPYING, but does not include the GFDL text that I can see.  Please ask upstream to include it in future releases.
- include available translations: OK
- package builds in mock: OK
- package builds on all supported architectures: DID NOT CHECK
- package functions as described: MINIMAL TESTING ONLY
- sane scriptlets: OK
- subpackages require the base package: OK
- pkgconfig files in -devel: OK
- file dependencies: OK

Finally, just a note that you can replace "-n %{name}-el" with just "el" in the %package, %description, and %files lines where it appears.

Comment 3 Tom Moertel 2009-01-28 05:09:51 UTC
Jerry, thanks for the quick and helpful review!

I have fixed the problems you identified:

* Updated the License field.

* Added the missing docs.

* Added source-URL comment per guidelines for VCS-pulled source.

* Tweaked BuildRequires.

* Also, I patched the .texi file to include the fdl.texi file,
  which strangely enough *was* included in the upstream source
  but never got included into the magit.texi file.  I submitted
  the patch upstream.

* Finally, thanks for the tip on simplifying the "-n %{name}-el"
  incantations.

The new spec replaces the old:

Spec URL: http://community.moertel.com/rpms/fedora/10/SPECS/emacs-magit.spec
SRPM URL: http://community.moertel.com/rpms/fedora/10/SRPMS/emacs-magit-0.7-2.20090122git.fc10.src.rpm

Please let me know if you find any other problems or can think of any meaningful improvements I can make to the package.

Cheers,
Tom

Comment 4 Jerry James 2009-01-28 15:16:16 UTC
Looks good, Tom.  APPROVED.

Comment 5 Tom Moertel 2009-01-28 16:38:14 UTC
New Package CVS Request
=======================
Package Name: emacs-magit
Short Description: Emacs interface to the most common Git operations
Owners: tmoertel
Branches: F-9 F-10
InitialCC:

Comment 6 Kevin Fenzi 2009-01-29 00:14:10 UTC
cvs done.

Comment 7 Fedora Update System 2009-01-29 23:12:29 UTC
emacs-magit-0.7-3.20090122git.fc10 has been pushed to the Fedora 10 testing repository.  If problems still persist, please make note of it in this bug report.
 If you want to test the update, you can install it with 
 su -c 'yum --enablerepo=updates-testing update emacs-magit'.  You can provide feedback for this update here: http://admin.fedoraproject.org/updates/F10/FEDORA-2009-1191

Comment 8 Jerry James 2009-05-13 16:43:49 UTC
I don't know why this didn't get auto-closed when emacs-magit was pushed to stable, but it wasn't.  I'll close it manually.


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