Bug 915337 - (nmon) Review Request: nmon - Nigel's performance MONitor for Linux
Review Request: nmon - Nigel's performance MONitor for Linux
Status: CLOSED ERRATA
Product: Fedora
Classification: Fedora
Component: Package Review (Show other bugs)
rawhide
All Linux
unspecified Severity medium
: ---
: ---
Assigned To: Hans de Goede
Fedora Extras Quality Assurance
:
: 524119 (view as bug list)
Depends On:
Blocks:
  Show dependency treegraph
 
Reported: 2013-02-25 09:42 EST by Palle Ravn
Modified: 2014-12-23 13:53 EST (History)
9 users (show)

See Also:
Fixed In Version:
Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of: 524119
Environment:
Last Closed: 2013-04-01 04:03:07 EDT
Type: ---
Regression: ---
Mount Type: ---
Documentation: ---
CRM:
Verified Versions:
Category: ---
oVirt Team: ---
RHEL 7.3 requirements from Atomic Host:
hdegoede: fedora‑review+
limburgher: fedora‑cvs+


Attachments (Terms of Use)

  None (edit)
Description Palle Ravn 2013-02-25 09:42:57 EST
Spec URL: https://docs.google.com/file/d/0Bws3TZd4V12pejI2QXdxa2NTUzA/edit?usp=sharing
SRPM URL: https://docs.google.com/file/d/0Bws3TZd4V12pY2RJcDRiaFV6WkE/edit?usp=sharing

Description: This systems administrator, tuner, benchmark tool gives you a huge amount of performance information about CPU, memory, network, disks (mini graphs or numbers), file systems, NFS, top processes, resources (Linux version & processors), in one go. The information can be displayed either on screen or saved to a comma separated (csv) file.


rpmlint:
nmon.src: W: file-size-mismatch Documentation.txt = 255, http://downloads.sf.net/project/nmon/Documentation.txt = 262
3 packages and 1 specfiles checked; 0 errors, 1 warnings.


Koji build:
http://koji.fedoraproject.org/koji/taskinfo?taskID=5053091

Fedora Account System Username: paller

This is my first package and I am seeking a sponsor.
Comment 1 Terje Røsten 2013-02-25 10:37:38 EST
*** Bug 524119 has been marked as a duplicate of this bug. ***
Comment 2 Palle Ravn 2013-02-27 15:34:19 EST
Files moved to better download location.

Spec URL: http://nmon.zom.dk/nmon.spec
SRPM URL: http://nmon.zom.dk/nmon-14g-1.fc18.src.rpm
Comment 3 Volker Fröhlich 2013-03-01 15:32:10 EST
-O2 and -Wall are already set in optflags and can therefore be removed. To do the PPC guys a favor, you could include a conditional clause to add "-D POWER", if built on PPC.

Make it ...%{name}.1* to allow for changes possible changes in compression.

I think you should use the name macro on the gcc and install invocation.

The timestamp of Source1 should be preserved.
Comment 4 Palle Ravn 2013-03-02 03:21:35 EST
Package updated

Spec URL: http://nmon.zom.dk/nmon.spec
SRPM URL: http://nmon.zom.dk/nmon-14g-2.fc18.src.rpm

Koji: http://koji.fedoraproject.org/koji/taskinfo?taskID=5069343

rpmlint: 3 packages and 1 specfiles checked; 0 errors, 0 warnings.

