Bug 514065 - Review Request: trac-tracnav-plugin - Navigation Bar for Trac
Summary: Review Request: trac-tracnav-plugin - Navigation Bar for Trac
Keywords:
Status: CLOSED ERRATA
Alias: None
Product: Fedora
Classification: Fedora
Component: Package Review
Version: rawhide
Hardware: All
OS: Linux
medium
medium
Target Milestone: ---
Assignee: Nobody's working on this, feel free to take it
QA Contact: Fedora Extras Quality Assurance
URL:
Whiteboard:
Depends On:
Blocks:
TreeView+ depends on / blocked
 
Reported: 2009-07-27 19:55 UTC by Thomas Moschny
Modified: 2009-08-25 04:25 UTC (History)
4 users (show)

Fixed In Version: 4.1-2.fc11
Doc Type: Bug Fix
Doc Text:
Clone Of:
Environment:
Last Closed: 2009-08-25 04:25:58 UTC
Type: ---
Embargoed:
dmaphy: fedora-review+
j: fedora-cvs+


Attachments (Terms of Use)

Description Thomas Moschny 2009-07-27 19:55:23 UTC
Spec URL: http://thm.fedorapeople.org/trac-tracnav-plugin/trac-tracnav-plugin.spec
SRPM URL: http://thm.fedorapeople.org/trac-tracnav-plugin/trac-tracnav-plugin-4.1-1.fc11.src.rpm

Description:
The TracNav macro implements a fully customizable navigation bar for
the Trac wiki engine. The contents of the navigation bar is a wiki
page itself and can be edited like any other wiki page through the web
interface. The navigation bar supports hierarchical ordering of
topics. The design of TracNav mimics the design of the TracGuideToc
that was originally supplied with Trac.

Comment 1 Dominic Hopf 2009-07-31 19:12:23 UTC
$ rpmlint trac-tracnav-plugin-4.1-1.fc11.src.rpm
1 packages and 0 specfiles checked; 0 errors, 0 warnings.

$ rpmlint trac-tracnav-plugin-4.1-1.fc11.noarch.rpm
1 packages and 0 specfiles checked; 0 errors, 0 warnings.

MUSTs
-----

OK: packaged is named according to the package naming guidelines
OK: specfile name matches %{name}.spec
OK: package seems to meet packaging guidelines
OK: license in specfile matches actual license and meets licensing guidelines
OK: license file is included in %doc
OK: specfile is written in AE
OK: specfile is legible
OK: sourcefile in the package is the same as provided in the mentioned source,
    md5sum fits
OK: package compiles successfully
OK: all build dependencies are listed in BuildRequires
N/A: package handles locales properly
     there are no locales installed with this package
N/A: call ldconfig in %post and %postun
     there is no binary installed with this package
OK: package is not designed to be relocatable
OK: package owns directorys it creates
OK: does not list a file more than once in the %files listing
OK: %files section includes %defattr and permissions are set properly
OK: %clean section is there and contains rm -rf %{buildroot}
OK: macros are consistently used
OK: package contains code
N/A: subpackage for large documentation files
     there are no large documentation files
OK: program runs properly without files listed in %doc
N/A: header files are in a -devel package
     there are no header files
N/A: static libraries are in a -static package
     there are no static libs
N/A: require pkgconfig if package contains a pkgconfig(.pc)
     there is no pkgconfig file
N/A: put .so-files into -devel package if there are library files with suffix
     there is no library with suffix, in fact there isn't any library
N/A: devel package includes fully versioned dependency for the base package
     there is no devel package
N/A: any libtool archives are removed
     there are no libtool archives
N/A: contains desktop file if it is a GUI application
     I assume this program will be called via webinterface, this may is a GUI,
     but I think a desktop file is not necessary for this kind of GUI
OK: package does not own any files or directories owned by other packages
OK: buildroot is removed at beginning of %install
N/A: filenames are encoded in UTF-8
     not necessary since there are no non-ASCII filenames


SHOULD
------
N/A: non-English translations for description and summary
     there are no other languages supported by this package, in fact it does not
     provide any localization. I assume localizations are not needed for this
     package.
OK: package builds in mock
N/A: package builds into binary rpms for all supported architectures
     this is a noarch package
N/A: program runs
     I did not test myself if the program works as it should since I do not have
     installed trac and don't use it
N/A: subpackages contain fully versioned dependency for the base package
     there are no subpackages
N/A: pkgconfig file is placed in a devel package
     there is no pkgconfig file
N/A: require package providing a file instead of the file itself
     no files outside of /etc, /bin, /sbin, /usr/bin, or /usr/sbin are required

Issues I found:
According to [1] %global is preferred over %define, so line 3 should be changed
to %global.

Try to bail out the 80 chars in the description.

I don't like the %{python_sitelib}/* construct in the %files section because it
does not contain any package or module name. Another packager would not see what
files and/or directories the package exactly includes and has to build and rpmls
the package first to find this out. I would like to see the module name mentioned
and would recommend to list the other directory as
%{python_sitelib}/TracNav-4.1-py*.egg-info/ to avoid problems or unneccessary
work with next python update.

[1] https://fedoraproject.org/wiki/PackagingDrafts/global_preferred_over_define

