* config files MUST not be under /usr; place them under /etc or /var
  (see below)

* horde requires write access to the config files (they are editable
  through the web interface); so permissions should be 0660 for
  root:apache or even apache ownership. These files should be located
  under /var

  Perhaps location of the config files can be changed in the code,
  perhaps you have to use symlinks for that

* the 'locale/*/horde.mo' files should be annotated with the corresponding
  %lang() tags; it would be probably the best to move them to the
  %regular /usr/share/locale and run '%find_lang horde'

* docs/ should be removed and packaged like

  | %doc docs/*

* it might be a good idea to restrict the initial visibility of Horde
  to localhost; e.g. by adding

  | <Directory /usr/share/horde>
  |   Allow from 127.0.0.1
  |   Deny  from all
  | </Directory>

  to the apache configuration.

  What is with the authentication during the initial setup? Is there
  a non-default password required for the 'Administrator' user? If
  not, some modifications shall be done to avoid that an unconfigured
  Horde installation can be run by unauthorized users.

Spec URL: http://theholbrooks.org/RPMS/horde.spec
SRPM URL: http://theholbrooks.org/RPMS/horde-3.1.1-9.7.src.rpm

(In reply to comment #1)
> * config files MUST not be under /usr; place them under /etc or /var
>   (see below)
> 
> * horde requires write access to the config files (they are editable
>   through the web interface); so permissions should be 0660 for
>   root:apache or even apache ownership. These files should be located
>   under /var
> 
>   Perhaps location of the config files can be changed in the code,
>   perhaps you have to use symlinks for that

Using symlinks, and rewriting horde's configuration a little, I have relocated
horde's config files to /var/lib/horde, all 0660 apache:apache

> 
> * the 'locale/*/horde.mo' files should be annotated with the corresponding
>   %lang() tags; it would be probably the best to move them to the
>   %regular /usr/share/locale and run '%find_lang horde'

I've done the first part, labeled all the locales with the %lang() macro, but
I'm not sure if %find_lang applied in this situation.  All the horde locales are
specified as ar_SY, bg_BG, en_US, etc... but most of the locales in
/usr/share/locale is just the 2-letter ar, bg, en, etc.  Is find_lang smart
enough to overcome this, should I run some logic to figure it out myself, or
should they be copied in as-is?

