Bug 502358 (mojomojo) - Review Request: mojomojo - Catalyst & DBIx::Class powered Wiki
Summary: Review Request: mojomojo - Catalyst & DBIx::Class powered Wiki
Keywords:
Status: CLOSED NEXTRELEASE
Alias: mojomojo
Product: Fedora
Classification: Fedora
Component: Package Review
Version: rawhide
Hardware: All
OS: Linux
low
medium
Target Milestone: ---
Assignee: manuel wolfshant
QA Contact: Fedora Extras Quality Assurance
URL: http://search.cpan.org/dist/MojoMojo/
Whiteboard:
Depends On: 532190
Blocks:
TreeView+ depends on / blocked
 
Reported: 2009-05-24 12:45 UTC by Iain Arnell
Modified: 2010-07-27 03:26 UTC (History)
7 users (show)

Fixed In Version:
Doc Type: Bug Fix
Doc Text:
Clone Of:
Environment:
Last Closed: 2010-07-27 03:26:38 UTC
Type: ---
Embargoed:
manuel.wolfshant: fedora-review+
kevin: fedora-cvs+


Attachments (Terms of Use)

Description Iain Arnell 2009-05-24 12:45:58 UTC
Spec URL: http://fedorapeople.org/~iarnell/review/mojomojo.spec
SRPM URL: http://fedorapeople.org/~iarnell/review/mojomojo-0.999029-1.fc12.src.rpm

Description:
Mojomojo is a sort of content managment system, borrowing many concepts from
wikis and blogs. It allows you to maintain a full tree-structure of pages, and
to interlink them in various ways. It has full version support, so you can
always go back to a previous version and see what's changed with an easy AJAX-
based diff system. There are also a bunch of other features like bult-in
fulltext search, live AJAX preview of editing, and RSS feeds for every wiki
page.

*rt-0.09

Comment 1 Iain Arnell 2009-05-24 13:04:28 UTC
No koji build at the minute due to bug #473551 (rrdtool dejavu font dependency problems again), but the noarch rpms are available too from the same location. And there is probably enough in the spec alone to raise a few issues (I've tried to package this as a complete out-of-the-box web application - so much more than a normal perl package).

Comment 2 Iain Arnell 2009-05-24 13:10:31 UTC
Oh, and unfortunately, it doesn't quite run straight out of the box due to bug #502273 - needs a chmod 755 /var/run/httpd (or chgrp apache and chmod 750 if you prefer).

Comment 3 Iain Arnell 2009-05-27 03:44:56 UTC
build now available: http://koji.fedoraproject.org/koji/taskinfo?taskID=1378830

Comment 4 Chris Weyl 2009-06-09 06:12:57 UTC
Ok.  One last package to get in rawhide and I can update Catalyst::Devel to latest over there...  Let me get those in and we can start with 5.8 as a baseline for this review.

I've only taken a cursory look at the spec so far, but given that this came from the CPAN we may want to explicitly "provide: perl-MojoMojo = %{version}-%{release}".

I'm also bringing HTML::Entities (perl-HTML-Parser) up to 3.60; this should allow for the versioned depends.

The cpan.org url is the canonical url: for packages from the CPAN, but given that http://mojomojo.org exists perhaps we should use it?

Comment 5 Iain Arnell 2009-06-10 03:11:59 UTC
Yep, all three of those make sense. And I only needed one tiny little bugfix (already in upstream) to make it build against the latest Catalyst-Runtime.

Spec URL: http://fedorapeople.org/~iarnell/review/mojomojo.spec
SRPM URL: http://fedorapeople.org/~iarnell/review/mojomojo-0.999029-2.fc12.src.rpm

Koji build: http://koji.fedoraproject.org/koji/taskinfo?taskID=1402924

Comment 6 Lorenzo Villani 2009-06-15 15:29:29 UTC
Today I tried to install this package but mojomojo refused to start because perl-JSON was not installed.

On a side note: I also had to make /var/lib/mojomojo/{index,upload} writable for the apache user.

