since in 3.1 we don't even report an error on that line...  moving to the 3.1 queue.

btw, there are better error messages in 3.1, if the code actually validates the input.  the standard ones do, 
the options that have its own code may or may not.  as an example:

[25654] warn: config: SpamAssassin failed to parse line, "" is not valid for "required_score", skipping: 
required_score

bumping up priority...

Subject: Re:  report_safe errors give crummy error messages

> btw, there are better error messages in 3.1, if the code actually
validates the input.  the standard ones do,
> the options that have its own code may or may not.  as an example:

Yup, knew that.  Didn't recall which release that had been cleaned up in,
but figured this one might have been missed.

Fixed in r171190.

the issue is larger than report_safe.  perhaps I didn't make that as clear as I could have.

we need to go through all the default config options and make sure they appropriately check values and 
return errors on failure.  it's not a blocking issue really, but in some places we're more lax than I think 
we should be.

a quick run through shows: allow_user_rules, score, possible the whitelist/blacklist stuff, 
rewrite_header, trusted_networks, internal_networks, bayes_ignore_header, report_safe_copy_headers, 
redirector_pattern, version_tag, loadplugin, replace_rules, spamcop_from_address, 
spamcop_to_address, uridnsbl, urirhsbl, urirhssub, uridnsbl_skip_domain.

they all, in some form, avoid returning errors.  either we ignore lines with blank parameters, or we don't 
check values before pushing onto arrays, etc.

revision 171195:

check for blank values for: bayes_ignore_header, report_safe_copy_headers,
allow_user_rules, redirector_pattern, version_tag, loadplugin

validate: allow_user_rules, redirector_pattern, loadplugin

ok, r178134 is the first blast at a test for this.  please feel free to hack
away, esp in adding support for the *many* other params we're not checking :(



