Bug 819480 - Review Request: limesurvey - a web-based survey application
Review Request: limesurvey - a web-based survey application
Status: CLOSED NOTABUG
Product: Fedora
Classification: Fedora
Component: Package Review (Show other bugs)
rawhide
All Linux
medium Severity medium
: ---
: ---
Assigned To: Kevin Fenzi
Dan Mashal
http://www.limesurvey.org/
:
Depends On:
Blocks: FE-DEADREVIEW
  Show dependency treegraph
 
Reported: 2012-05-07 07:20 EDT by srineth
Modified: 2015-01-12 03:14 EST (History)
9 users (show)

See Also:
Fixed In Version:
Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of:
Environment:
Last Closed: 2014-03-01 13:39:25 EST
Type: Bug
Regression: ---
Mount Type: ---
Documentation: ---
CRM:
Verified Versions:
Category: ---
oVirt Team: ---
RHEL 7.3 requirements from Atomic Host:
Cloudforms Team: ---


Attachments (Terms of Use)
This is limesurvey spec file (2.09 KB, text/x-rpm-spec)
2012-05-07 07:41 EDT, srineth
no flags Details
This is limesurvey source rpm (41 bytes, text/plain)
2012-05-07 09:25 EDT, srineth
no flags Details
limesurvey updated spec file (3.85 KB, text/x-rpm-spec)
2012-05-10 02:56 EDT, srineth
no flags Details
updated spec files and rpm packagings (52 bytes, text/plain)
2012-05-19 13:38 EDT, srineth
no flags Details

  None (edit)
Description srineth 2012-05-07 07:20:12 EDT
Description of problem:


Version-Release number of selected component (if applicable):


How reproducible:


Steps to Reproduce:
1.
2.
3.
  
Actual results:


Expected results:


Additional info:
Comment 1 srineth 2012-05-07 07:41:12 EDT
Created attachment 582629 [details]
This is limesurvey spec file
Comment 2 srineth 2012-05-07 09:25:43 EDT
Created attachment 582663 [details]
This is limesurvey source rpm
Comment 3 Volker Fröhlich 2012-05-07 14:54:45 EDT
http://fedoraproject.org/wiki/Join_the_package_collection_maintainers

Have you already read the above? Please create a review ticket as described in http://fedoraproject.org/wiki/Join_the_package_collection_maintainers#Create_Your_Review_Request and don't forget FE-NEEDSPONSOR.
Comment 4 Kevin Fenzi 2012-05-07 18:08:09 EDT
Adding needsponsor and cleaning things up a bit. 

I'm going to work with srineth on this.
Comment 5 Kevin Fenzi 2012-05-09 10:05:33 EDT
ok, a few general comments here. ;) 

First, now that we are in review, please remember to increase the release and add a changelog entry everytime you fix issues. This helps make sure that I am looking at the same version you are and avoids confusion. 

Attaching spec and srpms to the bug can be anoying for folks who wish to just look at the spec easily. See me about getting some space on fedorapeople.org to place your spec and src.rpm files on. 

Some misc small things we should fix: 

1) Please fix the Summary. "The limesurvey RPM" doesn't tell people what this is. Think of it as if you were someone who had never heard of limesurvey. Describe it in a short sentence. 

2) You will need to work on the files section some more: 

In %files, when you specify a directory, it takes that and everything under it. 
When you specify %dir it only includes that specific directory. 
You shouldn't include files/directories twice. This results in a bunch of: warning: File listed twice errors from rpmbuild. So, you are going to have to drop the: %{_datadir}/limesurvey/*
and list in turn _every_ directory in there, and also "%dir %{_datadir}/limesurvey/" to get the top directory. 

3) Once files is fixed up, we need to finish an install and see if it completes. I've not looked in detail at the install, but if it simply makes a config.php file or the like, we might consider shipping a placeholder one of those to make it easy for fedora users to simply edit the config file without needing to run the installer. 

