https://bugzilla.redhat.com/show_bug.cgi?id=1359412 --- Comment #2 from David Kaspar [Dee'Kej] <dkaspar@xxxxxxxxxx> --- So, I will lay down some questions/notes regarding the package, as they will come during the review: 0) Is this project already packaged for some other distributions? Which ones? Could you please provide the links those packages? 1) Is there any specific reason, why the project is named gawkextlib? I see your second review request for gawk-xml. If I should follow the Fedora packaging naming guidelines, I would prefer that this (current) package would be named gawk-extlib, so there will be uniformity between other gawk packages as well... 2) Please, remove the TABs in the specfile, so the indentation is same for everyone. Use whitespaces instead. 3) Try to keep the 'Summary' more simple, like for example: > Extension libraries for gawk ... Current summarry seems to me to vague, because using word library 2 times in a simple sentence is kind of confusing. 4) The Group tag is no longer necessary, and it shouldn't be in new specfiles. For more info, please, see: https://fedoraproject.org/wiki/Packaging:Guidelines#Tags_and_Sections 5) Unless you have multiple sources, it's less confusing to use 'Source' tag instead of 'Source0'. 6) (In reply to Andrew Schorr from comment #1) > FYI, I updated the spec file to make this minor change: > > @@ -5,7 +5,7 @@ Release: 1%{?dist} > License: GPLv3+ > Group: Development/Libraries > URL: http://sourceforge.net/projects/gawkextlib > -Source0: > http://sourceforge.net/projects/gawkextlib/files/%{name}-%{version}.tar.gz > +Source0: %{url}/files/%{name}-%{version}.tar.gz > BuildRequires: /usr/include/gawkapi.h > Requires: gawk Actually, I think this change make the Source URL shorter, but for new maintainers in the future, it might be a little bit obfuscating. My personal preference is to keep the full URL for the source tarball, with only %{name} and %{version} in it. However, this is just my opinion, the final decision about this will be on you... :) 7) I see you have used the /usr/include/gawkapi.h as the BuildRequires. Again, this is too much obfuscated, because other people would have to find out from where this header file comes from (is it from gawk or gawkextlib?). Since this is normally part of the gawk package (according to 'dnf whatprovides <path/file>), you should just use simple form of: > BuildRequires: gawk UPDATE: Looking at the gawk package itself, I will most likely split it into base package and devel supbackage. The /usr/include/gawkapi.h will be located inside the gawk-devel subpackage. 8) %description - it is generally more save to use the name of 'gawkextlib' in the description. According to Fedora Package Guidelines (FPG), there might be instances where the %{name} might not expand properly, which is not correct. And I wouldn't be affraid to use more specific description, like the one you have on the sourceforge webpage: "The gawkextlib project provides several extension libraries for gawk (GNU AWK), as well as libgawkextlib containing some APIs that are useful for building gawk extension libraries." In that case, it's the same for people comparing if the source code @ sourceforg is the same as the source code packaged in Fedora. :) 9) There's no need to use '-n %{name}-devel' in the %package section. rpmbuild should be clever enough these days to put the dash (-) there automatically. So you can just use: > %package devel Update: This applies in case if you want to have the %{name} followed by dash and the suffix. It can be useful in case you need something like this: > %package -n lib-%{name}-devel In this way, you can override the default behaviour of rpmbuild. 10) The 'Requires' tag for /usr/include/gawkapi.h in devel supbackage is not necessary, because the gawkextlib (base package) already requires gawk, and the devel subpackage is depending on its base package. 11) Similar to point 9), you can just use "%description devel". 12) Please, if not necessary for some significant reason, try *not* use %setup -q anymore. rpmbuild is able to %autosetup, which will automatically apply all the listed patches (PatchN), if they are correctly formatted. It makes maintaing of packages in the future much simple. More info you can find here: https://fedoraproject.org/wiki/Packaging:Guidelines#Applying_patches and here: http://www.rpm.org/wiki/PackagerDocs/Autosetup 13) The 'rm -rf %{buildroot}' is no longer necessary for building packages for Fedora. It is beening cleaned up automatically. 14) %makeinstall macro is "forbidden". Use some other macros instead: https://fedoraproject.org/wiki/Packaging:Guidelines#Why_the_.25makeinstall_macro_should_not_be_used 15) You're not calling the ldconfig for (un)installation of devel subpackages, which is not correct: https://fedoraproject.org/wiki/Packaging:Guidelines#Shared_Libraries So, basically, you're missing something like this: > %post devel -p /sbin/ldconfig > %postun devel -p /sbin/ldconfig 16) In %files section - %defattr is not needed anymore: https://fedoraproject.org/wiki/Packaging:Guidelines#File_Permissions 17) Please, consider if your package should use hardened build: https://fedoraproject.org/wiki/Packaging:Guidelines#PIE You know it better than I do. 18) Another thing I have noticed is that your devel supbackage does not use fully-qualified Requires tag. More info: https://fedoraproject.org/wiki/Packaging:Guidelines#Requiring_Base_Package 19) Looking at your last changelog, I would suggest make it more descriptive in the future. In case it was a rebase to newer version (my guess), than it should be stated there explicitly. It helps in case someone is trying to find changes that caused some problems. These are only notes regarding the specfile... I will look do the licensing check and other necessary steps soon. :) One more thing, I know I require much from you right now, but I was lead to this with my experience with maintainership. I will most likely maintain these packages, if they ever get into RHEL, so I don't want to have more work than necessary in the future. In case you're interested, here's example of one of the specfile I had cleaned up this year (tcsh): http://pkgs.fedoraproject.org/cgit/rpms/tcsh.git/tree/tcsh.spec It took me a lot of time to fix the mess which was in the specfile previously... :-/ Thank you for your understanding. -- You are receiving this mail because: You are on the CC list for the bug. You are always notified about changes to this product and component _______________________________________________ package-review mailing list package-review@xxxxxxxxxxxxxxxxxxxxxxx https://lists.fedoraproject.org/admin/lists/package-review@xxxxxxxxxxxxxxxxxxxxxxx