Bug 524107 - Review Request: qbrew - A Brewing Recipe Calculator
Summary: Review Request: qbrew - A Brewing Recipe Calculator
Keywords:
Status: CLOSED ERRATA
Alias: None
Product: Fedora
Classification: Fedora
Component: Package Review
Version: rawhide
Hardware: All
OS: Linux
low
medium
Target Milestone: ---
Assignee: Jon Stanley
QA Contact: Fedora Extras Quality Assurance
URL:
Whiteboard:
Depends On:
Blocks:
TreeView+ depends on / blocked
 
Reported: 2009-09-17 21:17 UTC by Chris St. Pierre
Modified: 2010-02-09 23:52 UTC (History)
5 users (show)

Fixed In Version: 0.4.1-6.fc11
Doc Type: Bug Fix
Doc Text:
Clone Of:
Environment:
Last Closed: 2009-10-29 15:47:16 UTC
Type: ---
Embargoed:
jonstanley: fedora-review+
j: fedora-cvs-


Attachments (Terms of Use)

Description Chris St. Pierre 2009-09-17 21:17:29 UTC
Spec URL: http://www.nebrwesleyan.edu/people/stpierre/qbrew.spec

SRPM URL: http://www.nebrwesleyan.edu/people/stpierre/qbrew-0.4.1-1.fc11.src.rpm

Description: QBrew is a homebrewer's recipe calculator. You can create and modify
ale and lager recipes as well as calculate gravity, color, and
bitterness. QBrew includes a database of styles, grains, hops, and
miscellaneous ingredients, plus a brewing tutorial.

Comment 1 Martin Gieseking 2009-09-18 10:20:00 UTC
Chris, have you already been sponsored? I can't find you in the account system, and in bug #435724 you mentioned you're looking for a sponsor. If this is still the case, please add FE-NEEDSPONSOR to the Blocks field above.

Comment 2 Martin Gieseking 2009-09-18 12:31:02 UTC
I'm not a sponsor but nonetheless some quick comments:

- the license tag must be BSD, as file LICENSE and the source headers mention the BSD license (two clause variant)

- in %Source0, give the full URL to the tarball

- Since with Fedora 10 it's not necessary to define a BuildRoot as rpm does this automatically. However, if you want to support older Fedora releases or EPEL <= 5, you have to define it this way:
http://fedoraproject.org/wiki/Packaging/Guidelines#BuildRoot_tag

- clear the execute flags of the source files, e.g. by adding 
    find src -type f -exec chmod 0644 {} \;
  to the %prep section
  
- in %files, replace %defattr(-,root,root) by %defattr(-,root,root,-)

- add the files AUTHORS, ChangeLog, and TODO to %doc

- add the version and revision number to the changelog entry:
  ... <chris.a.st.pierre> - 0.4.1-1


$ rpmlint /var/lib/mock/fedora-11-i386/result/qbrew-*
qbrew.i586: W: incoherent-version-in-changelog chris.a.st.pierre ['0.4.1-1.fc11', '0.4.1-1']
qbrew.i586: W: invalid-license Any permissive
qbrew.src: W: invalid-license Any permissive
qbrew-debuginfo.i586: W: invalid-license Any permissive
qbrew-debuginfo.i586: W: spurious-executable-perm /usr/src/debug/qbrew-0.4.1/src/textprinter.h
qbrew-debuginfo.i586: W: spurious-executable-perm /usr/src/debug/qbrew-0.4.1/src/textprinter.cpp
qbrew-debuginfo.i586: W: spurious-executable-perm /usr/src/debug/qbrew-0.4.1/src/configure.cpp
qbrew-debuginfo.i586: W: spurious-executable-perm /usr/src/debug/qbrew-0.4.1/src/configure.h
qbrew-debuginfo.i586: W: spurious-executable-perm /usr/src/debug/qbrew-0.4.1/src/qbrew.cpp
3 packages and 0 specfiles checked; 0 errors, 9 warnings.

