Home | Docs | Forums | Lists | Bugs | Planet | Store | GMN | Get Gentoo!

Full Text Bug Listing

Tracked down and fixed many bugs in this ebuild (patch follows):

gentoo-provided docs overriding upstream docs (again - see bug #83779)
upstream has renamed many of the docs and put them into a new subdir (doc),
  which is not being included (and consequently, some of the dodocs are no-ops
  now)
system.log was being created in / instead of LOGDIR
an obvious case of search-and-replace-run-amok resulted in many places where
"--enable-virtual-users-users" was substituted for "--enable-virtual-users",
"virtual-users_users.sql" for "virtual_users.sql", and
"virtual-users users" for "virtual users". I think I got all of them. Note that
  this is also mentioned in bug #85939.
The mandir-related sed-fu didn't match anything, and are not needed anyway
  because the Makefile honors DESTDIR already (I can only assume that at one
  time they did not ;) )
--with-dspam-home is not set because it is skipped if virtual-users is in USE.
  dspam-home needs to be set either way, and it seems likely that this logic was
  actually intended to be applied to --enable-homedir, not --with-dspam-home
  (this is also what was resulting in the creation of the extraneous dir,
  "/usr/var/dspam"). This should fix bug #81596.
Needlessly pervasive keepdirs:
  HOMEDIR (keepdir's purpose is to protect empty directories from being deleted
    by autocleans. HOMEDIR is not empty, so keepdir is unnecessary and
    inappropriate here)
  HOMEDIR/txt (ditto)
  LOGDIR (ditto)
  cron.daily (ditto, plus dspam is not the logical "owner" of /etc/cron.daily,
    anyway - cronbase is, so it's cronbase's job to keepdir this directory
    (which it, in fact, does))
  logrotate.d (ditto cron.daily, but logrotate instead of cronbase)

One other thing I wanted to mention that this patch DOESN'T fix: with all of the storage drivers except mysql and postgres, virtual-users is being enabled regardless of the virtual-users USE flag. I don't know anything about these drivers and don't feel like researching it, but I thought this might actually be intentional, so I'm not touching it. Just wanted to point it out just in case.

Created an attachment (id=54028) [details]
Patch to dspam-3.4.0-r1.ebuild to fix above problems.

Created an attachment (id=54029) [details]
New patch to dspam-3.4.0-r1.ebuild

Found one more bug. dspam.conf was not properly being modified for the mysql
driver (the sed line was missing the "-i" so the output was going to the screen
instead of editing the file itself). This patch includes the fix along with
everything in the first patch.

