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. Anybody want to be co-maintainer of this in F7? http://cvs.fedora.redhat.com/viewcvs/devel/squirrelmail/ Description: Squirrelmail is a terrible IMAP webmail client that hasn't improved much at all in the past several years, yet we continue to ship it because we were unable to find anything better to repace it that is also simple and self contained. =(
> 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 is the new owner of squirrelmail.
Looks like mhlavink 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).