Bug 245515 - Review Request: pv - A tool for monitoring the progress of data through a pipeline
Summary: Review Request: pv - A tool for monitoring the progress of data through a pip...
Status: CLOSED NEXTRELEASE
Alias: None
Product: Fedora
Classification: Fedora
Component: Package Review (Show other bugs)
(Show other bugs)
Version: rawhide
Hardware: All Linux
medium
medium
Target Milestone: ---
Assignee: Kevin Fenzi
QA Contact: Fedora Extras Quality Assurance
URL:
Whiteboard:
Keywords:
Depends On:
Blocks:
TreeView+ depends on / blocked
 
Reported: 2007-06-24 20:01 UTC by Jakub Hrozek
Modified: 2007-11-30 22:12 UTC (History)
2 users (show)

Fixed In Version:
Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of:
Environment:
Last Closed: 2007-07-31 19:45:46 UTC
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)

Description Jakub Hrozek 2007-06-24 20:01:53 UTC
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 19:52:36 UTC
I'd be happy to review this package. 

Look for a full review in a bit here... 

Comment 2 Kevin Fenzi 2007-07-06 20:10:10 UTC
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 21:22:31 UTC
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 22:22:45 UTC
Excellent. Have a great vacation! 

Comment 5 Jakub Hrozek 2007-07-26 19:15:07 UTC
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 11:24:25 UTC
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 18:48:10 UTC
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 12:56:54 UTC
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 13:10:04 UTC
Thanks for reviewing my package, Kevin!

Comment 10 Jason Tibbitts 2007-07-31 15:14:53 UTC
CVS done.

Comment 11 Jakub Hrozek 2007-07-31 19:45:46 UTC
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.