Bug 245515
Summary: | Review Request: pv - A tool for monitoring the progress of data through a pipeline | ||
---|---|---|---|
Product: | [Fedora] Fedora | Reporter: | Jakub Hrozek <jhrozek> |
Component: | Package Review | Assignee: | Kevin Fenzi <kevin> |
Status: | CLOSED NEXTRELEASE | QA Contact: | Fedora Extras Quality Assurance <extras-qa> |
Severity: | medium | Docs Contact: | |
Priority: | medium | ||
Version: | rawhide | CC: | fedora-package-review, notting |
Target Milestone: | --- | Flags: | kevin:
fedora-review+
j: fedora-cvs+ |
Target Release: | --- | ||
Hardware: | All | ||
OS: | Linux | ||
Whiteboard: | |||
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: | --- | Target Upstream Version: | |
Embargoed: |
Description
Jakub Hrozek
2007-06-24 20:01:53 UTC
I'd be happy to review this package. Look for a full review in a bit here... 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... 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). Excellent. Have a great vacation! 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? 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.
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. New Package CVS Request ======================= Package Name: pv Short Description: A tool for monitoring the progress of data through a pipeline Owners: jhrozek Branches: FC-6 F-7 EL-4 EL-5 InitialCC: N/A Thanks for reviewing my package, Kevin! CVS done. The package has been successfully built for the devel branch: http://koji.fedoraproject.org/koji/taskinfo?taskID=83662 I'm closing this review. |