Created attachment 3550 [details]
trivial patch

Thanks for making a ticket.

I'm -1 on the patch -- updates and def_rules_dir are separate things.  However,
I do think that Mail::SpamAssassin ought to have a vague default for at least
LOCAL_STATE_DIR if it's not passed in.  Unfortunately, this was an API change
from 3.1.0 due to the massive change from how sa-update was designed to work
originally. :(

I'm pretty sure the patch won't work for Windows where the default local state
dir created in ActivePerl 5.8.x is c:\perl\site\var\spamassassin. c:\perl\site
gets added as a prefix, so somehow /var/spamassassin would have to be specified
as the local state directory, I think. 

Created attachment 3624 [details]
first suggested patch

Ok, here's a first version.  I'm hoping for some comments before I commit it to
3.2 and let it get officially reviewed for 3.1.

There are four things addressed here:

1) the local state dir was an API addition made in 3.1.1 due to the change to
how sa-update was expected to function.  however, no default was set such that
third party tools had to keep up with the API change.  I added in defaults for
DEF_RULES_DIR, LOCAL_RULES_DIR, and LOCAL_STATE_DIR into M::SA::new() that get
replaced at "make" time.  This way, even if the caller doesn't pass them in,
the defaults will be usable.

2) There was no documentation for the path variables passed into M::SA::new(),
specifically PREFIX, DEF_RULES_DIR, LOCAL_RULES_DIR, and LOCAL_STATE_DIR.  I
added some, though somewhat terse.

3) There was no documented way to pass in a different local state dir ala
rules_filename, site_rules_filename, etc.  After looking at the code and seeing
no way to change the default rules directory other than changing the passed in
DEF_RULES_DIR, I decided to not add a new option local_state_directory and did
#2 instead, documenting LOCAL_STATE_DIR.

4) Even if someone figured out #3 (passing in LOCAL_STATE_DIR), SpamAssassin
would not use that specific directory for updates, instead looking at
local_state_dir/spamassassin/version instead.  I added in the local state
directory itself in between the previous directory and def_rules_dir.


I did not want to add another option to "spamassassin", "spamd", etc, for
"updatedir" or "local_state_dir".  Which means that from the commandline
there's no easy way to override where SA looks for these files.  I think the
alternate method of adding "include /alternate/path/update.cf" to the
appropriate config solves this issue, though I could be convinced that another
commandline option is the way to go instead.


thoughts, comments, rotten vegetables?

