[Bug 492091] Review Request: zikula-module-Content - Page editing module for Zikula

[bugzilla at redhat.com]

Please do not reply directly to this email. All additional
comments should be made in the comments box of this bug.


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





--- Comment #19 from Toshio Ernie Kuratomi <a.badger at gmail.com>  2009-07-15 14:00:08 EDT ---
Here's the complete review.  It has a few things we can work on while waiting
for the licensing to be resolved::

GOOD:
* Package named according to the Guidelines
* spec file also named well.
* spec file is legible and in American English
* Builds on x86
* Builds in koji
* No locale files
* Not relocatable
* Not a dynamic library package
* Owns all files and directories it creates and nothing belonging to others
* Permissions set properly
* Proper %clean
* Cleans the buildroot at the beginning of %install

NEEDSWORK:
* Sources don't match upstream.  However, it looks like the only difference is
  the zip in the SRPM is from an svn checkout (without expnsion of keywords)
  and the zip at http://code.zikula.org/content/downloads/3 has keywords
  expanded.  Please use the tarball we can retrieve for the official package so
  others can audit where it comes from if necessary.
* Licensing: pnincludes/lightboxXL/js/lightboxXL.js: Creative Commons
  license.  Creative commons licenses are not compatible with the GPL.
  Since the rest of this plugin is GPL+, this code cannot be used with it.
  :-(
* License tag should be GPL+.  Unfortunately, I could find no reference to a
  specific version of the GPL in any header files.  The generic GPLv2 text
  that's in the license.txt file allows people to use any version of the GPL
  (GPL+) unless we can find intent that a specific subset is meant (for
  instance, a header that says GPLv2+ GPL version 2 or later, etc). Often,
  the code authors don't know this so it's polite to let them know that they
  should specify what version(s) of the GPL they want somewhere.
* %post scriptlet isn't needed.  The symlinking should be done in the %install
  step.  Also, should use the filesystem path macros here too.  For example,
  change this::
    %post
    ln -sf /usr/share/php/php-simplepie
/usr/share/%{base}/modules/%{module}/pnincludes/simplepie

  into this::
    %install
    [...]
    ln -sf %{_datadir}/php/php-simplepie
%{buildroot}%{_datadir}/%{base}/modules/%{module}/pnincludes/simplepie

  - For the pndocs symlink.... I don't think that's needed at all.

* Standardise on either %{buildroot} or $RPM_BUILD_ROOT.  These two things
  mean the same thing but using both just gets people confused.  I think it got
  you confused here::
    find ${buildroot} -empty -exec rm -f {} \;
    find ${RPM_BUILD_ROOT} -empty -exec rm -f {} \;

  You probably just want this::
    find %{buildroot} -empty -exec rm -f {} \;

  (Notice the % instead of the $)

* rpmlint:
zikula-module-Content.noarch: W: dangerous-command-in-%post ln
2 packages and 0 specfiles checked; 0 errors, 1 warnings
  - See the earlier note about the %post section.

TO FIGURE OUT LATER:
* There are language files in pnlang but they aren't gettext locale files.
  We don't have tooling to detect these and mark them as belonging to a
  certain language but we probably should find some way to support that in
  the future..

-- 
Configure bugmail: https://bugzilla.redhat.com/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are on the CC list for the bug.