Bug 863985 - Review Request: weighttp - Small tool to benchmark webservers
Review Request: weighttp - Small tool to benchmark webservers
Status: CLOSED NEXTRELEASE
Product: Fedora
Classification: Fedora
Component: Package Review (Show other bugs)
rawhide
All Linux
medium Severity medium
: ---
: ---
Assigned To: Eduardo Echeverria
Fedora Extras Quality Assurance
:
Depends On:
Blocks:
  Show dependency treegraph
 
Reported: 2012-10-08 05:06 EDT by Mathieu Bridon
Modified: 2012-10-19 00:20 EDT (History)
5 users (show)

See Also:
Fixed In Version:
Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of:
Environment:
Last Closed: 2012-10-19 00:20:30 EDT
Type: ---
Regression: ---
Mount Type: ---
Documentation: ---
CRM:
Verified Versions:
Category: ---
oVirt Team: ---
RHEL 7.3 requirements from Atomic Host:
Cloudforms Team: ---
echevemaster: fedora‑review+
limburgher: fedora‑cvs+


Attachments (Terms of Use)

  None (edit)
Description Mathieu Bridon 2012-10-08 05:06:12 EDT
Spec URL: http://bochecha.fedorapeople.org/packages/weighttp.spec
SRPM URL: http://bochecha.fedorapeople.org/packages/weighttp-0.3-1.fc18.src.rpm

Description:
weighttp (pronounced weighty) is a lightweight and small benchmarking tool for
webservers.

It was designed to be very fast and easy to use and only supports a tiny
fraction of the HTTP protocol in order to be lean and simple.

weighttp supports multithreading to make good use of modern CPUs with multiple
cores as well as asynchronous i/o for concurrent requests within a single
thread.

For event handling, weighty relies on libev which fits the design perfectly,
being lightweight and fast itself. Thanks to that, weighty supports all modern
high-performance event interfaces like epoll or kqueue, that the major OSs
provide.

