Bug 199780 - Review Request: dstat
Review Request: dstat
Status: CLOSED CURRENTRELEASE
Product: Fedora
Classification: Fedora
Component: Package Review (Show other bugs)
rawhide
All Linux
medium Severity medium
: ---
: ---
Assigned To: Toshio Kuratomi
Fedora Package Reviews List
:
: 462034 (view as bug list)
Depends On:
Blocks: FE-ACCEPT
  Show dependency treegraph
 
Reported: 2006-07-21 17:46 EDT by Scott Baker
Modified: 2008-09-12 01:28 EDT (History)
3 users (show)

See Also:
Fixed In Version: 0.6.3-5
Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of:
Environment:
Last Closed: 2006-09-21 17:55:09 EDT
Type: ---
Regression: ---
Mount Type: ---
Documentation: ---
CRM:
Verified Versions:
Category: ---
oVirt Team: ---
RHEL 7.3 requirements from Atomic Host:
Cloudforms Team: ---


Attachments (Terms of Use)

  None (edit)
Description Scott Baker 2006-07-21 17:46:52 EDT
Spec URL: http://www.perturb.org/tmp/dstat-fedora.spec
SRPM URL: http://www.perturb.org/tmp/dstat-0.6.3-2.src.rpm
Description: Versatile resource statistics tool
Comment 1 Toshio Kuratomi 2006-07-21 18:00:04 EDT
Okay.  To start, a few links:

[1]_ General Guidelines the package must follow:
  http://www.fedoraproject.org/wiki/Packaging/Guidelines
[2]_ Python Specific Packaging Hints:
  http://www.fedoraproject.org/wiki/Packaging/Python
[3]_ Fedora Extras CVS.  Look for examples of how other people do things here:
  http://cvs.fedora.redhat.com/viewcvs/rpms/?root=extras

Some python applications you can look through there are qa-assistant, rpmlint,
and gramps.

The first thing that leaps out at me when I look at the spec is that the python
files aren't bytecompiled.  In addition to installing, you want to generate the
.pyc and .pyo byte compiled files and install them.  That way the package will
know that it needs to remove the files as well.

[1] and [3] should help you get through that.  Looking over a spec or two from
[3] and comparing to your rpm would be another good thing to do.

(And now you can jump back on IRC and ask specific questions and we'll be better
able to help ;-)
Comment 2 Devrim GUNDUZ 2006-07-21 18:09:22 EDT
The spec file should be dstat.spec, not dstat-fedora.spec. Also setup needs -q .
Comment 3 Scott Baker 2006-07-21 18:40:37 EDT
Added setup -q and renamed spec file to dstat.spec

http://www.perturb.org/tmp/dstat.spec

I will look into the python compiling to bytecode etc.
Comment 4 Scott Baker 2006-07-21 19:29:47 EDT
Updated the spec and the src RPM to bytecode compile the python.

Spec URL: http://www.perturb.org/tmp/dstat.spec
SRPM URL: http://www.perturb.org/tmp/dstat-0.6.3-2.src.rpm
Comment 5 Scott Baker 2006-07-24 11:34:02 EDT
What's the next step in getting this reviewed?
Comment 6 Paul F. Johnson 2006-07-24 11:39:13 EDT
Whoever is assigned to this bug reviews the fixes you've made, test builds, runs
rpmlint over the packages created, compiles in mock, installs and runs rpmlint
over the installed package to ensure nothing untoward happens.

This isn't an instant process - some bugs can be open for ages!
Comment 7 Paul F. Johnson 2006-07-24 11:45:46 EDT
Looking at the spec file you've got there, I can't see anything which bytecode
compiles the python. Remember a # before anything is just counted as a comment
and rpmbuild skips past it as if it wasn't there.

A couple of other niggles

In the Source: you have .../dstat/dstat-%{version}...

This can be substituted with .../%{name}/%{name}-%{version}. I know on packages
I've had reviewed on, this has been brought up.

The make install line can just be

make DESTDIR=%{buildroot} install - you don't need the quotes around the buildroot

Does the package come with any language translation files?
Comment 8 Toshio Kuratomi 2006-07-24 13:55:21 EDT
Quick note as I start reviewing:

You should always bump the release number, even when you are making new spec
files for review.  This lets me easily reference the -2 release as having
Problem A) and the -3 release fixed that but introduced problem B).  It makes it
easier to figure out what problems are still outstanding against a package and
which have been fixed.  Thanks.
Comment 9 Toshio Kuratomi 2006-07-25 15:47:52 EDT
MD5sums:
72917aa5eed385464d70ec731bd6d2b1  dstat-0.6.3-2.src.rpm
a2df5d7fecc0115f8eef84141a068e86  dstat-0.6.3.tar.bz2