Comment 2 Thomas Moschny 2009-07-31 20:25:49 UTC
(In reply to comment #1)
> Issues I found:
> According to [1] %global is preferred over %define, so line 3 should be changed
> to %global.

Fixed.

> Try to bail out the 80 chars in the description.

If you insist, but normally I have emacs' fill-column set to 70 (the default, as far as I know).

> I don't like the %{python_sitelib}/* construct in the %files section

Unless there's a guideline that forces me to do so, I'd prefer not to list individual files or directories here, because removing redundancy makes maintaining the package much easier.

Spec URL: http://thm.fedorapeople.org/trac-tracnav-plugin/trac-tracnav-plugin.spec
SRPM URL: http://thm.fedorapeople.org/trac-tracnav-plugin/trac-tracnav-plugin-4.1-2.fc11.src.rpm

Comment 3 Christoph Wickert 2009-07-31 21:02:55 UTC
(In reply to comment #2)
> Unless there's a guideline that forces me to do so, I'd prefer not to list
> individual files or directories here, because removing redundancy makes
> maintaining the package much easier.

On the other hand, life is easier for other maintainers, when they can read something directly out of the spec and don't need to use rpm -qpl.

Changed status to assigned, as Dominic already started the review.

Comment 4 Dominic Hopf 2009-07-31 21:05:25 UTC
$ rpmlint trac-tracnav-plugin-4.1-2.fc11.src.rpm
1 packages and 0 specfiles checked; 0 errors, 0 warnings.

$ rpmlint trac-tracnav-plugin-4.1-2.fc11.noarch.rpm
1 packages and 0 specfiles checked; 0 errors, 0 warnings.


MUSTs
-----

OK: packaged is named according to the package naming guidelines
OK: specfile name matches %{name}.spec
OK: package seems to meet packaging guidelines
OK: license in specfile matches actual license and meets licensing guidelines
OK: license file is included in %doc
OK: specfile is written in AE
OK: specfile is legible
OK: sourcefile in the package is the same as provided in the mentioned source,
    md5sum fits
OK: package compiles successfully
OK: all build dependencies are listed in BuildRequires
N/A: package handles locales properly
     there are no locales installed with this package
N/A: call ldconfig in %post and %postun
     there is no binary installed with this package
OK: package is not designed to be relocatable
OK: package owns directorys it creates
OK: does not list a file more than once in the %files listing
OK: %files section includes %defattr and permissions are set properly
OK: %clean section is there and contains rm -rf %{buildroot}
OK: macros are consistently used
OK: package contains code
N/A: subpackage for large documentation files
     there are no large documentation files
OK: program runs properly without files listed in %doc
N/A: header files are in a -devel package
     there are no header files
N/A: static libraries are in a -static package
     there are no static libs
N/A: require pkgconfig if package contains a pkgconfig(.pc)
     there is no pkgconfig file
N/A: put .so-files into -devel package if there are library files with suffix
     there is no library with suffix, in fact there isn't any library
N/A: devel package includes fully versioned dependency for the base package
     there is no devel package
N/A: any libtool archives are removed
     there are no libtool archives
N/A: contains desktop file if it is a GUI application
     I assume this program will be called via webinterface, this may is a GUI,
     but I think a desktop file is not necessary for this kind of GUI
OK: package does not own any files or directories owned by other packages
OK: buildroot is removed at beginning of %install
N/A: filenames are encoded in UTF-8
     not necessary since there are no non-ASCII filenames


SHOULD
------
N/A: non-English translations for description and summary
     there are no other languages supported by this package, in fact it does not
     provide any localization. I assume localizations are not needed for this
     package.
OK: package builds in mock
N/A: package builds into binary rpms for all supported architectures
     this is a noarch package
N/A: program runs
     I did not test myself if the program works as it should since I do not have
     installed trac and don't use it
N/A: subpackages contain fully versioned dependency for the base package
     there are no subpackages
N/A: pkgconfig file is placed in a devel package
     there is no pkgconfig file
N/A: require package providing a file instead of the file itself
     no files outside of /etc, /bin, /sbin, /usr/bin, or /usr/sbin are required


(In reply to comment #2)
> (In reply to comment #1)
> > Try to bail out the 80 chars in the description.
>
> If you insist, but normally I have emacs' fill-column set to 70 (the default,
> as far as I know).
I do not insist on that. I think it is also okay to have 70 chars per line,
even if I would prefer 80 and rpmlint allows until 79 chars per line. It's not
worth changing your emacs settings at all.

> > I don't like the %{python_sitelib}/* construct in the %files section
>
> Unless there's a guideline that forces me to do so, I'd prefer not to list
> individual files or directories here, because removing redundancy makes
> maintaining the package much easier.
Since there is no guideline about this - at least no guideline I know - there
unfortunately doesn't seem to be any way to force you to write the %file list more human readable. ;)

APPROVED.

Comment 5 Thomas Moschny 2009-07-31 21:24:50 UTC
Thanks for the review!


New Package CVS Request
=======================
Package Name: trac-tracnav-plugin
Short Description: Navigation Bar for Trac
Owners: thm
Branches: F-10 F-11 EL-5
InitialCC: none
Cvsextras Commits: yes

Comment 6 Jason Tibbitts 2009-08-01 16:00:57 UTC
Note that we haven't used "Cvsextras Commits" for quite some time now.  Otherwise, CVS done.

Comment 7 Fedora Update System 2009-08-16 08:26:11 UTC
trac-tracnav-plugin-4.1-2.fc11 has been submitted as an update for Fedora 11.
http://admin.fedoraproject.org/updates/trac-tracnav-plugin-4.1-2.fc11

Comment 8 Fedora Update System 2009-08-17 22:03:10 UTC
trac-tracnav-plugin-4.1-2.fc11 has been pushed to the Fedora 11 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 trac-tracnav-plugin'.  You can provide feedback for this update here: http://admin.fedoraproject.org/updates/F11/FEDORA-2009-8691

Comment 9 Fedora Update System 2009-08-25 04:25:53 UTC
trac-tracnav-plugin-4.1-2.fc11 has been pushed to the Fedora 11 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.