[Bug 798438] Review Request: uthash-devel - Hash table and linked list for C structures

Sat Mar 10 10:15:24 UTC 2012

Please do not reply directly to this email. All additional
comments should be made in the comments box of this bug.

https://bugzilla.redhat.com/show_bug.cgi?id=798438

Michael Schwendt <mschwendt at gmail.com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |mschwendt at gmail.com
             Blocks|                            |177841(FE-NEEDSPONSOR)

--- Comment #2 from Michael Schwendt <mschwendt at gmail.com> 2012-03-10 05:15:21 EST ---
The Package Review process is not about voting, however. What's needed is
people to create quality packages that work fine and to maintain them properly.

[...]

The linked uthash-devel requires a lot of work. It may seem to be a simple
package, but it contains several questionable items and is much different from
the typical Fedora package. That doesn't make it an easy review.

A brief look at the spec:

* Try to review your own package according to the 
  https://fedoraproject.org/wiki/Packaging:ReviewGuidelines
  which link to the PackagingGuidelines in many (if not all) places.

* Further reading: https://fedoraproject.org/wiki/PackageMaintainers

* Please run rpmlint on the src.rpm package and all built rpms, even if it
reports a few false positives. Copy its output into the review request and
comment on what you think about it.

> Name:               uthash-devel

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

Not just that, but with the upstream project using the name "uthash", it would
be unfortunate to not call the base package "uthash" either. And that would not
create any problems related to not building a base package with that name but
just an uthash-devel package.

> %define soname      1

What's the purpose of this macro? It's not used anywhere else.

> License:            Revised License (BSD-3-Clause)

See rpmlint output and:
https://fedoraproject.org/wiki/Packaging:Guidelines#Licensing

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

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

> BuildRequires:      zlib-devel

Why? This spec file does not contain a %build section.

> Provides:           uthash-devel = %{version}

To be deleted. Base %name Provides are automatic. Examine the built package to
find out what is provided automatically.

> Source:             http://prdownloads.sourceforge.net/uthash/uthash-%{version}.tar.bz2

http://fedoraproject.org/wiki/Packaging:SourceURL#Sourceforge.net

> %description
> Hash table and linked list for C structures
> This package provides uthash and utlist, C preprocessor
> [...]

The first line is not a full sentence but just a copy of the summary.

> %install
> mkdir -p $RPM_BUILD_ROOT/usr/include/
> install -m644 src/uthash.h $RPM_BUILD_ROOT/usr/include/

Please add option -p when installing or copying package files in order to
preserve timestamps. That's considered helpful by many users, since it gives a
hint on the age of files (not limited to documentation).

