Bug 670457

Hello Zachary,

This is a unofficial review as I am still seeking sponsorship myself, but I noticed a few things worth mentioning:

 * Your SPEC should use macros where ever possible, your Source0 line should use %{name} and %{version} (this will make version upgrades easier).

   http://fedoraproject.org/wiki/Packaging/ReviewGuidelines
      MUST: Each package must consistently use macros. [16]

 * Under %files you are listing all files under /usr/share/man/man1/ (this could overlap with Build Dependencies and should be more specific).

     %doc %{_mandir}/man1/toilet.1.gz

Hello,
Thank you for the review I have fixed the things you pointed out and uploaded new versions of the SRPM and SPEC to my web server:

Spec URL: http://kamokow.com/fedora/packaging/toilet/toilet.spec
SRPM URL: http://kamokow.com/fedora/packaging/toilet/toilet-0.2-2.fc14.src.rpm

Good:
* rpmlint clean
* Package follows naming guidelines
* Spec file name matches package name
* License is WTFPL in source and spec file
* WTFPL is an open source license:
     http://fedoraproject.org/wiki/Licensing#SoftwareLicenses
* Spec file is legible American English
* License is not included source tarball and thus not in %doc
     https://fedoraproject.org/wiki/Packaging/LicensingGuidelines#License_Text
* Source matches upstream: md5sum:
     4dec7585a2a2d716a765d553cdc1ddaf  toilet-0.2.tar.gz
     4dec7585a2a2d716a765d553cdc1ddaf  toilet-0.2.tar.gz-upstream
* Builds in koji/mock
* No locale files that need to be marked with %find_lang
* No shared libraries
* No bundled libraries
* No files listed more than once
* All files and directories created by the package owned by the package and no
others. (see below)
* No large documentation that needs to be in a separate subpackage
* Nothing in %doc used at runtime
* No GUI application included so no .desktop requirement
* All filenames are valid utf-8
* No scriptlets
* No file dependencies
* Man page marked as %doc and placed properly

Needswork:
* I would like to see your %file section a bit more specific (as mentioned in initial review)
    %{_bindir}/toilet rather than %{_bindir}/*


I looks like your package is nearly ready for approval, if you can make the change mentioned in 'Needswork' we should be one step closer.

(In reply to comment #1) 
>  * Under %files you are listing all files under /usr/share/man/man1/ (this
> could overlap with Build Dependencies and should be more specific).
> 
>      %doc %{_mandir}/man1/toilet.1.gz

This is actually quite wrong. The %file section can make use of * and actually uses it a lot (just see http://fedoraproject.org/wiki/Packaging/Python#Byte_compiling for example)
There is no way file listed under %file can overlap with Build Dependencies (these are not present in the buildroot where your files are). What you have to make sure in the review though is that there are files having the same name and place in the filesystem.

I have changed the SPEC to reflect what Jeffrey had asked in comment #3 and then came back to see what had been posted in comment #4. The updated Spec and SRPM were already been uploaded and are available at:

Spec URL: http://kamokow.com/fedora/packaging/toilet/toilet.spec
SRPM URL: http://kamokow.com/fedora/packaging/toilet/toilet-0.2-3.fc14.src.rpm

but I will keep what the poster of comment #4 said in mind for future reference.

Also, man pages are not normally marked as %doc. Using %doc on a file disable its installation while using rpm with the --excludedocs argument. Files marked as %doc should therefore not influence the behaviour of the program, in this respect man page are not and could be %doc, but in general they are not (see for example the yum.spec http://pkgs.fedoraproject.org/gitweb/?p=yum.git;a=blob_plain;f=yum.spec;hb=HEAD )

Okay, the manpages have been unmarked as %doc. The new versions of the SPEC and SRPM are uploaded here:

Spec URL: http://kamokow.com/fedora/packaging/toilet/toilet.spec
SRPM URL: http://kamokow.com/fedora/packaging/toilet/toilet-0.2-4.fc14.src.rpm

The reviews you've gotten look good for this package.  A few clarifications:

1) man pages are automatically marked as %doc by rpm.  That's why you don't mark them as %doc explicitly. (I'm not sure if your original spec file just did %doc toilet.1 or if it did %doc %{_mandir}/man*/toilet.*  I assume the latter which is otherwise correct)

2) I notice that you don't set the CFLAGS explictly when building the program.  This works fine for this package since it's being picked up from the Environment variables that rpmbuild sets.  However, some Makefiles don't do that and you have to either set that when you call %configure or when you call make build.  You can look at the output from building the program to see if you need to do that or not.  Just something to be aware of since Makefiles differ so much from package to package.

I see you need a sponsor to get into the packager group.  Have you done any reviews of other packages that I could look at?

(In reply to comment #8)
> 1) man pages are automatically marked as %doc by rpm.  That's why you don't
> mark them as %doc explicitly. (I'm not sure if your original spec file just did
> %doc toilet.1 or if it did %doc %{_mandir}/man*/toilet.*  I assume the latter
> which is otherwise correct)

I was wondering if it would be worthwhile to put few words about this at https://fedoraproject.org/wiki/Packaging/Guidelines#Man_pages.

(In reply to comment #8)
> I see you need a sponsor to get into the packager group.  Have you done any
> reviews of other packages that I could look at?

I've only done a review of one other package (https://bugzilla.redhat.com/show_bug.cgi?id=669910) due to not having much free time recently, however, I can definitely do more reviews to show that I know my way around the Fedora packaging guidelines.

Oh, and thank you for the clarifications.

Hmm, the spec from above gives a 404...

Any news here?

No response in ages; closing.