not ok 4
(wanted: (?-xism:failed to parse line, (?:no value provided|"" is not valid) for
"score", skipping:))
not ok 5
(wanted: (?-xism:failed to parse line, (?:no value provided|"" is not valid) for
"whitelist_from", skipping:))
not ok 6
(wanted: (?-xism:failed to parse line, (?:no value provided|"" is not valid) for
"whitelist_from", skipping:))
not ok 7
(wanted: (?-xism:failed to parse line, (?:no value provided|"" is not valid) for
"whitelist_from_rcvd", skipping:))
not ok 8
(wanted: (?-xism:failed to parse line, (?:no value provided|"" is not valid) for
"def_whitelist_from_rcvd", skipping:))
not ok 9
(wanted: (?-xism:failed to parse line, (?:no value provided|"" is not valid) for
"whitelist_allows_relays", skipping:))
not ok 10
(wanted: (?-xism:failed to parse line, (?:no value provided|"" is not valid) for
"unwhitelist_from_rcvd", skipping:))
not ok 11
(wanted: (?-xism:failed to parse line, (?:no value provided|"" is not valid) for
"blacklist_from", skipping:))
not ok 12
(wanted: (?-xism:failed to parse line, (?:no value provided|"" is not valid) for
"blacklist_from", skipping:))
not ok 13
(wanted: (?-xism:failed to parse line, (?:no value provided|"" is not valid) for
"whitelist_to", skipping:))
not ok 14
(wanted: (?-xism:failed to parse line, (?:no value provided|"" is not valid) for
"more_spam_to", skipping:))
not ok 15
(wanted: (?-xism:failed to parse line, (?:no value provided|"" is not valid) for
"all_spam_to", skipping:))
not ok 16
(wanted: (?-xism:failed to parse line, (?:no value provided|"" is not valid) for
"blacklist_to", skipping:))
not ok 17
(wanted: (?-xism:failed to parse line, (?:no value provided|"" is not valid) for
"rewrite_header", skipping:))
ok 18
ok 19
not ok 20
(wanted: (?-xism:failed to parse line, (?:no value provided|"" is not valid) for
"clear_headers", skipping:))
ok 21
not ok 22
(wanted: (?-xism:failed to parse line, (?:no value provided|"" is not valid) for
"ok_locales", skipping:))
not ok 23
(wanted: (?-xism:failed to parse line, (?:no value provided|"" is not valid) for
"trusted_networks", skipping:))
not ok 24
(wanted: (?-xism:failed to parse line, (?:no value provided|"" is not valid) for
"clear_trusted_networks", skipping:))
not ok 25
(wanted: (?-xism:failed to parse line, (?:no value provided|"" is not valid) for
"internal_networks", skipping:))
not ok 26
(wanted: (?-xism:failed to parse line, (?:no value provided|"" is not valid) for
"clear_internal_networks", skipping:))
not ok 27
(wanted: (?-xism:failed to parse line, (?:no value provided|"" is not valid) for
"always_trust_envelope_sender", skipping:))
not ok 28
(wanted: (?-xism:failed to parse line, (?:no value provided|"" is not valid) for
"skip_rbl_checks", skipping:))
ok 29
ok 30
not ok 31
(wanted: (?-xism:failed to parse line, (?:no value provided|"" is not valid) for
"use_bayes", skipping:))
not ok 32
(wanted: (?-xism:failed to parse line, (?:no value provided|"" is not valid) for
"use_bayes_rules", skipping:))
not ok 33
(wanted: (?-xism:failed to parse line, (?:no value provided|"" is not valid) for
"bayes_auto_learn", skipping:))
ok 34
not ok 35
(wanted: (?-xism:failed to parse line, (?:no value provided|"" is not valid) for
"bayes_ignore_from", skipping:))
not ok 36
(wanted: (?-xism:failed to parse line, (?:no value provided|"" is not valid) for
"bayes_ignore_to", skipping:))
ok 37
ok 38
not ok 39
(wanted: (?-xism:failed to parse line, (?:no value provided|"" is not valid) for
"bayes_learn_during_report", skipping:))
not ok 40
(wanted: (?-xism:failed to parse line, (?:no value provided|"" is not valid) for
"bayes_sql_override_username", skipping:))
not ok 41
(wanted: (?-xism:failed to parse line, (?:no value provided|"" is not valid) for
"bayes_use_hapaxes", skipping:))
ok 42
ok 43
not ok 44
(wanted: (?-xism:failed to parse line, (?:no value provided|"" is not valid) for
"bayes_auto_expire", skipping:))
not ok 45
(wanted: (?-xism:failed to parse line, (?:no value provided|"" is not valid) for
"bayes_learn_to_journal", skipping:))
ok 46
not ok 47
(wanted: (?-xism:failed to parse line, (?:no value provided|"" is not valid) for
"fold_headers", skipping:))
ok 48
not ok 49
(wanted: (?-xism:failed to parse line, (?:no value provided|"" is not valid) for
"envelope_sender_header", skipping:))
not ok 50
(wanted: (?-xism:failed to parse line, (?:no value provided|"" is not valid) for
"descriptions", skipping:))
not ok 51
(wanted: (?-xism:failed to parse line, (?:no value provided|"" is not valid) for
"report_charset", skipping:))
not ok 52
(wanted: (?-xism:failed to parse line, (?:no value provided|"" is not valid) for
"report_contact", skipping:))
not ok 53
(wanted: (?-xism:failed to parse line, (?:no value provided|"" is not valid) for
"report_hostname", skipping:))
ok 54
ok 55
ok 56
ok 57
ok 58
ok 59
ok 60
ok 61
ok 62
ok 63
ok 64
not ok 65
(wanted: (?-xism:failed to parse line, (?:no value provided|"" is not valid) for
"test", skipping:))
not ok 66
(wanted: (?-xism:failed to parse line, (?:no value provided|"" is not valid) for
"bayes_path", skipping:))
ok 67
ok 68
not ok 69
(wanted: (?-xism:failed to parse line, (?:no value provided|"" is not valid) for
"bayes_sql_dsn", skipping:))
not ok 70
(wanted: (?-xism:failed to parse line, (?:no value provided|"" is not valid) for
"bayes_sql_username", skipping:))
not ok 71
(wanted: (?-xism:failed to parse line, (?:no value provided|"" is not valid) for
"bayes_sql_password", skipping:))
not ok 72
(wanted: (?-xism:failed to parse line, (?:no value provided|"" is not valid) for
"bayes_sql_username_authorized", skipping:))
not ok 73
(wanted: (?-xism:failed to parse line, (?:no value provided|"" is not valid) for
"user_scores_dsn", skipping:))
not ok 74
(wanted: (?-xism:failed to parse line, (?:no value provided|"" is not valid) for
"user_scores_sql_username", skipping:))
not ok 75
(wanted: (?-xism:failed to parse line, (?:no value provided|"" is not valid) for
"user_scores_sql_password", skipping:))
not ok 76
(wanted: (?-xism:failed to parse line, (?:no value provided|"" is not valid) for
"user_scores_sql_custom_query", skipping:))
not ok 77
(wanted: (?-xism:failed to parse line, (?:no value provided|"" is not valid) for
"user_scores_ldap_username", skipping:))
not ok 78
(wanted: (?-xism:failed to parse line, (?:no value provided|"" is not valid) for
"user_scores_ldap_password", skipping:))
ok 79
ok 80
ok 81
not ok 82
(wanted: (?-xism:failed to parse line, (?:no value provided|"" is not valid) for
"uridnsbl", skipping:))
not ok 83
(wanted: (?-xism:failed to parse line, (?:no value provided|"" is not valid) for
"urirhsbl", skipping:))
not ok 84
(wanted: (?-xism:failed to parse line, (?:no value provided|"" is not valid) for
"urirhssub", skipping:))
not ok 85
(wanted: (?-xism:failed to parse line, (?:no value provided|"" is not valid) for
"uridnsbl_skip_domain", skipping:))
ok 86
not ok 87
(wanted: (?-xism:failed to parse line, (?:no value provided|"" is not valid) for
"hashcash_doublespend_path", skipping:))
ok 88
not ok 89
(wanted: (?-xism:failed to parse line, (?:no value provided|"" is not valid) for
"hashcash_accept", skipping:))
not ok 90
(wanted: (?-xism:failed to parse line, (?:no value provided|"" is not valid) for
"whitelist_from_spf", skipping:))
not ok 91
(wanted: (?-xism:failed to parse line, (?:no value provided|"" is not valid) for
"def_whitelist_from_spf", skipping:))
not ok 92
(wanted: (?-xism:failed to parse line, (?:no value provided|"" is not valid) for
"use_pyzor", skipping:))
ok 93
ok 94
ok 95
not ok 96
(wanted: (?-xism:failed to parse line, (?:no value provided|"" is not valid) for
"pyzor_path", skipping:))
not ok 97
(wanted: (?-xism:failed to parse line, (?:no value provided|"" is not valid) for
"spamcop_from_address", skipping:))
not ok 98
(wanted: (?-xism:failed to parse line, (?:no value provided|"" is not valid) for
"spamcop_to_address", skipping:))
ok 99
not ok 100
(wanted: (?-xism:failed to parse line, (?:no value provided|"" is not valid) for
"use_auto_whitelist", skipping:))
ok 101
not ok 102
(wanted: (?-xism:failed to parse line, (?:no value provided|"" is not valid) for
"user_awl_sql_override_username", skipping:))
not ok 103
(wanted: (?-xism:failed to parse line, (?:no value provided|"" is not valid) for
"auto_whitelist_factory", skipping:))
not ok 104
(wanted: (?-xism:failed to parse line, (?:no value provided|"" is not valid) for
"auto_whitelist_path", skipping:))
not ok 105
(wanted: (?-xism:failed to parse line, (?:no value provided|"" is not valid) for
"auto_whitelist_db_modules", skipping:))
ok 106
not ok 107
(wanted: (?-xism:failed to parse line, (?:no value provided|"" is not valid) for
"user_awl_dsn", skipping:))
not ok 108
(wanted: (?-xism:failed to parse line, (?:no value provided|"" is not valid) for
"user_awl_sql_username", skipping:))
not ok 109
(wanted: (?-xism:failed to parse line, (?:no value provided|"" is not valid) for
"user_awl_sql_password", skipping:))
not ok 110
(wanted: (?-xism:failed to parse line, (?:no value provided|"" is not valid) for
"user_awl_sql_table", skipping:))
ok 111
ok 112
ok 113
ok 114
ok 115
ok 116
ok 117
not ok 118
(wanted: (?-xism:failed to parse line, (?:no value provided|"" is not valid) for
"replace_rules", skipping:))
not ok 119
(wanted: (?-xism:failed to parse line, (?:no value provided|"" is not valid) for
"replace_start", skipping:))
not ok 120
(wanted: (?-xism:failed to parse line, (?:no value provided|"" is not valid) for
"replace_end", skipping:))

