Note: This is a beta release of Red Hat Bugzilla 5.0. The data contained within is a snapshot of the live data so any changes you make will not be reflected in the production Bugzilla. Also email is disabled so feel free to test any aspect of the site that you want. File any problems you find or give feedback here.
Bug 233522 - Review Request: nginx - http and reverse proxy server
Summary: Review Request: nginx - http and reverse proxy server
Keywords:
Status: CLOSED NEXTRELEASE
Alias: None
Product: Fedora
Classification: Fedora
Component: Package Review
Version: rawhide
Hardware: All
OS: Linux
medium
medium
Target Milestone: ---
Assignee: Kevin Fenzi
QA Contact: Fedora Package Reviews List
URL:
Whiteboard:
Depends On:
Blocks:
TreeView+ depends on / blocked
 
Reported: 2007-03-22 21:38 UTC by Jeremy Hinegardner
Modified: 2007-11-30 22:11 UTC (History)
1 user (show)

Fixed In Version:
Doc Type: Bug Fix
Doc Text:
Clone Of:
Environment:
Last Closed: 2007-03-24 19:20:18 UTC
kevin: fedora-review+
kevin: fedora-cvs+


Attachments (Terms of Use)

Description Jeremy Hinegardner 2007-03-22 21:38:20 UTC
Spec URL: http://www.hinegardner.org/fedora-extras/nginx.spec
SRPM URL: http://www.hinegardner.org/fedora-extras/nginx-0.5.15-1.src.rpm
Description: Nginx [engine x] is an HTTP(S) server, HTTP(S) reverse proxy and IMAP/POP3 proxy server

Rpmlint reports non-standard-uid and non-standard-gid for a couple of directories in the package. Part of the install adds an 'nginx' user, and the directories rpmlint reports are on are directories that should be owned by the 'nginx' user.

I used the Pound-2.2-2.src.rpm as a model and it also creates a new 'pound' user and it has this same rpmlint error.

I am also in need of a sponsor.

Comment 1 Jeremy Hinegardner 2007-03-23 01:22:45 UTC
MD5 (nginx-0.5.15.tar.gz) = 651feea215c9d934542b4bae1711d921

Comment 2 Kevin Fenzi 2007-03-23 02:17:35 UTC
I'd be happy to review this package (and sponsor you)... look for a full review
in a bit. 


Comment 3 Kevin Fenzi 2007-03-23 02:40:57 UTC
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 (BSD)
See below - 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:
651feea215c9d934542b4bae1711d921  nginx-0.5.15.tar.gz
651feea215c9d934542b4bae1711d921  nginx-0.5.15.tar.gz.1
OK - BuildRequires correct
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.
See below - 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 have dist tag
OK - Should package latest version

Issues:

1. Not a blocker, just a suggestion, but you might want to change your Source0
line from:
Source0:    http://sysoev.ru/nginx/nginx-0.5.15.tar.gz
to
Source0:    http://sysoev.ru/nginx/nginx-%{version}.tar.gz

So you don't need to change it every release.

2. The upstream calls the license "BSD-Like"... should the License here
be "BSD-Like" in the spec?

3. rpmlint says:

E: nginx non-standard-uid /var/log/nginx nginx
E: nginx non-standard-gid /var/log/nginx nginx
E: nginx non-standard-uid /var/lib/nginx nginx
E: nginx non-standard-gid /var/lib/nginx nginx

Can be ignored.

W: nginx strange-permission nginx.init 0775

You might make the init file in the src.rpm mode 644 and then install it with
755 perms? Not a blocker.

W: nginx rpm-buildroot-usage %build export DESTDIR=%{buildroot}

False positive.

E: nginx configure-without-libdir-spec

I see that you can't use %configure here, but you might add a comment
about having to do so.

W: nginx mixed-use-of-spaces-and-tabs (spaces: line 1, tab: line 1)

Pick tabs or spaces, don't mix.

4. Should the
/usr/share/nginx/html
be instead marked as %doc? which would put them under
/usr/share/doc/nginx-%{version}/
Or does it call those html files from the program?

5. The server doesn't start here after install... I get:

2007/03/22 20:36:50 [emerg] 8189#0: glob() "/usr/share/nginx/conf/mime.types"
failed (2: No such file or directory) in /etc/nginx/nginx.conf:18


Comment 4 Kevin Fenzi 2007-03-23 02:51:39 UTC
Oh, one final thing... this server listens on port 80 by default. 
That might conflict with any of the pile of webservers available. 
Should this listen on another port by default?