> 
> * docs/ should be removed and packaged like
> 
>   | %doc docs/*

Done

> 
> * it might be a good idea to restrict the initial visibility of Horde
>   to localhost; e.g. by adding
> 
>   | <Directory /usr/share/horde>
>   |   Allow from 127.0.0.1
>   |   Deny  from all
>   | </Directory>
> 
>   to the apache configuration.

Done

> 
>   What is with the authentication during the initial setup? Is there
>   a non-default password required for the 'Administrator' user? If
>   not, some modifications shall be done to avoid that an unconfigured
>   Horde installation can be run by unauthorized users.
> 

There isn't any authentication during the inital setup, the browser is
automatically logged in as Administrator.  By default, Horde's "Authentication
Mechanism" (configurable in 'Setup|Authentication') is set to "Automatically
authenticate as a certain user", and the end user can then change that to HTTP,
LDAP, whatever...  For an added level of obscurity, I could make HTTP the
default, and include an .htaccess file with a name and password, but it would be
the same password for every installation and not really any more secure than the
default no-password setup.  Is this unacceptable?

I'd really like to see this go in (because it would save me a massive amount of
time), but aren't there a huge number of prerequisite Pear modules that need to
be packaged first and be made prerequisites of this package?

A while ago, I packaged horde and horde-nag for recreational use. Packages and
specfiles can be found at http://people.redhat.com/dlutter/puppet-app/; the
packages did work for deploying horde automatically with puppet
(http://people.redhat.com/dlutter/puppet-app.html)

Hope you'll find them useful for finishing up this package. From a quick look at
the specfile, you need to add at least php-pear-Mail_Mime and php-pear-Log as
dependencies for horde, and possibly php-pear-DB for any of the horde applications.

So it's been a month; anything happening with this package?

I wish... I'm just in a (seemingly permanenet) holding pattern waiting for
somebody to give this a new review since my comment #2 submission.

How about starting by responding to comments #3 and #4 and then I'll try to take
a look.

Spec URL: http://theholbrooks.org/RPMS/horde.spec
SRPM URL: http://theholbrooks.org/RPMS/horde-3.1.1-9.10.src.rpm

I've gone ahead and added php-pear-DB (already in FE) php-pear-Log (bug 190101)
and php-pear-Mail_Mime (mine, bug 196824) as dependencies, Thanks David.  I've
also added a quite verbose message in %post with further instructions on
configuring, securing horde.

Spec URL: http://theholbrooks.org/RPMS/horde.spec
SRPM URL: http://theholbrooks.org/RPMS/horde-3.1.2-1.1.src.rpm

Horde released 3.1.2 today to fix some security holes, thought I'd go ahead and
package it up before the next review cycle :)

Well, it looks like all of the dependencies have been approved and should be in
the repo soon.  So who has what it takes to properly review this?

This package needs updating with the Requires, and obviously it does not handle
locales properly.

Spec URL: http://theholbrooks.org/RPMS/horde.spec
SRPM URL: http://theholbrooks.org/RPMS/horde-3.1.3-3.src.rpm

Bumped to 3.1.3, updated Requires:, properly tag %doc files.

rpmlint is not kind to my noarch.rpm.  IMO, all of them relating to /etc/horde
can be ignored, including non-standard-[gu]id and non-standard file/dir
permissions.  The *.dist files are %config, but also IMO replacable as new
config directives may trickle down from upstream and can then be compared
against the REAL *.php config files.  The rest seems like leftover cruft from
the way the files were packaged upstream.  It it our responsibility to run some
obligatory chmod()s before the files get packaged?  I set the final permissions
for all relevant files during %install...

Thanks for the comments Chris, cryptic as they may be.  By 'updating with the
Requires', I assume you meant 'rename php-pear-Mail_Mime to php-pear-Mail-Mime',
which I've done.  Sadly I'm not sure what you could mean by 'obviously does not
handle locales properly'...  this is my first encounter with locale-aware
software, and I haven't found any documentation for %find_lang that I can use. 
Can you be more specific with what this package needs to do differently to avoid
'obviously not handle locales properly', or at least the name of a package that
handles locales in a fashion similar to what horde needs?  From my Comment #2
that was never answered:

>> 
>> * the 'locale/*/horde.mo' files should be annotated with the corresponding
>>   %lang() tags; it would be probably the best to move them to the
>>   %regular /usr/share/locale and run '%find_lang horde'
>
>I've done the first part, labeled all the locales with the %lang() macro, but
>I'm not sure if %find_lang applied in this situation.  All the horde locales
>are specified as ar_SY, bg_BG, en_US, etc... but most of the locales in
>/usr/share/locale is just the 2-letter ar, bg, en, etc.  Is find_lang smart
>enough to overcome this, should I run some logic to figure it out myself, or
>should they be copied in as-is? 

I'm not sure of the answer to your question, but you can always give it a try
and see.  But not using %find_lang is a blocker.  Having a million different
%lang doesnt look right, all spec files ive seen just use %find_lang.  For more
information see,

http://fedoraproject.org/wiki/Packaging/Guidelines#head-8c605ebf8330f6d505f384e671986fa99a8f72ee

The requires look better now, except I'm not sure about
Requires:	php_database

Spec URL: http://theholbrooks.org/RPMS/horde.spec
SRPM URL: http://theholbrooks.org/RPMS/horde-3.1.3-4.src.rpm

Okay I've figured out how to use %find_lang... and it seems to work exactly like
it's supposed to.  All the .mo files end up in the proper location in
/usr/share/locale/

What are you unsure about php_database?  That it's a valid requirement or that
it is needed?  I assure you it's valid (at least in Core packages), just run
'yum provides php_database' and you'll see php-mysql, php-odbc, and php-pgsql. 
Since Horde supports all 3 of these DB backends (and many more), I'll settle for
anything that provides php_database.

Just a couple comments - you symlinked to the config files.  I think some 
people may need:
Options FollowSymLinks
...in the <Directory> section.

