Bug 979767 (kapow)
Summary: | Review Request: kapow - A punch clock program | ||
---|---|---|---|
Product: | [Fedora] Fedora | Reporter: | Ankur Sinha (FranciscoD) <sanjay.ankur> |
Component: | Package Review | Assignee: | Christopher Meng <i> |
Status: | CLOSED ERRATA | QA Contact: | Fedora Extras Quality Assurance <extras-qa> |
Severity: | medium | Docs Contact: | |
Priority: | medium | ||
Version: | rawhide | CC: | i, notting, package-review |
Target Milestone: | --- | Flags: | i:
fedora-review+
gwync: fedora-cvs+ |
Target Release: | --- | ||
Hardware: | All | ||
OS: | Linux | ||
Whiteboard: | |||
Fixed In Version: | kapow-1.4.4.1-1.fc20 | Doc Type: | Bug Fix |
Doc Text: | Story Points: | --- | |
Clone Of: | Environment: | ||
Last Closed: | 2013-10-31 03:05:12 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
Ankur Sinha (FranciscoD)
2013-06-30 11:51:28 UTC
I'd like to say the author is good at making money. However some problems: 1. Source0 can be found: http://gottcode.org/kapow/kapow-1.4.4.1-src.tar.bz2 2. Please leave less blank lines as far as possible. 3. No need to rm -rf $RPM_BUILD_ROOT 4. desktop-file-validate should be put into %check section. (In reply to Christopher Meng from comment #1) > I'd like to say the author is good at making money. It's perfectly acceptable for foss developers to request donations :) > > However some problems: > > 1. Source0 can be found: http://gottcode.org/kapow/kapow-1.4.4.1-src.tar.bz2 Updated > > 2. Please leave less blank lines as far as possible. > > 3. No need to rm -rf $RPM_BUILD_ROOT > > 4. desktop-file-validate should be put into %check section. This is not a MUST. The guidelines say that desktop-file-validate can be used in either the install or check sections. I've moved it to a separate check section now any way. http://fedoraproject.org/wiki/Packaging:Guidelines#desktop-file-install_usage Spec/f19 srpm updated. Since they are only cosmetic changes, I haven't rebuilt the binary rpms. I notice you haven't accepted the review ticket. Are you going to do a full review? :) Thanks, Warm regards, Ankur Yes. I'll do a full review. And please fix the issues above. Besides why there are %{_datadir}/%{name}/translations/qt_it.qm %{_datadir}/%{name}/translations/qt_nl.qm but not in lang file? URL 404. (In reply to Christopher Meng from comment #3) > Yes. I'll do a full review. > > And please fix the issues above. Fixed. > > Besides why there are > > %{_datadir}/%{name}/translations/qt_it.qm > %{_datadir}/%{name}/translations/qt_nl.qm > > but not in lang file? The don't get picked by find lang. I'm assuming they're something to do with qt5. I've commented in the spec. I hadn't realized that I deleted the spec/srpm. Reuploaded them. Apologies: Spec/srpm: http://ankursinha.fedorapeople.org/kapow/kapow.spec http://ankursinha.fedorapeople.org/kapow/kapow-1.4.4.1-1.fc21.src.rpm Thanks, Warm regards, Ankur [?]: Package must own all directories that it creates. Note: Directories without known owners: /usr/share/icons/hicolor/256x256/apps, /usr/share/icons/hicolor/24x24, /usr/share/icons/hicolor/64x64/apps, /usr/share/icons/hicolor/22x22, /usr/share/icons/hicolor/48x48/apps, /usr/share/icons/hicolor/22x22/apps, /usr/share/icons/hicolor/32x32/apps, /usr/share/icons/hicolor/24x24/apps, /usr/share/kapow, /usr/share/kapow/translations, /usr/share/icons/hicolor/16x16/apps, /usr/share/icons/hicolor/scalable/apps, /usr/share/icons/hicolor/16x16, /usr/share/icons/hicolor/128x128/apps, /usr/share/icons/hicolor/128x128, /usr/share/icons/hicolor/64x64, /usr/share/icons/hicolor/48x48, /usr/share/icons/hicolor/256x256, /usr/share/icons/hicolor, /usr/share/icons/hicolor/32x32, /usr/share/icons/hicolor/scalable AND kapow.src:43: W: macro-in-comment %find_lang kapow.src:44: W: macro-in-comment %find_lang kapow.src:61: W: macro-in-comment %files kapow.src:61: W: macro-in-comment %{name} Spec updated: http://ankursinha.fedorapeople.org/kapow/kapow.spec http://ankursinha.fedorapeople.org/kapow/kapow-1.4.4.1-1.fc21.src.rpm * Mon Oct 21 2013 Ankur Sinha <ankursinha AT fedoraproject DOT org> 1.4.4.1-1 - Update as per https://bugzilla.redhat.com/show_bug.cgi?id=979767#c6 - Remove comments - Own datadir/name directory - Own icon directories - Add an appdata file [asinha@ankur-laptop SRPMS]$ rpmlint /var/lib/mock/fedora-rawhide-x86_64/result/*.rpm ../SPECS/kapow.spec ./kapow-1.4.4.1-1.fc20.src.rpm kapow.x86_64: W: no-manual-page-for-binary kapow 4 packages and 1 specfiles checked; 0 errors, 1 warnings. [asinha@ankur-laptop SRPMS]$ Thanks, Warm regards, Ankur No, you are doing something wrong: %dir %{_datadir}/icons/hicolor/ %dir %{_datadir}/icons/hicolor/16x16/ %dir %{_datadir}/icons/hicolor/16x16/apps/ %dir %{_datadir}/icons/hicolor/22x22/ %dir %{_datadir}/icons/hicolor/22x22/apps/ %dir %{_datadir}/icons/hicolor/24x24/ %dir %{_datadir}/icons/hicolor/24x24/apps/ %dir %{_datadir}/icons/hicolor/32x32/ %dir %{_datadir}/icons/hicolor/32x32/apps/ %dir %{_datadir}/icons/hicolor/48x48/ %dir %{_datadir}/icons/hicolor/48x48/apps/ %dir %{_datadir}/icons/hicolor/64x64/ %dir %{_datadir}/icons/hicolor/64x64/apps/ %dir %{_datadir}/icons/hicolor/128x128/ %dir %{_datadir}/icons/hicolor/128x128/apps/ %dir %{_datadir}/icons/hicolor/256x256/ %dir %{_datadir}/icons/hicolor/256x256/apps/ %dir %{_datadir}/icons/hicolor/scalable/ %dir %{_datadir}/icons/hicolor/scalable/apps/ These dirs are used by other pkgs, too: [ ]: Package does not own files or directories owned by other packages. Note: Dirs in package are owned also by: /usr/share/icons/hicolor/256x256/apps(hicolor-icon-theme, fedora-logos), /usr/share/icons/hicolor/24x24(hicolor-icon-theme, fedora-logos), /usr/share/icons/hicolor/64x64/apps(hicolor-icon-theme), /usr/share/icons/hicolor/22x22(hicolor-icon-theme, fedora-logos), /usr/share/icons/hicolor/48x48/apps(hicolor-icon-theme, fedora-logos, metromap), /usr/share/icons/hicolor/22x22/apps(hicolor-icon-theme, fedora-logos), /usr/share/icons/hicolor/32x32/apps(hicolor-icon-theme, fedora-logos), /usr/share/icons/hicolor/48x48(hicolor-icon-theme, fedora- logos), /usr/share/icons/hicolor/24x24/apps(hicolor-icon-theme, fedora- logos), /usr/share/icons/hicolor/16x16/apps(hicolor-icon-theme, fedora- logos), /usr/share/icons/hicolor/scalable/apps(hicolor-icon-theme, fedora-logos), /usr/share/icons/hicolor/16x16(hicolor-icon-theme, fedora- logos), /usr/share/icons/hicolor/128x128/apps(hicolor-icon-theme), /usr/share/icons/hicolor/128x128(hicolor-icon-theme), /usr/share/icons/hicolor/64x64(hicolor-icon-theme), /usr/share/appdata (gnome-color-manager, ghex, baobab, gnome-contacts, gnome-calculator, gnome-documents, epiphany, gnome-system-monitor, simple-scan, gedit, filesystem, gnome-boxes), /usr/share/icons/hicolor/256x256(hicolor-icon- theme, fedora-logos), /usr/share/icons/hicolor(hicolor-icon-theme, fedora-logos), /usr/share/icons/hicolor/32x32(hicolor-icon-theme, fedora- logos), /usr/share/icons/hicolor/scalable(hicolor-icon-theme, fedora- logos) My opinion is that you should keep original list but only add these two: /usr/share/kapow, /usr/share/kapow/translations to be owned. Please fix issues above before import. ----------- PACKAGE APPROVED. Forgot to say: Use macro to ldflags: export LDFLAGS="%{__global_ldflags}" (In reply to Christopher Meng from comment #8) > No, you are doing something wrong: > > %dir %{_datadir}/icons/hicolor/ > %dir %{_datadir}/icons/hicolor/16x16/ > %dir %{_datadir}/icons/hicolor/16x16/apps/ > %dir %{_datadir}/icons/hicolor/22x22/ > %dir %{_datadir}/icons/hicolor/22x22/apps/ > %dir %{_datadir}/icons/hicolor/24x24/ > %dir %{_datadir}/icons/hicolor/24x24/apps/ > %dir %{_datadir}/icons/hicolor/32x32/ > %dir %{_datadir}/icons/hicolor/32x32/apps/ > %dir %{_datadir}/icons/hicolor/48x48/ > %dir %{_datadir}/icons/hicolor/48x48/apps/ > %dir %{_datadir}/icons/hicolor/64x64/ > %dir %{_datadir}/icons/hicolor/64x64/apps/ > %dir %{_datadir}/icons/hicolor/128x128/ > %dir %{_datadir}/icons/hicolor/128x128/apps/ > %dir %{_datadir}/icons/hicolor/256x256/ > %dir %{_datadir}/icons/hicolor/256x256/apps/ > %dir %{_datadir}/icons/hicolor/scalable/ > %dir %{_datadir}/icons/hicolor/scalable/apps/ > > These dirs are used by other pkgs, too: > > [ ]: Package does not own files or directories owned by other packages. > Note: Dirs in package are owned also by: > /usr/share/icons/hicolor/256x256/apps(hicolor-icon-theme, fedora-logos), > /usr/share/icons/hicolor/24x24(hicolor-icon-theme, fedora-logos), > /usr/share/icons/hicolor/64x64/apps(hicolor-icon-theme), > /usr/share/icons/hicolor/22x22(hicolor-icon-theme, fedora-logos), > /usr/share/icons/hicolor/48x48/apps(hicolor-icon-theme, fedora-logos, > metromap), /usr/share/icons/hicolor/22x22/apps(hicolor-icon-theme, > fedora-logos), /usr/share/icons/hicolor/32x32/apps(hicolor-icon-theme, > fedora-logos), /usr/share/icons/hicolor/48x48(hicolor-icon-theme, > fedora- > logos), /usr/share/icons/hicolor/24x24/apps(hicolor-icon-theme, fedora- > logos), /usr/share/icons/hicolor/16x16/apps(hicolor-icon-theme, fedora- > logos), /usr/share/icons/hicolor/scalable/apps(hicolor-icon-theme, > fedora-logos), /usr/share/icons/hicolor/16x16(hicolor-icon-theme, > fedora- > logos), /usr/share/icons/hicolor/128x128/apps(hicolor-icon-theme), > /usr/share/icons/hicolor/128x128(hicolor-icon-theme), > /usr/share/icons/hicolor/64x64(hicolor-icon-theme), /usr/share/appdata > (gnome-color-manager, ghex, baobab, gnome-contacts, gnome-calculator, > gnome-documents, epiphany, gnome-system-monitor, simple-scan, gedit, > filesystem, gnome-boxes), /usr/share/icons/hicolor/256x256(hicolor-icon- > theme, fedora-logos), /usr/share/icons/hicolor(hicolor-icon-theme, > fedora-logos), /usr/share/icons/hicolor/32x32(hicolor-icon-theme, > fedora- > logos), /usr/share/icons/hicolor/scalable(hicolor-icon-theme, fedora- > logos) > > My opinion is that you should keep original list but only add these two: > > /usr/share/kapow, /usr/share/kapow/translations > > to be owned. I'm slightly confused about this. I checked up the guidelines: It's OK to co-own packages if the functionality of this package doesn't depend on the other packages owning the same directories. I haven't been able to figure out if kapow will pull in fedora-logos or hicolor in the dep chain. (This is also probably why both fedora-logos and hicolor own the directories, since they aren't dependent on each other) https://fedoraproject.org/wiki/Packaging:Guidelines#The_directory_is_owned_by_a_package_which_is_not_required_for_your_package_to_function I double checked with my other specs, and yes, they don't own these directories. I'll remove them before import. I'll get this clarified for my own understanding later on though. > > Please fix issues above before import. > > ----------- > > PACKAGE APPROVED. (In reply to Christopher Meng from comment #9) > Forgot to say: > > Use macro to ldflags: > > export LDFLAGS="%{__global_ldflags}" Fixed. Thank you for the review. Warm regards, Ankur Updated spec/srpm http://ankursinha.fedorapeople.org/kapow/kapow.spec http://ankursinha.fedorapeople.org/kapow/kapow-1.4.4.1-1.fc21.src.rpm * Tue Oct 22 2013 Ankur Sinha <ankursinha AT fedoraproject DOT org> 1.4.4.1-1 - Correct directory ownership - Correct ld flags - https://bugzilla.redhat.com/show_bug.cgi?id=979767#c8 Making the SCM request now. Thanks again for the review, Warm regards, Ankur New Package SCM Request ======================= Package Name: kapow Short Description: A punch clock program Owners: ankursinha Branches: f19 f20 InitialCC: What Christopher fails to mention is that the "hicolor-icon-theme" is a "-filesystem" type of package. It only contains the basic structure for the hicolor icon theme tree, i.e. no icons but just directories, documentation, and a few support files. That's covered in the yellow box here: https://fedoraproject.org/wiki/Packaging:Guidelines#The_directory_is_owned_by_a_package_which_is_not_required_for_your_package_to_function As such, it is preferred if you "Requires: hicolor-icon-theme" to get right the ownership for those many dirs. The alternative would not be pretty: > %{_datadir}/icons/hicolor/*/apps/%{name}.* Isn't sufficient. At least these would not be included: %{_datadir}/icons/hicolor %{_datadir}/icons/hicolor/* %{_datadir}/icons/hicolor/*/apps (In reply to Michael Schwendt from comment #13) > What Christopher fails to mention is that the "hicolor-icon-theme" is a > "-filesystem" type of package. It only contains the basic structure for the > hicolor icon theme tree, i.e. no icons but just directories, documentation, > and a few support files. > > That's covered in the yellow box here: > https://fedoraproject.org/wiki/Packaging: > Guidelines#The_directory_is_owned_by_a_package_which_is_not_required_for_your > _package_to_function I did read that part. I didn't realize hicolor-icon-theme was a filesystem type package. > > As such, it is preferred if you "Requires: hicolor-icon-theme" to get right > the ownership for those many dirs. The alternative would not be pretty: I've added this to the spec. > > > %{_datadir}/icons/hicolor/*/apps/%{name}.* > > Isn't sufficient. At least these would not be included: > > %{_datadir}/icons/hicolor > %{_datadir}/icons/hicolor/* > %{_datadir}/icons/hicolor/*/apps Thanks for the clarification Michael. Warm regards, Ankur Git done (by process-git-requests). kapow-1.4.4.1-1.fc19 has been submitted as an update for Fedora 19. https://admin.fedoraproject.org/updates/kapow-1.4.4.1-1.fc19 kapow-1.4.4.1-1.fc20 has been submitted as an update for Fedora 20. https://admin.fedoraproject.org/updates/kapow-1.4.4.1-1.fc20 kapow-1.4.4.1-1.fc20 has been pushed to the Fedora 20 testing repository. kapow-1.4.4.1-1.fc19 has been pushed to the Fedora 19 stable repository. kapow-1.4.4.1-1.fc20 has been pushed to the Fedora 20 stable repository. |