(In reply to comment #3)
> -O2 and -Wall are already set in optflags and can therefore be removed. To
> do the PPC guys a favor, you could include a conditional clause to add "-D
> POWER", if built on PPC.

-O2 and -Wall removed. I have added the architecture conditional %ifarch for ppc and ppc64. I am using the %{power64} macro, but not sure if that is more correct than using ppc64?
I have tested the build with ppc-koji and the "-D POWER" parameter is invoked for both ppc and ppc64, see http://ppc.koji.fedoraproject.org/koji/taskinfo?taskID=962445.

> Make it ...%{name}.1* to allow for changes possible changes in compression.

Done.

> I think you should use the name macro on the gcc and install invocation.

That seems reasonable, done.
 
> The timestamp of Source1 should be preserved.

I didn't understand this at first, as the downloaded file had preserved the timestamp from the server. I assume that the timestamp should not change duo to the linebreak fix using sed? That is now corrected with touch, so Source1 is now dated correctly in the /usr/share/doc/... folder.
Comment 5 Hans de Goede 2013-03-16 12:39:56 EDT
Hi Palle, Volker,

Volker, thanks for your initial review of this packages.

Palle I think that it is great that you're working on the Fedora Design Packages WishList, as such I would like to Sponsor you.

The first step is me reviewing this and your other package submission, then you do a new revision (assuming I'll have some remarks on my first reviews), and I will review the new revision. Rince-repeat until both packages you've submitted are approved.

And then once I'm happy with both packages I'll sponsor you and you can continue with the next steps of:
https://fedoraproject.org/wiki/Join_the_package_collection_maintainers

I'll do a full review of the version you posted in comment 4 later tonight, and let you know what, if anything, needs to be fixed.

Regards,

Hans
Comment 6 Hans de Goede 2013-03-16 15:26:02 EDT
Hi,

Full review done:

Good:
- rpmlint checks return:
  3 packages and 1 specfiles checked; 0 errors, 0 warnings.
- package meets naming guidelines (but not versioning, see below)
- package meets packaging guidelines
- license (GPLv3) OK, but text not in %doc, matches source
- spec file legible, in am. english
- source matches upstream
- package compiles on devel (x86)
- no missing BR
- no unnecessary BR
- no locales
- not relocatable
- owns all directories that it creates
- no duplicate files
- permissions ok
- macro use consistent
- code, not content
- no need for -docs
- nothing in %doc affects runtime
- no need for .desktop file

Needs work:
-The Source0 and Source1 urls are wrong, we've a preferred down url form for sf.net, see:
 http://fedoraproject.org/wiki/Packaging:SourceURL#Sourceforge.net
-The generated manpage is rubish, it contains the same bit twice, and then a reference to the non-existent texinfo documentation. Luckily Debian has already packaged nmon and provides a manpage for us :)  See: ftp://ftp.nluug.nl/pub/os/Linux/distr/debian/pool/main/n/nmon/nmon_13g+debian-1.debian.tar.gz
-Drop the now no longer needed "BuildRequires:  help2man"
-The lmon14g.c file does not contain a clear copyright-header, and upstream does not provide a copy of the GPLv3 (typically upstream provides a COPYING.txt), this is not a blocker, but please mail upstream asking them to do a new release with a proper copyright-header, and while they are at it they should add
a COPYING.txt with the GPLv3 and put all the files together in a tarbal. As said this is not a blocker for getting this package into Fedora, but you *MUST* mail upstream asking them to do better for their next release. While mailing upstream, please attach the Debian manpage and ask them to add that to the tarbal too.
-manpages should not be marked %doc

Regards,

Hans
Comment 7 Palle Ravn 2013-03-17 20:04:12 EDT
(In reply to comment #6)
> Full review done:
> 
> Good:
> - ...
> - package meets naming guidelines (but not versioning, see below)

I'm looking below and I don't see more about the versioning, can you clarify this for me?

> Needs work:
> -The Source0 and Source1 urls are wrong, we've a preferred down url form for
> sf.net, see:

Now following the guidelines.

>  http://fedoraproject.org/wiki/Packaging:SourceURL#Sourceforge.net
> -The generated manpage is rubish, it contains the same bit twice, and then a
> reference to the non-existent texinfo documentation. Luckily Debian has
> already packaged nmon and provides a manpage for us :)  See:
> ftp://ftp.nluug.nl/pub/os/Linux/distr/debian/pool/main/n/nmon/
> nmon_13g+debian-1.debian.tar.gz

The Debian manpage has also been committed on sourceforge under patches, so I downloaded that one. However, I'm not able to follow the sourceforge guidelines for this link.

There's a second manpage, which covers more of the documentation, and I would like to use that one, but I has some unwanted references to a screenshot list in its intro. I'm considering rewriting it a bit and then use it, what are your thoughts on that?

link: http://sourceforge.net/tracker/?func=detail&aid=3295760&group_id=271307&atid=1153693

> -Drop the now no longer needed "BuildRequires:  help2man"

Now using install -D -p -m 0655 for the nmon.1 manpage.

> ... you
> *MUST* mail upstream asking them to do better for their next release. While
> mailing upstream, please attach the Debian manpage and ask them to add that
> to the tarbal too.

I have mailed Nigel using sourceforges mailing system, as I'm unable to find any other contact information, and asked for inclusion of license file and header, manpage, and tarball releases. The projects mailing list has a total of 2 messages, from the same person as he never received an answer, I'm guessing nobody reads it.

I could make feature requests for the above, but I'm not a fan of that, as it has nothing to do with the functionality of the program. 

> -manpages should not be marked %doc

Removed %doc.

New files
Spec URL: http://nmon.zom.dk/nmon.spec
SRPM URL: http://nmon.zom.dk/nmon-14g-3.fc18.src.rpm

rpmlint:
nmon.x86_64: W: unstripped-binary-or-object /usr/bin/nmon
2 packages and 1 specfiles checked; 0 errors, 1 warnings.

koji scratch build:
http://koji.fedoraproject.org/koji/taskinfo?taskID=5135115
PowerPC
http://ppc.koji.fedoraproject.org/koji/taskinfo?taskID=998047
Comment 8 Hans de Goede 2013-03-18 09:14:23 EDT
Hi,

(In reply to comment #7)
> (In reply to comment #6)
> > Full review done:
> > 
> > Good:
> > - ...
> > - package meets naming guidelines (but not versioning, see below)
> 
> I'm looking below and I don't see more about the versioning, can you clarify
> this for me?

My bad, usually we only allow numeric chars in Version:, so I was planning to point this out and point you to:
http://fedoraproject.org/wiki/Packaging:NamingGuidelines#Non-Numeric_Version_in_Release

But while reading that myself I noticed that there is one exception to the only numeric chars in the Version field, and this package matches that exception, so its use I fine,

Then I removed the comment on this, but not the referral to the comment you just quoted.

> > -The generated manpage is rubish, it contains the same bit twice, and then a
> > reference to the non-existent texinfo documentation. Luckily Debian has
> > already packaged nmon and provides a manpage for us :)  See:
> > ftp://ftp.nluug.nl/pub/os/Linux/distr/debian/pool/main/n/nmon/
> > nmon_13g+debian-1.debian.tar.gz
> 
> The Debian manpage has also been committed on sourceforge under patches, so
> I downloaded that one. However, I'm not able to follow the sourceforge
> guidelines for this link.
> 
> There's a second manpage, which covers more of the documentation, and I
> would like to use that one, but I has some unwanted references to a
> screenshot list in its intro. I'm considering rewriting it a bit and then
> use it, what are your thoughts on that?
> 
> link:
> http://sourceforge.net/tracker/
> ?func=detail&aid=3295760&group_id=271307&atid=1153693

That is fine. But please change the LICENSE section from "under  the  terms  of  the GNU General Public License as published by the Free Software Foundation; either version 3  of  the  License,  or  (at  your option) any later version."

To:

"under  the  terms  of  the GNU General Public License as published by the Free Software Foundation; version 3"

IOW drop the "either" and the ", or  (at  your option) any later version.", as the only license statement we've is the one from Documentation.txt which does not contain the or later version language.

You should then also submit the updated manpage upstream (not sure how useful that is in this case, but as a rule we always try to send all changes upstream in Fedora).

> 
> > -Drop the now no longer needed "BuildRequires:  help2man"
> 
> Now using install -D -p -m 0655 for the nmon.1 manpage.

That should be -m 0644 which I see you've gotten right in the specfile :)

> > ... you
> > *MUST* mail upstream asking them to do better for their next release. While
> > mailing upstream, please attach the Debian manpage and ask them to add that
> > to the tarbal too.
> 
> I have mailed Nigel using sourceforges mailing system, as I'm unable to find
> any other contact information, and asked for inclusion of license file and
> header, manpage, and tarball releases. 

Great, thanks. I know this rule may seem a bit stupid with upstreams which appear dead. But it is a good rule in general, and in my experience with dead upstreams sometimes they surprise you by not being dead after all.

> The projects mailing list has a total
> of 2 messages, from the same person as he never received an answer, I'm
> guessing nobody reads it.
> 
> I could make feature requests for the above, but I'm not a fan of that, as
> it has nothing to do with the functionality of the program. 

No need to file an RFE, if a direct mail won't do the trick usually nothing will.

> New files
> Spec URL: http://nmon.zom.dk/nmon.spec
> SRPM URL: http://nmon.zom.dk/nmon-14g-3.fc18.src.rpm

Looks good: approved!

So as promised I've added you to the packager group and sponsored you, so you can now move forward with:
https://fedoraproject.org/wiki/Join_the_package_collection_maintainers#Add_Package_to_Source_Code_Management_.28SCM.29_system_and_Set_Owner

2 minor nitpicks to further improve the package:

1) In the ChangeLog you write:
- Remove manpage from doc
This may lead to users thinking the manpage was removed. If I were you I would've written this instead:
- No longer mark manpage as %%doc

Note the %%, whenever you use % in the changelog (or in comments) you need to write %%

2) In %prep you do:
cp %{SOURCE2} .
and then in %install:
install -D -p -m 0644 %{name}.1 %{buildroot}%{_mandir}/man1/%{name}.1

