Bug 241757

+ build of the SRPM
+ rpmlint on SRPM and RPM
+ source is the one on www.drupal.org
+ license GPL
+ install and uninstall of the RPM
+ README.fedora explains how to finish the installation

- group Applications/System seems not appropriate: perhaps Applications/Publishing?
- doc in /usr/share/doc/drupal-5.1 should not be duplicated in /usr/share/drupal
- apache should be restarted after installation to load drupal.conf:

%postun
/sbin/service httpd condrestart > /dev/null 2>&1 || :

- perhaps install the cron job (as said in INSTALL.txt)?

Spec URL: http://zanoni.jcomserv.net/fedora/drupal/drupal.spec
SRPM URL: http://zanoni.jcomserv.net/fedora/drupal/drupal-5.1-2.src.rpm

Fixed group, duped docs, added apache restart.  Added it to %post as well. 
Added cron job.

- drupal need write access to /etc/drupal/default/settings.php during install
(i've just do a "chown apache:apache settings.php" to fix this)

- drupal status report says:
The directory 'files' does not exist. You may need to set the correct directory
at the file system settings page or change the current directory's permissions
so that it is writable.

perhaps add /var/lib/drupal (or /var/lib/drupal/files) and
/usr/share/drupal/files as a soft link to it

Spec URL: http://zanoni.jcomserv.net/fedora/drupal/drupal.spec
SRPM URL: http://zanoni.jcomserv.net/fedora/drupal/drupal-5.1-3.src.rpm

Corrected.  I assume having settings.php writable by default should be Ok, since
on a new install, it's locked down to localhost, and on an upgrade, it's
%config.  Of course, a malicious user on the same system could always configure
Drupal in the time between the RPM installation and when the admin goes to do it
herself, but then I suspect that's an extreme corner case. :)

Jon: that is _not_ an extreme corner case, but a serious security issue.
Drupal should _not_ be started before being configured. Please comment out
relevant lines in the Apache HTTPd counfiguration line snippet, and maybe
add a comment on which lines to uncomment when configured and how to configure.
This also removes the requirement for HTTPd restart.

Seems like I misunderstood your statement about settings.php being writable
by default, as it obviously isn't. If you were thinking about it, please
forget quickly. :) Also, it is probably a bad idea for settings.php to be
world-readable, as it holds the database password.

The cron job uses wget by default -- why not e.g. curl? And what about
dependency on wget then?

And, also the description could be written better. Please rewrite it to say
what Drupal actually is.

I've locked down the initial config, and made the description a bit better.

I've added Requires: wget, as it's recommended by upstream.

Spec URL: http://zanoni.jcomserv.net/fedora/drupal/drupal.spec
SRPM URL: http://zanoni.jcomserv.net/fedora/drupal/drupal-5.1-4.fc7.src.rpm

Jon: Please pardon the long delay.
Here's the rest of the review, other requirements are met:

* Rpmlint complains:
E: drupal non-standard-gid /var/lib/drupal apache
E: drupal non-standard-gid /etc/cron.hourly/drupal apache
  As these files have the same modes for the group as for the rest of the
  world, I see no reason for having these be owned by root group.

* BuildRoot should be %{_tmppath}/%{name}-%{version}-%{release}-root-%(%{__id_u} -n)

* Config files of drupal: /etc/drupal/all/README.txt shouldn't be here.
Documentation in /usr/share/doc is the right place for this, preferrably
README.Fedora.

Otherwise the package is superb. I'll approve it once you deal with the above.

Fixed README and buildroot.

Spec URL: http://zanoni.jcomserv.net/fedora/drupal/drupal.spec
SRPM URL: http://zanoni.jcomserv.net/fedora/drupal/drupal-5.1-5.fc7.src.rpm

Anything else?

Jon: No. As I said, the package is very fine now.
APPROVED.

Please request a CVS module now.
Thanks for the package!

New Package CVS Request
=======================
Package Name: drupal
Short Description: An open-source content-management platform
Owners: limb 
Branches: F-7 FC-6 EL-4 EL-5
InitialCC: 

Thank you for the review!

drupal-5.1-5.fc7 has been pushed to the Fedora 7 stable repository.  If problems still persist, please make note of it in this bug report.