Bug 492091 - Review Request: zikula-module-Content - Page editing module for Zikula
Summary: Review Request: zikula-module-Content - Page editing module for Zikula
Keywords:
Status: CLOSED DEFERRED
Alias: None
Product: Fedora
Classification: Fedora
Component: Package Review
Version: rawhide
Hardware: All
OS: Linux
low
medium
Target Milestone: ---
Assignee: Toshio Ernie Kuratomi
QA Contact: Fedora Extras Quality Assurance
URL:
Whiteboard:
Depends On: 506517 516662
Blocks: FE-ZIKULA
TreeView+ depends on / blocked
 
Reported: 2009-03-25 11:35 UTC by LukasHetzi
Modified: 2013-10-19 14:42 UTC (History)
6 users (show)

Fixed In Version:
Doc Type: Bug Fix
Doc Text:
Clone Of:
Environment:
Last Closed: 2011-06-26 16:34:38 UTC
Type: ---
Embargoed:


Attachments (Terms of Use)

Description LukasHetzi 2009-03-25 11:35:25 UTC
Spec URL: http://lukashetzi.lu.funpic.de/zikula-rpms/zikula-Content.spec
SRPM URL: http://lukashetzi.lu.funpic.de/zikula-rpms/zikula-Content-3.0.3.1.20091002svn-1.fc10.src.rpm
Description: 
zikula-Content is a page editing module for Zikula.
With it you can insert and edit various content items,
such as HTML texts, YouTube videos, Google maps and much more.

This is my first package and I'm seeking a sponsor.

Comment 2 Toshio Ernie Kuratomi 2009-03-25 15:08:58 UTC
Hi!

I'll sponsor you after you do some reviewing to show your understanding of the Packaging Guidelines ( http://fedoraproject.org/wiki/Packaging:Guidelines ) (or that you're learning them ;-).  I understand that ke4qq is going to be adding a few more zikula modules soon which should be good fodder for that.

Comment 3 Toshio Ernie Kuratomi 2009-03-25 16:13:24 UTC
I'll do a full review later today but as the spec looks so simple -- the one mistake I see right away is the Version/Release:

  Version: 3.0.3.1.%{alphatag}
  Release: 2%{?dist}

Should be more like:
  Version: 3.0.3.1
  Release: 2.%{alphatag}%{?dist}

Or (if 3.0.3.1 hasn't been released yet)
  Version: 3.0.3.1
  Release: 0.2.%{alphatag}%{?dist}

Having the alpha tag in the version can lead to situtations where the package will not upgrade due to rpm thinking that the current version is more recent than the new version.  For instance, if we had this:

  Current Version: 3.0.3.1.20090325svn
  Next Version: 3.0.3.1.3

Rpm will evaluate "20090325" as being larger than "3" and thus not upgrade the package.

Our naming and versioning rules are documented here:
  http://fedoraproject.org/wiki/Packaging/NamingGuidelines

Comment 4 LukasHetzi 2009-03-25 18:26:24 UTC
Hello,

thanks for your help. I mixes these two fields.

Here is a new version:
* Wed Mar 25 2009 Lukas Hetzenecker <LuHe> 3.0.3-2.20091002svn
- Corrected alphatag, version and release numbers
- Added BuildRoot tag

now rpmlint shouldn't complain anymore

Spec URL: http://lukashetzi.lu.funpic.de/zikula-rpms/zikula-module-Content.spec
SRPM URL: http://lukashetzi.lu.funpic.de/zikula-rpms/zikula-module-Content-3.0.3-2.20090210svn.fc10.src.rpm

Comment 5 LukasHetzi 2009-03-26 08:28:48 UTC
* Thu Mar 26 2009 Lukas Hetzenecker <LuHe> 3.0.3-3.20090210svn
- Remove htaccess file, add httpd config file
- Getting rid of zerolength files
- Fix wrong-file-end-of-line-encoding
- Add documentation

Spec URL: http://lukashetzi.lu.funpic.de/zikula-rpms/zikula-module-Content.spec
SRPM URL:
http://lukashetzi.lu.funpic.de/zikula-rpms/zikula-module-Content-3.0.3-3.20090210svn.fc10.src.rpm
CONF URL: http://lukashetzi.lu.funpic.de/zikula-rpms/zikula-module-Content.conf

Comment 6 Toshio Ernie Kuratomi 2009-04-01 02:54:02 UTC
Couple questions:

* Is this a post release or a prerelease?  ie: Has 3.0.3 already been released?
  If it has not, then Release should be 0.3.%{alphatag}%{?dist}
* Why are we naming things zikula-module-Content instead of zikula-Content?
  (Just want an explanation... no need to change if there's a reason.)

I see that this module bundles other libraries.  We'll have to fix that :-(  I'm guessing that everything in tags/version-3.0.3/pnincludes will need to be packaged separately.  Will explore this further as I complete the review.

Comment 7 LukasHetzi 2009-04-01 10:01:05 UTC
Hello,

This is a post release.
The link in the source is from Simon, who gave me the link from the tag in the SVN repository. But now it is also available here: http://code.zikula.org/content/downloads/3 
I'll change the link in the next spec file.

The module in the name is because I'll also package themes. And somebody pointed out that there could be conflicts between the names of modules and themes.

Don't know what to do with pnincludes, but I'll look at it.

Comment 8 Toshio Ernie Kuratomi 2009-04-23 16:08:13 UTC
Looks like the main zikula package is going to replace the libraries with symlinks to the php libraries installed on the system.  That should work here as well.

Comment 9 Toshio Ernie Kuratomi 2009-04-23 16:15:20 UTC
ke4qq has a pair of packages that need to be reviewed in order to get zikula using all system packages:

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

You can do the review to help demonstrate understanding of the packaging guidelines.  Basically what happens is that you go through and check that the packages

1) Meet all the packaging guidelines
2) Aren't doing anything that will cause problems and aren't covered by the guidelines.

