Bug 245515 - Review Request: pv - A tool for monitoring the progress of data through a pipeline
Review Request: pv - A tool for monitoring the progress of data through a pip...
Status: CLOSED NEXTRELEASE
Product: Fedora
Classification: Fedora
Component: Package Review (Show other bugs)
rawhide
All Linux
medium Severity medium
: ---
: ---
Assigned To: Kevin Fenzi
Fedora Extras Quality Assurance
:
Depends On:
Blocks:
  Show dependency treegraph
 
Reported: 2007-06-24 16:01 EDT by Jakub Hrozek
Modified: 2007-11-30 17:12 EST (History)
2 users (show)

See Also:
Fixed In Version:
Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of:
Environment:
Last Closed: 2007-07-31 15:45:46 EDT
Type: ---
Regression: ---
Mount Type: ---
Documentation: ---
CRM:
Verified Versions:
Category: ---
oVirt Team: ---
RHEL 7.3 requirements from Atomic Host:
Cloudforms Team: ---
kevin: fedora‑review+
tibbs: fedora‑cvs+


Attachments (Terms of Use)

  None (edit)
Description Jakub Hrozek 2007-06-24 16:01:53 EDT
Spec URL: http://hrozkovi.homelinux.org/pv/pv.spec
SRPM URL: http://hrozkovi.homelinux.org/pv/pv-0.9.9-1.fc8.src.rpm
Description:
PV ("Pipe Viewer") is a tool for monitoring the progress of data through a
pipeline.  It can be inserted into any normal pipeline between two processes
to give a visual indication of how quickly data is passing through, how long
it has taken, how near to completion it is, and an estimate of how long it
will be until completion.
Comment 1 Kevin Fenzi 2007-07-06 15:52:36 EDT
I'd be happy to review this package. 

Look for a full review in a bit here... 
Comment 2 Kevin Fenzi 2007-07-06 16:10:10 EDT
OK - Package meets naming and packaging guidelines
OK - Spec file matches base package name.
OK - Spec has consistant macro usage.
OK - Meets Packaging Guidelines.
OK - License (Artistic)
OK - License field in spec matches
OK - License file included in package
OK - Spec in American English
OK - Spec is legible.
OK - Sources match upstream md5sum:
a26e523e54c511ed8d263f16a7025330  pv-0.9.9.tar.gz
a26e523e54c511ed8d263f16a7025330  pv-0.9.9.tar.gz.1
See below - BuildRequires correct
OK - Spec handles locales/find_lang
OK - Package has %defattr and permissions on files is good.
OK - Package has a correct %clean section.
OK - Package has correct buildroot
OK - Package is code or permissible content.
OK - Packages %doc files don't affect runtime.
OK - Package has rm -rf RPM_BUILD_ROOT at top of %install

OK - Package compiles and builds on at least one arch.
OK - Package has no duplicate files in %files.
OK - Package doesn't own any directories other packages own.
OK - Package owns all the directories it creates.
OK - No rpmlint output.
OK - final provides and requires are sane.

SHOULD Items:

OK - Should build in mock.
OK - Should build on all supported archs
OK - Should function as described.
OK - Should have dist tag
OK - Should package latest version

Issues:

1. Not a blocker, but I see that the during the build the install attempts
to install a pv.info file, which doesn't exist. Might ask upstream about either
including that or removing the code which refers to it.

/usr/bin/install -c -m 644 doc/pv.info \
                           "/var/tmp/pv-0.9.9-1.fc8-c16338//usr/share/info/pv.info"
/usr/bin/install: cannot stat `doc/pv.info': No such file or directory
make: [install] Error 1 (ignored)

2. Also, it looks like 'man' is called during build:

man doc/quickref.1 | sed 's/.^H//g' | cat -s > doc/quickref.txt || :
/bin/sh: man: command not found

Should there be a BuildRequires: man ?
I suppose only if you wanted to package the quickref.txt file?
That file seems to be a copy of the man page...
Comment 3 Jakub Hrozek 2007-07-09 17:22:31 EDT
Hello Kevin,
Thanks for stepping up for the review and for spotting the issues with man and 
info!

I'm on vacation now, so I'll be able to look into the issues you described in 
more detail when I'm back (22-Jul).
Comment 4 Kevin Fenzi 2007-07-09 18:22:45 EDT
Excellent. Have a great vacation! 
Comment 5 Jakub Hrozek 2007-07-26 15:15:07 EDT
OK, I'm back :)

I queried upstream about the info file, I'll update this bug report when the 
reply comes.

Per the quickref issue - I personally don't think that including that file is 
necessary since it adds nothing compared to the man page..should I tune the 
install phase so it doesn't look for the quickref file? What are your thoughts 
on this?
Comment 6 Jakub Hrozek 2007-07-27 07:24:25 EDT
Reply from the maintainer:

> I work on packaging pv for Fedora and have one issue that I'd like to
> ask you about - it seems that during install, the install attempts
> to install a pv.info file, which doesn't exist:  

Oops. There used to be one, but I stopped maintaining the Texinfo
documentation because I failed to see the point.

Because the install command ignores failures (I think I did that so that
installation would still work on systems without texinfo) I never noticed.

The next release will fix this, but I've no idea when that will be. In the
meantime, something like "sed -i '/\.info/d' autoconf/make/unreal.mk" just
before running "configure" will stop the warning by removing the relevant
bits from the Makefile.
Comment 7 Kevin Fenzi 2007-07-30 14:48:10 EDT
ok, sounds good. 

Neither of those issues seems to be blockers, just wanted to make sure they were
addressed. Sounds like the info issue will be fixed upstream. I don't think it
adds much to package quickref.txt, as it's just a duplicate of the man page. You
can package it if you like, or leave it out, it's up to you. 

I see no further issues, so this package is APPROVED. 
Don't forget to close this once it's been imported and built. 

Also, do consider going and reviewing another waiting package to help spread out
the review load. 
Comment 8 Jakub Hrozek 2007-07-31 08:56:54 EDT
New Package CVS Request
=======================
Package Name: pv
Short Description: A tool for monitoring the progress of data through a 
pipeline
Owners: jhrozek@redhat.com
Branches: FC-6 F-7 EL-4 EL-5
InitialCC: N/A
Comment 9 Jakub Hrozek 2007-07-31 09:10:04 EDT
Thanks for reviewing my package, Kevin!
Comment 10 Jason Tibbitts 2007-07-31 11:14:53 EDT
CVS done.
Comment 11 Jakub Hrozek 2007-07-31 15:45:46 EDT
The package has been successfully built for the devel branch:
http://koji.fedoraproject.org/koji/taskinfo?taskID=83662

I'm closing this review.

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