Spec URL: http://theholbrooks.org/RPMS/horde.spec SRPM URL: http://theholbrooks.org/RPMS/horde-3.1.1-9.src.rpm Description: horde is an application framework written in PHP. It is the foundation for IMP webmail, Kronolith web calendar, and many more (RPMS to follow, pending comments to this request) Currently, rpmlint generates several errors and warnings, most of which I was able to clean up with some perl, the rest of which I am ignoring (for now). Among them are: E: file-in-usr-marked-as-conffile (all of horde resides in /usr/share, including its conf files) E: non-standard-gid apache (config files are 0640 root:apache, a sensible setup) E: non-readable 0640 (see above) E: non-standard-dir-perm 0750 (see above) E: htaccess-file (part of the horde distro, should I fedora-ize it to httpd/conf.d?) E: script-without-shellbang (Are we expected to fix upstream's mistakes like these?) E: non-executable-script 0644 (see above) Suggestions / comments are more than welcome on these or any other issues. I am currently trying to get my first package, php-shout, approved and in the system. Until that time, however, I am still a non-contributor. I have a sponsor working with me on php-shout, do I still need a sponsor for this package as well? After some feedback on this RPM, I will submit packages for horde applications (IMP, Turba, Kronolith, etc).
* 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