4) Once thats determined, we need to make sure the application actually works. ;)
Comment 6 Matthias Runge 2012-05-09 14:48:53 EDT
I see some other bundled applications/libs here:
- jquery (version 1.4.2 in devel-trunk, recent is something like 1.7.x
- jQuery UI - v1.8.19 - 2012-04-16 
- adodb (already packaged as php-adodb
- ckeditor and kcfinder
- ajaxupload.js 

to be continued, I'm pretty sure, I didn't found everything.
Comment 7 Kevin Fenzi 2012-05-09 14:58:20 EDT
Note that on the javascript ones: 
http://fedoraproject.org/wiki/Packaging:Guidelines#Duplication_of_system_libraries

"At this time JavaScript intended to be served to a web browser on another computer is specifically exempted from this but this will likely change in the future"

so, I think we are ok on jquery and ajaxupload at least.
Comment 8 Matthias Runge 2012-05-09 15:57:08 EDT
Yes, I wouldn't try to un-bundle jquery! In doubt, this would result in patching every javascript. I guess, we would get an exception for bundling jquery, if really necessary.
Comment 9 srineth 2012-05-10 02:56:54 EDT
Created attachment 583451 [details]
limesurvey updated spec file
Comment 10 srineth 2012-05-10 02:58:55 EDT
With the help of the Kevin and Smoogen, limesurvey spec file could be updated.
Comment 11 Kevin Fenzi 2012-05-10 16:02:04 EDT
ok, some misc comments on the version in comment #9: 

1) You did increase the release to 0.3%{?dist}, but you don't have any changelog entries. Please add changelog entries for each new release at the end explaining what you changed. 

2) Your links for the upload and templates still don't match up. You have: 

...

cp -pr upload ${RPM_BUILD_ROOT}%{_var}/www/limesurvey/upload
cp -pr templates ${RPM_BUILD_ROOT}%{_var}/www/limesurvey/templates
...

/bin/ln -sf ../../../var/limesurvey/upload ${RPM_BUILD_ROOT}%{_datadir}/limesurvey/upload
/bin/ln -sf ../../../var/limesurvey/templates ${RPM_BUILD_ROOT}%{_datadir}/limesurvey/templates
...

%attr(0775,apache,apache) %{_var}/www/limesurvey/templates
%attr(0775,apache,apache) %{_var}/www/limesurvey/upload

two of those use /var/www/ and one uses /var/limesurvery.
Comment 12 srineth 2012-05-19 13:38:56 EDT
Created attachment 585593 [details]
updated spec files and rpm packagings
Comment 13 Matthias Runge 2012-05-20 14:53:35 EDT
You should post the full urls to spec and srpm
(you simply put a part of a url in that attachment)

Corrected: 
SPEC: http://smooge.fedorapeople.org/Downloads/limesurvey/limesurvey.spec
SRPM: http://smooge.fedorapeople.org/Downloads/limesurvey/limesurvey-2.00-0.4.fc17.src.rpm

(RPM doesn't matter)
Comment 14 Kevin Fenzi 2012-05-22 17:23:31 EDT
Making some progress for sure here. ;) 

A few issues still however: 

1) The limesurvey.conf for apache needs adjusting for the new /var/www/limesurvey area. It's still pointing to /usr/share/ and not working. For that matter, perhaps we could put all but the 3 directories that need to be writable back in /usr/share? or does that not work?

2) The link under /var/www/limesurvey/application/third_party/ for the php-gettext isn't working (because of the move to /var/www/limesurvey/)
Now needs to be: 'ln -s ../../../../../usr/share/php/gettext/ ./php-gettext'

3) After fixing the above 2 items, the installer still seems to complain that 3 directories are "Found & Unwritable": 
/application/config
/upload
/templates

So, perhaps it's looking for those in the wrong place? We need to get past that error to continue in the install. ;(
Comment 15 Pierre-YvesChibon 2012-06-13 14:09:11 EDT
Just passing by I had a look at the last spec file (hosted by smooge)

Few remarks:
- You are using %define while you should not:
http://fedoraproject.org/wiki/Packaging/Guidelines#.25global_preferred_over_.25define

- /var/www is not allowed for web-app, see:
http://fedoraproject.org/wiki/Packaging/Guidelines#Web_Applications

- What is the different between %{name} and %{realname}?

- the source0 is not a URL, since upstream's website is down I cannot check, is this a checkout of the sources or just a release candidate which has no direct URL?

- The changelog is not correctly formatted (I would expect rpmlint to complain on this)

- If the intended target is Fedora and EL6, there are a couple of elements which can go away in the spec (the %clean section, the %defattr, the group)
Comment 16 Dan Mashal 2012-06-17 00:15:32 EDT
After talking with srineth on IRC here are my comments:

