Bug 233522

MD5 (nginx-0.5.15.tar.gz) = 651feea215c9d934542b4bae1711d921

I'd be happy to review this package (and sponsor you)... look for a full review
in a bit. 

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

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?

(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)

(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.

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.

(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.

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

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!

New Package CVS Request
=======================
Package Name: nginx
Short Description: Robust, small and high performance http and reverse proxy server
Owners: jeremy
Branches: FC-5, FC-6
InitialCC

Package Change Request
======================
Package Name: nginx
New Branches: EL-4 EL-5

cvs done.