Bug 979767 - (kapow) Review Request: kapow - A punch clock program
Review Request: kapow - A punch clock program
Status: CLOSED ERRATA
Product: Fedora
Classification: Fedora
Component: Package Review (Show other bugs)
rawhide
All Linux
medium Severity medium
: ---
: ---
Assigned To: Christopher Meng
Fedora Extras Quality Assurance
:
Depends On:
Blocks:
  Show dependency treegraph
 
Reported: 2013-06-30 07:51 EDT by Ankur Sinha (FranciscoD)
Modified: 2013-11-10 02:24 EST (History)
3 users (show)

See Also:
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-30 23:05:12 EDT
Type: ---
Regression: ---
Mount Type: ---
Documentation: ---
CRM:
Verified Versions:
Category: ---
oVirt Team: ---
RHEL 7.3 requirements from Atomic Host:
Cloudforms Team: ---
i: fedora‑review+
limburgher: fedora‑cvs+


Attachments (Terms of Use)

  None (edit)
Description Ankur Sinha (FranciscoD) 2013-06-30 07:51:28 EDT
Spec URL: http://ankursinha.fedorapeople.org/kapow/kapow.spec
SRPM URL: http://ankursinha.fedorapeople.org/kapow/kapow-1.4.4.1-1.fc19.src.rpm

Description: 
Kapow is a punch clock program designed to easily keep track of your hours,
whether you're working on one project or many. Simply clock in and out with the
Start/Stop button. If you make a mistake in your hours, you can go back and
edit any of the entries by double-clicking on the session in question. Kapow
also allows you to easily keep track of the hours since you last billed a
client, by providing a helpful "Billed" check box--the totals will reflect your
work after the last billed session. 


Fedora Account System Username: ankursinha


rpmlint issues:
[asinha@localhost  SRPMS]$ rpmlint ../SPECS/kapow.spec ./kapow-1.4.4.1-1.fc19.src.rpm /var/lib/mock/fedora-19-x86_64/result/*.rpm
../SPECS/kapow.spec:49: W: macro-in-comment %find_lang
../SPECS/kapow.spec:50: W: macro-in-comment %find_lang
../SPECS/kapow.spec:66: W: macro-in-comment %files
../SPECS/kapow.spec:66: W: macro-in-comment %{name}
../SPECS/kapow.spec: W: invalid-url Source0: kapow-1.4.4.1-src.tar.bz2
kapow.src: W: spelling-error %description -l en_US checkbox -> check box, check-box, checkbook
kapow.src:49: W: macro-in-comment %find_lang
kapow.src:50: W: macro-in-comment %find_lang
kapow.src:66: W: macro-in-comment %files
kapow.src:66: W: macro-in-comment %{name}
kapow.src: W: invalid-url Source0: kapow-1.4.4.1-src.tar.bz2
kapow.src: W: spelling-error %description -l en_US checkbox -> check box, check-box, checkbook
kapow.src:49: W: macro-in-comment %find_lang
kapow.src:50: W: macro-in-comment %find_lang
kapow.src:66: W: macro-in-comment %files
kapow.src:66: W: macro-in-comment %{name}
kapow.src: W: invalid-url Source0: kapow-1.4.4.1-src.tar.bz2
kapow.x86_64: W: spelling-error %description -l en_US checkbox -> check box, check-box, checkbook
kapow.x86_64: W: no-manual-page-for-binary kapow
4 packages and 1 specfiles checked; 0 errors, 19 warnings.
[asinha@localhost  SRPMS]$


Other rpms, for rawhide and F19 are available at http://ankursinha.fedorapeople.org/kapow/
Comment 1 Christopher Meng 2013-06-30 08:59:46 EDT
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.
Comment 2 Ankur Sinha (FranciscoD) 2013-06-30 09:52:21 EDT
(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
Comment 3 Christopher Meng 2013-07-21 22:12:41 EDT
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?
Comment 4 Christopher Meng 2013-10-20 23:13:55 EDT
URL 404.
Comment 5 Ankur Sinha (FranciscoD) 2013-10-20 23:49:14 EDT
(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
Comment 6 Christopher Meng 2013-10-21 01:17:49 EDT
[?]: 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}
Comment 7 Ankur Sinha (FranciscoD) 2013-10-21 05:01:51 EDT
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
Comment 8 Christopher Meng 2013-10-21 06:12:10 EDT
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.
Comment 9 Christopher Meng 2013-10-21 06:14:20 EDT
Forgot to say:

Use macro to ldflags:

export LDFLAGS="%{__global_ldflags}"
Comment 10 Ankur Sinha (FranciscoD) 2013-10-21 20:34:20 EDT
(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
Comment 11 Ankur Sinha (FranciscoD) 2013-10-21 20:54:13 EDT
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
Comment 12 Ankur Sinha (FranciscoD) 2013-10-21 20:55:04 EDT
New Package SCM Request
=======================
Package Name: kapow
Short Description: A punch clock program
Owners: ankursinha
Branches: f19 f20
InitialCC:
Comment 13 Michael Schwendt 2013-10-22 03:39:59 EDT
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
Comment 14 Ankur Sinha (FranciscoD) 2013-10-22 07:43:06 EDT
(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
Comment 15 Gwyn Ciesla 2013-10-22 07:44:10 EDT
Git done (by process-git-requests).
Comment 16 Fedora Update System 2013-10-22 09:09:05 EDT
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
Comment 17 Fedora Update System 2013-10-22 09:09:19 EDT
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
Comment 18 Fedora Update System 2013-10-22 14:52:01 EDT
kapow-1.4.4.1-1.fc20 has been pushed to the Fedora 20 testing repository.
Comment 19 Fedora Update System 2013-10-30 23:05:12 EDT
kapow-1.4.4.1-1.fc19 has been pushed to the Fedora 19 stable repository.
Comment 20 Fedora Update System 2013-11-10 02:24:04 EST
kapow-1.4.4.1-1.fc20 has been pushed to the Fedora 20 stable repository.

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