hmm... I wanted to avoid sedding the .pm files during compilation, but I see we
already do that for @@PREFIX@@. :(

I'm not sure -- it seems a bit icky.  need to think about this more....

for now I'm going to put this in review state.

(In reply to comment #4)
Patch works here on Windows 2003 and XP with no changes to the configuration.

Created attachment 3634 [details]
suggested patch

this is the same as 3624, except it adds in another line of documentation.

ok, I've come around to it ;)

+0.9.  there is still an issue, with:

  +=item PREFIX
  +
  +Used as the root for certain directory paths such as:
  +
  +  '__prefix__/etc/mail/spamassassin'
  +  '__prefix__/etc/spamassassin'
  +
  +Defaults to "@@PREFIX@@".


by default, that comes out as

  +=item PREFIX
  +
  +Used as the root for certain directory paths such as:
  +
  +  '__prefix__/etc/mail/spamassassin'
  +  '__prefix__/etc/spamassassin'
  +
  +Defaults to "/usr/local".

this is misleading -- nobody (except for the odd BSD person ;) will
have encountered /usr/local/etc/spamassassin.  a better example
would be the entry from @default_rules_path:

  +=item PREFIX
  +
  +Used as the root for certain directory paths such as:
  +
  +  '__prefix__/share/spamassassin'
  +
  +Defaults to "/usr/local".

a little nitpicky, sorry.  I'm striving for perfection -- in your code ;)

(In reply to comment #9)
> ok, I've come around to it ;)

:)

> +0.9.  there is still an issue, with:
> 
>   +=item PREFIX
>   +
>   +Used as the root for certain directory paths such as:
>   +
>   +  '__prefix__/etc/mail/spamassassin'
>   +  '__prefix__/etc/spamassassin'
>   +
>   +Defaults to "/usr/local".
> 
> this is misleading -- nobody (except for the odd BSD person ;) will
> have encountered /usr/local/etc/spamassassin.  a better example
> would be the entry from @default_rules_path:

I don't understand how this is misleading.  Just because most people don't have
that directory doesn't mean that we don't look for it, first on the list actually:

@site_rules_path = (
  '__local_rules_dir__',
  '__prefix__/etc/mail/spamassassin',
  '__prefix__/etc/spamassassin',
  '/usr/local/etc/spamassassin',
  '/usr/pkg/etc/spamassassin',
  '/usr/etc/spamassassin',
  '/etc/mail/spamassassin',
  '/etc/spamassassin',
);

>   +=item PREFIX
>   +
>   +Used as the root for certain directory paths such as:
>   +
>   +  '__prefix__/share/spamassassin'
>   +
>   +Defaults to "/usr/local".
> 
> a little nitpicky, sorry.  I'm striving for perfection -- in your code ;)

we can change it if you really think it's an issue, but I don't think most
people have encountered /usr/local/share/spamassassin either.


However, I came up with my own -1 on my patch. ;)

Adding in __local_state_dir__ by itself means that in a standard installation:

       /var/lib/spamassassin/3.002000
       /var/lib
       /usr/share/spamassassin

which is obviously bad.  So I'm not sure how to address #4 in my list, which
kind of sucks.  There are two things that I can think of immediately.  1) We
change how local_state_dir is defined by default to be <whatever>/spamassassin.
 Alternately, the original list for rules files was:

  '__local_state_dir__/spamassassin/__version__',
  '__def_rules_dir__',

So we could document that people can pass in a different DEF_RULES_DIR, and then
things will work as expected.  It's a bit confusing though.

I think I'll go with the first one, patch in a minute.  (patches while you wait?)

Created attachment 3637 [details]
suggested patch (2)

fixes attachment 3634 [details] by modifying the default local_state_dir to include
'spamassassin' as a directory (ie: /var/lib -> /var/lib/spamassassin), which
required other parts of the code doing "local_state_dir/spamassassin/version"
to be modified to the imo more correct anyway "local_state_dir/version".

JM: I left the doc section you have a problem with the same.  just remember,
docs can be changed CTR. :)

oh bleep.

