Bug 697829

Summary:	Review Request: libpipeline - A pipeline manipulation library
Product:	[Fedora] Fedora	Reporter:	Ivana Varekova <varekova>
Component:	Package Review	Assignee:	Jan Safranek <jsafrane>
Status:	CLOSED RAWHIDE	QA Contact:	Fedora Extras Quality Assurance <extras-qa>
Severity:	unspecified	Docs Contact:
Priority:	unspecified
Version:	rawhide	CC:	fedora-package-review, jsafrane, notting
Target Milestone:	---	Flags:	jsafrane: fedora-review+ j: fedora-cvs+
Target Release:	---
Hardware:	Unspecified
OS:	Unspecified
Whiteboard:
Fixed In Version:		Doc Type:	Bug Fix
Doc Text:		Story Points:	---
Clone Of:		Environment:
Last Closed:	2011-04-26 09:39:49 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 Ivana Varekova 2011-04-19 11:48:58 UTC

SPEC: http://people.redhat.com/varekova/libpipeline.spec
SRPM: http://people.redhat.com/varekova/libpipeline-1.2.0-1.fc13.src.rpm

Description: libpipeline is a C library for setting up and running pipelines of
processes, without needing to involve shell command-line parsing which is
often error-prone and insecure.  This alleviates programmers of the need to
laboriously construct pipelines using lower-level primitives such as fork(2)
and execve(2)

Comment 1 Jan Safranek 2011-04-19 12:05:38 UTC

I'll review it.

Comment 2 Jan Safranek 2011-04-19 12:58:34 UTC

Listing MUST review items where I have comments, all others are OK.

# MUST: rpmlint must be run on the source rpm and all binary rpms the build produces. The output should be posted in the review.[1]

rpmlint complains:
libpipeline.src: W: spelling-error %description -l en_US execve -> executive, execked, execute

IMHO it's OK and does not block the review.

# MUST: The License field in the package spec file must match the actual license. [3]
Looking at the code, the actual license is GPLv3+, not GPLv3. This should be fixed.

# MUST: In the vast majority of cases, devel packages must require the base package using a fully versioned dependency: Requires: %{name}%{?_isa} = %{version}-%{release} [21]

The %{?_isa} part is missing in the .spec file. Is it something new? It wasn't required when I did last review (it's a long time ago...).

# MUST: Packages must NOT bundle copies of system libraries.[11]

gnulib has exception, but Packaging guidelines suggest that the package should Provide: bundled(gnulib)

All SHOULD items are OK.

Looking at the .spec file itself, I notice few cosmetic nits:
- Source0 could contain %{version} so it does not need to be manually updated with every rebase.
- Both %description texts should end with dot '.' at the end.
- There is additional 'ls -la' in %install section - it does not harm, nor does it help anything.
- %post can be simple %post -p /sbin/ldconfig, like %postun is.
Feel free to ignore any of these comments.

So, apart from the license, suboptimal Requires: in devel subpackage and missing Provides:bundled(gnulib) the package is IMHO fine to be approved.

Comment 3 Ivana Varekova 2011-04-19 13:12:37 UTC

Thanks for the review, all points except  %post section one fixed, updated srpm and spec.

Comment 4 Jan Safranek 2011-04-19 13:16:28 UTC

I approve the package.

Comment 5 Ivana Varekova 2011-04-20 06:11:03 UTC

New Package SCM Request
=======================
Package Name: libpipeline
Short Description: A pipeline manipulation library
Owners: varekova
Branches: 
InitialCC:

Comment 6 Jason Tibbitts 2011-04-21 03:37:53 UTC

Git done (by process-git-requests).