Bug 432080 - Review Request: cstream - General-purpose stream-handling tool
Review Request: cstream - General-purpose stream-handling tool
Status: CLOSED NEXTRELEASE
Product: Fedora
Classification: Fedora
Component: Package Review (Show other bugs)
rawhide
All Linux
medium Severity medium
: ---
: ---
Assigned To: Parag AN(पराग)
Fedora Extras Quality Assurance
:
Depends On:
Blocks:
  Show dependency treegraph
 
Reported: 2008-02-08 13:58 EST by Hans Ulrich Niedermann
Modified: 2008-02-11 18:05 EST (History)
2 users (show)

See Also:
Fixed In Version:
Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of:
Environment:
Last Closed: 2008-02-11 18:05:11 EST
Type: ---
Regression: ---
Mount Type: ---
Documentation: ---
CRM:
Verified Versions:
Category: ---
oVirt Team: ---
RHEL 7.3 requirements from Atomic Host:
Cloudforms Team: ---
panemade: fedora‑review+
kevin: fedora‑cvs+


Attachments (Terms of Use)

  None (edit)
Description Hans Ulrich Niedermann 2008-02-08 13:58:43 EST
Spec URL: http://ndim.fedorapeople.org/cstream/2.7.4-3/cstream.spec
SRPM URL: http://ndim.fedorapeople.org/cstream/2.7.4-3/cstream-2.7.4-3.src.rpm
Description: General-purpose stream-handling tool

cstream filters data streams, much like the UNIX tool dd(1).

It has a more traditional commandline syntax, support for precise
bandwidth limiting and reporting and support for FIFOs.

Data limits and throughput rate calculation will work for files > 4 GB.
Comment 1 Parag AN(पराग) 2008-02-09 04:43:50 EST
1)you may like to add distag to release as
Release:  3%{?dist}

2)good if you add tests to %doc that will create rpmlint warning to silent it
add to %prep
chmod 644 tests/test1.sh


3) CFLAGS value is 
 CFLAGS='-O2 -g -pipe -Wall -Wp,-D_FORTIFY_SOURCE=2 -fexceptions
-fstack-protector --param=ssp-buffer-size=4 -m32 -march=i386 -mtune=generic
-fasynchronous-unwind-tables'

  So you don't need to add -Wall. Also, any reason to add 
"-Wextra -Wno-unused-parameter -Werror" to make install?
Comment 2 Hans Ulrich Niedermann 2008-02-09 17:10:40 EST
1) Of course. Added.

2) The Makefile.am remarks that the tests do not work (due to the output format
being slightly different). I'd presume running them at %check time or shipping
them as docs does not make sense, then.

3) Can a double -Wall hurt? Adding "-Wall -Wextra -Werror" makes it clear which
set of warnings/errors we want, so that's what I'd go for.

   Compilation should be done before "make install", so I do not deem it
necessary to define the CFLAGS there.

Updated package at http://ndim.fedorapeople.org/cstream/2.7.4-4.fc8/

* Sat Feb 09 2008 Hans Ulrich Niedermann <hun@n-dimensional.de> - 2.7.4-4
- Add %%{?dist} to Release:

* Fri Feb 08 2008 Hans Ulrich Niedermann <hun@n-dimensional.de> - 2.7.4-3
- More compile warnings (-Wall -Wextra -Werror).
- Redacted description down to the most important points.
Comment 3 Parag AN(पराग) 2008-02-11 01:09:48 EST
Review:
+ package builds in mock (rawhide i386).
koji build => http://koji.fedoraproject.org/koji/taskinfo?taskID=413928
+ rpmlint is silent for SRPM and for RPM.
+ source files match upstream.
eab4c98afef79766dd61a6a36dec845a  cstream-2.7.4.tar.gz
+ package meets naming and packaging guidelines.
+ specfile is properly named, is cleanly written
+ Spec file is written in American English.
+ Spec file is legible.
+ dist tag is present.
+ build root is correct.
+ license is open source-compatible.
+ License text is included in package.
+ %doc files present.
+ BuildRequires are proper.
+ Compiler flags are honored correctly.
+ defattr usage is correct.
+ %clean is present.
+ package installed properly.
+ Macro use appears rather consistent.
+ Package contains code.
+ no static libraries.
+ no .pc file present.
+ no -devel subpackage exists.
+ no .la files.
+ no translations are available.
+ Does owns the directories it creates.
+ no duplicates in %files.
+ file permissions are appropriate.
+ no scriptlets are used.
+ Not a GUI app.

 SHOULD:
   Though I am not happy with extra flags to compiler just because it gives
extra information, its ok to add them here.

APPROVED.
Comment 4 Hans Ulrich Niedermann 2008-02-11 05:41:05 EST
New Package CVS Request
=======================
Package Name: cstream
Short Description: General-purpose stream-handling tool
Owners: ndim
Branches: F-7 F-8
InitialCC: 
Cvsextras Commits: Yes
Comment 5 Kevin Fenzi 2008-02-11 12:56:51 EST
cvs done.
Comment 6 Hans Ulrich Niedermann 2008-02-11 18:05:11 EST
cstream-2.7.4-4 packages are in rawhide and the update queue for F-7, F-8.

Thanks for review and CVS.

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