so there's still a problem with 3637, which is that if people were using
sa-update before (ie: there's a local_state_dir already), and SA is upgraded,
then sa-update  needs to be run before SA is useful again (otherwise it finds
/var/lib/spamassassin, which has no *.cf or *.pre because it only has 3.*
directories in it...)

I hate local_state_dir. <grumble>

Created attachment 3638 [details]
suggested patch (3)

backs out my #4 addition of "__local_state_dir__" to the list of default rules
areas.	the original request in the ticket is fulfilled, there's a default for
local_state_dir now which does the right thing for third party code.

if someone uses sa-update --updatedir and wants to use that new directory, via
the code they can use def_rules_dir and aim at it, or via commandline use -C.

same caveat for jm's doc issue as 3637...

It occurred to me a few moment ago that the latest patch still has:

+=item @@LOCAL_STATE_DIR@@

in it, which is no longer valid in that patch.  I'm not going to make yet
another patch to fix it since it's just documentation, so let's remember to fix
it when committing. <Sigh>

Re comment #12.

I don't have an opinion here, but I do have an observation:

When I've found myself or others flailing around like this with a simple 
solution that just keeps not being simple, it often helps to conclude that 
maybe the whole simple idea was wrong in the first place, take a couple steps 
back, pause a few moments, and think the whole problem set (not SOLUTION set) 
through from the top again.  Make sure that ALL of the problem are listed this 
time, not just some of them.  Very often at that point a truely simple solution 
emerges that is completely different from the previous attempts.

Loren -- dude, that's not helpful  -- just *patronising*. ;)

Apologies, it wan't meant that way.  I meant what I said - at times when every 
time I fix something something else comes up and causes an 'oh darn', it 
sometimes helps to just take a step back and a deep breath and say: "Ok, is 
there some other way to get the result I want?  What result am I *really* 
trying to achieve?  A lot of the time I realize I wasn't really trying to solve 
the right problem in the first place, and that was why solutions kept having 
problems.  Maybe that doesn't happen for other people; except that I know it 
has happened a few times over the years with other people I've worked with, as 
well as with myself.  Maybe they were unique and I'm being presumptious that it 
might ever happen with someone else.

Now, maybe that isn't the case here.  Maybe there isn't any other solution to 
the problem.  But as I read over this rather long history of things here, that 
start with the comment in #2 of "trivial patch" and go on getting more and more 
complex from there, I have to think: waitaminute, there is somethinng that 
doesn't seem right here!

I personally don't have a solution, trivial or otherwise.  I wish I did; I'm 
sorry, I don't.  But what my mind is telling me as I read this patch history 
is: "Maybe the problem here isn't how to get local_state_dir to work.  Maybe 
the problem *IS* local_state_dir".  I keep wondering: how would we solve this 
problem if local_state_dir didn't exist?  Would the solution be simpler?

Now, maybe that isn't the right solution either.  But I can say that if I were 
working on this, about now I'd be taking a step back and asking myself if that 
might be the right solution.  And I'd doubtless be considering two or three 
other completely off the wall possibililties also.

well, the idea of a local_state_dir, storing the ruleset in /var/lib is the
cleanest option, given the other constraints that have to be considered.  For
example, the Linux FHS mandates that as the sensible location for such files,
and you see that clamav uses the same location too (/var/lib/clamav).   With the
addition of sa-update and frequent, downloaded ruleset updates, a local-state
directory was the only correct way to implement that.  if you go back through
the dev list archive to the discussions with Warren, that's readable there.

I don't think there's a more sensible alternative -- worth noting that, in
particular, /etc is not the right place to store frequent updates (again, see
the discussion a few months back where this was covered).

also, this isn't actually "flailing around" -- it's just theo making a
succession of small changes.  that's fine -- it's just iterative development ;)

Ok, updated rules have to be in var; fair enough.  But then that begs the 
question, should the SA installed rule base get installed into var in the first 
place?  Rather than being installed one place and then, to the mystery of many, 
completely overridden into another place on the first sa_update?

One can argue, "well, some people might not update it so for them it isn't 
variable".  One can argue "traditionally it hasn't been there, so moving it 
would be a surprise".  (And it seems that it is, since it not only moves, the 
name changes.)  One can argue "there are lots of legacy installations where it 
is somewhere else before the first sa_update run".

There are lots of considerations and arguments, and I just have this feeling 
that not all of the possibilities have been considered or argued completely.  

Perhaps the soltution should be, not to add another directory to the default 
rules path, but to outright move an existing directory.  For instance, there 
could be an sa_update_install that would move the system rules (that are going 
to be replaced anyway) out of lib/xxx or wherever they are 
into /var/spamassasssin/rules or the like, delete the old directory, and 
install a link to the new directory to eliminate confusion for the first few 
months.  Maybe install a Readme file in the directory describing why and where 
the directory moved.  

Installing a link would help eliminate initial administrator confusion, and 
possibly program confusion if there is non-SA stuff that is looking in or for 
the moved rules.  Having an sa_update_install program (rather than having magic 
happen on the first sa_update run) would notify admins that something is going 
to change in the way things work.  It would also eliminate (perhaps trivial 
amounts) of code from sa_update itself.

+1

+1

Sending        Makefile.PL
Sending        lib/Mail/SpamAssassin.pm
Sending        sa-update.raw
Sending        spamassassin.raw
Transmitting file data ....
Committed revision 433047.

:)

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