grabbing bug

r178359 validates the majority of the config options.  There may be a few that
could be a little stricter and there are probably some missing for optional
modules like Razor2 (I'll test those out tomorrow), so the test is still disabled.

The last commit was missing the updated test script -- updated in r178361.

After taking a quick look at all of the plugins set_config subs, I believe all
of their config options are validated -- at least minimally.  TextCat's string
config options could use stricter validation though (right now it only checks
that the string isn't blank).

I'm also unsure of what is intended with the test t/whitelist_to.t.

Subject: Re:  many config options are not validated

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

> 
> I'm also unsure of what is intended with the test t/whitelist_to.t.
> 

I was just looking at this file.

The test is obviously wrong and is passing due to using an email that
contains the test string.

Changing the whitelist-to to what I thought should be the right
whitelist_to still issues an error.  Maybe I'm missing something.  There
is for sure a problem here.

Michael
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.2.2 (GNU/Linux)
Comment: Using GnuPG with Thunderbird - http://enigmail.mozdev.org

iD8DBQFClUFUG4km+uS4gOIRAoL6AJ9ZlhsUR41jIjdZJHkZYQSE6TQTtACfcbG5
ceTaqPZJUXImx9jfMfuughs=
=h1M0
-----END PGP SIGNATURE-----

Subject: Re:  many config options are not validated

Currently the the validation, based on what the Conf.pm POD says/infers, 
requires an @ in the address.  So "procmail*" isn't valid according to 
the validation.

I don't know if the test should be changed or if the validation code 
should be changed.

Subject: Re:  many config options are not validated

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

> 
> Currently the the validation, based on what the Conf.pm POD says/infers, 
> requires an @ in the address.  So "procmail*" isn't valid according to 
> the validation.
> 
> I don't know if the test should be changed or if the validation code 
> should be changed.
> 

I disagree on your interpretation of the docs.  It even has *.domain.net
as an example.

The validation code needs to be changed.

Michael
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.2.2 (GNU/Linux)
Comment: Using GnuPG with Thunderbird - http://enigmail.mozdev.org

iD8DBQFClUQbG4km+uS4gOIRAs0HAJ48nBfZ5n+/lZEb1jCx7D8dWwry1gCfSybv
PWPZuX8RqVH9Eyuf0lhapPY=
=GpDe
-----END PGP SIGNATURE-----

Subject: Re:  many config options are not validated

> I disagree on your interpretation of the docs.  It even has *.domain.net
> as an example.

Ah, I missed that in the docs and neither t/whitelist_from.t nor 
t/whitelist_addrs.t actually test for that.

r178573 only requires that CONF_TYPE_ADDRLIST values not be blank.

this is no longer a P2 issue

booting to 3.1.1.... I think validation could be stricter in a few
circumstances, but it's now a lot better than it was before.

r191268 removes the requirement for whitelist_from_rcvd entries to have @ signs

Need to validate "score" values...

score RULE_NAME n.nn   # literally n.nn, if you can believe that

[10272] warn: Argument "n.nn" isn't numeric in addition (+) at
/usr/lib/perl5/site_perl/5.8.0/Mail/SpamAssassin/Conf.pm line 251.
[10272] warn: Argument "n.nn" isn't numeric in addition (+) at
/usr/lib/perl5/site_perl/5.8.0/Mail/SpamAssassin/Conf.pm line 251.
[10272] warn: Argument "n.nn" isn't numeric in addition (+) at
/usr/lib/perl5/site_perl/5.8.0/Mail/SpamAssassin/Conf.pm line 251.
[10272] warn: Argument "n.nn" isn't numeric in addition (+) at
/usr/lib/perl5/site_perl/5.8.0/Mail/SpamAssassin/Conf.pm line 251.

Also some more meta chacking couldn't hurt:

body __LW_TEST1 /foo/i
body __LW_TEST2 /bag/i
meta LW_TEST __LW_TEST1 && __LW+TEST2

No warning, no workee.

Comment #20 is fixed in bug 4795.

(In reply to comment #21)
> Also some more meta chacking couldn't hurt:
> 
> body __LW_TEST1 /foo/i
> body __LW_TEST2 /bag/i
> meta LW_TEST __LW_TEST1 && __LW+TEST2

Hrm.  I don't know if we want to flag that or not.  I know the above error is supposed to be the "+" 
instead of a "_" but assume it's not a syntax error and __LW or TEST2 is simply another rule which 
requires a plugin to be enabled.  Should lint fail because of this?

For a more concrete example:

meta DIGEST_MULTIPLE              RAZOR2_CHECK + DCC_CHECK + PYZOR_CHECK > 1

all of those dependencies are via plugin.  So if any of those are disabled, should this rule get flagged as 
having an error?

I think we should probably at least throw an info() for a missing dependency or a dependency with a 
score of 0.

I'll attach a short patch that causes a lint error to occur, but it can easily be modified to only throw an 
info().

Created attachment 3448 [details]
meta lint patch

I'd suggest throwing a lint warning, same as score for nonexistant rule, rather 
than a lint error.

I agree that it *could* be legit.  But if you are doing a lint and the plugin 
isn't enabled then it would probably still be worth mentioning that things may 
not be working as expected.  (Like maybe the guy thought the plugin WAS 
enabled.)

not it!

Created attachment 3574 [details]
info only patch

I'd prefer just an info() about missing/zeroed dependencies.

We certaintly can't ship anything that causes lint errors by default.  That
just doesn't make sense.

I don't think we should even warn without an error -- it'll turn into a support
nightmare for us and third party rule developlers.  There's been no noise about
this on the users' list so I think the debug info is more than enough.

Please vote on what to do and it's corrosponding patch.

* 3448: lint error when meta dependencies are missing/disabled

* 3574: provide debug info for missing/disabled meta dependencies

* do nothing

I'm +1 for 3574.

> Please vote on what to do and it's corrosponding patch.
> * 3448: lint error when meta dependencies are missing/disabled
> * 3574: provide debug info for missing/disabled meta dependencies
> * do nothing

I'm still in favor of a warning on a meta for the following cases:
a. Referenced rule name does not exist (ie: mis-spelled in meta, or not 
available in the current rules.
b. Meta depends on a disabled plugin.

I am NOT in favor of a warning in the following case:
c. Meta depends on a rule with zero score.  (But a debug message would be nice.)

Logic behind these suggestions:

Item A is a frequent problem in SARE masschecks.  I've been burned myself any 
number of times.  While griping on the user's list doesn't show up, it is 
frequent and loud on the SARE list where we frequently build metas.

There are really two problems here: mis-spelling a term in the meta, and 
forgetting to include all of the necessary required rules to run a masscheck 
(which runs with ONLY the submitted rules, not the standard rules.)  Both of 
those reasons end up causing more than their fair share to pulled hair and 
wasted masscheck runs.  A warning would give a strong hint of the problem.

In item B, if the meta depends on a disabled plugin, the meta should be in an 
ifplugin group itself.  After all, it ain't never gonna work right without the 
plugin, so there is no point in having it sitting there doing nothing but 
wasting space and time.  (Yea, one COULD make a meta that depended in OR 
condition on 5 different plugins, with the hope of making a score if any of 
them happen to be enabled.  I have a simple solution for that case: don't.)

For item C, we don't warn now for zero-score rules, because a zero score is how 
you disable a rule.  It follows that this will disable a meta based on that 
rule (again, unless in an OR condition with something still alive).  Since we 
don't warn for zeroed rules, we shouldn't warn for effectively zeroed metas 
that are dependent, either.  However, a debug message would be worthwhile IF 
the meta score is not itself also zeroed.

General comment on the concept of warning on a non-existant dependency: you do 
this right now for scores and descriptions (which are dependencies of the 
actual rule) if the actual rule is missing.  I don't see a meta as being any 
different than a description in this case.  Either there should be a warning 
for a missing dependency on a meta, or the warning for missing rule in 
descriptions and scores should be removed.  Obviously I don't vote for removing 
the existing warnings.  But it is worth considering, if it is decided that a 
meta shouldn't warn for missing dependency.

+1 on 3574.

The DIGEST_MULTIPLE case:

  meta DIGEST_MULTIPLE   RAZOR2_CHECK + DCC_CHECK + PYZOR_CHECK > 1

where RAZOR2_CHECK, DCC_CHECK, PYZOR_CHECK could all be defined by unloaded
plugins means that we should not make this an error or visible by default, I
agree.

A possible way around that would be if we could add a new config setting that
indicated "RAZOR2_CHECK is a valid rule name, although it's not currently
defined, so do not issue missing-meta-dependency warnings about it".
That would allow 3448.
But  I don't think there's any hurry to do that... unless Loren fancies
coding it up ;)

Well, the obvious solution would seem to be to add 'tflags plugin' for plugin 
rules, allow this with the rule name (obviously) undefined; that is, you DO NOT 
put it inside the 'ifplugin', and check it.

Like:

+	if (!exists $self->{conf}->{scores}->{$token}) {
           if (exists $self->(conf)->(tflags)->($token) &&
              $self->(conf)->(tflags)->($token) ~= /\bplugin/ {
                # this is a plugin rule, it is ok if it doesn't exist
                if ($self->{main}->{lint_rules}) {
                  my $msg =
 	            "rules: meta test $rulename depends on undefined plugin 
rule '$token'";
                  warn $msg."\n";
              }
           }
           else {		
+            $self->{rule_errors}++; # flag to --lint that there was an 
error ...
+            my $msg =
+	      "rules: meta test $rulename has undefined dependency '$token'";
+            if ($self->{main}->{lint_rules}) {
+              warn $msg."\n";
+            }
+            else {
+              info($msg);
+            }
           }
+        }
+	elsif ($self->{conf}->{scores}->{$token} == 0) {

After some more thought, I'm -1 on changing the behaviour of --lint on a stable
branch since it'd cause way too many people unnecessary work/problems, so
anything like 3448 (including something that would use additional config options
to silence warnings) would have to wait for 3.2.

For now, "spamassassin --lint -Dinfo" gives the output Loren wants, it's just
lacking a non-zero return code.

One more vote for 3574 for 3.1 please.

(In reply to comment #32)
> For now, "spamassassin --lint -Dinfo" gives the output Loren wants, it's just
> lacking a non-zero return code.
> 
> One more vote for 3574 for 3.1 please.

+1 for 3574

> After some more thought, I'm -1 on changing the behaviour of --lint 
> on a stable branch since it'd cause way too many people unnecessary
> work/problems, so

Well, I'm not sure I'd agree with the conclusion; I doubt anyone would notice, 
much less it would cause problems.  

That said, I do agree that this probably shouldn't go in the stable branch.  
I'd be in favor of retargeting the original request at 3.2x after putting 3574 
into 3.1.

(In reply to comment #34)
> > After some more thought, I'm -1 on changing the behaviour of --lint 
> > on a stable branch since it'd cause way too many people unnecessary
> > work/problems, so
> 
> Well, I'm not sure I'd agree with the conclusion; I doubt anyone would notice, 
> much less it would cause problems.  

Nobody would notice?  Anyone with their own meta rules that they publish across
all their systems that depend on plugins that may or may not be enabled would be
affected.  When things stopped linting I'm sure they'd notice.  My conclusion
wasn't based on speculation.  I *know* that 3 of the 4 major ISPs in my area
would be affected by such a change.  I really don't imagine that my friends are
*that much* weirder than anyone else's.  It'd certainly affect a significant
amount of people, especially in the context of a maintenance branch.


> That said, I do agree that this probably shouldn't go in the stable branch.  
> I'd be in favor of retargeting the original request at 3.2x after putting 3574 
> into 3.1.

Yeah, retargeting at 3.2.  I still think this'd be better off as a "strict lint"
that was discussed elsewhere before.


[dos@FC5-VPC 3.1]$ svn ci -m "provide debug info about missing/disabled meta
dependencies"
Sending        lib/Mail/SpamAssassin/PerMsgStatus.pm
Transmitting file data .
Committed revision 422038.

[dos@FC5-VPC trunk]$ svn ci -m "provide debug info about missing/disabled meta
dependencies"
Sending        lib/Mail/SpamAssassin/PerMsgStatus.pm
Transmitting file data .
Committed revision 422039.

totally agreed with Daryl.  --lint is run implicitly by sa-update, for example;
even if the admins don't run lint checks regularly themselves, if they use
updates, they'd run into changes pretty quickly.

(In reply to comment #36)
> totally agreed with Daryl.  --lint is run implicitly by sa-update, for example;
> even if the admins don't run lint checks regularly themselves, if they use
> updates, they'd run into changes pretty quickly.

Just an FYI about this...  sa-update runs --lint w/out any .pre files, so things
that will normally be on (like MIMEHeader) aren't, leading to things like:

[2533] info: rules: meta test TVD_FW_MESG1 has undefined dependency '__GIF_ATTACH'

this definitely shouldn't be considered an error.

(In reply to comment #37)
> Just an FYI about this...  sa-update runs --lint w/out any .pre files, so things
> that will normally be on (like MIMEHeader) aren't, leading to things like:

It's also worth noting that if someone runs "spamassassin --lint -L", we don't
want to throw an error because the score for a net rule is 0 in a non-network
scoreset, ie:

[19950] info: rules: meta test DIGEST_MULTIPLE has dependency 'RAZOR2_CHECK'
with a zero score
[19950] info: rules: meta test DIGEST_MULTIPLE has dependency 'DCC_CHECK' with a
zero score
[19950] info: rules: meta test DIGEST_MULTIPLE has undefined dependency
'PYZOR_CHECK'

> It's also worth noting that if someone runs "spamassassin --lint -L", we 
> don't want to throw an error because the score for a net rule is 0 in a 
> non-network scoreset, ie:

But then, its perhaps worth noting that I never requested that this be an 
*error*, although everyone seems to have interpreted the original request that 
way.  The original request was that it throw a warning, just exactly like 
the 'info' messages it is now cranking out.  With one exception that is - I 
wanted them to come out on normal lint without -D to bury them in noise.

Aug  2 16:44:46 master spamd[7290]: rules: meta test DIGEST_MULTIPLE has
undefined dependency 'DCC_CHECK'

I understand the reasoning for this change, but it is not good that messages like 
this appear by default in a user's logs.  Users will think this is an error, and
it will forever be a cause for concern and a waste of time for both users and
support persons.

Is it really appropriate to have this logged by default?  Why not keep it only
in lint?

If logging by default is appropriate, can anything be done to suppress these
particular messages that would ALWAYS happen by default?

Perhaps this belongs in a separate bug?

'Aug  2 16:44:46 master spamd[7290]: rules: meta test DIGEST_MULTIPLE has
undefined dependency 'DCC_CHECK'

I understand the reasoning for this change, but it is not good that messages like 
this appear by default in a user's logs.  Users will think this is an error, and
it will forever be a cause for concern and a waste of time for both users and
support persons.'

totally agreed; even if it might seem logical, the amount of *work* it'd
generate in spurious errors and warnings would be massive.

I think the best fix (as I said above) is to provide a new setting that
indicates "DCC_CHECK is a valid rule name, even though it may be undefined for
various reasons"; then if DCC_CHECK appears in a meta declaration, it won't
generate this warning.  something like "tflags DCC_CHECK exists" maybe?

(In reply to comment #41)
> I think the best fix (as I said above) is to provide a new setting that
> indicates "DCC_CHECK is a valid rule name, even though it may be undefined for
> various reasons"; then if DCC_CHECK appears in a meta declaration, it won't
> generate this warning.  something like "tflags DCC_CHECK exists" maybe?

Let's please stop overcomplicating things and overloading tflags with stuff it
doesn't need.  The problem is that people are complaining about the code doing
what it's supposed to be doing ...   DCC_CHECK, for example, *DOES NOT EXIST* if
the plugin isn't loaded -- I really don't see the necessity to put a "tflag RULE
exists" for every rule inside of an ifplugin statement, that's ridiculous imo.

It seems as if the easiest course of action is:

if --lint, do info

possibly a more complex version:

if --lint, do info
else if debug channel rules is specified, do dbg

then most people will never notice it, unless they run "--lint -D" or "-D
rules", wherein they really should be told about this stuff because they're
looking for things like this.



FWIW, something that came up in IRC while chatting w/ wtogami was that the "rule
is undefined" message could be made nicer if it were "rule is undefined because
plugin X is not loaded".  Which is doable, except we'd have to completely change
how ifplugin and such is handled (like, don't completely skip everything in the
block, and parse it enough to get rule names out but don't actually include the
rule in the conf object, and ...)

I think users would be happiest and least confused with any solution that hides
this warning by default in logs, although I suppose a more informative message
would be a distant second in acceptability.

'we'd have to completely change
how ifplugin and such is handled (like, don't completely skip everything in the
block, and parse it enough to get rule names out but don't actually include the
rule in the conf object, and ...)'

yeah -- that'd be pretty tricky.  consider a "urirhssub" rule definition; that
setting is just invalid gibberish if the plugin isn't loaded.


let's just keep it as it currently is.

Is there a lint output message level that isn't logged by default when you 
simply start SA?  For instance, how does the lint 'score for nonexistant rule' 
warning appear?  Is that something that would be logged on every SA start?

Or am I misunderstanding and somehow SA --lint is being run and any output at 
all from it is being logged?

If the 'score for nonexistant rule' message isn't logged under whatever these 
conditions are, I'd just change the meta rule missing dependencies message to 
the same form.

(In reply to comment #45)
> Is there a lint output message level that isn't logged by default when you 
> simply start SA?  For instance, how does the lint 'score for nonexistant rule' 
> warning appear?  Is that something that would be logged on every SA start?

No, there's no special lint level.  However, when lint is running, typically
messages get bumped up in priority ala:

    if ($self->{main}->{lint_rules}) {
      warn $msg."\n";
    }
    else {
      info($msg);
    }

there's usually also a counter increase of config errors which will cause lint
to throw the error and set the exit code.

> If the 'score for nonexistant rule' message isn't logged under whatever these 
> conditions are, I'd just change the meta rule missing dependencies message to 
> the same form.

that throws a warn() and sets the lint error code.

see bug 5040: ''meta test has dependency with zero score' should ignore rules in
other scoresets'

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

fyi, I just marked bug 5085 as a dupe of this one.  the issue there is that via
spamd, debug (with -D), info(), and warn() are logged in syslog, and generally
speaking mail.* or mail.info is used, which means these "dependency not defined"
info messages are going into syslog.

this is actually fixed nowadays, isn't it?

if rules are referred to, but are nonexistent (or in non-loaded ifplugin
blocks), nothing is output by default.  if you use -D, however, you'll see them:

[30895] info: rules: meta test STOCK_IMG_HTML has undefined dependency
'__HTML_IMG_ONLY'
[30895] info: rules: meta test STOCK_IMG_CTYPE has undefined dependency
'__HTML_IMG_ONLY'
[30895] info: rules: meta test STOCK_IMG_HDR_FROM has undefined dependency
'__HTML_IMG_ONLY'

looks good to me...

(In reply to comment #50)
> this is actually fixed nowadays, isn't it?

No, it's not.  These particular info messages still get logged to syslog, even
without asking for -Dinfo.  They need the would_log(whatever) == 2, or however
that works. ;)

r486500 will stop the standard ruleset from nagging about undefined meta
dependencies in the syslog when some plugins (like DCC) are disabled.

Anyone looking to debug meta rules should run a message through SA with -Drules.

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