Comment 3 Martin Gieseking 2009-09-18 12:51:09 UTC
(In reply to comment #2)
> - in %Source0, give the full URL to the tarball

Typo, sorry. I meant the Source0 field should contain the full URL to the tarball.

BTW, the spec file in the SRPM and http://www.nebrwesleyan.edu/people/stpierre/qbrew.spec are not identical. :)

Comment 4 Chris St. Pierre 2009-09-18 13:58:28 UTC
Specfile and SRPM updated.

Your rpmlint gave loads more errors than mine; is there an extra magical rpmlint config file out there somewhere?

Thanks!

Comment 5 Martin Gieseking 2009-09-18 14:13:05 UTC
(In reply to comment #4)
> Your rpmlint gave loads more errors than mine; is there an extra magical
> rpmlint config file out there somewhere?

No, I don't think so. I just ran rpmlint over all rpm files created by mock (srpm, binary rpm, and rpm containing the debuginfo). As you can see in the output above, most warnings were related to the debuginfo package. Maybe you simply left it out in your check.

Comment 6 Jon Stanley 2009-09-23 01:18:23 UTC
taking review

Comment 7 Jon Stanley 2009-09-23 01:29:26 UTC
One thing that I immediately see is that there's no history in this review.  What I mean by that is whenever you make a change to the package in support of the review request, you need to bump the release and add a changelog entry.  When the package finally gets approved, you then import that version exactly as approved - i.e. don't start over at 1.  If it takes four iterations in review, then you would import 0.4.1-4.

Comment 8 Jon Stanley 2009-09-23 02:30:05 UTC
Some other random tidbits:

The upstream tarball includes a stout.qbrew and a paleale.qbrew as example files of what you can do.  It would be most helpful to include these as %doc as well.

There's a translation to german in the upstream tarball as well.  It doesn't seem to build into a .qm even when I extract the tarball and just manually run ./configure ; make - my l10n-fu is non-existent, though. I can reach out for help on that one if you need it.

This is really nit-picky, but the upstream source files don't have consistent headers.  Some mention that the license is in the tarball, and others explicitly spell out what the license is.  The latter is preferred if you can convince upstream to do that in their next release.  Not a blocker.

Comment 9 Chris St. Pierre 2009-09-23 13:23:46 UTC
I've updated the specfile and RPM a) with a reconstructed revision history; and b) to include the example .qbrew files in %doc.

I browsed through the Makefile, but I couldn't figure out what it was doing or supposed to be doing with the qbrew_de.ts file, so I'll need some help getting that into the RPM.

The other issue I've run into is that qbrew looks for its help files in /usr/share/doc/qbrew/, while they're actually in /usr/share/doc/qbrew-E.V-R, so the Help doesn't work.  What's the appropriate way to solve this?  Patch the source?

Thanks!

Comment 10 Chris St. Pierre 2009-09-23 13:29:13 UTC
Forgot to mention: new SRPM URL is http://www.nebrwesleyan.edu/people/stpierre/qbrew-0.4.1-3.fc11.src.rpm

Comment 11 Chris St. Pierre 2009-09-28 15:35:31 UTC
A little bit of googling has shown me the way to Qt translation enlightenment, so the German translation (and any other translations that are added in the future) is now built and installed.  This works just dandy with the new package:

LC_ALL=de_DE qbrew

New spec file is uploaded and new SRPM is at http://www.nebrwesleyan.edu/people/stpierre/qbrew-0.4.1-4.fc11.src.rpm

Comment 12 Chris St. Pierre 2009-10-06 20:32:58 UTC
Following a conversation with Kevin Fenzi, I've made a few changes:

- Help files have been moved to /usr/share/doc/qbrew/, which is where qbrew expects them to be.  Help now works.
- Help files are no longer marked as %doc, since they are required for the software to function.
- Simplified the inclusion of things that actually are %doc.

A new specfile is up and a new SRPM is at http://www.nebrwesleyan.edu/people/stpierre/qbrew-0.4.1-5.fc11.src.rpm

Comment 13 Jon Stanley 2009-10-07 01:16:12 UTC
OK - Package meets naming and packaging guidelines
OK - Spec file matches base package name.
OK - Spec has consistant macro usage.
OK - Meets Packaging Guidelines.
OK - License
OK - License field in spec matches
NO - License file included in package
OK - Spec in American English
NO (see notes) - Spec is legible.
OK- Sources match upstream md5sum:
$ md5sum qbrew-0.4.1.tar.gz ../SOURCES/qbrew-0.4.1.tar.gz 
bf5009cf5ce5f3ea5069161012966cf7  qbrew-0.4.1.tar.gz
bf5009cf5ce5f3ea5069161012966cf7  ../SOURCES/qbrew-0.4.1.tar.gz

OK - Package needs ExcludeArch
OK - BuildRequires correct
OK - Spec handles locales/find_lang
N/A - Package is relocatable and has a reason to be.
OK - Package has %defattr and permissions on files is good.
OK - Package has a correct %clean section.
OK - Package has correct buildroot
%{_tmppath}/%{name}-%{version}-%{release}-root-%(%{__id_u} -n)
OK - Package is code or permissible content.
N/A- Doc subpackage needed/used.
OK - Packages %doc files don't affect runtime.

N/A - Headers/static libs in -devel subpackage.
N/A- Spec has needed ldconfig in post and postun
N/A - .pc files in -devel subpackage/requires pkgconfig
N/A - .so files in -devel subpackage.
N/A - -devel package Requires: %{name} = %{version}-%{release}
N/A - .la files are removed.

OK - Package is a GUI app and has a .desktop file

OK - Package compiles and builds on at least one arch.
OK - Package has no duplicate files in %files.
OK - Package doesn't own any directories other packages own.
OK - Package owns all the directories it creates.
NO - No rpmlint output.
qbrew.src:84: W: macro-in-%changelog doc
qbrew.src:85: W: macro-in-%changelog doc
qbrew.src:90: W: macro-in-%changelog doc

You can't include macros (%doc) in the changelog without escaping them - i.e. %%doc.
 
OK - final provides and requires are sane:


SHOULD Items:

OK - Should build in mock.
OK - Should build on all supported archs - http://koji.fedoraproject.org/koji/taskinfo?taskID=1732086
OK - Should function as described.1
OK - Should have sane scriptlets.
N/A - Should have subpackages require base package with fully versioned depend.
OK - Should have dist tag
OK - Should package latest version
N/A - check for outstanding bugs on package. (For core merge reviews)

Notes:

1. Include a blank line between versions of a changelog entry.  As such from one of my packages:

* Fri Feb 20 2009 Jon Stanley <jonstanley> - 0.914-4
- Fix *.ttf to *.otf

* Fri Feb 20 2009 Jon Stanley <jonstanley> - 0.914-3
- Remove comments from spec
- Something else
- Something 3

2. Include the LICENSE file as %doc (as well as in addition to where it already is if it's actually needed by the help)

Just fix these things and it should be good!

Comment 14 Chris St. Pierre 2009-10-07 15:13:47 UTC
Fixed and fixed.  New spec file is up, and new SRPM is at http://www.nebrwesleyan.edu/people/stpierre/qbrew-0.4.1-5.fc11.src.rpm

Comment 15 Jon Stanley 2009-10-27 23:23:38 UTC
Aside from the fact that your link was wrong (the SRPM is -6 instead of -5 :D), that package looks good.

APPROVED, sorry for the delay.

Comment 16 Chris St. Pierre 2009-10-28 15:49:09 UTC
New Package CVS Request
=======================
Package Name: qbrew
Short Description: A Brewing Recipe Calculator
Owners: cstpierre
Branches: F-11 F-12
InitialCC:

Comment 17 Kevin Fenzi 2009-10-29 00:09:59 UTC
cvs done.

Comment 18 Fedora Update System 2009-10-29 15:06:21 UTC
qbrew-0.4.1-6.fc11 has been submitted as an update for Fedora 11.
http://admin.fedoraproject.org/updates/qbrew-0.4.1-6.fc11

Comment 19 Chris St. Pierre 2009-10-29 15:47:16 UTC
Tag request submitted for F12.  All appears to be well in the world.

Comment 20 Fedora Update System 2009-11-27 21:50:14 UTC
qbrew-0.4.1-6.fc11 has been pushed to the Fedora 11 stable repository.  If problems still persist, please make note of it in this bug report.

Comment 21 Chris St. Pierre 2010-02-09 21:33:28 UTC
Package Change Request
======================
Package Name: qbrew
New Branches: F-13

Comment 22 Jason Tibbitts 2010-02-09 23:52:22 UTC
Too early to request F13 branches.  Actually, with no-frozen-rawhide, I don't think there will be any early branching at all.


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