Not an official review as I'm not yet sponsored
Mock build for development i386 is sucessfull
MUST Items:
     - MUST: rpmlint shows no error. 
     - MUST: dist tag is present.
     - MUST: The package is named according to the Package Naming Guidelines.
     - MUST: The spec file name matching the base package dircproxy, in the
format dircproxy.spec.
      - MUST: This package meets the Packaging Guidelines.
      - MUST: The package is licensed with an open-source compatible license GPL.
      - MUST: The sources used to build the package matches the upstream source,
as provided in the spec URL. md5sum is correct (bd6abe933f90d80fbc71a00563f9c7de)
      - MUST: This package owns all directories that it creates. 
      - MUST: This package did not contain any duplicate files in the %files
listing.
      - MUST: This package  have a %clean section, which contains rm -rf
$RPM_BUILD_ROOT.
      - MUST: This package used macros.
      - MUST: Document files are included like INSTALL README, RFCs, SPEC, PROTOCOL.
      - MUST: Package did NOT contained any .la libtool archives.
      * Source URL is present and working.
      * BuildRoot is correct BuildRoot:       
%{_tmppath}/%{name}-%{version}-%{release}-root-%(%{__id_u} -n)
      * BuildRequires is correct
      * preun and postun installing service correctly.

Blocker:
* runs as root although daemon supports non-root operation ("switch_user" feature)

The daemon can be run as any user, but it won't work for multiple users unless
run as root. The "switch_user" feature does not run the daemon as non-root, it
only briefly seteuid's to display another name:

 6.1  How do I change what username is presented on IRC?

     The obvious answer is to run dircproxy under that username,
     but that doesn't help if you're proxying for multiple people.
     Another option is to use one of the many fake ident daemons
     to return a false answer for you.

     There's also a third option, which is available to those
     running dircproxy as root (either as a daemon or from inetd).
     You can use the 'switch_user' configuration file directive.
     This ensures that the connection to the server appears as from
     whatever local username you give it (by seteuid()ing to that
     briefly) while the dircproxy process remains as root.

Exactly what should be done here, I dunno.

Anyone want to pick up this review?... Please?...

Jarod --

As I start going through the review, any reason to not build this with ssl
support?  It would seem that enabling it is as easy as appending --enable-ssl to
%configure, and add openssl-devel as a br.

Huh, not sure why I didn't have ssl turned on... Oh yeah, from the dircproxy
wiki (http://dircproxy.securiweb.net/):

New feature in 1.2.0 (currently beta)
[..]
    * SSL Support (client mode and server mode, --enable-ssl ) (merge not
finished yet) 

However, now that I look at it more, the README.ssl file (added to the svn trunk
post-beta tarball) seems to indicate it actually works reasonably well (just a
few caveats), so I'll turn it on and include the README.ssl file...

Okay, just pushed a new spec and srpm out.

And thank you for picking up the review. :)

Whew!  ok :)

1. It appears to me that this proxy can be run as a non-root user and still be
able to do everything needed except use the "switch_user" command.  (Read
another way, it looks like you only need to run dircproxy as root if you want
to use "switch_user".)  Let's find a way to have this service start up as a
non-root user by default (perhaps just "nobody" as README.identd suggests).

It would seem to make sense to patch /etc/init.d/dircproxy to read values from
/etc/sysconf/dircproxy (as other init scripts do) to determine what user to
run under, etc.

2. The release tag here, as this is a beta package, should be something like:

    0.x.beta%{?dist}

Where x is the actual release.  (On coming out of beta, it should start being
the normal "x%{?dist}".)  See wiki - Packaging/NamingGuidelines.

3. rpmlint emits two warnings -- both of these are easy enough to fix.

