Bug 472098 - Review Request: dekiwiki - a powerful opensource wiki which runs on Mono [NEEDINFO]
Summary: Review Request: dekiwiki - a powerful opensource wiki which runs on Mono
Keywords:
Status: ASSIGNED
Alias: None
Product: Fedora
Classification: Fedora
Component: Package Review
Version: rawhide
Hardware: All
OS: Linux
medium
medium
Target Milestone: ---
Assignee: Andreas Thienemann
QA Contact: Fedora Extras Quality Assurance
URL:
Whiteboard:
Depends On:
Blocks:
TreeView+ depends on / blocked
 
Reported: 2008-11-18 18:45 UTC by Mathieu OUDART
Modified: 2020-07-10 00:48 UTC (History)
6 users (show)

Fixed In Version:
Doc Type: Bug Fix
Doc Text:
Clone Of:
Environment:
Last Closed:
Type: ---
tibbs: fedora-review?
package-review: needinfo? (andreas)


Attachments (Terms of Use)

Description Mathieu OUDART 2008-11-18 18:45:34 UTC
Spec URL: http://nightlybuild.mindtouch.com/Submitted_Packages/Fedora/src/dekiwiki.spec
SRPM URL: http://nightlybuild.mindtouch.com/Submitted_Packages/Fedora/src/dekiwiki-8.08.11662-1.1.src.rpm

Description: MindTouch Deki is a free open source wiki and application platform for communities and enterprises. Deki is an easy to use and sophisticated wiki for authoring, aggregating, organizing, and sharing content. Deki is also a platform for creating collaborative applications, or adding wiki capabilities to existing applications.

Thanks for helping us with this package.

Comment 1 David Nalley 2008-11-18 19:45:14 UTC
Hi Mathieu, 

I'll do a quick informal review as I can't sponsor you, but hopefully it will get you in better shape for reviews down the road. 

Using rpmlint:

./dekiwiki.spec:11: W: hardcoded-packager-tag services@mindtouch.com

Fedora packages don't specify the packager for a number of different reasons - so a quick removal of that line will take care of at least one error. 

./dekiwiki.spec:13: W: unversioned-explicit-provides mono(mindtouch.dream)

There is no version number with mindtouch.dream and there should be so that version changes become more obvious. 

./dekiwiki.spec:50: W: setup-not-quiet

No one wants to see all of the output of installation when using an rpm. 

./dekiwiki.spec: E: no-cleaning-of-buildroot %install

It actually looks like you are cleaning the buildroot. It may be looking for the line without the brackets. 

./dekiwiki.spec: W: mixed-use-of-spaces-and-tabs (spaces: line 2, tab: line 155)

Obvious 


In addition I noticed in the installation instructions that you had: 

yum install wv links pdftohtml tidy

You probably want to add those as requires in your spec file. 

Obviously princexml can't be included, but perhaps dompdf or a similar html2pdf app that's licensed as open source as be used.

Comment 2 Mathieu OUDART 2008-11-19 19:34:03 UTC
Thanks a lot David for this review. I rebuilt the package and fixed the rpmlint messages you described.

I didn't change the dependencies for now as these are optional and not really needed to run Deki. I would likely add them as "soft dependencies" or "recommended" but i couldn't find such tag in RPM specs.

Here are the new links :

Spec URL:
http://nightlybuild.mindtouch.com/Submitted_Packages/Fedora/src/dekiwiki.spec
SRPM URL:
http://nightlybuild.mindtouch.com/Submitted_Packages/Fedora/src/dekiwiki-8.08.11680-1.1.src.rpm

Cheers.

Comment 3 Andreas Thienemann 2008-11-20 14:35:56 UTC
David mentioned you'll be needing someone sponsering you. I'll therefore continue with the review.

About the dependencies: RPM as used by fedora currently does not offer soft dependencies. Either they are must be fulfilled or they are not needed.
I'm usually leaning towards including them as hard dependencies for the users sake. Not everyone reads manuals. :)

Some notes about the spec:

Wrapping lines at 78chars makes it much more readable. For Requires or BuildRequires lines, the tag can be repeated several times.

For the sake of readability: It might be worth considering different .spec files for fedora based and opensuse based distros, the %if/%endif conditionals make reading the spec a bit harder.
If you want to keep the single .spec file, at least consider using only one %if %ifelse %endif block and execute all needed actions inside of it instead of repeating the %if %endif construct for a single command.


The mkdir -p/cp combo used in the %install phase can be abbreviated as "install -D -m perms source dest". This way the directories leading up to it are created automatically and usually the install part can be written much more condensed.


The bigger blocker is IMHO the %post script.
Most of the commands it executes are unnecessary and can be implemented during the %install phase. e.g. mkdirs are better done during the %install phase and packaged in the %file section. That way these files are included in the rpmdb and are linked to a package.
chmod commands are equally best done during the %install phase. %attr macros can be used in the %file part of the spec to assign special permissions to single files.

Calling mozroots to change a local certificate store is a bad idea IMHO. This way possibly existing local policies are circumvented in a install script.

Starting mysqld is a bad idea as well. It might be that the real mysqld is on a different machine and the local mysqld is therefore not needed. The dependency on it is acceptable IMHO, but starting it specifically is not. Furthermore, what if the mysqld init script is disabled? Just starting it once will not survice a reboot.

Updating the database schema might be acceptable but the general suggestion is to not execute any SQL commands during a %post script but leave it to the system admin in question. They know their system much better then any packager can ever do.

David's complaint about the %post script echoing messages is perfectly valid. RPM installations but especially upgrades might be performed at night and fully automated. Nobody is going to see these messages. The concept of rpm is unattended installation and this design principle should not be violated.