Comment 7 Iain Arnell 2009-06-17 03:15:52 UTC
Thanks for the feedback.

I'll add the necessary requires for perl(JSON), but I think the real problem lies upstream (Catalyst::View::JSON defaults to requiring JSON rather than let JSON::Any pick an implementation automatically - will see if upstream are okay to change that behaviour).

And I'll explicitly add the index and upload directories to %files.

Comment 8 Jason Tibbitts 2009-07-15 14:45:52 UTC
Setting the fedora-review flag since that seems to have been overlooked.

Comment 9 Iain Arnell 2009-07-31 09:02:46 UTC
There's a new upstream version now with additional dependencies. I'm in the process of packaging them, so will take a little while before this gets updated again (I knew I should have been quicker to fix the issues in comment #7).

Comment 12 Iain Arnell 2010-01-20 05:49:01 UTC
And now that I've got the latest new dependencies available, yet another upstream update.

SRPM URL: http://iarnell.fedorapeople.org/review/mojomojo-0.999042-1.fc13.src.rpm
SPEC URL: http://iarnell.fedorapeople.org/review/mojomojo.spec
KOJI URL: http://koji.fedoraproject.org/koji/taskinfo?taskID=1933200

Comment 13 Iain Arnell 2010-05-21 15:01:54 UTC
Almost a year later and 1.00 is released. It builds cleanly on F13 is working well for me. It even builds in dist-f14-perltest.

SRPM URL: http://iarnell.fedorapeople.org/review/mojomojo-1.00-1.fc14.src.rpm
SPEC URL: http://iarnell.fedorapeople.org/review/mojomojo.spec
KOJI URL: http://koji.fedoraproject.org/koji/taskinfo?taskID=2201369

Are you still interested in reviewing this Chris?

Comment 14 Iain Arnell 2010-07-09 03:37:29 UTC
I'm throwing this back into the pool since Chris hasn't responded in a long while.

New upstream release too.

SRPM: http://iarnell.fedorapeople.org/review/mojomojo-1.01-1.fc14.src.rpm
SPEC: http://iarnell.fedorapeople.org/review/mojomojo.spec
KOJI: https://koji.fedoraproject.org/koji/taskinfo?taskID=2307741

The nasty looking error message generated by t/03podcoverage.t during %check can be safely ignored.

Comment 15 manuel wolfshant 2010-07-21 22:44:26 UTC
minor nitpick, to be fixed before import: "BuildRequires:  perl(HTML::TagCloud)" is included twice.

bonus points for having the selinux part sorted, but I suggest to talk with dwalsh and have the policy included in fedora's selinux package.

Time permitting. I'll try to look at this package this week.

Comment 16 manuel wolfshant 2010-07-22 08:58:55 UTC
I do not have a rawhide handy to test, but I've never seen before the construct that you have used in the config file for httpd, that is:

Alias /mojomojo/ /usr/bin/mojomojo_fastcgi.pl/
<Location /mojomojo>
Options ExecCGI
Order allow,deny
Allow from all
AddHandler fcgid-script .pl
</Location>

where  /usr/bin/mojomojo_fastcgi.pl is a script, not a directory as the Alias directive might seem to indicate.

http://httpd.apache.org/docs/2.2/mod/mod_alias.html#alias does not reference this type of use either. Is it really correct ?

Comment 17 manuel wolfshant 2010-07-22 09:37:24 UTC
Package Review
==============

Key:
 - = N/A
 x = Check
 ! = Problem
 ? = Not evaluated

=== REQUIRED ITEMS ===
 [x] Package is named according to the Package Naming Guidelines.
 [x] Spec file name must match the base package %{name}, in the format %{name}.spec.
 [x] Package meets the Packaging Guidelines.
 [x] Package successfully compiles and builds into binary rpms on at least one supported architecture.
     Tested on: devel/x86_64
 [x] Rpmlint output:
source RPM:
mojomojo.src: W: spelling-error %description -l en_US fulltext -> full text, full-text, Fullerton
=> ignorable
binary RPM:
mojomojo.noarch: W: spelling-error %description -l en_US fulltext -> full text, full-text, Fullerton
=> ignorable
mojomojo.noarch: E: non-standard-dir-perm /var/lib/mojomojo/uploads 0750L
mojomojo.noarch: E: non-readable /etc/mojomojo.conf 0640L
mojomojo.noarch: E: non-standard-dir-perm /var/lib/mojomojo/index 0750L
mojomojo.noarch: E: non-readable /var/lib/mojomojo/mojomojo.db 0640L
=> intended, security reasons
mojomojo-selinux.noarch: W: cross-directory-hard-link /usr/share/selinux/strict/mojomojo.pp /usr/share/selinux/targeted/mojomojo.pp
=> ignorable, chances for /targeted and strict to reside on different devices are close to nil. Not to mention that the content of this package should be transferred to selinux-policy.
 [x] Package is not relocatable.
 [x] Package is licensed with an open-source compatible license and meets other legal requirements as defined in the legal section of Packaging Guidelines.
 [x] License field in the package spec file matches the actual license.
     License type: GPL+ or Artistic
 [x] If (and only if) the source package includes the text of the license(s) in its own file, then that file, containing the text of the license(s) for the package is included in %doc.
 [x] Spec file is legible and written in American English.
 [x] Sources used to build the package match the upstream source, as provided in the spec URL.
     SHA1SUM of source file: eab857f0e22824dfbc16afba63dc70ec3612c87a  MojoMojo-1.01.tar.gz
 [x] Package is not known to require ExcludeArch
 [x] Package is not known to require ExcludeArch
 [x] All build dependencies are listed in BuildRequires, except for any that are listed in the exceptions section of Packaging Guidelines.
 [-] The spec file handles locales properly.
 [-] ldconfig called in %post and %postun if required.
 [x] Package must own all directories that it creates.
 [x] Package requires other packages for directories it uses.
 [x] Package does not contain duplicates in %files.
 [x] Permissions on files are set properly.
 [x] Package consistently uses macros.
 [x] Package contains code, or permissable content.
 [-] Large documentation files are in a -doc subpackage, if required.
 [x] Package uses nothing in %doc for runtime.
 [-] Header files in -devel subpackage, if present.
 [-] Static libraries in -devel subpackage, if present.
 [-] Package requires pkgconfig, if .pc files are present.
 [-] Development .so files in -devel subpackage, if present.
 [-] Fully versioned dependency in subpackages, if present.
 [x] Package does not contain any libtool archives (.la).
 [-] Package contains a properly installed %{name}.desktop file if it is a GUI application.
 [x] Package does not own files or directories owned by other packages.
 [!] Final provides and requires are sane.
I guess a bit of filtering is needed here, as we have, under requires:
perl(Catalyst)
perl(Catalyst) >= 5.8000
perl(Data::Page)
perl(Data::Page) >= 2.00
perl(DateTime)
perl(DateTime) >= 0.28
perl(File::MMagic)
perl(File::MMagic) >= 1.27
perl(HTML::Entities)
perl(HTML::Entities) >= 3.60
perl(HTML::Strip)
perl(HTML::Strip) >= 1.04
perl(MRO::Compat)
perl(MRO::Compat) >= 0.10
perl(Text::Context)
perl(Text::Context) >= 3.5
perl >= 0:5.008004
/usr/bin/perl


=== SUGGESTED ITEMS ===
 [x] Latest version is packaged.
 [x] Package does not include license text files separate from upstream.
 [-] Description and summary sections in the package spec file contains translations for supported Non-English languages, if available.
 [x] Reviewer should test that the package builds in mock.
     Tested on: devel, noarch
 [x] Package should compile and build into binary rpms on all supported architectures.
     Tested on: devel, noarch
 [?] Package functions as described.
 [x] Scriptlets must be sane, if used.
 [-] The placement of pkgconfig(.pc) files is correct.
 [-] File based requires are sane.
 [x] %check is present and the test passes.

=== OPTIONAL ITEMS ===
 [x] Buildroot is correct (%{_tmppath}/%{name}-%{version}-%{release}-root-%(%{__id_u} -n))
 [x] Package has a %clean section, which contains rm -rf %{buildroot} (or $RPM_BUILD_ROOT).

=== Issues ===
 See above (comment 16 and the "Requires" section)

Comment 18 Iain Arnell 2010-07-22 15:13:18 UTC
(In reply to comment #16)

Thanks for picking this one up. (And the rest you've done recently - I must owe you a whole bunch of shiny beads).

> I do not have a rawhide handy to test, but I've never seen before the construct
> that you have used in the config file for httpd, that is:
> 
> Alias /mojomojo/ /usr/bin/mojomojo_fastcgi.pl/
> <Location /mojomojo>
> Options ExecCGI
> Order allow,deny
> Allow from all
> AddHandler fcgid-script .pl
> </Location>
> 
> where  /usr/bin/mojomojo_fastcgi.pl is a script, not a directory as the Alias
> directive might seem to indicate.
> 
> http://httpd.apache.org/docs/2.2/mod/mod_alias.html#alias does not reference
> this type of use either. Is it really correct ?

Yes, I didn't believe it when I first saw it either (I got the snippet from http://blog.hjksolutions.com/articles/2007/07/19/catalyst-deployment-with-apache-2-and-mod_fcgid). But it certainly works. The Catalyst::Engine::FastCGI docs claim that the trailing slashes are important for setting PATH_INFO properly.

There should be no need to test under rawhide - this review has been around long enough that it works out of the box on F-13 (and probably F-12 too).

I'll wait until I hear back from dwalsh on the selinux bits before posting an updated rpm.

Comment 19 Iain Arnell 2010-07-23 14:46:09 UTC
Wow, Dan is fast! Today's rawhide already has selinux policy labels for mojomojo. And I've put together a better policy that has been sent upstream.

So new build no longer contains selinux sub-package and has addressed the duplicate dependencies. I also tweaked the httpd config slightly to be more consistent using trailing slashes everywhere.



Spec URL: http://fedorapeople.org/~iarnell/review/mojomojo.spec
SRPM URL: http://fedorapeople.org/~iarnell/review/mojomojo-1.01-2.fc14.src.rpm

Koji build: http://koji.fedoraproject.org/koji/taskinfo?taskID=2346709

Comment 20 manuel wolfshant 2010-07-23 18:26:16 UTC
APPROVED

Comment 21 Iain Arnell 2010-07-24 05:53:11 UTC
New Package CVS Request
=======================
Package Name: mojomojo
Short Description: Catalyst & DBIx::Class powered Wiki
Owners: iarnell
Branches: F-12 F-13
InitialCC:

Comment 22 Ralf Corsepius 2010-07-24 06:01:27 UTC
(In reply to comment #21)
> InitialCC:    
Why no perl-sig?

Comment 23 Iain Arnell 2010-07-24 06:13:13 UTC
New Package CVS Request
=======================
Package Name: mojomojo
Short Description: Catalyst & DBIx::Class powered Wiki
Owners: iarnell
Branches: F-12 F-13
InitialCC: perl-sig


(In reply to comment #22)
> Why no perl-sig?

Because it looks like Chris' reviewtool only adds that automatically for perl- packages.

Comment 24 Ralf Corsepius 2010-07-24 06:48:43 UTC
(In reply to comment #23)
>
> (In reply to comment #22)
> > Why no perl-sig?
> 
> Because it looks like Chris' reviewtool only adds that automatically for perl-
> packages.
No idea what this reviewtool does, but I'd recommend to explicitly add it.

This package clearly is a perl package (It's even CPAN hosted).

Comment 25 Iain Arnell 2010-07-24 07:00:00 UTC
I added perl-sig to the request in comment #23.

Comment 26 Kevin Fenzi 2010-07-26 22:27:38 UTC
CVS done (by process-cvs-requests.py).


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