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@xxxxxxxxx> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |mschwendt@xxxxxxxxx Blocks| |177841(FE-NEEDSPONSOR) --- Comment #2 from Michael Schwendt <mschwendt@xxxxxxxxx> 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. _______________________________________________ package-review mailing list package-review@xxxxxxxxxxxxxxxxxxxxxxx https://admin.fedoraproject.org/mailman/listinfo/package-review