[Bug 734248] Review Request: apf - Adventure PHP Framework

[bugzilla at redhat.com]

https://bugzilla.redhat.com/show_bug.cgi?id=734248

--- Comment #6 from Jason Tibbitts <tibbs at math.uh.edu> ---
OK, let me take a quick look and make more specific comments.

This does build fine; I get a few rpmlint complaints:
  apf.noarch: W: wrong-file-end-of-line-encoding
/var/www/apf-1.15/README_en.txt
  apf.noarch: W: wrong-file-end-of-line-encoding /var/www/apf-1.15/lgpl-3.0.txt
  apf.noarch: W: wrong-file-end-of-line-encoding /var/www/apf-1.15/gpl-3.0.txt
  apf.noarch: W: wrong-file-end-of-line-encoding
/var/www/apf-1.15/README_de.txt
These all need to be cleaned up to remove DOS line endings.

Pretty much everything in this package is installed in the wrong place.  You
cannot install into /var/www.  This is discussed explicitly in
http://fedoraproject.org/wiki/Packaging:Guidelines#Web_Applications
/usr/share/apf (not versioned) or even a less generic
/usr/share/adventure-framework would be a more reasonable place, I'd think.

BuildRoot, the first line of %install, the entire %clean section and the
%defattr line in %files are completely unnecessary in Fedora and should be
removed.

You don't need an empty %build section at all.

Packages should not depend on the base php package.  There's a draft about this
here: http://fedoraproject.org/wiki/PackagingDrafts/PHP which should make its
way into the actual guideline relatively soon.  Since this does include an
apache conf file, depending on httpd is OK.

Why does this package require a local mysql server?  Wouldn't it work with a
remote server?

Macro forms of basic comments like %{__rm} should not be used.  Just use "rm"
instead; it's a whole lot less typing, and we're not going to remove rm from
the path pretty much ever.

You call dos2unix without having any dependency on it.  I have no idea why it
doesn't fail; maybe the find calls aren't working as you expect.

It's pretty suboptimal to do selinux setup in scriptlets.  It would be better,
once a proper location for the files is chosen, to get the base selinux package
to include the proper file contexts.  The selinux folks are very responsive.

You should use a proper systemd call to restart httpd, and include the proper
dependencies for it, if indeed you really feel like doing that.  Personally I
don't think that installing this package should mess with a running httpd, and
the other webapps I checked don't do that.

There's no need at all to do this:
  %dir %{_localstatedir}/www/apf-%{version}/
  %{_localstatedir}/www/apf-%{version}/*
Instead, just use one line:
  %{_localstatedir}/www/apf-%{version}/

You shoud put the README files somewhere under docdir instead of spreading
through all through the application directory, unless there's some specific
reason they need to be where they are.

-- 
You are receiving this mail because:
You are on the CC list for the bug.