You might also consider adding:
php_admin_flag  safe_mode        off
php_admin_flag  register_globals off
... as recommended:
http://www.horde.org/horde/docs/?f=SECURITY.html
http://wiki.horde.org/SecurityTips

Also, are you sure you want %config(noreplace) on the .xml file(s)?  I don't 
think conf.xml needs to be modified ever, and it would change from release to 
release slightly I think.

Spec URL: http://theholbrooks.org/RPMS/horde.spec
SRPM URL: http://theholbrooks.org/RPMS/horde-3.1.3-5.src.rpm

I agree with your suggestions, and have implemented all of them.  Note that
rpmlint now complains with conffile-without-noreplace on the .dist and .xml
files in /etc, but I have explained it in a note in the %files secion.

Sort of a catch-22 that Horde requires php >= 4.3, but is impossible to write a 
spec file that works for php4 and php5 (at the moment) because of this bug:
https://bugzilla.redhat.com/bugzilla/show_bug.cgi?id=215655 

Just making a note that if that bug is fixed, you could require php-domxml 
rather than php-xml and then RHEL4 and RHEL5 users could use this horde rpm.

(In reply to comment #17)
> Just making a note that if that bug is fixed, you could require php-domxml 
> rather than php-xml and then RHEL4 and RHEL5 users could use this horde rpm.

Of course that would also have to be cloned and fixed in the Fedora versions 
you plan to make this available for, so it's more of a long-term suggestion.

(In reply to comment #14)
> What are you unsure about php_database?  That it's a valid requirement or that
> it is needed?  

It may not be needed in all configurations, so I think it should be removed.  
It is not in the 'Required' section of http://www.horde.org/horde/docs/?
f=INSTALL.html#prerequisites - "If a preference container is not configured, no 
preference options will be configurable via Horde's web interface - the default 
values stored in each applications config/prefs.php file will be used"

I've found myself needing to get horde running in the relatively near term, and
the CentOS packaging seems to be bogus in that they don't actually have the
entire dependency chain packaged.  (You can install horde and such but you still
don't have the PEAR modules needed to run it.)

So I'm willing to put in some review time if there's still interest in getting
this in.

I'm extremely still interested in getting this package in... I've just not had
anyone committed to a full review.  I'll try to get some new RPMs out the door
tonight that include php-domxml instead of php-xml per Greg's comments.

WRT php-database, I'm torn.  Granted, it's not required for horde to be
operational, but most users will want to use horde's DB storage for preferences,
etc.  What is FE's usual "best-practice" for optional dependencies: minimalistic
dependency chains for advanced users, or added dependencies for more default
functionality?

The thing about requiring php_database is that if someone just does "yum install
horde" then yum will find the packages providing php_database and if none is
installed then it will pick the one with the shortest name.  Since php-odbc is
likely to be the least useful of all the packages, this is rather bad.

So in general the dependency doesn't really help anyone.  You'll still need to
know to install whichever php-??sql you need, and in addition you'd need to get
rid of the errant php-odbc package.

So if the package is at all useful without one of those packages, I'd say to
leave out the dependency.

> I'll try to get some new RPMs out the door
> tonight that include php-domxml instead of php-xml per Greg's comments.
Thanks, but if you're planning to make horde available for FC6, you'll still 
need to use php-xml - I think the issue was only fixed so far in Rawhide and 
RHEL5.  Don't bother changing it - I'm sure it's more correct for you to 
require php-xml - my comments about that were off-topic for this bug report - 
sorry - I can't wait for Enterprise Extras though ;-)

Also wanted to note that php-pear(File) is a suggested but optional pear module 
to install with Horde.  I think it's the only pear module the horde apps can 
use that isn't in Extras.

I hesitate to suggest it, but theoretically someone could push a php-domxml
package into extras that has no files and a single dependency on php-xml.

But in the end it should be easy to just conditionalize the dependency based on
the Fedora (or RHEL) release.

Spec URL: http://theholbrooks.org/RPMS/horde.spec
SRPM URL: http://theholbrooks.org/RPMS/horde-3.1.3-6.src.rpm

As promised, new RPMs.  I've removed all mention of php_database and (hint):
php-mysql.  I've also taken the liberty of requiring php-pear-File, whose review
this bug is depending on.  I haven't changed php-xml.  What I have should be
sufficient to pass review for FC[56], and can be conditional-ized later for FC7
and EL.

Also, instead of spamming the RPM install with configuration instructions, I
have placed the instructions in a README.fedora file and subtly refer users to
that file during the RPM install.

Spec URL: http://theholbrooks.org/RPMS/horde.spec
SRPM URL: http://theholbrooks.org/RPMS/horde-3.1.3-7.src.rpm

Oops!  Fixed a couple incorrect paths pointing users to documentation.

Should you be restarting httpd in post and postun?  I can't find a guideline on 
that on the Fedora wiki, but for example the moin rpm in Extras doesn't.

Also http://wiki.horde.org/SecurityTips has a good example of apache 
configuration.  I think some improvements can be made with the addition of the 
two sections:
# Deny access to files that are not served directly by the webserver
# Deny access to the test.php files except from localhost

> for example the moin rpm in Extras doesn't.

Sorry, that was a dumb example as it doesn't put anything in conf.d.  
phpMyAdmin is a better example and it does do condrestart so I guess that's 
right for Fedora Extras (although not my preference).

While I agree that a full restart is unnecessary, at least HUPing apache is
required in order for it to read the new conf entries (Aliases, VirtualHosts,
whatever the case may be).  Since we only want to signal httpd if it's already
running, but can't guarantee it's running on every end user's box, we have to
use condrestart.  An ideal solution would be a 'condreload' command in apache's
init script.  Would the benefits of reload vs. restart warrant filing an RFE
against httpd in Core?  What other concerns would need to be considered?

Perhaps "service httpd status |grep running && service httpd graceful"?

> at least HUPing apache is required in order for it to read the new conf
> entries 

Yes, but people are going to manually edit that to remove the 127.0.0.1 line 
and switch the deny/allow, etc, so really it only needs to be done after you 
edit that file, not on every minor update from 3.1.3 to 3.1.4, etc (that file 
is marked noreplace).  

How about just changeing the README.fedora bit to say:
"By default, Horde is only accessible from localhost. To enable wider access, 
edit:
/etc/httpd/conf.d/%{name}.conf and then issue the command 'service httpd 
grafeful'"

or better:
"By default, Horde is only configured to be accessible only from localhost.  It 
is unlikely that this is sufficient.  To enable wider access and apply the 
configuration, edit /etc/httpd/conf.d/%{name}.conf and then issue the 
command 'service httpd grafeful'"

Yes thanks Greg but if you read that paragraph in context we don't WANT them to
enable wider access until after they've been to the horde configuration web site
and set up their own Authentication backend.  Since horde comes predefined to
auto-login every web session as Administrator, we want them to go to the site,
set up something else, THEN and only THEN let other IPs hit the site.  Otherwise
they would be opening their horde site for lots of other people to play
Administrator.

If you think it's important I tell users that they need to graceful apache after
editing the conf file, that's not a big deal, but that doesn't preclude this
RPM's need to re[load|start]ing apache in %post.

Now, if you don't mind, I'd like to give Jason a chance to do a real review on
this package and give his feedback.  This review has been open for 8 months...

No problem, just my opinions and you both are free to ignore them.  Thanks for 
putting this package up for review, and I do appreciate your efforts, and 
Jason's effort to do the review.

Sorry one more comment ;-) - Something like your first paragraph in Comment 33 
would be a great addition to the readme file as a warning to users to maybe 
only widen access at first to their local network or single remote IP.

Well, I had set aside time to look at this and then promptly got busy with
holiday preparations.  But I'm off tomorrow, and so I went ahead and build this
and did some preliminary investigations.  Here are a few questions:

What's the registry.php file for?

I see the configuration is in /etc/horde, but that you originally had them in
/var/lib/horde.  I'm wondering how selinux might ever be made to tolerate apache
writing to things under /etc.  Frankly I had anticipated them under /var/lib
because I'd expect that most users would edit them only via the configuration
interface, but I'm honestly not sure which location is more appropriate.

Some packages I've seen disable the test.php script.  I'm not sure why; it's
pretty useful for an administrator to check things out, but I suppose it's not
needed in normal use.  What do you think?

You should probably add a note to README.fedora about how to install the proper
database module, just to be kind.

Why do you explicitly list out the files/directories under %{_datadir}/%{name}?

In general I really like the look of this package.

> What's the registry.php file for?

Compare it to registry.php.dist.  It looks to be modified as horde expects 
the /horde directory to be '..' relative to the config directory, but that 
won't work obviously if the config directory is in /etc/horde or /var/lib/horde.
eg:
    'fileroot' => dirname(__FILE__) . '/..',
    'templates' => dirname(__FILE__) . '/../templates',
is changed to:
    'fileroot' => $fileroot,
    'templates' => $fileroot . '/templates',
and he's added:
    $fileroot = '/usr/share/horde';

Spec URL: http://theholbrooks.org/RPMS/horde.spec
SRPM URL: http://theholbrooks.org/RPMS/horde-3.1.3-8.src.rpm

Thanks for the dialog guys, this is going to be one solid package by the time it
gets approved :D

Greg is right about registry.php, that's the one file I had to edit in order to
relocate the config files to an arbitrary location.  Whether they reside in
/etc/horde or /var/lib/horde is a very trivial change and completely up to you..
I'm also not sure which is more appropriate.

I've also added some additional Apache security params per Greg's suggestion in
comment 27, which also addresses your question about test.php: it's now only
accessible from localhost.  I did NOT include horde's recommended "expose_php
off" or "display_errors off" because they seem a little TOO paranoid at the
application-level, and more appropriate for the sysadmin to set globally if he
desires.

Finally, I've added a LOT more to README.fedora, including being more specific
about the security implications of opening horde to the world and a whole
paragraph of additional recommended actions (pear modules and such)

Incidentally, horde flips out if you access it at http://localhost/horde/ and
logs: "Session cookies will not work without a FQDN and with a non-empty cookie
domain".  It causes my FF to reload infinitely until it freezes and has to be
'kill -9'ed.  Using http://localhost.localdomain/horde/ or
http://127.0.0.1/horde/ has no problems.  Should we mention this in the README?

Now that I'm starting to think about packaging horde applications, what is the
appropriate naming convention, "horde-imp" or just "imp"?  If we go with
horde-imp (which seems the logical choice to me), should we try to "Provides:
imp" for people who try: "yum install imp" or should we expect them to know imp
is a subpackage of the horde framework?

I would just call it "imp".  IMP is a web application that happens to require
the Horde framework, but I wouldn't call it a subpackage of Horde.

About the registry.php file, one thing I noticed when diffing against
registry.php.dist is that the original file has moved on a bit, and thus there
are many differences between the file you ship and the one in the source.  Would
you consider rebasing it?

I still can't decide about the configuration files, but I've found myself doing
plenty of manual edits to those files lately so I'm leaning towards /etc being
more appropriate.  After all, they're still config files, not random internal state.

I do think it would be a good idea to mention your localhost issue in the
readme; it certainly can't hurt.

I'm in the FESCo meeting at the moment, so I'll take a more complete look at
this a bit later.

Spec URL: http://theholbrooks.org/RPMS/horde.spec
SRPM URL: http://theholbrooks.org/RPMS/horde-3.1.3-9.src.rpm

I've gone ahead and rebased registry.php to 1.255.2.17 (the version that ships
with 3.1.3), but there were only 2 differences (lines 53 and 55) when
calculating $webroot.  All the other diff entries you saw involving $fileroot
are intentional and required.  By default, registry.php assumes that it is
living in "$webroot/config/", so it assigns fileroot to be './../appname' which
normally resolves to "$webroot/appname/" and the world keeps on turning. 
However, since our registry.php lives in /etc/horde/, './../appname' resolves to
'/etc/appname' which doesn't exist let alone contain web content, so horde dies.

To fix this issue, I created the $fileroot variable that statically contains
'/usr/share/horde', and then manually tweak all the application-specific
sections to reference $fileroot instead of __FILE__.  Since I agree it's not
very clear this is intentional, I've renamed $fileroot to a constant:
FEDORA_FILEROOT, to make it obvious this is an intentional distro-specific tweak.

I also added a bit about localhost in the README

> I would just call it "imp".  

I hesitate to agree that is the best choice for the long run, although I 
wouldn't object if you named it imp-h3, like the upstream tarball, and 
CentOS...  If 'imp' ends up being the choice, out of courteosy could you add...

# Obsolete latest version in CentOS Extras
Obsoletes: imp-h3 <= 4.1.3-1.c4 
Provides: imp-h3 = %{version}

...to your spec file.  Especially once Enterprise Extras gets going that would 
be appreciated, as I'm sure they won't maintain their own version then.

(The CentOS imp-h3 rpm obsoletes 'imp' without a version number...oops.  I will 
raise that as a bug in their tracker if the Fedora Extras rpm will be 
named 'imp'.)

imp-h3 makes no sense for a distro-level package, other than blindly naming it
the same as upstream's tarball.  'h3' is merely to designate this branch of imp
development is designed to run on horde 3.x instead of 2.x so users don't mess
up their dependencies.  Since we are distro packagers in this case, we have
built-in mechanisms to ensure version dependencies, and don't need to use the
package name to further 'enforce' this idea.  For now (fedora rawhide) I'll make
no mention of imp-h3... we can cross the Obsoletes: bridge if/when imp branches
for EPEL.

I just opened bug 220577 review for imp, further discussions about imp can take
place over there.  I started with centos's RPM and heavily updated it to reflect
the progress we've made in here so far.

Jason, one last imp-related question that does pertain to this package.  How far
are we interested in isolating these various applications from horde (wrt file
paths and URL paths)?  By default horde applications plant themselves beneath
the horde/ directory, which is also reflected in the URL.  Imp, for instance, is
found at http://site.com/horde/imp/.  My imp RPM reflects this by putting config
files in /etc/horde/imp and web files in /usr/share/horde/imp/.  How all this
pertains to this package is the registry.php file needs to know these relative
relationships for everything to work.  If we'd like to move these applications
to /etc/imp and /usr/share/imp, we need to reflect that in registry.php before
this gets approved to avoid having to parse-and-edit with each application later.

I urge caution in trying to add symbols that might satisfy whatever CentOS is
doing; this package won't cleanly replace that one since it puts the config
files under /usr/share/horde.

We're very nearly done here; I just need to run through my checklist and
double-check everything.  Unfortunately holiday issues keep intruding; I really
hope to have it done today.

Regarding how the various consumers of the Horde infrastructure store their
files, I think it's rather expected that, say, IMP files show up under the Horde
directory and changing that would break various user expectations.

Don't push yourself; it's Christmas!  I'll be in and out until Tuesday anyway,
so no need to rush.

Here's a few more lines you could add to the directory section if you want to 
make sure the all the test.php tests pass, even with a more restrictive php 
configuration (Not crucial stuff, don't let this interfere with your review):

    php_admin_flag      magic_quotes_runtime    off
    php_flag    session.use_trans_sid   off
    php_flag    session.auto_start      off
    php_admin_flag      file_uploads    on

# Optional - required for weather block in Horde to function
#    php_admin_flag      allow_url_fopen on

# Set to your preference - memory_limit, should be at least 32M
# and be greater than the value set for post_max_size
    php_value   memory_limit   32M
    php_value   post_max_size   20M
    php_value   upload_max_filesize     10M

OK, finally some time.

I note that you emit a message in %post; you generally shouldn't do this. 
(There's a good chance that the packaging committee will add a guideline banning
this sort of output soon.)

I set up a test machine with a minimal OS install, installed this package and
started httpd.  Everything works fine, so that's good.  Still, I wonder if we
couldn't have this package spit out a subpackage that pulls in all of the
optional dependencies (save php-mysql and php-pgsql, leaving the admin to
choose).  This would make it even easier for an admin to get up and running.

The stuff in comment 45 might be useful as well, although we should carefully
consider whether increasing the limits like that is safe.  Perhaps you could
include the lines but comment them out and include some useful info, like how
high you need to increase the limits to handle attachments of a certain size
(assuming it's possible to calculate that).

Now, let's deal with the rpmlint output.  I'll put any issues worth considering
at the front.  Only two things I consider blockers:

E: horde script-without-shebang /usr/share/horde/lib/Net/IMSP/Auth/imtest.php
   This could be an issue; this is executable, but it has no shebang line and
thus won't do anything useful if executed.  Perhaps it shouldn't be executable?

W: horde symlink-should-be-relative /usr/share/horde/config /etc/horde
   This is valid; the symlink should be relative to avoid breaking various odd
setups like chroots.

W: horde strange-permission registry.php 0640
W: horde mixed-use-of-spaces-and-tabs (spaces: line 143, tab: line 1)
  These are not a big deal; clean them up if you like.  I think checking into
CVS will fix the first one, as it's only complaining about the permissions of
the file in the SRPM.

E: horde htaccess-file /usr/share/horde/lib/.htaccess
E: horde htaccess-file /usr/share/horde/locale/.htaccess
E: horde htaccess-file /usr/share/horde/po/.htaccess
E: horde htaccess-file /usr/share/horde/scripts/.htaccess
E: horde htaccess-file /usr/share/horde/templates/.htaccess
  Yes, indeed, these are htaccess files, and they need to be there.

E: horde non-executable-script /usr/share/horde/scripts/temp-cleanup.cron 0644
  Not a big deal, although it does open the question of whether we should
consider running that.  I've never done so on any of my systems.

E: horde non-readable /etc/horde/conf.php 0660
E: horde non-readable /etc/horde/conf.php.dist 0640
E: horde non-readable /etc/horde/conf.xml 0660
E: horde non-readable /etc/horde/mime_drivers.php 0660
E: horde non-readable /etc/horde/mime_drivers.php.dist 0640
E: horde non-readable /etc/horde/motd.php 0660
E: horde non-readable /etc/horde/motd.php.dist 0640
E: horde non-readable /etc/horde/nls.php 0660
E: horde non-readable /etc/horde/nls.php.dist 0640
E: horde non-readable /etc/horde/prefs.php 0660
E: horde non-readable /etc/horde/prefs.php.dist 0640
E: horde non-readable /etc/horde/registry.php 0660
E: horde non-readable /etc/horde/registry.php.dist 0640
   Yes, and they need to be non-readable.

E: horde non-standard-dir-perm /etc/horde 0770
   Again, this is necessary.

E: horde non-standard-gid /etc/horde apache
E: horde non-standard-gid /etc/horde/conf.php apache
E: horde non-standard-gid /etc/horde/conf.php.dist apache
E: horde non-standard-gid /etc/horde/conf.xml apache
E: horde non-standard-gid /etc/horde/mime_drivers.php apache
E: horde non-standard-gid /etc/horde/mime_drivers.php.dist apache
E: horde non-standard-gid /etc/horde/motd.php apache
E: horde non-standard-gid /etc/horde/motd.php.dist apache
E: horde non-standard-gid /etc/horde/nls.php apache
E: horde non-standard-gid /etc/horde/nls.php.dist apache
E: horde non-standard-gid /etc/horde/prefs.php apache
E: horde non-standard-gid /etc/horde/prefs.php.dist apache
E: horde non-standard-gid /etc/horde/registry.php apache
E: horde non-standard-gid /etc/horde/registry.php.dist apache
E: horde non-standard-uid /etc/horde apache
E: horde non-standard-uid /etc/horde/conf.php apache
E: horde non-standard-uid /etc/horde/conf.php.dist apache
E: horde non-standard-uid /etc/horde/conf.xml apache
E: horde non-standard-uid /etc/horde/mime_drivers.php apache
E: horde non-standard-uid /etc/horde/mime_drivers.php.dist apache
E: horde non-standard-uid /etc/horde/motd.php apache
E: horde non-standard-uid /etc/horde/motd.php.dist apache
E: horde non-standard-uid /etc/horde/nls.php apache
E: horde non-standard-uid /etc/horde/nls.php.dist apache
E: horde non-standard-uid /etc/horde/prefs.php apache
E: horde non-standard-uid /etc/horde/prefs.php.dist apache
E: horde non-standard-uid /etc/horde/registry.php apache
E: horde non-standard-uid /etc/horde/registry.php.dist apache
   Again, these need to be owned by that UID.

W: horde conffile-without-noreplace-flag /etc/horde/conf.php.dist
W: horde conffile-without-noreplace-flag /etc/horde/conf.xml
W: horde conffile-without-noreplace-flag /etc/horde/mime_drivers.php.dist
W: horde conffile-without-noreplace-flag /etc/horde/motd.php.dist
W: horde conffile-without-noreplace-flag /etc/horde/nls.php.dist
W: horde conffile-without-noreplace-flag /etc/horde/prefs.php.dist
W: horde conffile-without-noreplace-flag /etc/horde/registry.php.dist
   It is normal for distributed example config files to be installed without
noreplace.

Review:
* source files match upstream:
   fbc56c608ac81474b846b1b4b7bb5ee7  horde-3.1.3.tar.gz
* package meets naming and packaging guidelines.
* specfile is properly named, is cleanly written and uses macros consistently.
* dist tag is present.
* build root is correct.
* license field matches the actual license.
* license is open source-compatible.  License text included in package.
* latest version is being packaged.
O BuildRequires are proper (BR: perl is unnecessary, but not a blocker)
* %clean is present.
* package builds in mock (development, x86_64).
* package installs properly
X rpmlint has a couple of valid complaints
* final provides and requires are sane:
   config(horde) = 3.1.3-9.fc7
   horde = 3.1.3-9.fc7
  =
   /bin/sh
   /sbin/service
   /usr/bin/php
   config(horde) = 3.1.3-9.fc7
   php >= 4.3.0
   php-pear(DB)
   php-pear(File)
   php-pear(Log)
   php-pear(Mail_Mime)
   php-xml
* %check is not present; no test suite upstream.  Manual tests show everything's
working OK.
* owns the directories it creates.
* doesn't own any directories it shouldn't.
* no duplicates in %files.
* locales seem to be handled properly.
X file permissions are appropriate (imtest.php)
X scriptlets not OK; %post has intentional output
* code, not content.
* documentation is small, so no -docs subpackage is necessary.
* %docs are not necessary for the proper functioning of the package.

Spec URL: http://theholbrooks.org/RPMS/horde.spec
SRPM URL: http://theholbrooks.org/RPMS/horde-3.1.3-10.src.rpm

* Wed Dec 27 2006 Brandon Holbrook <fedora at theholbrooks.org> 3.1.3-10
- Remove execute permission from all php scripts under horde/lib/
- Make /usr/share/horde/config/ symlink relative
- Don't echo anything in %%post

Thanks, Jason.  I've addressed your blockers in this next RPM.  All the php
files under horde/lib/ are just class definitions and aren't supposed to be
executed.  upstream must have let that slip by.  I put a find section in prep to
clear these bits.  I also made the config symlink relative.

WRT output in %post, I've been keeping my eye on the thread in the list with
interest, knowing that I output in this package.  The only issue I have is that
this (and some of my other) packages don't work "out of the box", but first
require per-site configuration.  Coupled with this is the fact that
/usr/share/doc is almost unheard of outside of our die-hard linux users circle,
and even less so that some packages create a README.Fedora file targeted
specifically at users for post-install instructions.  We need to find a way to
better educate our userbase about /usr/share/doc and README.Fedora files.

OK, the issues I had are fixed; the symlink is relative, the errant executable
permissions are fixed and %post is slient.

I agree with you that we need a better way of notifying administrators about
things like additional necessary configuration work, but I doubt my suggestion
for a simple script to notify the admin in a configurable fashion will get much
traction and I doubt anyone would actually see anything sent to syslog.  One
think you might do is mention README.fedora in your %description.

I did ask around about the feasibility of having a subpackage which pulls in the
optional dependency and the concensus seems to be that it's not a bad idea.  It
would be trivial to add and would save the admin some typing.

In any case, though, this review is finally done.

APPROVED

* I went ahead and created a horde-enhanced subpackage as 3.1.3-11 before I
imported it that pulls in all the suggested packages
* devel build succeeded (logs at
http://buildsys.fedoraproject.org/logs/fedora-development-extras/24665-horde-3.1.3-11.fc7/)
* FC-6 branch requested