I'll follow your work there.  Once you and ke4qq deem the packages ready, I'll do a quick review to make sure everything is sound and either approve the package or sponsor you so you can approve it yourself.

Some of the relevant guideline pages for reviewing php apps:

https://fedoraproject.org/wiki/Packaging:ReviewGuidelines
http://fedoraproject.org/wiki/Packaging:PHP

Comment 10 Jason Tibbitts 2009-07-10 18:19:12 UTC
Did anything happen here?  Toshio, this is assigned to you; are you planning to review?  I note that the two bugs in comment 9 have been closed.

Comment 11 Toshio Ernie Kuratomi 2009-07-10 19:56:28 UTC
Lukas, what's the story on the bundled libraries?

Comment 12 LukasHetzi 2009-07-10 20:44:55 UTC
Hello,

we need to package the some libraries first:
 - pnincludes/greybox:
   seems to be from http://orangoo.com/labs/GreyBox/Download/
   The used files are in the folder "GreyBox_v5_53/greybox"

 - pnincludes/lightboxXL
   Download from http://webeaters.blogspot.com/2008/02/lightboxxl-for-prototypejs.html

 - pnincludes/phpFlickr
   Download from http://www.sourceforge.net/projects/phpflickr/
   Should be named php-pear-Flickr

 - pnincludes/simplepie
   Already packaged as php-simplepie.noarch

When these packages are available the folders could be replaced by symlinks.

Comment 13 LukasHetzi 2009-07-10 21:02:10 UTC
Bug 506517 -  Review Request: phpFlickr - PHP client library for Flickr

this package was approved by Nick Bebout on 2009-07-09 23:16:15 EDT

