> Squirrelmail is a terrible IMAP webmail client...

that's funny... I've heard very good things about http://www.roundcube.net/

> that's funny... I've heard very good things about http://www.roundcube.net/

1. Licensed under GPL and bundles code incompatible with GPL.

2. implements features that are not implemented in SquirrelMail because they are
unstable and require too many safety checks. Roundcube implementation of output
character set does not include required safety checks.

3. Less stable than SquirrelMail.

4. Spell check depends on external third party API.

5. PHP array based translations. See horror stories of translation maintainers.

6. Heavy dependency on JavaScript.

(In reply to comment #1)
> > Squirrelmail is a terrible IMAP webmail client...
> 
> that's funny... I've heard very good things about http://www.roundcube.net/

or ilohamail.org ?

Sticking to the review, since the default config gets login/passwords from the
regular accounts on the machine, whay don't you force SSL on the apache.conf file? 

I am regularly doing that on every new install by adding:

<Location /webmail>
  SSLRequireSSL
</Location>

(In reply to comment #3)
> why don't you force SSL on the apache.conf file? 

Bug 183189 - RFE: Ship secure_login plugin by default.

> <Location /webmail>
>  SSLRequireSSL
> </Location>

We can't add this because it would break existing users who upgrade their RPM. 
Is this acceptable?

Adding documentation with an example redirect to SSL syntax, and other HOWTO to
enable SSL, would be doable.

Hi, I'm working on packaging roundcube.
https://bugzilla.redhat.com/bugzilla/show_bug.cgi?id=225575  Which code is
GPL-incompatible?

I found it, I think.  Roundcube ships Googiespell 2.1, which says it's MIT
Licensed, so that's fine.  The current is 3.4, which is GPL.

It also includes code Licensed under PHP License 3.0 and 2.02, which are GPL
incompatible.

Based on
http://fedoraproject.org/wiki/Packaging/Guidelines#head-76294f12c6b481792eb001ba9763d95e2792e825,
I don't think any of this is a problem.  Has been posted to the roundcube bug.

Changed Summary for the Big Merge

*** Bug 226432 has been marked as a duplicate of this bug. ***

(In reply to comment #0)
> Requesting review in preparation for the Fedora merge.  This package is in a
horrible, horrible mess, with language specific patches (more yet coming...) and
a mass encoding change.  Fedora needs some serious help in merging these things
upstream, and convincing upstream to handle the l10n encodings in a
non-braindead way so we don't have to ship with these horrible ugly hacks.
> 

When was the last time you have submitted your SquirrelMail patches to upstream?
I think Fedora SquirrelMail rpm contains several patches that are never
submitted to upstream. And you don't listen when upstream developer warns you
about uninitialized variables in your patches.

You assume that #195639 is specific to Japanese, but same thing happens in
Chinese and Korean. #195639 issue happens only when translation uses euc or gb
or big5 character sets. rawurlencode is needed, if CJK translation is used in utf-8.

You keep CJK translations in their original encodings, because "UTF-8 is
unrealistic". I think you haven't checked SquirrelMail extra decoding library
functions. euc-kr, gb2312 and big5 translations are unrealistic in RedHat,
because SquirrelMail does not support these charsets in encoding functions and
RedHat does not enable PHP extensions (#106755) that allow to implement CJK
encoding functions. If encoding functions are not implemented, you must write
more XTRA_CODE hacks for Chinese and Korean, because without those hacks replies
to gb2312-hz, iso-2022 and utf8 emails are broken in gb2312/big5 Chinese and
euc-kr Korean. UTF-8 is unrealistic only when you want to accept Chinese emails
with mime formating violations.

You ask to push patch to upstream in #209105, when in fact issue is caused by
smart B encoding in some other product. SquirrelMail does not make that type of
encoding decisions in B encoding.

> Squirrelmail is a terrible IMAP webmail client that hasn't improved...

And these words are coming from a person that does not cooperate with upstream
developers and hasn't moved a finger to push Fedora fixes to upstream.

http://search.gmane.org/search.php?query=warren+togami&author=&group=gmane.mail.squirrelmail.user&sort=date&submit=submit
http://search.gmane.org/search.php?query=warren+togami&author=&group=gmane.mail.squirrelmail.internationalization&sort=date&submit=submit
http://search.gmane.org/search.php?query=warren+togami&author=&group=gmane.mail.squirrelmail.devel&sort=date&submit=submit
http://search.gmane.org/search.php?query=warren+togami&author=&group=gmane.mail.squirrelmail.plugins&sort=date&submit=submit

One email about broken patches and one email by other person related to lack of
mbstring support in older redhat packages. Zero emails about #196017, #195639,
#195452, #194457, #194598.

Thank you for explaining the source of our problems.  I was not aware of the
existence of the "extra decoding library" until recently.  I do not fully
understand the implications this yet, but I suspect we did things in a stupid
way in an attempt to workaround individual problems.  Additionally, I did not
have the priority or time to investigate deeply into this software.

I have to admit that my opinions about squirrelmail have been negatively colored
over the past years having been forced to add more and more ugly encoding hacks
to our package due to widespread complaints by our users.  Nobody had pointed me
at the "extra decoding library" and instead continued to pile more patches. 
This led me to believe (perhaps improperly) that squirrelmail was not evolving
to modern standards, hence my "terrible" comment.

To be honest, I still have a negative feeling about squirrelmail.  For years now
I've been wanting to remove it from Fedora and ship something else.  But I can
be convinced otherwise, if squirrelmail is capable of being standards compliant,
usable to users, and easy to maintain.

I need help.  I would like to undo the ugly hacks that we have included due to
our lack of understanding, and ship squirrelmail as close to upstream as
possible.  I guess the first step is to remove all the re-encoding stuff, add
the extra decoding library, and see if any of our patches are relevant for upstream.

Would you be interested to co-maintain the squirrelmail package within Fedora? 
We will soon have the ability to grant you commit and build access to the
package so that we can more easily work together in package maintainership.

I have resigned from SquirrelMail development four months ago. Now I maintain
only Lithuanian SquirrelMail translation, plugins that I wrote and my own forked
SquirrelMail version. I don't have write access to SquirrelMail software
repository and can't push your updates.

I can provide patches similar to squirrelmail 1.4.8.utf8-1
(http://sourceforge.net/project/showfiles.php?group_id=311&package_id=200705)
and improve decoding functions for better CJK support, but these updates will
contain my copyrights. I won't assign copyrights to some other person or
organization, because I don't know how they will react when copyrights are violated.

I don't think that you will like me as maintainer. I will refuse to make core
code modifications that break MIME rfcs. See "when charset Does not exist" mails
on SquirrelMail devel mailing list. MIME violations are quite common. They will
be visible in interface which uses utf8 and not some default country's character
set.

I strongly recommend talking to SquirrelMail devels and trying to push utf-8
updates to stable SquirrelMail 1.4 version or to some branch from 1.4.x.
SquirrelMail tracker has a bug report which can't be fixed without switching to
single interface character set.

I can help Fedora and SquirrelMail in maintenance of utf-8 SquirrelMail version,
if SquirrelMail developers respect my copyrights and updates are included in
some 1.4.x version. I will refuse to work on code included only in squirrelmail
1.5.x/2.x. SquirrelMail devels won't be able to add exemptions to current GPL
v.2 SquirrelMail license, if SquirrelMail HEAD branch contains my code written
after 2006-10-09.

mbacovsk@redhat.com is the new owner of squirrelmail.

Looks like mhlavink@redhat.com owns this package now, so I'll update the CC.  I've been staring at this, the very first merge review, for nearly two years now and would love to see it go away.  I'm not sure if I have what it takes to do a proper review of it, but I can at least look at the current packaging and make some comments.

First off, I wonder how much of the above discussion about Fedora's version being heavily patched is currently true.  I see only four patches applied; one is a one-liner, one is a set of translation changes for, I believe, Turkish, and the other two don't really patch in all that much (under fifteen lines each).  It would indeed be nice to include the upstream status of those patches in accordance with http://fedoraproject.org/wiki/Packaging/Guidelines#All_patches_should_have_an_upstream_bug_link_or_comment but I'm just not seeing how our current version is significantly patched.  Maybe someone did some significant work on things in the last 18 months.  Or maybe the problem is with some of the locale tweaks that happen directly in the specfile.

It would also be nice to clean the patches up a bit, remove the unapplied patches from CVS and get rid of the commented-out bits of the spec where those patches used to be applied.

Here's what current rpmlint has to say about the src.rpm:

  37: E: prereq-use httpd, perl
Prereq: shouldn't be used; something is needed for a scriptlet, it should be
reflected in a fine-grained dependency like Requires(pre).  There are no
scriptlets here, so I think those should be regular runtime dependencies,
specified with Requires:.  Those happen to already be there, so I don't really
see the point in the Prereq: line.


  38: W: unversioned-explicit-provides squirrelmail-i18n
We always want to provide something with a version, because without one it
becomes impossible to ever provide a separate squirrelmail-i18n package.
However, is there anything which might actually depend on squirrelmail-i18n?
Why does it need to be provided by this package?

  217: E: use-of-RPM_SOURCE_DIR
Shouldn't this just refer to %{SOURCE1} instead?

  W: mixed-use-of-spaces-and-tabs (spaces: line 76, tab: line 76)
Nobody cares about this.  Fix it if you like.

and about the built noarch.rpm:

  W: non-standard-uid /var/lib/squirrelmail/prefs apache
  W: non-standard-gid /var/lib/squirrelmail/prefs apache
  E: non-standard-dir-perm /var/lib/squirrelmail/prefs 0700
  W: non-standard-uid /var/spool/squirrelmail/attach apache
  W: non-standard-gid /var/spool/squirrelmail/attach apache
  E: non-standard-dir-perm /var/spool/squirrelmail/attach 0700
  W: non-standard-gid /etc/squirrelmail/config.php apache
  E: non-readable /etc/squirrelmail/config.php 0640
  W: non-standard-gid /etc/squirrelmail/config_local.php apache
  E: non-readable /etc/squirrelmail/config_local.php 0640
  W: non-standard-gid /etc/squirrelmail/default_pref apache
  E: non-readable /etc/squirrelmail/default_pref 0640
  W: non-standard-gid /etc/squirrelmail/sqspell_config.php apache
  E: non-readable /etc/squirrelmail/sqspell_config.php 0640
These are all perfectly OK.

  E: htaccess-file
   /usr/share/squirrelmail/plugins/squirrelspell/modules/.htaccess
Generally it's a bad idea to use a .htaccess file in a package; better to put
such things in /etc/httpd/conf.d/squirrelmail.conf so the admin can see all of
the access restrictions in one place.  It's also common to disable htaccess
checks for reasons of speed.

  W: file-not-utf8 /usr/share/doc/squirrelmail-1.4.17/ChangeLog
It would be significantly better if anyone who gets this package would be able
to actually read the names in the ChangeLog, but such things tend to get messy
and bits in various different character sets are often added at different
times.  Fixing it is generally a manual procedure and needs to be coordinated
with upstream (because generally iconv can't fix these problems and carrying a
patch to the ChangeLog is pointless).  I don't think the package review is the
appropriate place to force that to happen.

  W: wrong-file-end-of-line-encoding
   /usr/share/doc/squirrelmail-1.4.17/ReleaseNotes/1.4/Notes-1.4.12.txt
  W: wrong-file-end-of-line-encoding
   /usr/share/doc/squirrelmail-1.4.17/ReleaseNotes/1.4/Notes-1.4.13.txt
These, however, should be fixed with a couple of quick calls to tr.

  E: zero-length
   /usr/share/squirrelmail/locale/es_ES/LC_MESSAGES/serversidefilter.po.new
Why do we ship this?

  W: devel-file-in-non-devel-package
   /usr/share/squirrelmail/plugins/filters/bulkquery/bulkquery.c
  E: non-executable-script /usr/share/squirrelmail/plugins/demo/getpot 0644
What are these for?  If they're not usable as is, perhaps they should be
removed from the actual squirrelmail installation and packaged as
documentation.

That's it for rpmlint.  I'll look at the spec later if anyone is listening on the other end of this ticket.

plugins/filters/bulkquery/bulkquery.c can be used to compile program, which used in non-default filters plugin configuration. In current stable it is not useful for packaged SquirrelMail versions, because people have to modify files in /usr/share/squirrelmail/plugins/filters/ in order to enable it.

That makes a pretty good case that the bulkquery and demo plugins should not be packaged where they are.  I don't see a problem with moving them to live with the documentation, or with just not packaging them at all.

(In reply to comment #16)
> That makes a pretty good case that the bulkquery and demo plugins should not be
> packaged where they are.

You missed test plugin.

I'll look at this soon

first round done
rmplint squirrelmail.spec is quiet
rmplint squirrelmail-*-src.rpm is quiet

lot of not used/not useful/obsolete patches removed, some of them are not removed from cvs repository *for now*

rpmlint noarch/squirrelmail-*-noarch.rpm is little less verbose (just a little)

> htaccess-file

moved to squirrelmail.conf

> W: wrong-file-end-of-line-encoding
>   /usr/share/doc/squirrelmail-1.4.17/ReleaseNotes/1.4/Notes-1.4.12.txt
> W: wrong-file-end-of-line-encoding
>   /usr/share/doc/squirrelmail-1.4.17/ReleaseNotes/1.4/Notes-1.4.13.txt

fixed with sed in spec

> E: zero-length
>  /usr/share/squirrelmail/locale/es_ES/LC_MESSAGES/serversidefilter.po.new

removed in spec

>  W: devel-file-in-non-devel-package
>   /usr/share/squirrelmail/plugins/filters/bulkquery/bulkquery.c
>  E: non-executable-script /usr/share/squirrelmail/plugins/demo/getpot 0644

I still haven't decided what to do with these two... (not only demo/getpot but demo/*)

1) move to /usr/share/doc/squirrelmail
2) remove completely
3) move to subpackage


----------

Tomas, if you could explain me (irc?) something about the utf-8 vs. CJK mess, it would be really appreciated. I've got this package with some fixes for CJK and that confuses me... I thought utf-8 should be able to handle it, so I'm little bit confused about all CJK workarounds/patches.

(In reply to comment #19)
> Tomas, if you could explain me (irc?) something about the utf-8 vs. CJK mess,
> it would be really appreciated. I've got this package with some fixes for CJK
> and that confuses me... I thought utf-8 should be able to handle it, so I'm
> little bit confused about all CJK workarounds/patches.

If you need explanation about patches in squirrelmail-1.4.16-1.fc10.src.rpm, see information in bugzilla.

squirrelmail-1.4.6-japanese-multibyte-view-body.patch - RH bug #194457
squirrelmail-1.4.6-japanese-multibyte-view-text.patch - RH bug #195452
squirrelmail-1.4.6-zenkaku-subject-convert.patch - RH bug #196017

All patches written by RH/Fedora and information about them was never submitted to upstream. I can't explain why scripts are modified without seeing broken emails. zenkaku-subject-convert.patch might be needed because according to Wikipedia article on Katakana, hankaku (halfwidth form of katakana) is not supported by ISO-2022-JP.

If you need explanation why SquirrelMail Japanese translation works that way - it was written that way by third party (risumail.jp) contributors in SquirrelMail 1.3-dev branch. In SquirrelMail 1.4.4 and older utf-8 and charset conversion support was very simple and limited to message display. Only SquirrelMail 1.4.5 improved charset handling in compose. I suspect that risumail.jp just wanted to get things working and they did it in their own "works for us, lets ignore other part of the world" way.

This package looks quite a bit better now.

Regarding the stuff in the demo directory, I think any of those possibilities would work, although a subpackage is probably overkill.

Regarding the patches, it would be nice to document them somehow (at least adding comments to the spec referring to the above bugzilla tickets) but I'm not really sure it's within the scope of this review to insist that those remaining three patches be reconciled with upstream.  It would certainly be a good idea to work with upstream to somehow make them unnecessary, and of course Fedora is steadfastly against letting more of this kind of thing creep in (see http://fedoraproject.org/wiki/Packaging/Guidelines#All_patches_should_have_an_upstream_bug_link_or_comment,  and http://fedoraproject.org/wiki/PackageMaintainers/WhyUpstream) but I recognize that there is some historical cruft that may not be simple to get rid of.

So really I'd say that if those patches were documented in the spec with links to the above bugzilla tickets, and something is done with the demo directory then I would consider the package OK.  I would still urge further work with upstream and perhaps the previous maintainers of this package to understand the patches and either get them sent upstream or dropped from the package.

new package: squirrelmail-1.4.17-2.fc11

changes:
* plugins/demo moved to /usr/share/doc/squirrelmail
* bugzila numbers for patches
* new patch for big UIDs and 32b systems - with upstream bugzilla link

I'm sorry for my slow response, but I have a lot of work now and I prefer to fix not working things first. Anyway, thanks for doing this review.

Michal

I don't think your response has been at all slow, especially compared with the speed of most merge reviews.  (I have some where I haven't been able to get a response for 18 months.)

I'll go ahead and do a full review now.  Honestly the only thing I can add is that that you can drop the versioned dependencies on php and tmpwatch, since the required versions have been present in every release as far back as I can see (FC1, at least).  Not really a big deal.

* source files match upstream.  sha256sum:
  8fa5b82bb2e4448da80d2ccc42ec9874df8674691358736da6c7c3f7bbbae639  
   all_locales-1.4.13-20071220.tar.bz2
  b14d3ef3735f8c7b49b091533c567048b3a06eb633eccbfeddd7cd8fdd2ffe25  
   squirrelmail-1.4.17.tar.bz2

* package meets naming and versioning guidelines.
* specfile is properly named, is cleanly written and uses macros consistently.
* dist tag is present.
* build root is OK.
* license field matches the actual license.
* license is open source-compatible.
* license text included in package.
* latest version is being packaged.
* BuildRequires are proper.
* %clean is present.
* package builds in mock (rawhide, x86_64).
* package installs properly.
* rpmlint has acceptable complaints
* final provides and requires are sane:
   config(squirrelmail) = 1.4.17-2.fc11
   squirrelmail = 1.4.17-2.fc11
  =
   /bin/sh
   /usr/bin/env
   /usr/sbin/sendmail
   aspell
   config(squirrelmail) = 1.4.17-2.fc11
   httpd
   perl
   perl(Cwd)
?  php >= 4.0.4
   php-mbstring
?  tmpwatch >= 2.8

* owns the directories it creates.
* doesn't own any directories it shouldn't.
* no duplicates in %files.
* file permissions are appropriate.
* no generically named files
* code, not content.
* documentation is small, so no -doc subpackage is necessary.
* %docs are not necessary for the proper functioning of the package.

APPROVED, and closed.  Thanks for your time.

(In reply to comment #23)
> * final provides and requires are sane:
...
> ?  php >= 4.0.4
>    php-mbstring

mbstring extension was introduced in PHP 4.0.6.

SquirrelMail 1.4.x requires PHP 4.1.0 or better. Code has issues in php 4.0.x (some quirks with used objects) and last PHP 4.0.x compatibility code was removed in 1.4.11.

thanks for the review

tmpwatch
rawhide, F10, F9 : 2.9.13
RHEL-3,4,5 : 2.8.4, 2.9.1, 2.9.7

php
rawhide, F10, F9 : 5.2.8
RHEL-3,4,5 : 4.3.2, 4.3.9, 5.1.6


I'll drop versioned dependencies, it seems versions are not needed anymore

Package Change Request
======================
Package Name: squirrelmail
New Branches: EL-6
Owners: mhlavink

cvs done.

Package Change Request
======================
Package Name: squirrelmail
New Branches: epel7
Owners: mhlavink

Git done (by process-git-requests).