Fedora Account System Username: bochecha
Comment 1 Eduardo Echeverria 2012-10-08 22:37:32 EDT
Hi Mathieu:
Initial Comments:
waf is available in fedora 
http://koji.fedoraproject.org/koji/packageinfo?packageID=5794
Please Remove the waf binary on the source, and add waf on BuildRequires
Best Regards
Comment 2 Mathieu Bridon 2012-10-08 23:04:32 EDT
(In reply to comment #1)
> Hi Mathieu:
> Initial Comments:
> waf is available in fedora 
> http://koji.fedoraproject.org/koji/packageinfo?packageID=5794
> Please Remove the waf binary on the source, and add waf on BuildRequires

Is that an absolute requirement?

I'd like to avoid diverging too much from upstream on this. The waf version in Fedora is much more recent, I'd have to almost rewrite the wscript (I actually did it at first, before reverting and submitting the package using the bundled waf).

I couldn't find anything in the Packaging Guidelines about this, the only reference I found was this email:
  https://www.redhat.com/archives/fedora-packaging/2009-February/msg00038.html

But that doesn't seem to be conclusive on whether or not the system waf must be used.

So to sum it up, I'll do it if it becomes a review requirement, but I'd like to avoid it if possible.
Comment 3 Eduardo Echeverria 2012-10-09 00:31:35 EDT
Hi again Mathieu:
Midori suffers from the same problem with the waf version in fedora (not build the package), therefore uses the executable of waf included,  but the question is, it's easy for you maintain change in wscript through time with successive versions of waf?, if the answer is positive, use the version of waf in fedora, if not, I see no problem as there is a precedent, So be should documented in the spec if  you use the binary included

Btw, You searched if there are manual pages for this program?


For the readers:
- Download http://kojipkgs.fedoraproject.org//packages/midori/0.4.7/1.fc18/src/midori-0.4.7-1.fc18.src.rpm
- Extract the spec with rpm2cpio midori-0.4.7-1.fc18.src.rpm | cpio -idmv
- Open midori.spec
Verify this
%build
export CFLAGS="%{optflags}"
## Currently does not build against Fedora waf
./waf	--prefix=%{_usr}			\
	--docdir=%{_docdir}/%{name}-%{version}	\
	--libdir=%{_libdir}			\
	--enable-apidocs			\
	configure
./waf %{?_smp_mflags} build
Comment 4 Mathieu Bridon 2012-10-09 01:01:53 EDT
(In reply to comment #3)
> Hi again Mathieu:
> Midori suffers from the same problem with the waf version in fedora (not
> build the package), therefore uses the executable of waf included,  but the
> question is, it's easy for you maintain change in wscript through time with
> successive versions of waf?

Heh, I have absolutely no idea.

The first time I heard about waf was when I started trying to build this package yesterday. :)

>, if the answer is positive, use the version of
> waf in fedora, if not, I see no problem as there is a precedent, So be
> should documented in the spec if  you use the binary included

Agreed, I should definitely leave a comment about it in there.

> Btw, You searched if there are manual pages for this program?

There is none in the upstream source tree:
    http://cgit.lighttpd.net/weighttp.git/tree/

The command is quite well self-documented with the -h option though, so I'll happily include it in the package if someone else does and submits it upstream, but I won't do so myself.
Comment 5 Eduardo Echeverria 2012-10-13 12:33:21 EDT
Hi Mathieu any update here?
Comment 6 Mathieu Bridon 2012-10-15 00:51:31 EDT
(In reply to comment #5)
> Hi Mathieu any update here?

I'm not sure what you're asking for, the only thing I can see is that I agreed to your suggestion to add a comment explaining why I'm using the bundled waf.

I honestly thought it was trivial enough that I could add it in the next submission. I don't believe I make perfect specs on the first try, so I had no doubt you'd find a few things to fix during the review, and I would have added that comment along with the other changes you would request. :)

Anyway, here's a new package with that comment added.

Spec URL: http://bochecha.fedorapeople.org/packages/weighttp.spec
SRPM URL: http://bochecha.fedorapeople.org/packages/weighttp-0.3-2.fc18.src.rpm
Comment 7 Eduardo Echeverria 2012-10-16 04:08:40 EDT
Sorry for not being as explicit.
Frankly I was waiting for an update with the comment,  and you were waiting for me to give more feedback about the spec.
So step to give my comments on the spec.

rpmlint out 
weighttp-0.3-2.fc17.x86_64.rpm
          weighttp-debuginfo-0.3-2.fc17.x86_64.rpm
          weighttp-0.3-2.fc17.src.rpm
weighttp.x86_64: W: spelling-error Summary(en_US) webservers -> web servers, web-servers, observers
weighttp.x86_64: W: spelling-error %description -l en_US benchmarking -> bench marking, bench-marking, benchmark
weighttp.x86_64: W: spelling-error %description -l en_US webservers -> web servers, web-servers, observers
weighttp.x86_64: W: spelling-error %description -l en_US multithreading -> multitasking
weighttp.x86_64: W: spelling-error %description -l en_US libev -> libel, believe
weighttp.x86_64: W: spelling-error %description -l en_US epoll -> poll, e poll
weighttp.x86_64: W: spelling-error %description -l en_US kqueue -> queue, k queue, liqueur

are spelling errors, and can be ignored

weighttp.x86_64: W: no-manual-page-for-binary weighttp
You should contact the upstream for to be add in the near future

weighttp.src: W: invalid-url Source0: weighttp-0.3.tar.xz
You explain clearly in the spec, how to retrieve the source.

- Since the package uses bundled waf, you need BR's python2-devel

The only issue I see so far is the inclusion of bundled waf, therefore you should ensure that the upstream be notified of this situation, 
and he try out build with the latest version of waf

So I will leave a note in the formal review that says:
The package does NOT bundle copies of system libraries (except waf, Which is used only for building)
Comment 8 Mathieu Bridon 2012-10-16 06:12:21 EDT
(In reply to comment #7)
> - Since the package uses bundled waf, you need BR's python2-devel

Are you sure about that?

I mean, if I had a "BuildRequires: waf" instead (to use the Fedora version), that wouldn't drag python-devel in the buildroot:
  $ repoquery --releasever=18 --requires waf
  /usr/bin/python
  python(abi) = 2.7

Both are provided by the python package.

So I don't understand why I should add that BR. :-/

> The only issue I see so far is the inclusion of bundled waf, therefore you
> should ensure that the upstream be notified of this situation, 
> and he try out build with the latest version of waf
> 
> So I will leave a note in the formal review that says:
> The package does NOT bundle copies of system libraries (except waf, Which is
> used only for building)

I'm unclear on what you mean here. Do you mean that the package is not approved until I unbundle waf? Or is that a nice to have for the future, but it doesn't block the review?
Comment 9 Eduardo Echeverria 2012-10-16 06:58:18 EDT
1.- If  (in theoric case) you build the package with waf (fedora version),  not need python-devel as it is an explicit dependency, but in this case you built with bundled waf and buildroot does not recognize the dependency. Do the test in koji and you will understand what I say

2.- No, doesn't block the review.
Comment 10 Mathieu Bridon 2012-10-17 00:04:36 EDT
(In reply to comment #9)
> 1.- If  (in theoric case) you build the package with waf (fedora version), 
> not need python-devel as it is an explicit dependency, but in this case you
> built with bundled waf and buildroot does not recognize the dependency. Do
> the test in koji and you will understand what I say

Ouch, I missed that one because Python was already installed in my mock chroot. Koji uses a clean mock chroot every time, which explains why it failed.

Obviously, after cleaning my mock chroot, I get the same failure. Good catch!

So the actual failure is:
    + ./waf configure --prefix=/usr
    /usr/bin/env: python: No such file or directory

This is resolved by adding a BR on python, not python-devel.

If I used the Fedora waf package, it would drag in python, not python-devel.

As such, I really don't understand why you talk about python-devel. :-/

In any case, here is a new package with an added BR on python, which now builds properly:
    http://koji.fedoraproject.org/koji/taskinfo?taskID=4597589

Spec URL: http://bochecha.fedorapeople.org/packages/weighttp.spec
SRPM URL: http://bochecha.fedorapeople.org/packages/weighttp-0.3-3.fc18.src.rpm
Comment 11 Eduardo Echeverria 2012-10-18 00:13:52 EDT
Hi Mathieu:

You are absolutely right. The issue is that I wanted applied the guidelines of python here. Considering that wscript is a python script. So I thought: Eduardo!! Obviously this is a program written in C, not a python script that is have to build.
Hope apologize me for this error. u_u

Anyway there is a issue:
Spec file as given by url is not the same as in SRPM
Diff spec file in url and in SRPM
---------------------------------
--- /home/makerpm/weighttp2/863985-weighttp/srpm/weighttp.spec  2012-10-17 22:2$
+++ /home/makerpm/weighttp2/863985-weighttp/srpm-unpacked/weighttp.spec 2012-10$
@@ -19,5 +19,5 @@
 BuildRequires:    libev-devel

-# The build uses a bundled copy of waf, which requires python. If we ever
+# The build use a bundled copy of waf, which requires python. If we ever
 # start using the Fedora waf package instead, this will come in implictly.
 BuildRequires:    python

Please fix this issue, and I proceed to the formal review

Best Regards
Comment 12 Mathieu Bridon 2012-10-18 00:24:51 EDT
(In reply to comment #11)
> Hope apologize me for this error. u_u

No need to apologize, we all make mistakes. :)

Plus, you actually made me add a missing BuildRequires.

> Anyway there is a issue:
> Spec file as given by url is not the same as in SRPM

Gah! Sorry about that. >_<

I guess I mist have fixed the typo in the spec file after having uploaded the srpm.

I re-uploaded the srpm in-place, just rebuilt with the correct spec but without any other change.
Comment 13 Eduardo Echeverria 2012-10-18 01:21:37 EDT
Koji Build Rawhide 
http://koji.fedoraproject.org/koji/taskinfo?taskID=4602339
Koji Build F18 
http://koji.fedoraproject.org/koji/taskinfo?taskID=4602348
Koji Build f17
http://koji.fedoraproject.org/koji/taskinfo?taskID=4602356
Koji Build f16
http://koji.fedoraproject.org/koji/taskinfo?taskID=4602363

Package Review
==============

Key:
[x] = Pass
[!] = Fail
[-] = Not applicable
[?] = Not evaluated
[ ] = Manual review needed



===== MUST items =====

C/C++:
[x]: Header files in -devel subpackage, if present.
[x]: Package does not contain any libtool archives (.la)
[x]: Package does not contain kernel modules.
[x]: Package contains no static executables.
[x]: Rpath absent or only used for internal libs.

Generic:
[x]: Package is licensed with an open-source compatible license and meets
     other legal requirements as defined in the legal section of Packaging
     Guidelines.
[x]: Package successfully compiles and builds into binary rpms on at least one
     supported primary architecture.
[x]: %build honors applicable compiler flags or justifies otherwise.
[x]: All build dependencies are listed in BuildRequires, except for any that
     are listed in the exceptions section of Packaging Guidelines.
[x]: Package contains no bundled libraries.
The package does NOT bundle copies of system libraries (except waf, Which is used only for building)
[x]: Changelog in prescribed format.
[x]: Package does not run rm -rf %{buildroot} (or $RPM_BUILD_ROOT) at the
     beginning of %install.
[x]: Sources contain only permissible code or content.
[x]: Each %files section contains %defattr if rpm < 4.4
[x]: Macros in Summary, %description expandable at SRPM build time.
[-]: Package contains desktop file if it is a GUI application.
[-]: Development files must be in a -devel package
[x]: Package requires other packages for directories it uses.
[x]: Package uses nothing in %doc for runtime.
[x]: Package is not known to require ExcludeArch.
[x]: Package does not contain duplicates in %files.
[x]: Permissions on files are set properly.
[x]: Package complies to the Packaging Guidelines
[x]: Spec file lacks Packager, Vendor, PreReq tags.
[-]: Large documentation files are in a -doc subpackage, if required.
[x]: If (and only if) the source package includes the text of the license(s)
     in its own file, then that file, containing the text of the license(s)
     for the package is included in %doc.
[x]: License field in the package spec file matches the actual license.
     Note: Checking patched sources after %prep for licenses. Licenses found:
     "Unknown or generated". 1 files have unknown license. Detailed output of
     licensecheck in /home/makerpm/weighttp2/863985-weighttp/licensecheck.txt
The package is licensed under MIT License

[x]: Package consistently uses macro is (instead of hard-coded directory
     names).
[x]: Package is named using only allowed ASCII characters.
[x]: Package is named according to the Package Naming Guidelines.
[x]: Package does not generate any conflict.
     Note: Package contains no Conflicts: tag(s)
[x]: Package do not use a name that already exist
[x]: Package obeys FHS, except libexecdir and /usr/target.
[x]: If the package is a rename of another package, proper Obsoletes and
     Provides are present.
[x]: Package must own all directories that it creates.
[x]: Package does not own files or directories owned by other packages.
[x]: Package installs properly.
[x]: Package is not relocatable.
[x]: Requires correct, justified where necessary.
[x]: CheckResultdir
[x]: Rpmlint is run on all rpms the build produces.
     Note: There are rpmlint messages (see attachment).
[x]: Sources used to build the package match the upstream source, as provided
     in the spec URL.
[x]: Spec file is legible and written in American English.
[x]: Spec file name must match the spec package %{name}, in the format
     %{name}.spec.
[-]: Package contains systemd file(s) if in need.
[x]: File names are valid UTF-8.
[x]: Useful -debuginfo package or justification otherwise.

===== SHOULD items =====

Generic:
[x]: Reviewer should test that the package builds in mock.
[x]: Buildroot is not present
[x]: Package has no %clean section with rm -rf %{buildroot} (or
     $RPM_BUILD_ROOT)
[x]: If the source package does not include license text(s) as a separate file
     from upstream, the packager SHOULD query upstream to include it.
[x]: Dist tag is present.
[x]: No file requires outside of /etc, /bin, /sbin, /usr/bin, /usr/sbin.
[x]: Final provides and requires are sane (rpm -q --provides and rpm -q
     --requires).
[?]: Package functions as described.
[x]: Latest version is packaged.
[x]: Package does not include license text files separate from upstream.
[-]: Patches link to upstream bugs/comments/lists or are otherwise justified.
[x]: The placement of pkgconfig(.pc) files are correct.
[x]: SourceX tarball generation or download is documented.
     Note: Package contains tarball without URL, check comments
[x]: SourceX / PatchY prefixed with %{name}.
[-]: SourceX is a working URL.
The package contains comments about retrieve the source
[-]: Description and summary sections in the package spec file contains
     translations for supported Non-English languages, if available.
[x]: Package should compile and build into binary rpms on all supported
     architectures.
[-]: %check is present and all tests pass.
[x]: Packages should try to preserve timestamps of original installed files.
[x]: Spec use %global instead of %define.

===== EXTRA items =====

Generic:
[x]: Rpmlint is run on all installed packages.
     Note: There are rpmlint messages (see attachment).
[x]: Spec file according to URL is the same as in SRPM.


Rpmlint
-------
Checking: weighttp-0.3-3.fc17.x86_64.rpm
          weighttp-debuginfo-0.3-3.fc17.x86_64.rpm
          weighttp-0.3-3.fc17.src.rpm
weighttp.x86_64: W: spelling-error Summary(en_US) webservers -> web servers, web-servers, observers
weighttp.x86_64: W: spelling-error %description -l en_US benchmarking -> bench marking, bench-marking, benchmark
weighttp.x86_64: W: spelling-error %description -l en_US webservers -> web servers, web-servers, observers
weighttp.x86_64: W: spelling-error %description -l en_US multithreading -> multitasking
weighttp.x86_64: W: spelling-error %description -l en_US libev -> libel, believe
weighttp.x86_64: W: spelling-error %description -l en_US epoll -> poll, e poll
weighttp.x86_64: W: spelling-error %description -l en_US kqueue -> queue, k queue, liqueur
weighttp.x86_64: W: no-manual-page-for-binary weighttp
weighttp.src: W: spelling-error Summary(en_US) webservers -> web servers, web-servers, observers
weighttp.src: W: spelling-error %description -l en_US benchmarking -> bench marking, bench-marking, benchmark
weighttp.src: W: spelling-error %description -l en_US webservers -> web servers, web-servers, observers
weighttp.src: W: spelling-error %description -l en_US multithreading -> multitasking
weighttp.src: W: spelling-error %description -l en_US libev -> libel, believe
weighttp.src: W: spelling-error %description -l en_US epoll -> poll, e poll
weighttp.src: W: spelling-error %description -l en_US kqueue -> queue, k queue, liqueur
weighttp.src: W: invalid-url Source0: weighttp-0.3.tar.xz
3 packages and 0 specfiles checked; 0 errors, 16 warnings.




Rpmlint (installed packages)
----------------------------
# rpmlint weighttp-debuginfo weighttp
weighttp.x86_64: W: spelling-error Summary(en_US) webservers -> web servers, web-servers, observers
weighttp.x86_64: W: spelling-error %description -l en_US benchmarking -> bench marking, bench-marking, benchmark
weighttp.x86_64: W: spelling-error %description -l en_US webservers -> web servers, web-servers, observers
weighttp.x86_64: W: spelling-error %description -l en_US multithreading -> multitasking
weighttp.x86_64: W: spelling-error %description -l en_US libev -> libel, believe
weighttp.x86_64: W: spelling-error %description -l en_US epoll -> poll, e poll
weighttp.x86_64: W: spelling-error %description -l en_US kqueue -> queue, k queue, liqueur
weighttp.x86_64: W: no-manual-page-for-binary weighttp
2 packages and 0 specfiles checked; 0 errors, 8 warnings.
# echo 'rpmlint-done:'



Requires
--------
weighttp-0.3-3.fc17.x86_64.rpm (rpmlib, GLIBC filtered):
    
    libc.so.6()(64bit)  
    libev.so.4()(64bit)  
    libpthread.so.0()(64bit)  
    rtld(GNU_HASH)  

weighttp-debuginfo-0.3-3.fc17.x86_64.rpm (rpmlib, GLIBC filtered):
    



Provides
--------
weighttp-0.3-3.fc17.x86_64.rpm:
    
    weighttp = 0.3-3.fc17
    weighttp(x86-64) = 0.3-3.fc17

weighttp-debuginfo-0.3-3.fc17.x86_64.rpm:
    
    weighttp-debuginfo = 0.3-3.fc17
    weighttp-debuginfo(x86-64) = 0.3-3.fc17



MD5-sum check
-------------
makerpm@echevemaster srpm-unpacked$ sha256sum weighttp-0.3.tar.xz*
b7fd59ca15df21a0de780e559b6015640cc505503c41ba57ffcb8bf110db178b  weighttp-0.3.tar.xz
b7fd59ca15df21a0de780e559b6015640cc505503c41ba57ffcb8bf110db178b  weighttp-0.3.tar.xz1

----------------

PACKAGE APPROVED

----------------
Comment 14 Mathieu Bridon 2012-10-18 01:40:05 EDT
Thanks for the review Eduardo!

New Package SCM Request
=======================
Package Name: weighttp
Short Description: Small tool to benchmark webservers
Owners: bochecha
Branches: devel
Comment 15 Gwyn Ciesla 2012-10-18 07:14:21 EDT
Git done (by process-git-requests).
Comment 16 Mathieu Bridon 2012-10-19 00:20:30 EDT
Thanks for the git process Jon!

Built in Rawhide, closing.

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