Bug 680268 - Review Request: cputnik - Dockapp which displays cpu and memory usage
Summary: Review Request: cputnik - Dockapp which displays cpu and memory usage
Keywords:
Status: CLOSED ERRATA
Alias: None
Product: Fedora
Classification: Fedora
Component: Package Review
Version: rawhide
Hardware: All
OS: Linux
medium
medium
Target Milestone: ---
Assignee: Martin Gieseking
QA Contact: Fedora Extras Quality Assurance
URL:
Whiteboard:
Depends On:
Blocks:
TreeView+ depends on / blocked
 
Reported: 2011-02-24 20:53 UTC by Mario Blättermann
Modified: 2011-03-10 20:51 UTC (History)
4 users (show)

Fixed In Version: cputnik-0.2.0-4.fc14
Doc Type: Bug Fix
Doc Text:
Clone Of:
Environment:
Last Closed: 2011-03-06 03:43:48 UTC
martin.gieseking: fedora-review+
tibbs: fedora-cvs+


Attachments (Terms of Use)

Description Mario Blättermann 2011-02-24 20:53:04 UTC
Spec URL: http://dl.dropbox.com/u/19373040/Fedora/SPECS/cputnik.spec
SRPM URL: http://dl.dropbox.com/u/19373040/Fedora/cputnik-0.2.0-1.fc14.src.rpm
Description: This is small WindowMaker dockapp which displays cpu and memory usage. Fedora lacks of such dockapps, although we ship some window managers which can handle such applets. Time to change this!

Comment 1 Pavel Zhukov 2011-02-25 04:35:07 UTC
Few comments:
- Please read http://fedoraproject.org/wiki/Packaging/Guidelines#Exceptions_2 
You SHOULD NOT point bash to BuildRequirements
- libXpm-devel depends on libx11-devel and libXext-devel.
- If you plan build package for EPEL you must add %clean section. If not, you can remove BuildRoot section: http://fedoraproject.org/wiki/Packaging/Guidelines#BuildRoot_tag



-- 
Fedora Bugzappers volunteer triage team
https://fedoraproject.org/wiki/BugZappers

Comment 2 Martin Gieseking 2011-02-25 08:27:55 UTC
Some additional notes:

- please use macros and variables consistently:
  $RPM_BUILD_ROOT and $RPM_OPT_FLAGS or
  %{buildroot} and %{optflags}

- you can simplify the %build section by using the single line 
  make CFLAGS='%{optflags}' %{?_smp_mflags}
  (cd is not required here, and the given LIB dir doesn't exist in Fedora)

- in the %install section, install the binary with
  install -m 755 %{name} %{buildroot}%{_bindir}

- The 3 additional folders %_iconsdir, %_iconsdir, and %_lconsdir are not used.
  The corresponding installs should therefore be dropped.

- please also set the file permissions of the source tarball to 644

- personally, I'd prefer a German summary like this:
  "Dockapp zur Anzeige von Prozessorlast und Speicherverbrauch"
  Something similar could also be used in the %description to avoid mentioning 
  the grammatical gender of "Dockapp" which is female rather than neuter to my
  feeling. :)