Can the different %{webroot}/dekiwiki/bin %{webroot}/dekiwiki/editor etc. lines in the %file-section be condensed into one single %{webroot}/dekiwiki/ line?

I'll go about building and installing the software later today for detailed review. So much for my first notes about the .spec file.

Comment 4 Mathieu OUDART 2008-11-21 23:15:31 UTC
Thanks for the input Andreas.

About the dependencies: i've had a quick test on fresh installs for Fedora9, CentOS5 and RHEL5 for the optional dependencies "wv xlhtml poppler-utils html2text" and unfortunately none of them resolves on all 3 systems :(
That would really bother me if any Deki install failed because of these unresolved packages so I'd prefer to let them apart for now.

about the spec: i applied your suggestions.

I kept the %if conditions but tried to make it more readable. It's much more convenient to build from a single source on all platforms.

wiki db update : I kept it for now considering that package updates can be done in the background/automatically. Upgrading the software files but not the db could result in a broken website until the sysadmin takes his actions. For the upgrade to be as safe and smooth as possible, I think it's better to update the wiki db schema at the same time we update the software files.

about the %post : I let you have a look at it, I followed your recommendations.

I now still have to figure out how to have users to complete the installation since I removed mozroots, mysqld start and all the output. Any best practices about that ?

new links :

Spec URL:
http://nightlybuild.mindtouch.com/Submitted_Packages/Fedora/src/dekiwiki.spec
SRPM URL:
http://nightlybuild.mindtouch.com/Submitted_Packages/Fedora/src/dekiwiki-8.08.11722-1.1.src.rpm

Cheers

Comment 5 Mathieu OUDART 2008-12-29 21:25:38 UTC
Here is an update including the latest bits :

Spec URL:
http://nightlybuild.mindtouch.com/Submitted_Packages/Fedora/src/dekiwiki.spec
SRPM URL:
http://nightlybuild.mindtouch.com/Submitted_Packages/Fedora/src/dekiwiki-8.08.12159-1.1.src.rpm

I also had to remove the %elseif statements because they're not supported on all platforms.

Please let me know if there's still some work to do on this package.
Cheers

Comment 6 Jon Stanley 2009-01-04 03:22:56 UTC
A few issues with this package (I haven't done a full review of it yet, these are just some things that stick out):

I have no idea where or how to get this exact source. If this is an svn snapshot (which from the version looks like it is), you'll want to include instructions in the spec file on how to obtain this exact version - i.e. something like 'svn export -r1234 http://svn.example.com/svn/example example ; tar -czvf example.tar.gz example/' or whatever).

You'll also want to version the package per the guidance found at https://fedoraproject.org/wiki/Packaging/NamingGuidelines#Pre-Release_packages

rpmlint complains about the summary not starting with a capital letter, pretty simple to fix.

The changelog is not in a proper format.  You have:

%changelog
*  Mon Dec 29 2008 MindTouch <services@mindtouch.com>
   - 8.08.12159 nightly built

Which should be:

* Mon Dec 29 2008 MindTocuh <services@mindtouch.com> - 0.08.12159
- Nightly built

or something similar. Refer to https://fedoraproject.org/wiki/Packaging/Guidelines#Changelogs fore more info (It looks like you're using the third format sort of, but I've never seen anyone use that in practice and was kind of surprised to see it in the guidelines).

In %post, you're adding users - please see https://fedoraproject.org/wiki/Packaging:UsersAndGroups for guidance on doing this. Similarly, according to the guidelines, we don't want to delete the user for the reasons stated.

You're also installing content into /var/www - according to the guidelines, this should be instead in /usr/share/dekiwiki - see https://fedoraproject.org/wiki/Packaging:Guidelines#Web_Applications for more details.

I also noticed that your logfile is %ghost'ed - I wouldn't do that (but I don't think there's a guideline against it) because the administrator might want to keep the logfile around from the uninstalled package, and it doesn't do anything for logfiles that have been rotated.

I think that's it for now, I'd like someone more familiar with mono than myself to look this over too. The guidelines for mono packages can be found at https://fedoraproject.org/wiki/Packaging:Mono

Comment 7 Mathieu OUDART 2009-01-15 01:06:28 UTC
Thanks for the feedback.

So far our packages intended to make the Wiki work "out of the box" but many things seem to be contradictory with the distribution(s) packaging guidelines.

So we decided to start over and created new packages. The objective is to be as standard as possible. Please have a look at it and let us know what you think.

Here are the links :
SPEC file : http://nightlybuild.mindtouch.com/Submitted_Packages/NEW_RPMS/SRPMS/dekiwiki.spec
SRPM : http://nightlybuild.mindtouch.com/Submitted_Packages/NEW_RPMS/SRPMS/dekiwiki-8.08.2-1.src.rpm
Fedora RPM : http://nightlybuild.mindtouch.com/Submitted_Packages/NEW_RPMS/RPMS/Fedora/dekiwiki-8.08.2-1.noarch.rpm

Any suggestions to make this package better would be greatly appreciated, thanks.

Comment 8 Karsten Wade 2011-12-14 03:45:47 UTC
Removing myself for these bug components as I'm either no longer involved in that aspect of the project, or no longer care to watch this particular bug. Sorry if you are caught in a maelstrom of bug changes as a result!

Comment 9 Package Review 2020-07-10 00:45:28 UTC
This is an automatic check from review-stats script.

This review request ticket hasn't been updated for some time, but it seems
that the review is still being working out by you. If this is right, please
respond to this comment clearing the NEEDINFO flag and try to reach out the
submitter to proceed with the review.

If you're not interested in reviewing this ticket anymore, please clear the
fedora-review flag and reset the assignee, so that a new reviewer can take
this ticket.

Without any reply, this request will shortly be resetted.


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