Comment 5 Jeremy Hinegardner 2007-03-23 05:33:11 UTC
(In reply to comment #3)
> 1. Not a blocker, just a suggestion, but you might want to change your Source0
> line from:
> Source0:    http://sysoev.ru/nginx/nginx-0.5.15.tar.gz
> to
> Source0:    http://sysoev.ru/nginx/nginx-%{version}.tar.gz
> 
> So you don't need to change it every release.

Fixed.
 
> 2. The upstream calls the license "BSD-Like"... should the License here
> be "BSD-Like" in the spec?

Fixed to: License: BSD-like 
 
> 3. rpmlint says:
> 
> E: nginx non-standard-uid /var/log/nginx nginx
> E: nginx non-standard-gid /var/log/nginx nginx
> E: nginx non-standard-uid /var/lib/nginx nginx
> E: nginx non-standard-gid /var/lib/nginx nginx
> 
> Can be ignored.
> 
> W: nginx strange-permission nginx.init 0775
> 
> You might make the init file in the src.rpm mode 644 and then install it with
> 755 perms? Not a blocker.

Fixed, nginx.int is 0644 in src.rpm and installs as 0755
 
> W: nginx rpm-buildroot-usage %build export DESTDIR=%{buildroot}
> 
> False positive.
> 
> E: nginx configure-without-libdir-spec
> 
> I see that you can't use %configure here, but you might add a comment
> about having to do so.

Comments about the patches added.  I will also be contacting upstream
source to see if they want to accept the patches.

> W: nginx mixed-use-of-spaces-and-tabs (spaces: line 1, tab: line 1)
> 
> Pick tabs or spaces, don't mix.

Fixed.

> 4. Should the
> /usr/share/nginx/html
> be instead marked as %doc? which would put them under
> /usr/share/doc/nginx-%{version}/
> Or does it call those html files from the program?

Those files are served up by the program as default web pages and as
default error pages. 
> 5. The server doesn't start here after install... I get:
> 
> 2007/03/22 20:36:50 [emerg] 8189#0: glob() "/usr/share/nginx/conf/mime.types"
> failed (2: No such file or directory) in /etc/nginx/nginx.conf:18

Patch made to the default installed nginx.conf to make sure all the
paths are correct.

nginx starts correctly now, so long as something else isn't listening on port 80
(by default)

Comment 6 Jeremy Hinegardner 2007-03-23 05:37:23 UTC
(In reply to comment #4)
> Oh, one final thing... this server listens on port 80 by default. 
> That might conflict with any of the pile of webservers available. 
> Should this listen on another port by default?

Not really, this is a mainly a reverse proxy/load balancer server, so it will
spread out the requests it gets to other servers.

I would imagine anyone install nginx will not use the default configuration, it
will require custom configuration depending upon their deployment architecture.
 Pound is a similar application and it is starting on port 80 by default.

Comment 7 Ruben Kerkhof 2007-03-23 07:45:10 UTC
Hi Jeremy,

- zlib-devel is already required by openssl-devel
- You can even drop the whole Requires line. rpm is smart enough to calculate that by itself.
- Replace /usr/sbin/nginx in your %install section with %{_sbindir}/nginx
- You're missing a forward slash in --lock-path=%{_localstatedir}lock/subsys/%{name}

Nothing that should block this review, but I hope it helps.


Comment 8 Jeremy Hinegardner 2007-03-23 17:16:34 UTC
(In reply to comment #7)
> Hi Jeremy,
> 
> - zlib-devel is already required by openssl-devel
> - You can even drop the whole Requires line. rpm is smart enough to calculate
that by itself.

Unless someone objects, I'll leave these as they are.

> - Replace /usr/sbin/nginx in your %install section with %{_sbindir}/nginx
> - You're missing a forward slash in
--lock-path=%{_localstatedir}lock/subsys/%{name}

Both of these are now fixed

> Nothing that should block this review, but I hope it helps.

Thanks for the feedback, it always helps.



Comment 9 Jeremy Hinegardner 2007-03-23 17:22:06 UTC
New spec and srpm fixing items from comment #3 and comment #7

Spec URL: http://www.hinegardner.org/fedora-extras/nginx.spec
SRPM URL: http://www.hinegardner.org/fedora-extras/nginx-0.5.15-3.src.rpm

Comment 10 Kevin Fenzi 2007-03-23 18:23:34 UTC
Looks like the standalone spec and the one in the src.rpm aren't quite the same
(the standalone one has a added changelog entry for -3). You will want to use 
that spec when importing your package... 

All the blockers I see appear to be solved, so this package is APPROVED. 

I will go ahead and sponsor you... continue the process from: 
http://fedoraproject.org/wiki/PackageMaintainers/Join#head-a601c13b0950a89568deafa65f505b4b58ee869b

Let me know if you have any questions!

Comment 11 Jeremy Hinegardner 2007-03-23 22:33:51 UTC
New Package CVS Request
=======================
Package Name: nginx
Short Description: Robust, small and high performance http and reverse proxy server
Owners: jeremy@hinegardner.org
Branches: FC-5, FC-6
InitialCC

Comment 12 Jeremy Hinegardner 2007-08-17 01:44:54 UTC
Package Change Request
======================
Package Name: nginx
New Branches: EL-4 EL-5

Comment 13 Kevin Fenzi 2007-08-17 02:04:49 UTC
cvs done.


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