Spec URL: http://randomuser.org/brewtarget/brewtarget.spec SRPM URL: http://randomuser.org/brewtarget/brewtarget-1.2.5-rc1.fc17.src.rpm Description: Brewtarget is an open source beer recipe creation tool. It automatically calculates color, bitterness, and other parameters for you while you drag and drop ingredients into the recipe. Brewtarget also has many other tools such as priming sugar calculators, OG correction help, and a unique mash designing tool. It also can export and import recipes in BeerXML. Fedora Account System Username: immanetize I've identified the need for three patches. One is a minor syntax correction for brewtarget desktop, another to place documentation in the correct folder, and the third to enable the application to warn and continue if the docs don't exist, rather than exit. All patches have been communicated upstream with context, as noted in the spec. Koji scratch builds have completed successfully: https://koji.fedoraproject.org/koji/taskinfo?taskID=4690714 https://koji.fedoraproject.org/koji/taskinfo?taskID=4690662 I haven't encountered any notable usage issues during cursory testing of the application.
Some comments: * I am confused by your release tag usage, why the rc prefix and why not increase on update? Please consult: https://fedoraproject.org/wiki/Packaging:NamingGuidelines#Release_Tag * why are " pairs used in license tag? * consider to use one line for each buildreq. * validate the desktop file during install * add empty line before %check * add space after * in %changelog * use VERBOSE=1 in make to verify options used when building
Some more: * use %{version} in source url * ctest don't seems to do anything useful, remove * no need for %{_docdir}/%{name}-%{version} in %files * app seems to ship mp3 files, that might be a problem
(In reply to comment #1) > Some comments: > > * I am confused by your release tag usage, why the rc prefix and why not > increase on update? > > Please consult: > https://fedoraproject.org/wiki/Packaging:NamingGuidelines#Release_Tag > It seemed appropriate, because the package has not been released in Fedora but is a candidate for it. I see the convention is to follow upstream's release canidates here, corrected. > * why are " pairs used in license tag? A syntax error on my part, corrected. > * consider to use one line for each buildreq. Done. > * validate the desktop file during install Done. > * add empty line before %check Done. > * add space after * in %changelog Done. I see I missed an entry here for Patch2, adding it now. > * use VERBOSE=1 in make to verify options used when building Done.
(In reply to comment #2) > Some more: > > * use %{version} in source url This seems a bit excessive. It would break validation tools, and require users trying to download the source tarball to manually substitute in the version number. > * ctest don't seems to do anything useful, remove Following http://fedoraproject.org/wiki/Packaging:Cmake here. I'm not familiar with cmake, so I'll take your word for it, thanks. Removed empty %clean section. > * no need for %{_docdir}/%{name}-%{version} in %files Fixed. > * app seems to ship mp3 files, that might be a problem I agree it isn't ideal, but several other packages are shipping mp3 files. I can test to verify the application functions if there is no decoder. Thanks for your comments, Terje! I also corrected the following issues: - Included two license documents in %doc that were shipped from upstream, - disposed of the superfluous `rm -rf %{_docdir}/%{name}-%{version}/*` in %install, this was leftover from before the related patch. Updated files are: SPEC: http://randomuser.org/brewtarget/brewtarget.spec SRPM: http://randomuser.org/brewtarget/brewtarget-1.2.5-2.fc17.src.rpm Koji: https://koji.fedoraproject.org/koji/taskinfo?taskID=4697931
Thanks for updates! Some more: rpmlint issues: brewtarget.x86_64: W: invalid-license WTFPLv2 brewtarget.x86_64: W: invalid-license LGPLv2.1 According to https://fedoraproject.org/wiki/Licensing:Main?rd=Licensing#Software_License_List the correct short names are LGPLv2+ and WTFPL brewtarget.x86_64: W: no-manual-page-for-binary brewtarget There is in fact a man page available in brewtarget-1.2.5/doc/brewtarget.1 Add that by doing e.g. in %install install -D 0644 doc/brewtarget.1 ${buildroot}%{_mandir}/man1/brewtarget.1 and %{_mandir}/man1/brewtarget.1* in %files. Pedantic space and empty lines hang ups: - no empty lines before %description - remove leading space in make %{?_smp_mflags} line - just a single empty line before %install and %changelog - one empty line between each item in %changelog BTW: the reason for %{version} in source0 is to avoid trouble when updating version and forgetting to update source0, you only need it the filename, not in the url.
(In reply to comment #5) > Thanks for updates! > > Some more: > > rpmlint issues: > > brewtarget.x86_64: W: invalid-license WTFPLv2 > brewtarget.x86_64: W: invalid-license LGPLv2.1 > > According to > https://fedoraproject.org/wiki/Licensing: > Main?rd=Licensing#Software_License_List > the correct short names are LGPLv2+ and WTFPL Corrected. After reading through a few licensing pages, I have a better understanding of the requirement here - the shortnames are more generalized than I thought. > brewtarget.x86_64: W: no-manual-page-for-binary brewtarget > > There is in fact a man page available in > > brewtarget-1.2.5/doc/brewtarget.1 > > Add that by doing e.g. in %install > > install -D 0644 doc/brewtarget.1 ${buildroot}%{_mandir}/man1/brewtarget.1 Good point, thanks for that. > %{_mandir}/man1/brewtarget.1* > > in %files. Done. I assume the wildcard is to accommodate future, additional manpages, so why not '%{_mandir}/man*/brewtarget*' ? > Pedantic space and empty lines hang ups: > > - no empty lines before %description > - remove leading space in make %{?_smp_mflags} line > - just a single empty line before %install and %changelog > - one empty line between each item in %changelog Done. > BTW: the reason for %{version} in source0 is to avoid trouble when > updating version and forgetting to update source0, you only need it the > filename, not in the url. I've been reading through reviews and doing some mock ones of my own - I see this convention is standard and I've corrected my URL accordingly. Thanks again, Terje. I really appreciate the help. Updated URLS: SPEC: http://randomuser.org/brewtarget/brewtarget.spec SRPM: http://randomuser.org/brewtarget/brewtarget-1.2.5-3.fc17.src.rpm KOJI: https://koji.fedoraproject.org/koji/taskinfo?taskID=4698972
I'll be happy to review this and look at sponsoring you...
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 See below - License file included in package OK - Spec in American English OK - Spec is legible. OK - Sources match upstream sha256sum: bda33075ed54c6702412e035f0f7eee417d4f6084ba4137ecb75912f65d4d114 brewtarget_1.2.5.orig.tar.gz bda33075ed54c6702412e035f0f7eee417d4f6084ba4137ecb75912f65d4d114 brewtarget_1.2.5.orig.tar.gz.orig OK - BuildRequires correct OK - Package has %defattr and permissions on files is good. OK - Package has a correct %clean section. OK - Package has correct buildroot OK - Package is code or permissible content. OK - Packages %doc files don't affect runtime. 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. OK - Package obey's FHS standard (except for 2 exceptions) See below - No rpmlint output. OK - final provides and requires are sane. SHOULD Items: OK - Should build in mock. OK - Should build on all supported archs OK - Should have dist tag OK - Should package latest version OK - Should not use file requires outside of /etc, /bin, /sbin, /usr/bin, or /usr/sbin Issues: 1. Might ask upstream to include a copy of LGPL? (not a blocker) 2. rpmlint says: brewtarget.i686: W: spurious-executable-perm /usr/share/man/man1/brewtarget.1.gz 3 packages and 0 specfiles checked; 0 errors, 1 warnings. Might fix the perms on the man page when installing it. Should be 644. Thats a pretty minor issue, so if you could fix that before importing thats fine with me and this package is APPROVED. I will go ahead and sponsor you, please let me know if you run into any questions... You can continue the process from: https://fedoraproject.org/wiki/Join_the_package_collection_maintainers#Add_Package_to_Source_Code_Management_.28SCM.29_system_and_Set_Owner Welcome to packaging fun! :)
New Package SCM Request ======================= Package Name: brewtarget Short Description: beer recipe tool Owners: immanetize Branches: f16 f17 f18 InitialCC:
<snip> > > Issues: > > 1. Might ask upstream to include a copy of LGPL? (not a blocker) > > 2. rpmlint says: > > brewtarget.i686: W: spurious-executable-perm > /usr/share/man/man1/brewtarget.1.gz > 3 packages and 0 specfiles checked; 0 errors, 1 warnings. > > Might fix the perms on the man page when installing it. Should be 644. > > Thats a pretty minor issue, so if you could fix that before importing thats > fine > with me and this package is APPROVED. > > I will go ahead and sponsor you, please let me know if you run into any > questions... > > You can continue the process from: > https://fedoraproject.org/wiki/ > Join_the_package_collection_maintainers#Add_Package_to_Source_Code_Management > _.28SCM.29_system_and_Set_Owner > > Welcome to packaging fun! :) I'll send a bug upstream on LGPL inclusion, and have corrected my install evocation. Kevin, thank you for your review, your sponsorship, and in general, the incalculable value you add to the project.
> Done. I assume the wildcard is to accommodate future, additional manpages, > so why not '%{_mandir}/man*/brewtarget*' ? No, rpmbuild will compress all man pages and brewtarget.1 will in fact be brewtarget.1.gz: $ rpm -qpli brewtarget-1.2.5-3.fc17.x86_64.rpm|grep man1/brewtarget.1 /usr/share/man/man1/brewtarget.1.gz rpmbuild might in future switch to xz, using wildcard to make things a little more robust.
Summary and SCM request package names don't match, please correct.
New Package SCM Request ======================= Package Name: brewtarget Short Description: An open source beer recipe creation tool Owners: immanetize Branches: f16 f17 f18 InitialCC:
Done.
brewtarget-1.2.5-4.fc16 has been submitted as an update for Fedora 16. https://admin.fedoraproject.org/updates/brewtarget-1.2.5-4.fc16
brewtarget-1.2.5-4.fc17 has been submitted as an update for Fedora 17. https://admin.fedoraproject.org/updates/brewtarget-1.2.5-4.fc17
brewtarget-1.2.5-4.fc18 has been submitted as an update for Fedora 18. https://admin.fedoraproject.org/updates/brewtarget-1.2.5-4.fc18
brewtarget-1.2.5-4.fc17 has been pushed to the Fedora 17 stable repository.
brewtarget-1.2.5-4.fc18 has been pushed to the Fedora 18 stable repository.
brewtarget-1.2.5-4.fc16 has been pushed to the Fedora 16 stable repository.
Any reason to keep this ticket open?
Nope. Not sure why bodhi didn't close it. ;(
Thanks, Kevin.