Instead you can just do the following in %install, without anything in %prep:
install -D -p -m 0644 %{SOURCE2}  %{buildroot}%{_mandir}/man1/%{name}.1

Regards,

Hans
Comment 9 Palle Ravn 2013-03-18 14:35:15 EDT
New Package SCM Request
=======================
Package Name: nmon
Short Description: Nigel's performance Monitor for Linux
Owners: paller
Branches: f17 f18 f19
InitialCC:
Comment 10 Jon Ciesla 2013-03-18 14:49:03 EDT
Git done (by process-git-requests).
Comment 11 Fedora Update System 2013-03-19 06:08:55 EDT
nmon-14g-3.fc18 has been submitted as an update for Fedora 18.
https://admin.fedoraproject.org/updates/nmon-14g-3.fc18
Comment 12 Fedora Update System 2013-03-19 06:11:54 EDT
nmon-14g-3.fc17 has been submitted as an update for Fedora 17.
https://admin.fedoraproject.org/updates/nmon-14g-3.fc17
Comment 13 Fedora Update System 2013-03-20 17:41:48 EDT
nmon-14g-3.fc18 has been pushed to the Fedora 18 testing repository.
Comment 14 Athmane Madjoudj 2014-12-23 10:36:08 EST
Package Change Request
======================
Package Name: nmon
New Branches: el5 el6 epel7
Owners: athmane
Comment 15 Jon Ciesla 2014-12-23 13:53:34 EST
Git done (by process-git-requests).

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