Oh and if anyone cares, the patched ebuild appears to work for dspam-3.4.1, as
well (which I haven't tested in production).

While we are editing this ebuild, could we please change this part as well?

from:
PGUSER=${DSPAM_PgSQL_USER} PGPASSWORD=${DSPAM_PgSQL_PWD} /usr/bin/psql -d ${DSPAM_PgSQL_DB} -U ${DSPAM_PgSQL_USER} -f ${HOMEDIR}/pgsql_virtual_users.sql 1>/dev/null 2>&1

To
if use neural ; then
	PGUSER=${DSPAM_PgSQL_USER} PGPASSWORD=${DSPAM_PgSQL_PWD} /usr/bin/psql -d ${DSPAM_PgSQL_DB} -U ${DSPAM_PgSQL_USER} -f ${HOMEDIR}/pgsql_virtual_users.sql 1>/dev/null 2>&1
fi

AAGGGRAR!!! Copy & Paste error!!

should be:

from:
PGUSER=${DSPAM_PgSQL_USER} PGPASSWORD=${DSPAM_PgSQL_PWD} /usr/bin/psql -d ${DSPAM_PgSQL_DB} -U ${DSPAM_PgSQL_USER} -f ${HOMEDIR}/pgsql_virtual_users.sql 1>/dev/null 2>&1

to:
if use virtual-users ; then
        PGUSER=${DSPAM_PgSQL_USER} PGPASSWORD=${DSPAM_PgSQL_PWD} /usr/bin/psql -d ${DSPAM_PgSQL_DB} -U ${DSPAM_PgSQL_USER} -f ${HOMEDIR}/pgsql_virtual_users.sql 1>/dev/null 2>&1
fi

Since using just one sed command is common in the ebuild, the initial
configuration building could be as well simplified to:

        # build some initial configuration data
        cp src/dspam.conf ${T}/dspam.conf
        if use cyrus; then
                sed -e 's:^#*\(UntrustedDeliveryAgent\)[\t ]*.*:\1
\"/usr/lib/cyrus/deliver %u\":gI' \
                        -e 's:^\(TrustedDeliveryAgent\)[\t ]*.*:\1
\"/usr/lib/cyrus/deliver\":gI' \
                        -i ${T}/dspam.conf
        elif use exim; then
                sed -e 's:^#*\(UntrustedDeliveryAgent\)[\t ]*.*:\1
\"/usr/sbin/exim -oMr spam-scanned %u\":gI' \
                        -e 's:^\(TrustedDeliveryAgent\)[\t ]*.*:\1
\"/usr/sbin/exim -oMr spam-scanned %u\":gI' \
                        -i ${T}/dspam.conf
        elif use maildrop; then
                sed -e 's:^#*\(UntrustedDeliveryAgent\)[\t ]*.*:\1
\"/usr/bin/maildrop -d %u\":gI' \
                        -e 's:^\(TrustedDeliveryAgent\)[\t ]*.*:\1
\"/usr/bin/maildrop\":gI' \
                        -i ${T}/dspam.conf
        elif use procmail; then
                sed -e 's:^#*\(UntrustedDeliveryAgent\)[\t ]*.*:\1
\"/usr/bin/procmail -d %u\":gI' \
                        -e 's:^\(TrustedDeliveryAgent\)[\t ]*.*:\1
\"/usr/bin/procmail":gI' \
                        -i ${T}/dspam.conf
        else
                sed -e 's:^#*\(UntrustedDeliveryAgent\)[\t ]*.*:\1
\"/usr/sbin/sendmail\":gI' \
                        -e 's:^\(TrustedDeliveryAgent\)[\t ]*.*:\1
\"/usr/sbin/sendmail\":gI' \
                        -i ${T}/dspam.conf
        fi

The Oracle and SQLite part should honor the new virtual-users USE flag as well:

	elif use oci8 ; then
		myconf="${myconf} --with-storage-driver=ora_drv"
		myconf="${myconf} --with-oracle-home=${ORACLE_HOME}"
		use virtual-users && myconf="${myconf} --enable-virtual-users"

		# I am in no way a Oracle specialist. If someone knows
		# how to query the version of Oracle, then let me know.
		if (expr ${ORACLE_HOME/*\/} : 10 1>/dev/null 2>&1); then
			myconf="${myconf} --with-oracle-version=10"
		fi
	elif use sqlite3 ; then
		myconf="${myconf} --with-storage-driver=sqlite3_drv"
		use virtual-users && myconf="${myconf} --enable-virtual-users"
	elif use sqlite ; then
		myconf="${myconf} --with-storage-driver=sqlite_drv"
		use virtual-users && myconf="${myconf} --enable-virtual-users"
	else

Created an attachment (id=54085) [details]
Yet another updated patch

I've added in the changes from comment #6 and made similar changes in a couple
of other places as well (as long as I'm providing patches, I thought I'd help
out as much as possible).

Regarding comment #5 and comment #7, as I said originally, I didn't touch this
because I thought it might actually be intentional for some reason beyond my
ken. At this point, I'm pretty sure that's not the case, though, so this patch
rolls in those changes, as well.

Created an attachment (id=54090) [details]
1/Xth patch update, as X approaches zero (j/k)

Sigh...found a still more. These are in pkg_config:

For mysql, postgres, and oracle, there were lines like:

  [[ -f ${HOMEDIR}/mysql.data ]] && mv -f ${HOMEDIR}/mysql.data ${HOMEDIR}

I'm not sure what was intended, but it definitely does nothing to move a file
from one directory into the same directory (running the ebuild config says as
much ;) ) I've removed these.

In the mysql section, there were array variables like $DSPAM_DB_DATA[2]. The
{}'s are required with this syntax. (i.e. "${DSPAM_DB_DATA[2]"). This was
causing ebuild config to fail completely.

As usual, this is a cumulative patch for all problems mentioned in this bug.

Please do not optimize this part:
-	sed -i "s:^\(Purge.*\):###\1:g" ${T}/dspam.conf
-	sed -i "s:^#\(Purge.*\):\1:g" ${T}/dspam.conf
-	sed -i "s:^###\(Purge.*\):#\1:g" ${T}/dspam.conf
+	sed -e "s:^\(Purge.*\):###\1:g" \
+		-e "s:^#\(Purge.*\):\1:g" \
+		-e "s:^###\(Purge.*\):#\1:g" \
+		-i ${T}/dspam.conf

This MUST be runing in sequential order. Actualy this does disable the normal purge commands and switches to the one made for SQL backends. I think the above command will work the way it should work, but we need to test it, before rushing again and publishing it into CVS.

Okay... I tested this:
-       sed -i "s:^\(Purge.*\):###\1:g" ${T}/dspam.conf
-       sed -i "s:^#\(Purge.*\):\1:g" ${T}/dspam.conf
-       sed -i "s:^###\(Purge.*\):#\1:g" ${T}/dspam.conf
+       sed -e "s:^\(Purge.*\):###\1:g" \
+               -e "s:^#\(Purge.*\):\1:g" \
+               -e "s:^###\(Purge.*\):#\1:g" \
+               -i ${T}/dspam.conf

It is okay. It can be summarized and does not to call 3 times the sed command. One allone is okay.

I actually did think about that, but if I'm not mistaken, multiple -e options
to sed *still* cause them to be executed in a deterministic sequential order
(per-line in this case), so I was 95% (-ish ;D) sure it was safe. Thanks for
confirming.

Okay... now change please:

from:
einfo "Pleae read your dspam.conf, oracle.data and 

to:
einfo "Please read your dspam.conf, oracle.data and 


cheers

SteveB


btw: If you look at the ebuilds of Lim, then you will realize that he does not much like if statements :)

If you could change:

from:
		if has_version sys-kernel/linux26-headers; then
			myconf="${myconf} --enable-daemon"
		fi


to:
has_version sys-kernel/linux26-headers && myconf="${myconf} --enable-daemon"


then I think he would be very happy....

Those fixes seem much more complicated and scare me. :P

But seriously, I might actually roll in a few spelling corrections if I do end up updating this patch due to another functional bug. :)

Can we add a change to make the permissions of /usr/bin/dspam 6755 instead of
4755? Then you can add apache to the "dspam" group and don't have to do a bunch
of suidexec stuff for it to access the dspam data when running dspam-web since
everything will belong to the "dspam" group.

The original thought for this bug was to help with some bug fixes and cleanup
in the ebuild. I don't want to go changing actual functionality (I feel like I
may have overstepped a bit already with the sed cleanups). Especially since the
actual maintainer has not yet chimed in on this patch. You should probably
create a new bug with the permissions suggestion, Manuel.

No! Why a new bug report? It is fine to post/ask the permission problem to be
fixed in this bug.

Created an attachment (id=54274) [details]
Now for the new dspam-3.4.1.ebuild

I see that st_lim added a new ebuild for dspam-3.4.1 to the tree, with almost
none of these bugs fixed (two of the lines with the "virtual_users-users" typos
were fixed, but those were the only changed. Here's a new patch against
dspam-3.4.1.ebuild to fix the rest.

Lim Swee Tat!! Are you there? Is this thing on? :P

I'm not sure if it is related to missing if clauses (and if it is fixed in
patch provided above), in the original Ebuild there is no way to get dspam
compiled without all that sql stuff, just plain old libdb4.
Atleast offer the option to upgrade to sqlite etc.
If any old user with old libdb4 databases upgrades now, he can't access his old
spam dbs anymore.

Most amavisd-new users with dspam functionality enabled will probably stil have
the old libdb4 dbs, regardless if it's not "good" to use it anymore and all the
others are  way faster.

I noticed that plain db support is missing, too. I would like to stay on db as
well. Anyway, to convert the db to SQL you can use dspam_2sql.

Yes. Only SQL based database storage is available in the current DSPAM ebuild.
But internaly DSPAM still allows to use Barkley DB. We need to change one block
inside the ebuild to still allow Barkley DB.


From:
        elif [ "${multiple_dbs}" -eq "0" ]; then
                echo
                ewarn "You need to select at least one database backend in your
USE flags."
                ewarn "Please enable one of the following USE flags:"
                ewarn "  ${supported_dbs}"
                echo
                die "Database support missing"


To:
        elif [ "${multiple_dbs}" -eq "0" ]; then
                echo
                ewarn "You did not select any SQL based database backend. DSPAM
will use"
                ewarn "Berkeley DB for storing data. If you don't want that,
then enable"
                ewarn "one of the following USE flags:"
                ewarn "  ${supported_dbs}"
                echo
                ewarn "Waiting 30 seconds before starting..."
                ewarn "(Control-C to abort)..."
                epause 30




cheers

Steve

Hi,
  This is just great. :)

Ciao
ST Lim

Re: comment #21...wouldn't it be better to add support for the berkdb USE flag
instead of it being implicit (and adding an 'if use berkdb...conf...' line in
the appropriate spot)?

This has been fixed for some time.  But the last comment just kinda caught me
there.