Comment 14 David Nalley 2009-07-14 21:05:34 UTC
(In reply to comment #12)
> Hello,
> 
> we need to package the some libraries first:
>  - pnincludes/greybox:
>    seems to be from http://orangoo.com/labs/GreyBox/Download/
>    The used files are in the folder "GreyBox_v5_53/greybox"
> 
>  - pnincludes/lightboxXL
>    Download from
> http://webeaters.blogspot.com/2008/02/lightboxxl-for-prototypejs.html
> 
>  - pnincludes/phpFlickr
>    Download from http://www.sourceforge.net/projects/phpflickr/
>    Should be named php-pear-Flickr
> 
>  - pnincludes/simplepie
>    Already packaged as php-simplepie.noarch
> 
> When these packages are available the folders could be replaced by symlinks.  

Toshio: 

The bottom two are packaged already. 
The top two appear at very cursory glance to be JS libs, and per our conversation and IRC at the moment those can be bundled. Does that accurately describe the situation? If so looks like we are ready to move ahead and link to the libs here. 
If my analysis is faulty, please let me know, so we can start work on packaging the first two libs. 

Thanks

Comment 15 Toshio Ernie Kuratomi 2009-07-15 07:39:05 UTC
Yep, that is accurate.  Lukas, if you can spin a new package package with the two php modules replaced with symlinks to the system libraries I can review this.

Comment 16 LukasHetzi 2009-07-15 16:10:28 UTC
Done.

* Wed Jul 15 2009 Lukas Hetzenecker <LuHe> 3.0.3-4.20090210svn
- Create symlinks to system libraries
- Update Source URL

Spec URL: http://lukashetzi.lu.funpic.de/zikula-rpms/zikula-module-Content.spec
SRPM URL:
http://lukashetzi.lu.funpic.de/zikula-rpms/zikula-module-Content-3.0.3-4.20090210svn.fc11.src.rpm
CONF URL: http://lukashetzi.lu.funpic.de/zikula-rpms/zikula-module-Content.conf

Comment 17 Toshio Ernie Kuratomi 2009-07-15 17:25:30 UTC
I'm doing a full review, but I just found something that will need to be addressed and isn't pretty:

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.  :-(

We'll have to figure out if there's a replacement or we can use it under another license or if it's safe to remove it or....

Comment 18 eric 2009-07-15 17:52:56 UTC
(In reply to comment #17)
> I'm doing a full review, but I just found something that will need to be
> addressed and isn't pretty:
> 
> 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.  :-(
> 
> We'll have to figure out if there's a replacement or we can use it under
> another license or if it's safe to remove it or....  

I'll follow up with the Zikula folks about this.

Comment 19 Toshio Ernie Kuratomi 2009-07-15 18:00:08 UTC
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..

Comment 20 David Nalley 2009-07-23 11:06:01 UTC
I fired an email off to the lightboxXL author asking if he would consider dual-licensing under something GPL-compatible.

Comment 21 John (J5) Palmieri 2009-08-03 20:12:58 UTC
Lightwindow is MIT licensed, uses prototype + Scriptaculous and can do all the fancy media types.  It is a bit hefty of a library compared to other lightboxes though (61k).

http://stickmanlabs.com/lightwindow

Comment 22 John (J5) Palmieri 2009-08-03 20:20:26 UTC
BTW the lightboxXL author can not change the license by himself.  Looking at the code it looks like this is a modification lightbox++ which itself is a modification of lightbox2 both of which are cc-by 2.5

Comment 23 LukasHetzi 2009-09-21 11:54:44 UTC
Any news concerning the license for lightboxXL.js?
Should I fix the other problems first?

Comment 24 Toshio Kuratomi 2009-09-21 16:44:24 UTC
(In reply to comment #23)
> Any news concerning the license for lightboxXL.js?

David, Eric, J5, did any of you hear anything back about this?

IIRC -- the first option was lighthbox relicense and the analysis here was that would probably not happen.  David, I know you sent off an email; did you hear back?

The second option was that upstream would replace lightbox.  Eric did you hear back on that?

The third option was that we'd coordinate with upstream on what a valid replacement library would be and perform the port.  J5, how is that going?

> Should I fix the other problems first?  

From a package review standpoint, yes.  If no one(here or upstream) is working on the licensing issue, we aren't going to get this package in, though :-(

Comment 25 David Nalley 2009-10-06 03:08:01 UTC
(In reply to comment #24)
> (In reply to comment #23)
> > Any news concerning the license for lightboxXL.js?
> 
> David, Eric, J5, did any of you hear anything back about this?
> 
> IIRC -- the first option was lighthbox relicense and the analysis here was that
> would probably not happen.  David, I know you sent off an email; did you hear
> back?
> 

I heard no response back from lightbox folks.

Comment 26 Paul W. Frields 2011-06-26 15:36:42 UTC
Recommend closing as WONTFIX.

Comment 27 Toshio Ernie Kuratomi 2011-06-26 16:34:38 UTC
Closing deferred.  If someone wants to get zikula-module-Content in at a later date, they're welcome to.  They'll need to resolve the same license conflict as mentioned here as part of that.


Note You need to log in before you can comment on or make changes to this bug.