> rm -f $RPM_BUILD_ROOT/usr/share/doc/uthash-dev/html/*.svg

That's a comment that asks for a comment in the spec file. It's obvious that it
deletes an installed file, but the "why" ought to be explained.

> cp doc/txt/ChangeLog.txt $RPM_BUILD_ROOT/usr/share/doc/uthash-dev/changelog
> gzip -9 $RPM_BUILD_ROOT/usr/share/doc/uthash-dev/changelog

There won't ever be guidelines about that, but really keep it uncompressed as
you won't try to optimise for space-saving in thousands of other %doc files
either. Renaming it would be unusual, too.

> %clean
> %{?buildroot:%__rm -rf "%{buildroot}"}

https://fedoraproject.org/wiki/Packaging:Guidelines#.25clean
and
https://fedoraproject.org/wiki/Packaging:Guidelines#Macros

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

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

Plus, the %files section is overly and unnecessarily complex. Let me quote for
a few comments below:

%files
%defattr(-,root,root)
/usr/include/utarray.h
/usr/include/uthash.h
/usr/include/utlist.h
/usr/include/utstring.h
%doc %attr(0644,root,root)   /usr/share/doc/uthash-dev/changelog.gz
%doc %attr(0644,root,root)   /usr/share/doc/uthash-dev/html/html/ChangeLog.html
%doc %attr(0644,root,root)   /usr/share/doc/uthash-dev/html/html/img/banner.png
%doc %attr(0644,root,root)   /usr/share/doc/uthash-dev/html/html/img/banner.svg
%doc %attr(0644,root,root)  
/usr/share/doc/uthash-dev/html/html/img/grad_blue.png
%doc %attr(0644,root,root)  
/usr/share/doc/uthash-dev/html/html/img/grad_blue.svg
%doc %attr(0644,root,root)   /usr/share/doc/uthash-dev/html/html/img/rss.png
%doc %attr(0644,root,root)  
/usr/share/doc/uthash-dev/html/html/img/uthash-mini.png
%doc %attr(0644,root,root)  
/usr/share/doc/uthash-dev/html/html/img/uthash-mini.svg
%doc %attr(0644,root,root)   /usr/share/doc/uthash-dev/html/html/img/uthash.png
%doc %attr(0644,root,root)   /usr/share/doc/uthash-dev/html/html/index.html
%doc %attr(0644,root,root)   /usr/share/doc/uthash-dev/html/html/license.html
%doc %attr(0644,root,root)   /usr/share/doc/uthash-dev/html/html/styles.css
%doc %attr(0644,root,root)   /usr/share/doc/uthash-dev/html/html/tdh-quirks.css
%doc %attr(0644,root,root)   /usr/share/doc/uthash-dev/html/html/tdh.css
%doc %attr(0644,root,root)   /usr/share/doc/uthash-dev/html/html/toc.css
%doc %attr(0644,root,root)   /usr/share/doc/uthash-dev/html/html/userguide.html
%doc %attr(0644,root,root)   /usr/share/doc/uthash-dev/html/html/utarray.html
%doc %attr(0644,root,root)   /usr/share/doc/uthash-dev/html/html/utlist.html
%doc %attr(0644,root,root)   /usr/share/doc/uthash-dev/html/html/utstring.html
%doc %attr(0644,root,root)   /usr/share/doc/uthash-dev/html/html/userguide.pdf
%doc %attr(0644,root,root)   /usr/share/doc/uthash-dev/pdf/pdf/userguide.pdf
%doc %attr(0644,root,root)   /usr/share/doc/uthash-dev/txt/txt/ChangeLog.txt
%doc %attr(0644,root,root)   /usr/share/doc/uthash-dev/txt/txt/sflogo.txt
%doc %attr(0644,root,root)   /usr/share/doc/uthash-dev/txt/txt/toc.txt
%doc %attr(0644,root,root)   /usr/share/doc/uthash-dev/txt/txt/topnav.txt
%doc %attr(0644,root,root)  
/usr/share/doc/uthash-dev/txt/txt/topnav_utarray.txt
%doc %attr(0644,root,root)  
/usr/share/doc/uthash-dev/txt/txt/topnav_utlist.txt
%doc %attr(0644,root,root)  
/usr/share/doc/uthash-dev/txt/txt/topnav_utstring.txt
%doc %attr(0644,root,root)   /usr/share/doc/uthash-dev/txt/txt/userguide.txt
%doc %attr(0644,root,root)   /usr/share/doc/uthash-dev/txt/txt/utarray.txt
%doc %attr(0644,root,root)   /usr/share/doc/uthash-dev/txt/txt/utlist.txt
%doc %attr(0644,root,root)   /usr/share/doc/uthash-dev/txt/txt/utstring.txt
%dir    /usr/share/doc/uthash-dev
%dir    /usr/share/doc/uthash-dev/html
%dir    /usr/share/doc/uthash-dev/html/html
%dir    /usr/share/doc/uthash-dev/html/html/img
%dir    /usr/share/doc/uthash-dev/pdf
%dir    /usr/share/doc/uthash-dev/pdf/pdf
%dir    /usr/share/doc/uthash-dev/txt
%dir    /usr/share/doc/uthash-dev/txt/txt

1) You should avoid using %attr for _ordinary_ file permissions like that, so a
special %attr sticks out. Mode 0644 and root:root ownership for files is the
default. Fix unusual permissions in the %install section or even earlier in
%prep.

2) It can make sense to list the four include files explicitly as you do it.
That way the build of an upgrade would break if any file is absent. A great
early-warning system for unexpected changes. However: It doesn't make sense to
spell out the individual files and directories of the large documentation tree.
Use a single line to include the entire tree (which will include any
directories, too). To make that convenient, don't install all these files into
the buildroot, but let the %doc macro include these files/dirs from your build
directory. They will end up in %_defaultdocdir/%name-%version just as with
thousands of other packages.
For example, if you just used

  %doc doc/html doc/pdf doc/txt

that would create a fine documentation tree in your package.

-- 
Configure bugmail: https://bugzilla.redhat.com/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are on the CC list for the bug.