Bug 876865
| Summary: | Review Request: brewtarget - An open source beer recipe creation tool | ||
|---|---|---|---|
| Product: | [Fedora] Fedora | Reporter: | Pete Travis <me> |
| Component: | Package Review | Assignee: | Kevin Fenzi <kevin> |
| Status: | CLOSED CURRENTRELEASE | QA Contact: | Fedora Extras Quality Assurance <extras-qa> |
| Severity: | medium | Docs Contact: | |
| Priority: | medium | ||
| Version: | rawhide | CC: | notting, package-review, sanjay.ankur, terje.rosten |
| Target Milestone: | --- | Flags: | kevin:
fedora-review+
|
| Target Release: | --- | ||
| Hardware: | All | ||
| OS: | Linux | ||
| Whiteboard: | |||
| Fixed In Version: | Doc Type: | Bug Fix | |
| Doc Text: | Story Points: | --- | |
| Clone Of: | Environment: | ||
| Last Closed: | 2013-01-01 18:26:14 UTC | Type: | --- |
| Regression: | --- | Mount Type: | --- |
| Documentation: | --- | CRM: | |
| Verified Versions: | Category: | --- | |
| oVirt Team: | --- | RHEL 7.3 requirements from Atomic Host: | |
| Cloudforms Team: | --- | Target Upstream Version: | |
| Embargoed: | |||
|
Description
Pete Travis
2012-11-15 07:39:07 UTC
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. |