+ package meets naming and packaging guidelines.
+ specfile is properly named, is cleanly written and uses macros consistently.
+ dist tag is present.
X release tag doesn't meet naming guidelines.
+ build root is correct.
+ license field matches the actual license.
+ license is open source-compatible.  License text included in package.
+ source files match upstream:
bd6abe933f90d80fbc71a00563f9c7de  dircproxy-1.2.0-beta.tar.bz
bd6abe933f90d80fbc71a00563f9c7de  dircproxy-1.2.0-beta.tar.bz.srpm
+ latest version is being packaged.
+ BuildRequires are proper.
+ package builds in mock (fc5/x86_64).
X rpmlint is silent.
[build@zeus dircproxy]$ rpmlint dircproxy-1.2.0-0.beta.1.fc5.src.rpm
W: dircproxy mixed-use-of-spaces-and-tabs
[build@zeus x86_64]$ rpmlint dircproxy-1.2.0-0.beta.1.fc5.x86_64.rpm
W: dircproxy wrong-file-end-of-line-encoding
/usr/share/doc/dircproxy-1.2.0/README.ssl
+ final provides and requires are sane:
 ** dircproxy-1.2.0-0.beta.1.fc5.x86_64.rpm
 == provides
 config(dircproxy) = 1.2.0-0.beta.1.fc5
 dircproxy = 1.2.0-0.beta.1.fc5
 == requires
 /bin/sh
 /usr/bin/perl
 config(dircproxy) = 1.2.0-0.beta.1.fc5
 perl(strict)
 perl(vars)
+ no shared libraries are present.
+ package is not relocatable.
+ owns the directories it creates.
+ doesn't own any directories it shouldn't.
+ no duplicates in %files.
+ file permissions are appropriate.
+ %clean is present.
O %check is not present, but there are no tests defined
O scriptlets look sane, and conform to ScriptletSnippets
+ code, not content.
+ documentation is small, so no -docs subpackage is necessary.
+ %docs are not necessary for the proper functioning of the package.
+ no headers.
+ no pkgconfig files.
+ no libtool .la droppings.
+ not a GUI app.
+ not a web app.

No pressure, just want to note the state :)

(In reply to comment #7)
> Whew!  ok :)

I know that feeling... Been a bit overloaded with other stuff, finally getting back to this...

> 1. It appears to me that this proxy can be run as a non-root user and still be
> able to do everything needed except use the "switch_user" command.  (Read
> another way, it looks like you only need to run dircproxy as root if you want
> to use "switch_user".)  Let's find a way to have this service start up as a
> non-root user by default (perhaps just "nobody" as README.identd suggests).

Won't work as nobody:

# su nobody -c "/usr/bin/dircproxy -f /etc/dircproxyrc"
This account is currently not available.

I'll have to add an account w/a valid login shell, but that's easy enough.

> It would seem to make sense to patch /etc/init.d/dircproxy to read values from
> /etc/sysconf/dircproxy (as other init scripts do) to determine what user to
> run under, etc.

Will do.

> 2. The release tag here, as this is a beta package, should be something like:
> 
>     0.x.beta%{?dist}

Fixed locally.

> 3. rpmlint emits two warnings -- both of these are easy enough to fix.
[...]
> [build@zeus dircproxy]$ rpmlint dircproxy-1.2.0-0.beta.1.fc5.src.rpm
> W: dircproxy mixed-use-of-spaces-and-tabs
> [build@zeus x86_64]$ rpmlint dircproxy-1.2.0-0.beta.1.fc5.x86_64.rpm
> W: dircproxy wrong-file-end-of-line-encoding

Both fixed locally.

Just need to fix up the bits to run as non-root, which I'll have to save for tomorrow...

(In reply to comment #9)
[...]
> Just need to fix up the bits to run as non-root, which I'll have to save for tomorrow...

I lied. Can't sleep at night if I leave things unfinished. :)

http://wilsonet.com/packages/dircproxy/dircproxy-1.2.0-0.3.beta.fc6.src.rpm
http://wilsonet.com/packages/dircproxy/dircproxy.spec

rpmlint, as it is wont to do, bestows upon us two errors:

E: dircproxy executable-marked-as-config-file /etc/sysconfig/dircproxy
E: dircproxy script-without-shellbang /etc/sysconfig/dircproxy

A chmod -x /etc/sysconfig/dircproxy should do the trick.

Aside from that, it works and looks good to me!

APPROVED

Erm, in full that should have read "APPROVED, provided that the above is addressed."

(In reply to comment #11)
> rpmlint, as it is wont to do, bestows upon us two errors:
> 
> E: dircproxy executable-marked-as-config-file /etc/sysconfig/dircproxy
> E: dircproxy script-without-shellbang /etc/sysconfig/dircproxy
> 
> A chmod -x /etc/sysconfig/dircproxy should do the trick.

Even better, fix a stupid copy-n-paste error, use install -m 0644 instead of
install -m 755. :) (done in rel 0.4.beta)

> Aside from that, it works and looks good to me!
> 
> APPROVED

Cool, importing rel 0.4.beta shortly...

Imported and built, closing ticket.