1) source code is RC1, RC2 is out.
2) package will fail to install without httpd. I highly recommend making this package independent of apache for people who use webservers such as nginx.
3) I was able to build the package after changing/commenting a few things in the spec file.

links to files:

http://vicodan.fedorapeople.org/limesurvey-2.00-0.5.fc17.noarch.rpm http://vicodan.fedorapeople.org/limesurvey-2.00-0.5.fc17.noarch.srpm http://vicodan.fedorapeople.org/limesurvey.spec
Comment 17 Dan Mashal 2012-06-17 00:16:06 EDT
Also rpmlint is your friend.
Comment 18 srineth 2012-06-20 14:09:39 EDT
This is new Spec file and src.rpm file
SPEC : http://srineth.fedorapeople.org/limesurvey.spec
src.rpm : http://srineth.fedorapeople.org/
Comment 19 Dan Mashal 2012-06-23 23:07:55 EDT
Per our conversation on IRC after running rpmlint you got the following error:

limesurvey.spec: W: invalid-url Source0: http://download.limesurvey.org/Unstable_releases/limesurvey200RC2-build120528.tar.bz2 HTTP Error 404: Not Found
0 packages and 1 specfiles checked; 0 errors, 1 warnings.

Please change "Source0" line to the following URL  below and run rpmlint again.

Source0:        http://download.limesurvey.org/Unstable_releases/limesurvey200RC2-build120528.tar.gz
Comment 20 Dan Mashal 2012-06-24 00:18:17 EDT
After further investigation RC4 has been released.

This brings me to my first point:

1) If you are going to become the package maintainer for something you MUST KEEP UP WITH UPSTREAM! If you can't then don't package a release candidate or package the latest stable version!
2) Your spec file was a complete mess. I had to clean it up and redo it by hand. Please review the spec file that I have done and try to install the rpm I have built by hand.
3) Again, I do not like the idea of making this an apache specific package. While I understand apache is the official/default webserver of Fedora not everyone runs "httpd". Please keep this in mind and plan for it in your package.
4) I would even go so far as to remove the httpd in the requires field or create a seperate package for nginx and/or lighthttpd or script it in a way so that the configure script checks for the webserver that is installed on the end user's machine.


I have built RC4 successfully after revising the spec file and using the latest source, and changing the URL to my fedorapeople URL.

links:

http://vicodan.fedorapeople.org/limesurvey.spec
http://vicodan.fedorapeople.org/limesurvey200RC4-build120622.tar.gz
http://vicodan.fedorapeople.org/limesurvey-2.0-1.fc17.src.rpm
http://vicodan.fedorapeople.org/limesurvey-2.0-1.fc17.noarch.rpm
Comment 21 Dan Mashal 2012-06-24 00:19:21 EDT
[dan@Fedora17 SPECS]$ rpmlint limesurvey.spec 
limesurvey.spec:38: W: macro-in-comment %{SOURCE1}
limesurvey.spec:38: W: macro-in-comment %{_sysconfdir}
limesurvey.spec:57: W: macro-in-comment %config
limesurvey.spec:57: W: macro-in-comment %{_sysconfdir}
limesurvey.spec:58: W: macro-in-comment %config
limesurvey.spec:58: W: macro-in-comment %{_sysconfdir}
limesurvey.spec:59: W: macro-in-comment %attr
limesurvey.spec:59: W: macro-in-comment %{limedir}
limesurvey.spec:60: W: macro-in-comment %attr
limesurvey.spec:60: W: macro-in-comment %{limedir}
limesurvey.spec:61: W: macro-in-comment %attr
limesurvey.spec:61: W: macro-in-comment %{limedir}
limesurvey.spec:62: W: macro-in-comment %attr
limesurvey.spec:62: W: macro-in-comment %{limedir}
0 packages and 1 specfiles checked; 0 errors, 14 warnings.

(lines commented out for obvious reasons)
Comment 22 Dan Mashal 2012-06-24 00:21:40 EDT
@Kevin,

Any elder wisdom you could provide would help here.
Comment 23 Kevin Fenzi 2012-07-05 12:08:19 EDT
I just got back today, but I'm going to try in the next few days to take a closer look at where we are here... 

Dan: I'm working with srineth here. They are new to rpm packaging, so there's definitely a learning curve. Things need to be balanced between teaching and learning here. I don't want to write the entire spec here, as I would like srineth to learn how to. As to the httpd issues, I suggest you take them up on the packaging list and come up with a proposal for the Fedora Packaging Committee.  

