Bug 634906

Summary: Review Request: http-parser - HTTP request/response parser for C
Product: [Fedora] Fedora Reporter: Lubomir Rintel <lkundrak>
Component: Package ReviewAssignee: Peter Lemenkov <lemenkov>
Status: CLOSED NEXTRELEASE QA Contact: Fedora Extras Quality Assurance <extras-qa>
Severity: medium Docs Contact:
Priority: low    
Version: rawhideCC: fedora-package-review, lemenkov, notting, tchollingsworth
Target Milestone: ---Flags: lemenkov: fedora-review+
kevin: 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: 2010-09-23 13:33:54 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:
Bug Depends On:    
Bug Blocks: 634911    

Description Lubomir Rintel 2010-09-17 10:50:26 UTC
SPEC: http://v3.sk/~lkundrak/SPECS/http-parser.spec
SRPM: http://v3.sk/~lkundrak/SRPMS/http-parser-0.3-1.20100911git.fc13.src.rpm

Description:

This is a parser for HTTP messages written in C. It parses both requests and
responses. The parser is designed to be used in performance HTTP applications.
It does not make any syscalls nor allocations, it does not buffer data, it can
be interrupted at anytime. Depending on your architecture, it only requires
about 40 bytes of data per message stream (in a web server that is per
connection).

Comment 1 Peter Lemenkov 2010-09-20 09:07:26 UTC
Taking this.

Comment 2 Peter Lemenkov 2010-09-20 10:19:37 UTC
Koji scratch build for F-13:

http://koji.fedoraproject.org/koji/taskinfo?taskID=2476815

REVIEW:

Legend: + = PASSED, - = FAILED, 0 = Not Applicable

- rpmlint isnt' silent:

work ~: rpmlint ~/Desktop/http-parser-*
http-parser.i686: W: spelling-error %description -l en_US syscalls -> miscalls, systemically, scallops
http-parser.i686: E: no-ldconfig-symlink /usr/lib/libhttp_parser.so.0.3
http-parser.i686: E: library-without-ldconfig-postin /usr/lib/libhttp_parser.so.0.3
http-parser.i686: E: library-without-ldconfig-postun /usr/lib/libhttp_parser.so.0.3
http-parser-devel.i686: W: spelling-error %description -l en_US htt -> ht, hit, hat
http-parser-devel.i686: W: no-documentation
3 packages and 0 specfiles checked; 3 errors, 3 warnings.
work ~:

In particular,  ldconfig-related messages are definitely must be addressed (see my notes below).

- The package must be named according to the  Package Naming Guidelines. Please, mention particular git commit ID in the package's version.

+ The spec file name matches the base package %{name}, in the format %{name}.spec.

- The package doesn't fully meet the Packaging Guidelines:
1. The mentioned above issue with missing ldconfig invocation in %post and %postun sections
2. The package's versioning scheme must contain git commit id.

+ The package is licensed with a Fedora approved license and meets the Licensing Guidelines .
+ The License field in the package spec file matches the actual license. (MIT)
+ The file, containing the text of the license(s) for the package, is included in %doc.
+ The spec file is written in American English.
+ The spec file for the package is legible.
+ The sources used to build the package must match the upstream source, as provided in the spec URL. I can't use md5/sha256 here since tarball contains timestamps, uids, gids and other mutable data. I just diffed them against local copy (fetched as described in spec).

Sulaco ~/rpmbuild/BUILD: diff -ru http-parser.orig/ http-parser
Sulaco ~/rpmbuild/BUILD: 

+ The package successfully compiles and builds into binary rpms on at least one primary architecture. See koji link above.
+ All build dependencies are listed in BuildRequires.
0 No need to handle locales.

- Every binary RPM package (or subpackage) which stores shared library files (not just symlinks) in any of the dynamic linker's default paths, must call ldconfig in %post and %postun. See my notes above.

+ The package does NOT bundle copies of system libraries.
0 The package is not designed to be relocatable.
+ The package owns all directories that it creates.
+ The package does not list a file more than once in the spec file's %files listings.
+ Permissions on files are set properly.
+ The package consistently uses macros.
+ The package contains code, or permissible content.
0 No extremely large documentation files.
+ Anything, the package includes as %doc, does not affect the runtime of the application.
+ Header files are in a -devel package.
0 No static libraries.
+  The library files that end in .so (without suffix) placed in a -devel package.
+ The devel package requires the base package using a fully versioned dependency: Requires: %{name} = %{version}-%{release}
+ The package does NOT contain any .la libtool archives.
0 Not a GUI application.
+ The package does not own files or directories already owned by other packages.
+ All filenames in the package are valid UTF-8.

OK, sommarizing things - I've found only two issues - ldconfig and git id in version. Please fix them and I'll finish my review.

Comment 3 Lubomir Rintel 2010-09-20 19:28:27 UTC
(In reply to comment #2)
> - The package must be named according to the  Package Naming Guidelines.
> Please, mention particular git commit ID in the package's version.

Well, I'm not required to do that and would prefer not to [1]; both for sake of consistency and usefulness (I find it useless).

[1] http://fedoraproject.org/wiki/Packaging:NamingGuidelines#Snapshot_packages

> - The package doesn't fully meet the Packaging Guidelines:
> 1. The mentioned above issue with missing ldconfig invocation in %post and
> %postun sections

Thanks, fixed.

SPEC: http://v3.sk/~lkundrak/SPECS/http-parser.spec
SRPM: http://v3.sk/~lkundrak/SRPMS/http-parser-0.3-2.20100911git.fc13.src.rpm

Comment 4 Peter Lemenkov 2010-09-21 04:11:46 UTC
Ok, this package is

APPROVED.

Comment 5 Lubomir Rintel 2010-09-21 08:07:01 UTC
New Package SCM Request
=======================
Package Name: http-parser
Short Description: HTTP request/response parser for C
Owners: lkundrak
Branches: f14 el6

Comment 6 Kevin Fenzi 2010-09-22 16:57:45 UTC
Git done (by process-git-requests).

Comment 7 Lubomir Rintel 2010-09-23 13:33:54 UTC
Imported, built and three times distilled. Thank you!

Comment 8 T.C. Hollingsworth 2014-06-20 20:58:42 UTC
Package Change Request
======================
Package Name: http-parser
New Branches: el5
Owners: patches vpaan
InitialCC:

Comment 9 Kevin Fenzi 2014-06-21 17:14:07 UTC
Git done (by process-git-requests).