Comment 3 Mario Blättermann 2011-02-25 19:31:08 UTC
(In reply to comment #1)
> Few comments:
> - Please read http://fedoraproject.org/wiki/Packaging/Guidelines#Exceptions_2 
> You SHOULD NOT point bash to BuildRequirements
Removed.
> - libXpm-devel depends on libx11-devel and libXext-devel.
No, mock build fails without libXext-devel.
> - If you plan build package for EPEL you must add %clean section. If not, you
> can remove BuildRoot section:
> http://fedoraproject.org/wiki/Packaging/Guidelines#BuildRoot_tag
Don't know if I should provide a dockapp package for EPEL.


(In reply to comment #2)
> Some additional notes:
> 
> - please use macros and variables consistently:
>   $RPM_BUILD_ROOT and $RPM_OPT_FLAGS or
>   %{buildroot} and %{optflags}
> 
> - you can simplify the %build section by using the single line 
>   make CFLAGS='%{optflags}' %{?_smp_mflags}
>   (cd is not required here, and the given LIB dir doesn't exist in Fedora)
> 
cd is required here, because the Makefile resides in /src.
> - in the %install section, install the binary with
>   install -m 755 %{name} %{buildroot}%{_bindir}
> 
> - The 3 additional folders %_iconsdir, %_iconsdir, and %_lconsdir are not used.
>   The corresponding installs should therefore be dropped.
> 
Removed.
> - please also set the file permissions of the source tarball to 644
> 
Done.
> - personally, I'd prefer a German summary like this:
>   "Dockapp zur Anzeige von Prozessorlast und Speicherverbrauch"
>   Something similar could also be used in the %description to avoid mentioning 
>   the grammatical gender of "Dockapp" which is female rather than neuter to my
>   feeling. :)
I have changed it. However, the modern »apps« as known from the »app stores« are female. But »Dockapp« means »Dockable applet«, and this is not female. Grrr, in German we have thirteen cases of the English »the«, it's sometimes difficult...

Spec URL: http://dl.dropbox.com/u/19373040/Fedora/SPECS/cputnik.spec
SRPM URL: http://dl.dropbox.com/u/19373040/Fedora/cputnik-0.2.0-2.fc14.src.rpm

Comment 4 Mario Blättermann 2011-02-25 19:41:47 UTC
Another question: A dockapp is a graphical application. That's why I have actually to add a *.desktop file to make it available from the menus. But in fact, nobody starts a dockapp from the menu. In almost all cases it will be added to a startup script. What should I do here? The less examples in the repos don't have *.desktop files.

Comment 5 Martin Gieseking 2011-02-25 20:32:34 UTC
(In reply to comment #3)
> cd is required here, because the Makefile resides in /src.

There's also a Makefile in the base directory that calls make for src/. However, both variants work here anyway.
But please shorten the install line as mentioned in comment #2. If you build inside of src/ your probably have to prepend "src/" to %{name}.

> I have changed it. However, the modern »apps« as known from the »app stores«
> are female. But »Dockapp« means »Dockable applet«, and this is not female.

Ah, right. Now that you mention it, I'm convinced. :)

> Grrr, in German we have thirteen cases of the English »the«, it's sometimes
> difficult...

Yes, indeed. Especially, if you have to choose a gender for foreign words...


(In reply to comment #4)
> Another question: A dockapp is a graphical application. That's why I have
> actually to add a *.desktop file to make it available from the menus. 

According to the guidelines, only GUI applications require a .desktop file. Dockapps don't fall into this category to me. The following old thread seems to confirm that:
http://www.redhat.com/archives/fedora-extras-list/2006-October/msg00332.html

Maybe you should ask on the packaging mailing list to get a reconfirmation.

Comment 6 Martin Gieseking 2011-02-25 20:48:34 UTC
Please also drop the BuildRoot field as it's not required any longer either.

Comment 7 Mario Blättermann 2011-02-27 12:45:55 UTC
OK, here we go:
Spec URL: http://dl.dropbox.com/u/19373040/Fedora/SPECS/cputnik.spec
SRPM URL: http://dl.dropbox.com/u/19373040/Fedora/cputnik-0.2.0-3.fc14.src.rpm

With a prepended src/, the install call works now.

Comment 8 Martin Gieseking 2011-02-27 19:31:56 UTC
OK, looks good now. I merely suggest to add articles to the English description: "The configuration ... using a text editor."


$ rpmlint /var/lib/mock/fedora-14-x86_64/result/*.rpm
cputnik.src: W: spelling-error Summary(en_US) Dockapp -> Dock app, Dock-app, Dockage
cputnik.src: W: spelling-error Summary(de) Dockapp -> Dock, Andocke, Documenta
cputnik.src: W: spelling-error %description -l en_US Dockapp -> Dock app, Dock-app, Dockage
cputnik.src: W: spelling-error %description -l en_US cpu -> CPU, cup, cp
cputnik.src: W: spelling-error %description -l en_US rc -> RC, cr, r
cputnik.src: W: spelling-error %description -l de Dockapp -> Dock, Andocke, Documenta
cputnik.src: W: spelling-error %description -l de clay -> Claude, Claudia, clausus
cputnik.src: W: spelling-error %description -l de rc -> RTC, RFC, roch
cputnik.x86_64: W: spelling-error Summary(en_US) Dockapp -> Dock app, Dock-app, Dockage
cputnik.x86_64: W: spelling-error Summary(de) Dockapp -> Dock, Andocke, Documenta
cputnik.x86_64: W: spelling-error %description -l en_US Dockapp -> Dock app, Dock-app, Dockage
cputnik.x86_64: W: spelling-error %description -l en_US cpu -> CPU, cup, cp
cputnik.x86_64: W: spelling-error %description -l en_US rc -> RC, cr, r
cputnik.x86_64: W: spelling-error %description -l de Dockapp -> Dock, Andocke, Documenta
cputnik.x86_64: W: spelling-error %description -l de clay -> Claude, Claudia, clausus
cputnik.x86_64: W: spelling-error %description -l de rc -> RTC, RFC, roch
cputnik.x86_64: W: no-manual-page-for-binary cputnik
3 packages and 0 specfiles checked; 0 errors, 17 warnings.

- The spelling errors are false positive and can be ignored.


---------------------------------
key:

[+] OK
[.] OK, not applicable
[X] needs work
---------------------------------

[+] MUST: The package must be named according to the Package Naming Guidelines.
[+] MUST: The spec file name must match the base package %{name}.
[+] MUST: The package must meet the Packaging Guidelines.
[+] MUST: The package must be licensed with a Fedora approved license.
    - GPLv2+ according to source file headers

[+] MUST: The License field in the package spec file must match the actual license.
[+] MUST: The file containing the text of the license(s) for the package must be included in %doc.
[+] MUST: The spec file must be written in American English.
[+] MUST: The spec file for the package MUST be legible.
[+] MUST: The sources used to build the package must match the upstream source.
    $ md5sum cputnik-0.2.0.tar.gz*
    54df4b3b22a06ac8e0913ee54d121c18  cputnik-0.2.0.tar.gz
    54df4b3b22a06ac8e0913ee54d121c18  cputnik-0.2.0.tar.gz.1

[+] MUST: The package MUST successfully compile and build into binary rpms on at least one primary architecture.
[+] MUST: If the package does not successfully compile, build or work on an architecture, ...
[+] MUST: All build dependencies must be listed in BuildRequires.
[+] MUST: When compiling C, C++, or Fortran files, %{optflags} must be applied.
[.] MUST: The spec file MUST handle locales properly.
[.] MUST: Packages storing shared library files (not just symlinks) must call ldconfig in %post and %postun.
[+] MUST: Packages must NOT bundle copies of system libraries.
[.] MUST: If the package is designed to be relocatable, ...
[+] MUST: A package must own all directories that it creates. 
[+] MUST: A Fedora package must not list a file more than once in %files.
[+] MUST: Permissions on files must be set properly.
[+] MUST: Each package must consistently use macros.
[+] MUST: The package must contain code, or permissable content.
[.] MUST: Large documentation files must go in a -doc subpackage.
[+] MUST: Files in %doc must not affect the runtime of the application.
[.] MUST: Header files must be in a -devel package.
[.] MUST: Static libraries must be in a -static package.
[.] MUST: If a package contains library files with a suffix (e.g. libfoo.so.1.1), ...
[.] MUST: devel packages must require the base package using a fully versioned dependency.
[.] MUST: Packages must NOT contain any .la libtool archives.
[.] MUST: Packages containing GUI applications must include a %{name}.desktop file
    - Dockapps are no GUI applications and thus don't require a .desktop file

[+] MUST: Packages must not own files or directories already owned by other packages.
[+] MUST: All filenames in rpm packages must be valid UTF-8.

[.] SHOULD: If the source package does not include license text(s) as a separate file from upstream, ...
[+] SHOULD: The reviewer should test that the package builds in mock.
[+] SHOULD: The package should compile and build into binary rpms on all supported architectures.
[+] SHOULD: The reviewer should test that the package functions as described.
[.] SHOULD: If scriptlets are used, those scriptlets must be sane.
[.] SHOULD: Usually, subpackages other than devel should require the base package using a fully versioned dependency.
[.] SHOULD: pkgconfig(.pc) files should be placed in a -devel pkg.
[.] SHOULD: If the package has file dependencies outside of /etc, /bin, /sbin, /usr/bin, or /usr/sbin ...
[.] SHOULD: Your package should contain man pages for binaries/scripts.


----------------
Package APPROVED
----------------

Comment 9 Mario Blättermann 2011-02-27 19:48:54 UTC
I've corrected the description.

Spec URL: http://dl.dropbox.com/u/19373040/Fedora/SPECS/cputnik.spec
SRPM URL: http://dl.dropbox.com/u/19373040/Fedora/cputnik-0.2.0-4.fc14.src.rpm

Comment 10 Martin Gieseking 2011-02-27 19:57:56 UTC
OK, fine. As the package is already approved, you can also request the desired Git branches.

Comment 11 Mario Blättermann 2011-02-27 20:00:02 UTC
New Package SCM Request
=======================
Package Name: cputnik
Short Description: Dockapp which displays CPU and memory usage
Owners: mariobl
Branches: f14 f15

Comment 12 Martin Gieseking 2011-02-27 20:22:20 UTC
Please don't forget to set the fedora-cvs flag to "?".

Comment 13 Mario Blättermann 2011-02-28 18:30:26 UTC
(In reply to comment #12)
> Please don't forget to set the fedora-cvs flag to "?".

Done. Thanks for the hint.

Comment 14 Jason Tibbitts 2011-03-01 14:54:42 UTC
Git done (by process-git-requests).

Comment 15 Fedora Update System 2011-03-01 17:25:14 UTC
Package cputnik-0.2.0-4.fc15:
* should fix your issue,
* was pushed to the Fedora 15 updates-testing repository,
* should be available at your local mirror within two days.
Update it with:
# su -c 'yum update --enablerepo=updates-testing cputnik-0.2.0-4.fc15'
as soon as you are able to, then reboot.
Please go to the following url:
https://admin.fedoraproject.org/updates/cputnik-0.2.0-4.fc15
then log in and leave karma (feedback).

Comment 16 Fedora Update System 2011-03-01 17:25:25 UTC
Package cputnik-0.2.0-4.fc14:
* should fix your issue,
* was pushed to the Fedora 14 updates-testing repository,
* should be available at your local mirror within two days.
Update it with:
# su -c 'yum update --enablerepo=updates-testing cputnik-0.2.0-4.fc14'
as soon as you are able to, then reboot.
Please go to the following url:
https://admin.fedoraproject.org/updates/cputnik-0.2.0-4.fc14
then log in and leave karma (feedback).

Comment 17 Fedora Update System 2011-03-01 21:36:47 UTC
cputnik-0.2.0-4.fc15 has been pushed to the Fedora 15 testing repository.  If problems still persist, please make note of it in this bug report.
 If you want to test the update, you can install it with 
 su -c 'yum --enablerepo=updates-testing update cputnik'.  You can provide feedback for this update here: https://admin.fedoraproject.org/updates/cputnik-0.2.0-4.fc15

Comment 18 Fedora Update System 2011-03-06 03:43:40 UTC
cputnik-0.2.0-4.fc15 has been pushed to the Fedora 15 stable repository.  If problems still persist, please make note of it in this bug report.

Comment 19 Fedora Update System 2011-03-10 20:51:02 UTC
cputnik-0.2.0-4.fc14 has been pushed to the Fedora 14 stable repository.  If problems still persist, please make note of it in this bug report.


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