Blocker:
* Your patch adds #!/usr/bin/python to the modules files.  Since the modules are
 not intended to be run from the command line, this is improper behaviour.  You
 should make the files non-executable instead.
* It's customary to put the defattr line first in the files section.  I believe
  that rpm uses the defattr line even on lines that preceed it in the file
  listings but I don't know if that's documented behaviour.  If that behaviour
  ever changes, then you could suddenly have files with incorrect ownership and
  permissions.
* Remove the execute permissions from the scripts in the examples directory.
* Package needs to own the dstat directory like this:
  %dir %{_datadir}/dstat

Cosmetic:
* It looks like you're patching out dos line endings for html files.  This is
  okay if you're going to submit the change upstream.  As a general rule (for
  instance, if upstream were to not accept the change and you had to continue
  to carry it around in the package) it's better to use sed or dos2unix to fix
  this.  Otherwise your patch becomes filled with whole files where the only
  difference is the EOL character.
* Some people still use the default rpm defined topdir, sourcedir, etc.  When
  this is so, installing the source rpm places the files in the same directory
  as a multitude of other rpms.  So your patch: patch-to-clean-for-fedora.patch
  should really have a less generic name.  Something like dstat-eol.patch or
  dstat-eol-cleanup.pach.
* I'd remove the commented out code as it doesn't add any value to the spec.
* You don't need to Require: python; this is being picked up automatically.

Good:
* Source matches upstream.
* Package Name follows the Naming Guidelines.
* Spec file follows the %{name}.spec format.
* License is GPL, matches the license field, and is included in the rpm.
* Package built on x86_64 as a noarch package.
* All BuildRequires not listed in the exceptions have been satisfied.
* No locale files, so no need to use %find_lang.
* No libraries or pkgconfig files.
* Package is not relocatable.
* No duplicate files.
* Package has a proper %clean section.
* Package uses proper macros from Packaging Guidelines.
* Code, not content.
* No large doc files.
* Doc files do not affect the runtime behaviour.
* Not a GUI application.
* Does not own directories that belong to another package.
* No scriptlets.
* Package built in mock.

After you fix the listed issues, I can run a last rpmlint check and make sure
the program runs and then APPROVE the package.

Since you need a sponsor, I need to know that you understand the guidelines
before SPONSORing you.  This package was a good indication of understanding on
your part.  If you do a review of someone else's package that continues to
demonstrate your knowledge of the Fedora Guidelines in particular and rpm
packaging in general, I'll sponsor you as well.
Comment 10 Scott Baker 2006-07-25 17:24:44 EDT
Ok I bumped the release number to 3, and re-uploaded it. I removed my patch, and
did the work in the spec file. Sed is used to convert the documentation line
endings, and I chmoded the .py as requested. Moved defattr to the top, and added
the %dir lines. Commented out code was removed as was the require python.

rpmlint is only giving out warnings about the line endings as well as the a
stale symlink. I've done my best to handle that in the .spec file (see the rm
and sed lines) but it's just not working. I'm over my head.