Anyhow, I intend to look at things and update this bug in the next few days with status... thanks for all the comments folks.
Comment 24 Kevin Fenzi 2012-07-08 15:49:50 EDT
Ok. I have taken the various spec files and merged in all the changes.

Here's the notes on the changes I made:

- Updated to RC4.
- Cleaned up requres. mysql shouldn't be required. httpd was in twice.
- Adjusted httpd conf: fixed paths, added FollowSymLinks.
- Fixed link to application/config (was missing another ../)

SPEC: http://www.scrye.com/~kevin/fedora/limesurvey/limesurvey.spec
SRPM: http://www.scrye.com/~kevin/fedora/limesurvey/limesurvey-2.00-0.7.fc18.src.rpm

After doing that I built things again and installed in a test vm.

The installer doesn't want to find things because its looking at
the permissions of 'application/config' expecting it to be a writable
directory. We have it as a link to a writable directory.

If I manually:
rm application/config
cp -a /var/lib/limesurvey/application/config /usr/share/limesurvey/application/config

The installer works and completes for me.

So, I think we need to report this as a bug to the limesurvey folks
and see if they can get it fixed before 2.00 is released.

If they cannot or don't want to, we will have to look at making
all of it install in /var/lib/limesurvey but I would prefer to avoid that.

srineth: Can you file a bug with limesurvey on this? I can assist you in
forming it if you like. http://bugs.limesurvey.org/ should be the place.

If any other folks would care to look at this spec (aside from the above issue) and provide more feedback that would be great too.
Comment 25 Matthias Runge 2012-12-14 06:59:53 EST
Any progress here?
Comment 26 Kevin Fenzi 2012-12-14 11:58:08 EST
Not much. I filed the upstream bug a while back: 

http://bugs.limesurvey.org/view.php?id=6362

no reply upstream that I see, but it's possible that it's fixed in a newer version. 

I've not had time to package up the newest upstream, but that would be the next thing to do, and see if they have this fixed upstream already or if we can work around the issue in some other way.
Comment 27 Dridi Boukelmoune 2013-10-20 04:54:07 EDT
How can I help move this review forward ?
Comment 28 Zbigniew Jędrzejewski-Szmek 2013-10-23 23:35:55 EDT
2.05 will be out soon.
Comment 29 Christopher Meng 2013-10-24 00:00:43 EDT
Any ideas from the submitter? We'd like to hear some words from you, too.
Comment 30 Dridi Boukelmoune 2013-12-01 10:55:17 EST
(In reply to Matthias Runge from comment #6)
> I see some other bundled applications/libs here:
> - jquery (version 1.4.2 in devel-trunk, recent is something like 1.7.x
> - jQuery UI - v1.8.19 - 2012-04-16 
> - adodb (already packaged as php-adodb
> - ckeditor and kcfinder
> - ajaxupload.js 

FYI, I'll first work on a kcfinder package.

(In reply to Dan Mashal from comment #16)
> 2) package will fail to install without httpd. I highly recommend making
> this package independent of apache for people who use webservers such as
> nginx.

It is currently not possible to be server agnostic, because Fedora's ckeditor package requires httpd.
Comment 31 Dridi Boukelmoune 2013-12-01 13:23:10 EST
There are two third_party directories in the current release:
https://github.com/LimeSurvey/LimeSurvey/tree/2.00_plus_131107/third_party
https://github.com/LimeSurvey/LimeSurvey/tree/2.00_plus_131107/application/third_party

The 2.05 betas contain even more bundled libraries. *sigh*

About kcfinder, I've started a quick-n-dirty spec, but I'm having trouble figuring how to handle multi-tenancy. With the current spec, two packages requiring kcfinder will use the same configuration and upload directory.

https://bitbucket.org/dridi/fedora_packages/src/16de96c/kcfinder.spec

Working with upstream may be hard, the latest kcfinder release is from 2011, and the last commit is from 2012... At least limesurvey uses the latest version.

Any ideas ?
Comment 32 Christopher Meng 2014-03-01 13:39:25 EST
I can't see any updates from the submitter since 2012/06, so closing it should be the proper way.

If you want to submit it, please reopen it.
Comment 33 Parag AN(पराग) 2015-01-12 03:14:40 EST
Removing FE-NEEDSPONSOR from the closed review tickets.

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