Bug 670457 - Review Request: toilet - colorful ASCII art generator
Review Request: toilet - colorful ASCII art generator
Status: CLOSED NOTABUG
Product: Fedora
Classification: Fedora
Component: Package Review (Show other bugs)
rawhide
All Linux
low Severity medium
: ---
: ---
Assigned To: Nobody's working on this, feel free to take it
Fedora Extras Quality Assurance
:
Depends On:
Blocks: FE-DEADREVIEW
  Show dependency treegraph
 
Reported: 2011-01-18 06:12 EST by Zachary Doherty
Modified: 2012-04-25 10:39 EDT (History)
8 users (show)

See Also:
Fixed In Version:
Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of:
Environment:
Last Closed: 2012-04-25 10:39:23 EDT
Type: ---
Regression: ---
Mount Type: ---
Documentation: ---
CRM:
Verified Versions:
Category: ---
oVirt Team: ---
RHEL 7.3 requirements from Atomic Host:
Cloudforms Team: ---


Attachments (Terms of Use)

  None (edit)
Description Zachary Doherty 2011-01-18 06:12:18 EST
Spec URL: http://kamokow.com/fedora/packaging/toilet/toilet.spec
SRPM URL: http://kamokow.com/fedora/packaging/toilet/toilet-0.2-1.fc14.src.rpm
Description: TOIlet is a program for generating ASCII art text in the terminal, and optionally passing it through a set of filters which apply additional effects (colors, etc).

The package builds successfully through mock, and rpmlint reports no errors. 

PS. This is my first package and I'm looking for a sponsor.
Comment 1 Jeffrey Ness 2011-01-18 14:46:05 EST
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
Comment 2 Zachary Doherty 2011-01-18 15:32:22 EST
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
Comment 3 Jeffrey Ness 2011-01-22 13:20:25 EST
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.
Comment 4 Pierre-YvesChibon 2011-01-22 15:34:18 EST
(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.
Comment 5 Zachary Doherty 2011-01-22 16:09:40 EST
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.
Comment 6 Pierre-YvesChibon 2011-01-23 14:40:44 EST
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 )
Comment 7 Zachary Doherty 2011-01-23 15:39:40 EST
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
Comment 8 Toshio Ernie Kuratomi 2011-01-25 13:42:55 EST
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?
Comment 9 Pierre-YvesChibon 2011-01-25 15:21:26 EST
(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.
Comment 10 Zachary Doherty 2011-01-26 14:50:23 EST
(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.
Comment 11 Thomas Spura 2011-11-03 14:51:33 EDT
Hmm, the spec from above gives a 404...
Comment 12 Thomas Spura 2012-03-10 20:59:25 EST
Any news here?
Comment 13 Jason Tibbitts 2012-04-25 10:39:23 EDT
No response in ages; closing.

Note You need to log in before you can comment on or make changes to this bug.