SPEC: http://www.perturb.org/tmp/dstat.spec
SRC: http://www.perturb.org/tmp/dstat-0.6.3-3.src.rpm
Comment 11 Toshio Kuratomi 2006-07-25 19:01:08 EDT
* The files don't get moved into _docdir until after %install.  So instead of
operating on %{buildroot}/%{_docdir}/* you want to operate on the files in your
source tree.

* You've commented out the %clean section.  You'll want to put that back.
* There are other pieces of commented code that could be removed:
  #BuildRequires:..
  #Requires:....
  #/usr/lib/rpm/brp-python-bytecompile....
  #%config(noreplace) %{_sysconfdir}/dstat.conf....

You can leave them in with explanations if you think they make the spec clearer
and easier to understand (I see that you have commented on two of those listed
already.  The choice is yours.)

* For every release bump, there must also be a changelog entry.  So instead of
just bumping the release number on the changelog line, add a whole new entry
that tells what you've changed.
Comment 12 Scott Baker 2006-07-31 13:19:05 EDT
Oh lame... I posted last week and it didn't make it.

Spec: http://www.perturb.org/tmp/dstat.spec
SRPM: http://www.perturb.org/tmp/dstat-0.6.3-4.src.rpm

- Removed some commeted lines in the .spec file that weren't needed
- Changed the permissions on the examples/* scripts
- Converted the HTML documentation to unix line endings
- Removed the erroneous commenting of the %clean section
Comment 13 Toshio Kuratomi 2006-08-05 14:33:00 EDT
MD5sums:
ed346ba8fb71514e5be2202d50a17568  ../dstat-0.6.3-4.src.rpm
a2df5d7fecc0115f8eef84141a068e86  dstat-0.6.3.tar.bz2
0862411cacd97a31182dd2b0199c512f  dstat.spec

Most previously noted problems fixed.  Here's what remains:
* Remove the execute permissions from the scripts in the examples directory.
* macros (Like %clean) are expanded even when they're in the changelog section.
  You'll want to escape the macro in your changelog with an additional "%"::
  - Removed the erroneous commenting of the %%clean section

After you fix these issues, I can do a last check and APPROVE the package.
You will also need to be sponsored so you can import the package into the
repository.  I am willing to do this if you show your understanding of the
packaging guidelines.  This is best done by reviewing someone else's package
according to the packaging guidelines and lettting me know so I can watch the
process.  You can refer to these two documents for help::

http://www.fedoraproject.org/wiki/Extras/HowToGetSponsored
http://www.fedoraproject.org/wiki/Packaging/ReviewGuidelines
Comment 14 Scott Baker 2006-08-11 17:00:47 EDT
SPEC: http://www.perturb.org/tmp/dstat.spec
SRPM: http://www.perturb.org/tmp/dstat-0.6.3-5.src.rpm

I fixed the above mentioned things. rpmlint is clean on both the srpm and the
binary rpm. I don't know how much time I'll have to review another project
however. Plus I'm not sure I have the expertise.
Comment 15 Toshio Kuratomi 2006-08-17 12:10:13 EDT
e3bc24955e52a78166ff297f1a5e14ee  dstat-0.6.3-5.src.rpm
f833f3e0f8bb34ed50bf1388b7281cb7  dstat.spec
a2df5d7fecc0115f8eef84141a068e86  dstat-0.6.3.tar.bz2

You can get rid of this line if you'd like:
  /usr/lib/rpm/brp-python-bytecompile # FC4 and FC5 run this automatically

as you've noted, it gets run automatically on FC4+.  If you are building for
FC3, that script won't be present at all so the spec file will fail to build.

All blockers have been resolved.  I'm ready to ACCEPT this package, we just need
to get you sponsored.

As for reviewing and sponsorship.  You get sponsored after you show an
understanding for the guidelines.  Understanding of the guidelines is all it
takes to be able to do reviews.  And since you're in the sponsorship process,
I'll be watching over your shoulder so I can correct anything that you do wrong.
 (And you already know how to get into #fedora-extras on IRC which is a great
resource for asking questions that might come up during the review process.)

Basically, once you are sponsored, you will be able to make changes to any
package in the CVS tree and be able to approve other people's packages.  So
having some examples showing you have the knowledge to do that well is essential.
Comment 16 Scott Baker 2006-08-28 12:35:44 EDT
So what's next? All I want to do is get this package in the extras tree.
Comment 17 Paul F. Johnson 2006-08-28 13:04:31 EDT
You need sponsorship. The best way to do that is submit a couple of packages
then those who have the power to do so can correctly evaluate if you understand
the packaging rules.
Comment 18 Toshio Kuratomi 2006-08-28 13:17:11 EDT
(In reply to comment #16)
> So what's next? All I want to do is get this package in the extras tree.

And qcomicbook? :-)

You can also do reviews.

This is explained in Comment #13, Comment #15, and
http://www.fedoraproject.org/wiki/Extras/HowToGetSponsored

If you'd like, I can unassign this bug and you can seek someone else on
fedora-extras to sponsor you.  They'll probably review qcomicbook and look at
this bug to judge how knowledgable you are and then decide if they want you to
do reviews or anything else to show your knowledge.  Your call.
Comment 19 Scott Baker 2006-08-28 13:19:15 EDT
I'd prefer not to do reviews, I'm not sure if my knowledge is ready for that.
Comment 20 Toshio Kuratomi 2006-09-15 17:54:22 EDT
Congratulations!

APPROVING
Comment 21 Dan Horák 2008-09-12 01:28:43 EDT
*** Bug 462034 has been marked as a duplicate of this bug. ***

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