Bug 233522 - Review Request: nginx - http and reverse proxy server
Review Request: nginx - http and reverse proxy server
Status: CLOSED NEXTRELEASE
Product: Fedora
Classification: Fedora
Component: Package Review (Show other bugs)
rawhide
All Linux
medium Severity medium
: ---
: ---
Assigned To: Kevin Fenzi
Fedora Package Reviews List
:
Depends On:
Blocks:
  Show dependency treegraph
 
Reported: 2007-03-22 17:38 EDT by Jeremy Hinegardner
Modified: 2007-11-30 17:11 EST (History)
1 user (show)

See Also:
Fixed In Version:
Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of:
Environment:
Last Closed: 2007-03-24 15:20:18 EDT
Type: ---
Regression: ---
Mount Type: ---
Documentation: ---
CRM:
Verified Versions:
Category: ---
oVirt Team: ---
RHEL 7.3 requirements from Atomic Host:
Cloudforms Team: ---
kevin: fedora‑review+
kevin: fedora‑cvs+


Attachments (Terms of Use)

  None (edit)
Description Jeremy Hinegardner 2007-03-22 17:38:20 EDT
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-22 21:22:45 EDT
MD5 (nginx-0.5.15.tar.gz) = 651feea215c9d934542b4bae1711d921
Comment 2 Kevin Fenzi 2007-03-22 22:17:35 EDT
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-22 22:40:57 EDT
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-22 22:51:39 EDT
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 01:33:11 EDT
(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 01:37:23 EDT
(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 03:45:10 EDT
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 13:16:34 EDT
(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 13:22:06 EDT
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 14:23:34 EDT
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 18:33:51 EDT
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-16 21:44:54 EDT
Package Change Request
======================
Package Name: nginx
New Branches: EL-4 EL-5
Comment 13 Kevin Fenzi 2007-08-16 22:04:49 EDT
cvs done.

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