Bug 798438

I'll vote for this - we need it to compile burp

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.

Thanks for the review i think o solved almost all of the isues 

new version availebol at:

http://dl.dropbox.com/u/64647042/uthash-devel.spec
http://dl.dropbox.com/u/64647042/uthash-devel-1.9.5-1.src.rpm


Please review it again

RPM lint warning ignord,

i ignord rpmplint warning about the build section because there is nothing to build

> RPM lint warning ignord,

Consult "rpmlint -i ..." please.   Also, you've ignored much of what I've written in comment 2.

Sorrie i did rpmlint -i on the spec file that was wrong, now i fixed a lot of extra issues

the new downloads are:
http://dl.dropbox.com/u/64647042/uthash-devel.spec
http://dl.dropbox.com/u/64647042/uthash-devel-1.9.5-2.src.rpm

The following two rpmlint warnings are invallid:
uthash-devel.src: W: spelling-error %description -l en_US utlist -> titlist, list
The value of this tag appears to be misspelled. Please double-check.

utlist is a name becouse of that not in spelling dictionaire

uthash-devel.src: W: spelling-error %description -l en_US preprocessor -> processor, predecessor, process's
The value of this tag appears to be misspelled. Please double-check.

preprocessor is not in the spelling dictionaire

* The License tag "BSD" is okay now.


* The empty %build section belongs _before_ %install not after it.


* The empty %clean section may be removed completely. It makes no sense to keep it, since the default is sufficient.


* Closer reading of the %description reveals this unclear sentence:

| It is a development package without a source or binary package as
| there are only header files.

"without a source or binary package" - How is that meant to be understood? A "source package" commonly is the src.rpm package. A "binary package" is the build, no matter whether "noarch" or arch-specific.

Before that, the description already explains that it is a "C preprocessor implementation". In case that needs further explanation, you could write: "There is no shared library for uthash, just C header files."


* Several items mentioned in comment 2 are not okay yet. Would you mind checking your own package against the
  https://fedoraproject.org/wiki/Packaging:ReviewGuidelines
page? For example, here a few items that must be dealt with:

MUST: rpmlint must be run on the source rpm and all binary rpms the build produces. The output should be posted in the review.

MUST: The package must be named according to the Package Naming Guidelines .

MUST: If (and only if) the source package includes the text of the license(s) in its own file, then that file, containing the text of the license(s) for the package must be included in %doc.

MUST: Permissions on files must be set properly. Executables should be set with executable permissions, for example.
https://fedoraproject.org/wiki/Packaging/Guidelines#FilePermissions


* With regard to "MUST: Static libraries must be in a -static package",
strictly speaking, this header-only package (that doesn't build a shared library) would need to adhere to the Static Library Packaging guidelines and provide a virtual -static subpackage "Provides". In case of a security vulnerability, for example, one would need to query for all packages that BuildRequires uthash-static and rebuild them.
However, header-only packages are somewhat of a grey area and are not covered by the guidelines yet. One would need to query for BuildRequires uthash-devel instead, which could be a little bit confusing.

Hence my proposal is: Add an important note at the top of the spec file, explaining that uthash-devel is a special header-only package, and any packages that build with it may need special treatment for crucial fixes in uthash.


* With regard to the package naming guidelines and comment 2, it is still confusing why you introduce the "uthash-dev" namespace for the documentation of a package that is called uthash-devel. Do you disagree with the hint in comment 2? Once you would follow the licensing guidelines (one of the "MUST" items above), you would introduce a second doc directory anyway. You'll see.

It's been a few months; any response to the above commentary?

If one followed today's decision made by the Fedora Packaging Committee about a similar package, they would do what I pointed out in comment 2: Name the package "uthash" (src.rpm name and upstream name), and additionally, they would store all files in a built uthash package, too, not in a uthash-devel. I hope they will clarify the guidelines as there is an inconsistency in them.

I'de like to help out in getting this package (and burp) into Fedora.
Bas, are you still interested in handling this? If not, I'll open a new review request.

Hi,

I need this package, too.

If you won't response to this review request I'll mark it as duplicate of mine.

*** This bug has been marked as a duplicate of bug 972568 ***