+1

(In reply to Kevin A. McGrail from comment #0)
> and slave will be changed to child.

"killing child" sounds socially acceptable...

Is this a joke?

(In reply to macosx10.4.11 from comment #3)
> Is this a joke?

No.

Using colors as a good/bad metaphorical idiom is inaccurate for how SA uses such lists and has the added problem of colliding with the intractable idiomatic use of "Black" and "White" for arbitrary classes of people. Naming collisions are inherently bad, and we can't fix the larger side of this one. Switching to allow/block better reflects what SA does without requiring a subtle understanding of any English metaphorical idiom. 

The use of master/slave in SA is limited to tools in the build directory, which are of interest primarily to active members of the SA development, project sysadmin, and masscheck submitter communities. There *may* be some sites with independent automc and rule QA rigs, but there's no evidence of that. The usage includes scripts for Jenkins, which dropped 'slave' terminology in v2.0 (2016.) I am not entirely conversant with the automc & buildbot architecture where 'slave' is used, but a cursory examination leads me to believe that 'child' or perhaps 'worker' or 'robot' is a more apt metaphor for the tasks where 'slave' is currently used. 

In spamd, 'child' is already used consistently. 

(In reply to AXB  from comment #2)
> "killing child" sounds socially acceptable...

Moot point: there are no references in any SA code or comments to killing slaves. 

We already announce that we are "killing failed child" in lib/Mail/SpamAssassin/SpamdForkScaling.pm which does seem to fall short of the modern norms of good parenting. However, the standard terminology of the Unix/Linux/POSIX process model makes it unwieldy to avoid references to killing children.

First test is done with allowlist_to replacing whitelist_to
Committed revision 1879456.

NOTE: Not sure how active.list for rules will handle the new rules that are version based for this but the goal is that rules will work on older versions and 4.0.0 has an alias for whitelist_to so both configuration lines are equivalent.

Based on this Proof of Concept, will continue working on more changes.

Kevin you gonna break everyone running old trunk versions with the eval function name changes.

We should not check for "version 4.0.0", instead always use has_feature_xxx.

Atleast leave the old eval functions as compatibility layer. Just call the new ones from the old.

Henrik,

I considered that but with 4.0 unreleased, there is an expectation that trunk will be bleeding edge.  I want the future move to remove the backwards compatibility to be simple.

And since plugin WLBLEval will be changing to ALBLEval.pm, there is a level of breakage that is coming from this change with 4.0 that has_xxx and leaving old routines won't help.

I should give a heads-up to the list though for people running trunk that they might want to keep an eye on things on list.

KAM

did this motion get the required PCM votes?

(In reply to AXB from comment #9)
> did this motion get the required PCM votes?

I mean PMC

(In reply to AXB from comment #9)
> did this motion get the required PCM votes?

Yeah though atleast few votes were conditional on backwards compatibility..

I haven't seen any discussion agreeing on changing module names. Are we going to replace everything including autoWHITELIST, WHITELISTsubject, etc.. atleast do NOT commit these as random single commits over time. Do _all_ changes in a single commit so there's atleast some consistent revision where compatibility is broken.

Also care should be taken if renaming modules, as new "make install" will just leave the old ones lingering. Also nothing will change the old *.pre files to use the new modules! It requires careful planning to not break anything.

" The PMC has voted to support changing whitelist to allowlist and blacklist to blocklist as well as master to [arent and slave will be changed to child."

Whre are all those votes?


Without keeping backward compatibilty for a decade, these changes should be immediately reversed.

Nobody can predict what the outcome will be if changes are done in batches.

-1

I'd recommend a separate branch to test things.

(In reply to Henrik Krohns from comment #15)
> I'd recommend a separate branch to test things.

I can relate to that.
Also, plans for such changes should be transparently shared with the SA user's list as these will be the ones affected.

(In reply to Henrik Krohns from comment #6)
> Kevin you gonna break everyone running old trunk versions with the eval
> function name changes.
> 
> We should not check for "version 4.0.0", instead always use has_feature_xxx.

don't masscheckers use trunk?

(In reply to AXB from comment #17)
> 
> don't masscheckers use trunk?

masscheckers are supposed to automatically download latest revision matching the rules.. (atleast automasscheck script handles this)

Seems atleast last masscheck broke

Jul  5 04:40:24.987 [18525] warn: rules: failed to run __FROM_ADDRLIST_GOV test, skipping:
Jul  5 04:40:24.987 [18525] warn: \t(Can't locate object method "_check_whitelist" via package "Mail: [...]:SpamAssassin::Plugin::WLBLEval" at ../lib/Mail/SpamAssassin/Plugin/WLBLEval.pm line 119.
Jul  5 04:40:24.987 [18525] warn: )

So how bout we rollback the patch and start with a branch?

(In reply to Henrik Krohns from comment #19)
> Seems atleast last masscheck broke

> So how bout we rollback the patch and start with a branch?

good early morning & 

+1

(In reply to Henrik Krohns from comment #19)

> So how bout we rollback the patch and start with a branch?

+1

> Jul  5 04:40:24.987 [18525] warn: \t(Can't locate object method
> "_check_whitelist" via package "Mail: [...]:SpamAssassin::Plugin::WLBLEval"
> at ../lib/Mail/SpamAssassin/Plugin/WLBLEval.pm line 119.

Also, re terminology: we can retain "W" (as in "WLBLEval") if we adopt "W" = "Welcome": WelcomeList/BlockList.

That might help avoid some of the renaming.

Also, the internal method names should be retained for backwards compatibility. They are not generally visible to non-developers and may be in use by third-party plugins.

Hi All,

Since trunk is an unreleased branch, I am -1 to to switch to another branch.  Users will be carefully alerted via an UPGRADE file and release announcements for 4.0.

We did one patch to the system to prove it worked before we went through all the various functions one by one.  I wanted the patch to be bite size and reviewable.

To fix masscheck, a temporary stub for backwards compatibility until all the functions are done should work like this:

sub _check_whitelist {
  return _check_allowlist(@_);
}

I like the idea of welcome and block to replace white and black. I +1 that and can revert this change, switch from allowlist to welcomelist and add the stub if others concur.

(In reply to Kevin A. McGrail from comment #22)
 
> I like the idea of welcome and block to replace white and black. I +1 that
> and can revert this change, switch from allowlist to welcomelist and add the
> stub if others concur.

+1 for the acronym-preserving nomenclature.
 
+1 for moving forward on trunk with stubs as needed. The only way to get any exercise of new code is by putting it on trunk, as virtually no one will run the head of an explicit test branch.

(In reply to Bill Cole from comment #23)
> (In reply to Kevin A. McGrail from comment #22)
>  
> > I like the idea of welcome and block to replace white and black. I +1 that
> > and can revert this change, switch from allowlist to welcomelist and add the
> > stub if others concur.
> 
> +1 for the acronym-preserving nomenclature.
>  
> +1 for moving forward on trunk with stubs as needed. The only way to get any
> exercise of new code is by putting it on trunk, as virtually no one will run
> the head of an explicit test branch.

As I don't understand the need for all these changes I will abstain from any 
further votes.

(In reply to Bill Cole from comment #23)
>  
> +1 for moving forward on trunk with stubs as needed. The only way to get any
> exercise of new code is by putting it on trunk, as virtually no one will run
> the head of an explicit test branch.

Branch will allow testing out the changes safely. Alternatively, don't commit unfinished and untested tryouts then. Trunk as a mass check branch, is not a test bed for these kinds of invasive changes.

If Kevin is doing this alone and disappearing for days every time masschecks and things break, it's not looking good for the project.

(In reply to Kevin A. McGrail from comment #22)
> 
> We did one patch to the system to prove it worked before we went through all
> the various functions one by one.  I wanted the patch to be bite size and
> reviewable.

Who is "we"? It was obvious within seconds from looking at the commit that it's going to break things. If you don't have time to properly review things yourself, commit a branch or post a patch, so others can review it without breaking things.

> To fix masscheck, a temporary stub for backwards compatibility until all the
> functions are done should work like this:
> 
> sub _check_whitelist {
>   return _check_allowlist(@_);
> }

The backwards compatibility should be for check_to_in_whitelist eval function, not _check_whitelist. And why should it be temporary?? Just leave it there! People can have local rules too which sa-update has no control over.

Created attachment 5709 [details]
Unbreak allowlist

Quick fix to unbreak masscheck, more fixes needed to have good-looking code.
I think backwards compatibility should be a must due to custom rules that could depend on changed eval().

Personally I consider this whole concept to be extremely ill-advised and strongly suggest that it be dropped. 

Has anyone ever considered bringing this change up on the SA users list and discussing it there? It seems that users should have some say in a matter that will affect most all of them, such as forcing virtually everyone with local rules to rewrite their rules. I can find no reference to it in the SA users list.

It's all well and good for some shadowy very small group of people to make overall policy decisions that will affect many without soliciting input from the people affected. But it isn't always the wisest choice.

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

Sorry guys, I did not want to partake in this bug, but it seems the ball has been dropped completely.

Being PMC member and probably the most active developer, I use my right to emergency rollback the painful original commit.

Transmitting file data .......done
Committing transaction...
Committed revision 1879684.

I want to see SA work and prosper, but this is just embarassing. A week since faulty commit and all that happens is just some mumbling about it.

Now that the slate is clean, PLEASE create a branch for it. Count the votes or vote again if you want. But do not proceed commiting such individual one by one changes which make no sense.

Thanks for making Spamassassin work again.
(Whatever you guys do, remember to do a
https://en.wikipedia.org/wiki/Smoke_testing_(software)
when ever you alter the main tree.

Spamassassin is very frail: there is no simple "revert" SVN button I can
push here downstream once I swallow any changes.
My Spamassassin is "out of business" until someone upstream fixes it
and I can download a new version.)

Thank you for diving in.  I appreciate the attention this matter is getting because I think it is very important.  

Three notes:

Please remember with votes being made on this thread, you should be waiting 72 hours for people to weigh in. 

Please leave this ticket for me to work on.  I'm working on it and rolling things back is breaking a lot of work we are doing on two systems in the background.  

Trunk is unreleased and no rules are pending release so masscheck being broken is a null impact.

-KAM

Kevin, did you notice bug 7836, linked as a duplicate of this?

          Reporter: jidanni@jidanni.org
  Target Milestone: Undefined

Updating spamassassin makes it unusable.

sa-update shows several

rules: failed to run __FROM_ADDRLIST_BANKS test, skipping:
        (Can't locate object method "_check_whitelist" via package
"Mail::SpamAssassin::Plugin::WLBLEval" at
/home/jidanni/.spamassassin-tree/share/perl/5.30.3/Mail/SpamAssassin/Plugin/WLBLEval.pm
line 119.

(In reply to Kevin A. McGrail from comment #32)
> 
> Please remember with votes being made on this thread, you should be waiting
> 72 hours for people to weigh in. 

People have weighted in.

> Please leave this ticket for me to work on.  I'm working on it and rolling
> things back is breaking a lot of work we are doing on two systems in the
> background.  

Again, who is "we"? Why would _trunk_ have any effect in your "two systems"? Use a branch as suggested, so no other change in trunk will bother your testings etc. And people will actually have something to weight in, before things break!!

> Trunk is unreleased and no rules are pending release so masscheck being
> broken is a null impact.

Uh huh. No.

From the way this bug started, I have little faith in the way you are going on about it. Is there even a plan?? Where is it? What things are you going to rename? What things are you going to break? "I'm going to rename WLBL to ALBL... uhh no it's not necessary to rename it..", how about actually posting the complete plan and waiting for people to weight in on all the renames? Or do as suggested and go on implementing the things in trunk so we have review and test the changes easily, instead of yet again more questions in mailing lists on why there are errors and why rule updates are not working etc.

(In reply to Henrik Krohns from comment #34)
> Or do as suggested and go on implementing the things in trunk

... meaning branch of course :-)

I just committed a second version of the proof of concept for whitelist_to:

svn commit -m 'changing whitelist_to to welcomelist_to including backward compatibility stubs and add a feature check instead of using a version test for rules.'
Sending        MANIFEST
Sending        lib/Mail/SpamAssassin/Conf.pm
Sending        lib/Mail/SpamAssassin/Plugin/WLBLEval.pm
Sending        rules/30_text_de.cf
Sending        rules/30_text_fr.cf
Sending        rules/30_text_pl.cf
Sending        rules/30_text_pt_br.cf
Sending        rules/50_scores.cf
Sending        rules/60_whitelist.cf
Sending        rules/active.list
Sending        rules/v343.pre
Sending        t/whitelist_to.t
Transmitting file data ............
Committed revision 1879735.

As noted in the commit log, this uses welcomelist, has stubs for compatibility and uses if can(Mail::SpamAssassin::Conf::feature_blocklist_welcomelist) for the rule encapsulation.

Using WelcomeList and BlockList will allow all acronyms like WLBLEval and RBL to remain the same.

As with before, if this proves to work, we will start on whitelist_from and move on to more routines until finished.

(In reply to Kevin A. McGrail from comment #36)
> I just committed a second version of the proof of concept for whitelist_to:
> 
> svn commit -m 'changing whitelist_to to welcomelist_to including backward
> compatibility stubs and add a feature check instead of using a version test
> for rules.'
> Sending        MANIFEST
> Sending        lib/Mail/SpamAssassin/Conf.pm
> Sending        lib/Mail/SpamAssassin/Plugin/WLBLEval.pm
> Sending        rules/30_text_de.cf
> Sending        rules/30_text_fr.cf
> Sending        rules/30_text_pl.cf
> Sending        rules/30_text_pt_br.cf
> Sending        rules/50_scores.cf
> Sending        rules/60_whitelist.cf
> Sending        rules/active.list
> Sending        rules/v343.pre
> Sending        t/whitelist_to.t
> Transmitting file data ............
> Committed revision 1879735.
> 
> As noted in the commit log, this uses welcomelist, has stubs for
> compatibility and uses if
> can(Mail::SpamAssassin::Conf::feature_blocklist_welcomelist) for the rule
> encapsulation.
> 
> Using WelcomeList and BlockList will allow all acronyms like WLBLEval and
> RBL to remain the same.
> 
> As with before, if this proves to work, we will start on whitelist_from and
> move on to more routines until finished.

why is a POC put in trunk? - PLEASE, PLEASE put the playpen in a branch

Axb, please see my response on list re: branches but for those only reading the bug, we have no mechanism for testing ruleqa and masscheck on anything but trunk.

We will do our best to avoid problems but if you are running trunk, you are running bleeding edge development code and you should expect issues.  

I was talked out of creating a 4.0 branch however and I think now that should be reconsidered.  I am worried that this bug aside, we have a lot of code planned for 4.0 so I don't want to see people so upset.

(In reply to Kevin A. McGrail from comment #38)

> I was talked out of creating a 4.0 branch however and I think now that
> should be reconsidered.  I am worried that this bug aside, we have a lot of
> code planned for 4.0 so I don't want to see people so upset.

Then start a branch for 5.0 or name it Milk&Coffee - just don't play around with trunk which is what third party developers, integrators, etc work with.

You can't expect the rest of the world to keep track & review what "you and your staff"  does in small batches.

It's not your project...  it's not mine... 

We owe respect to EVERYBODY and playing around with the code isn't cutting it.

This is a bug tracker not a mailing list.  Please move this onlist where there is already discussion about creating a 4.0 branch.

(In reply to Kevin A. McGrail from comment #40)
> This is a bug tracker not a mailing list.  Please move this onlist where
> there is already discussion about creating a 4.0 branch.

I don't see a problem in having a disussion regarding such drastic changes being documented in the bug tracker. This is not our daily quota of blah.

Found two new files that I added to MANIFEST but not to svn add.  Committed.

svn commit -m 'Artifacts in MANIFEST but not in SVN for BZ 7826'
Adding         rules/v400.pre
Adding         t/welcomelist_to.t
Transmitting file data ..
Committed revision 1879755.

Plan to wait the weekend out on this task to make sure things look good before continuing with whitelist_from.

> we have no mechanism for testing ruleqa and masscheck on anything but trunk.
All I know is then make a copy of trunk and test on that.

And be sure to always make sure to run an email through a basic spam check to make sure nothing is broken, and if it is back out (revert changes) within five minutes. Thanks.

(In reply to jidanni from comment #43)
> > we have no mechanism for testing ruleqa and masscheck on anything but trunk.
> All I know is then make a copy of trunk and test on that.

Afraid it's not that simple.  See https://bz.apache.org/SpamAssassin/show_bug.cgi?id=7837

Also see bug 7838 for a description on one of the new rules causing a warning.

(In reply to Kevin A. McGrail from comment #0)
> - Old configuration terms will continue to work for no less than one year
> and will not be dropped until a version change such as 4.0 to 4.1.

We should also emit lint warnings for use of the old option names.

(In reply to John Hardin from comment #46)
> (In reply to Kevin A. McGrail from comment #0)
> > - Old configuration terms will continue to work for no less than one year
> > and will not be dropped until a version change such as 4.0 to 4.1.
> 
> We should also emit lint warnings for use of the old option names.

Good idea.  We are using an existing configuration option "aliases" not really any new configuration parsing.  Would need something like a aliases_warn option, I think.

(In reply to Kevin A. McGrail from comment #0)

> - Old configuration terms will continue to work for no less than one year
> and will not be dropped until a version change such as 4.0 to 4.1.

I also want to make it clear I *strongly* suggest that the old terms continue to be supported as aliases *indefinitely*, absent a strong technical reason to end such support. And I feel "to keep the code clean" isn't sufficiently strong in this case.

Do we need a PMC vote on this detail?

(In reply to John Hardin from comment #48)
> Do we need a PMC vote on this detail?

Votes on a bug imply a blocker to this bug which I'm posting here to say I'm assuming that is NOT your intent.  Please post discussion/vote/results threads on the pmc list.  

IMO, yes, we will need a vote on ending the backwards compatibility but it will be way down the line when we start a 4.1 branch.  Voting on it now will just be lost to the sands of time and should be left for the future PMC at that time to decide.

(In reply to Kevin A. McGrail from comment #49)
> (In reply to John Hardin from comment #48)
> > Do we need a PMC vote on this detail?
> 
> Votes on a bug imply a blocker to this bug which I'm posting here to say I'm
> assuming that is NOT your intent.  Please post discussion/vote/results
> threads on the pmc list.  
> 
> IMO, yes, we will need a vote on ending the backwards compatibility but it
> will be way down the line when we start a 4.1 branch.  Voting on it now will
> just be lost to the sands of time and should be left for the future PMC at
> that time to decide.

OK, that works.(In reply to Kevin A. McGrail from comment #49)
> (In reply to John Hardin from comment #48)
> > Do we need a PMC vote on this detail?
> 
> Votes on a bug imply a blocker to this bug which I'm posting here to say I'm
> assuming that is NOT your intent.  Please post discussion/vote/results
> threads on the pmc list.  
> 
> IMO, yes, we will need a vote on ending the backwards compatibility but it
> will be way down the line when we start a 4.1 branch.  Voting on it now will
> just be lost to the sands of time and should be left for the future PMC at
> that time to decide.

Not a blocker.

I was more thinking voting now to commit to and publicly declare making the support for the historical terminology permanent. This is primarily to quell objections on the list to forcing the change on users, regardless of how far down the road we kick that can.

Just an update that the rules for whitelist_to are working work for 3.3.1 to 3.4.X .  For 4.0, we'll keep updating the has feature function so those rules don't go active until 4.0 is released.  Working on whitelist_from.

Next commit lays more groundwork and changes whitelist_from to welcomelist_from.

svn commit -m 'changing whitelist_from to welcomelist_from including backward compatibility stubs and using feature check instead of using a version test for rules.  Also added a new plugin for allowing the RaciallyCharged language and added it disabled to v400.pre.  Logic and scoring added for the new plugin as well to the rules for welcomelist_from and welcomelist_to.'
Sending        MANIFEST
Sending        ldap/README
Sending        lib/Mail/SpamAssassin/Conf/Parser.pm
Sending        lib/Mail/SpamAssassin/Conf.pm
Sending        lib/Mail/SpamAssassin/PerMsgStatus.pm
Sending        lib/Mail/SpamAssassin/Plugin/DKIM.pm
Adding         lib/Mail/SpamAssassin/Plugin/RaciallyCharged.pm
Sending        lib/Mail/SpamAssassin/Plugin/SPF.pm
Sending        lib/Mail/SpamAssassin/Plugin/WLBLEval.pm
Sending        rules/50_scores.cf
Sending        rules/60_whitelist.cf
Sending        rules/user_prefs.template
Sending        rules/v400.pre
Sending        sql/README
Sending        t/data/01_test_rules.cf
Sending        t/data/01_test_rules.pre
Sending        t/debug.t
Adding         t/spamd_welcomelist_leak.t
Adding         t/welcomelist_from.t
Sending        t/welcomelist_to.t
Sending        t/whitelist_to.t
Transmitting file data .....................
Committed revision 1880494.

I have to say that I strongly object to naming a standard plugin (or any standard visible interface) "RaciallyCharged". Clearly that is what the author perceives a lot of terms to be. But a whole lot of other people in a large part of the world don't see the terms as having anything to do with race, and will see this as deliberate political labeling of terms that, while perhaps not neutral, were not "racially" charged until deemed so by others. Will the next plugin be "MorallyCharged" when someone goes after some other term?

I'd MUCH prefer a more neutral and, dare I say it, "inclusive" term like "DepreciatedTerms". This can be used as a waste basket for all terms that people decide to not like, whether they consider them racial, moral, religious, traditional, or any other negative connotation.

I would prefer a Deprecate.pm plugin, it could be useful in the future too if we would like to deprecate other keywords or plugins.

(In reply to Giovanni Bechis from comment #54)
> I would prefer a Deprecate.pm plugin, it could be useful in the future too
> if we would like to deprecate other keywords or plugins.

Have a look at the 60_whitelist.cf logic and the v400.pre.  This isn't a plugin to disable features (i.e. deprecate), it's to enable them.

By having the plugin, the old options work and this can extend to tests like spamd_whitelist_leak.t for 4.0 so we can ensure that the backwards compatibility with racially charged options works.

If you need a deprecate plugin, it would be trivial to add another plugin for that.  The current version on revision 1880494 is very simple.

I might be making it more complex though to put the extra linting concepts we want to add.  No digging has been done into that.  So this revision is noted on purpose because it's a very very simple plugin to add.

(In reply to Loren Wilton from comment #53)
> I have to say ...

This is a bug forum not a discussion forum.  Discussions belong on-list.

(In reply to Kevin A. McGrail from comment #55)

> This isn't a plugin to disable features (i.e. deprecate), it's to enable them.

So name it "EnableDeprecatedTerminology.pm"

I second the objection to "RaciallyCharged.pm". Doing that is essentially scolding anyone who enables backwards compatibility features.

(In reply to John Hardin from comment #57)
> (In reply to Kevin A. McGrail from comment #55)
> 
> > This isn't a plugin to disable features (i.e. deprecate), it's to enable them.
> 
> So name it "EnableDeprecatedTerminology.pm"
> 
> I second the objection to "RaciallyCharged.pm". Doing that is essentially
> scolding anyone who enables backwards compatibility features.

The RaciallyCharged plugin doesn't enabled deprecated terminology, it enables the racially charged language. It's used in the rules specifically for this issue and the tests as well for backwards compatible.  It won't be suitable as a generic plugin.  The plugin describes exactly what the plugin does and I stand by the name.  People should stop using the language.

svn commit -m 'whitelist_from_rcvd to welcomelist_from_rcvd, unwhitelist_from_rcvd to unwelcomelist_from_rcvd, def_whitelist_from_rcvd to def_welcomelist_from_rcvd for bug 7826'
Sending        lib/Mail/SpamAssassin/Conf.pm
Sending        lib/Mail/SpamAssassin/Plugin/DKIM.pm
Sending        lib/Mail/SpamAssassin/Plugin/WLBLEval.pm
Sending        rules/60_whitelist.cf
Sending        t/spamd_welcomelist_leak.t
Sending        t/welcomelist_from.t
Transmitting file data ......
Committed revision 1881803.

svn commit -m 'whitelist_auth to welcomelist_auth, 
unwhitelist_auth to unwelcomelist_auth,
def_whitelist_auth to def_welcomelist_auth, 
check_forged_in_whitelist to check_forged_in_welcomelist, 
check_from_in_default_whitelist to check_from_in_default_welcomelist, 
check_uri_host_in_whitelist to check_uri_host_in_welcomelist, 
check_from_in_blacklist to check_from_in_blocklist,
check_to_in_blacklist to check_from_in_blocklist, 
check_to_in_blacklist to check_to_in_blocklist,
and whitelist_bounce_relays to welcomelist_bounce_relays for bug 7826'

Sending        MANIFEST
Sending        lib/Mail/SpamAssassin/Conf.pm
Sending        lib/Mail/SpamAssassin/Plugin/DKIM.pm
Sending        lib/Mail/SpamAssassin/Plugin/FreeMail.pm
Sending        lib/Mail/SpamAssassin/Plugin/SPF.pm
Sending        lib/Mail/SpamAssassin/Plugin/VBounce.pm
Sending        lib/Mail/SpamAssassin/Plugin/WLBLEval.pm
Sending        rules/20_vbounce.cf
Sending        rules/50_scores.cf
Sending        rules/60_whitelist.cf
Sending        rules/60_whitelist_auth.cf
Adding         t/blocklist_autolearn.t
Sending        t/data/01_test_rules.cf
Adding         t/freemail_welcome_block.t
Adding         t/spf_welcome_block.t
Transmitting file data ...............
Committed revision 1884922.

svn commit -m 'Found missing rule eval registration in VBounce.pm for check_whitelist_bounce_relays and changed 60_whitelist.cf hierarchy to handle ifplugin/if/else/endif problems'
Sending        lib/Mail/SpamAssassin/Plugin/VBounce.pm
Sending        rules/60_whitelist.cf
Transmitting file data ..done
Committing transaction...
Committed revision 1884961.

svn commit -m 'added logic for vbounce rules for 3.4 vs 4.0 with the new code changes and fixed two meta rule misspellings as well as cleaned up a lot of whitespace for bug 7826'
Sending        rules/20_vbounce.cf
Sending        rules/60_whitelist.cf
Transmitting file data ..done
Committing transaction...
Committed revision 1884976.

$ svn commit lib/Mail/SpamAssassin/Conf.pm lib/Mail/SpamAssassin/Plugin/DKIM.pm rules/50_scores.cf rules/60_whitelist.cf rules/60_whitelist_dkim.cf -m"Add [welcome,block]list_from_dkim and [welcome,block]list_from_uri_host"
Sending        lib/Mail/SpamAssassin/Conf.pm
Sending        lib/Mail/SpamAssassin/Plugin/DKIM.pm
Sending        rules/50_scores.cf
Sending        rules/60_whitelist.cf
Sending        rules/60_whitelist_dkim.cf
Transmitting file data .....done
Committing transaction...
Committed revision 1891820.

Is there a revision to sa-learn?

Jan 16 12:58:48.291 [] info: config: failed to parse line, skipping, in "/home/realger1/.spamassassin/user_prefs": welcomelist_from *@github.com

and all other "welcome list from user_prefs



Jan 16 12:58:47.238 [] dbg: generic: SpamAssassin version 3.3.1
Jan 16 12:58:47.238 [] dbg: generic: Perl 5.016003, PREFIX=/usr, DEF_RULES_DIR=/usr/share/spamassassin, LOCAL_RULES_DIR=/etc/mail/spamassassin, LOCAL_STATE_DIR=/var/lib/spamassassin
Jan 16 12:58:47.238 [] dbg: config: timing enabled
Jan 16 12:58:47.239 [] dbg: config: score set 0 chosen.
Jan 16 12:58:47.240 [] dbg: util: running in taint mode? yes
Jan 16 12:58:47.240 [] dbg: util: taint mode: deleting unsafe environment variables, resetting PATH

(In reply to Dennis German from comment #64)
> Is there a revision to sa-learn?

Specific to this bug? No. No need. 

> 
> Jan 16 12:58:48.291 [] info: config: failed to parse line, skipping, in
> "/home/realger1/.spamassassin/user_prefs": welcomelist_from *@github.com
> 
> and all other "welcome list from user_prefs
> 
> 
> 
> Jan 16 12:58:47.238 [] dbg: generic: SpamAssassin version 3.3.1

That is an antique *OBSOLETE* version of SA which does not have any of the changes related to this bug. You cannot use the new terminology with an old version of SA. See the standard rules distribution for examples of how you can write your own configurations that work across SA versions. 

> Jan 16 12:58:47.238 [] dbg: generic: Perl 5.016003, PREFIX=/usr,

Also a rather old Perl. Still should work with current SA and rules. Might not work with the release version of 4.0.0, but that is not yet set.

I guess I'll peer into this barrel of worms, though with great trepidation.

It seems we are about rapidly approaching the release of 4.0.0, I don't understand how we can

1. fulfill a stated intention to remove "whitelist" and "blacklist" from our code in the same time frame as fixing the other milestone 4.0.0 issues

2. avoid creating additional worse problems with backwards compatibility if we release 4.0.0 without the changes to whitelist and blacklist, then try to get them in a point release

3. release 4.0.0 from trunk in its current state where it seems that a large start to the whitelist blacklist changeover has already been committed but not completed

I know that there are many people running from trunk and that mass-check depends on trunk, so whatever has been committed must be usable at this time. But does what is in trunk right now make sense as a coherent release with a partway change in terminology? Kevin, can you explain what the current state is, the roadmap for the rest of it, and your understanding of how an imminent 4.0.0 release can happen?

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

small wish from my side, spamassassin should not have any plugin loaded pr default, admins have to deside imho

also is it posible to not load check.pm ?

is its the main plugin always needed, if so why is it a plugin ?

It's probably fruitless to wait for KAM to respond. This is not a bug or feature owned by KAM. There have been many objections to changes done before, yet nothing was done about them:

- Seeing trunk as some "unstable" test bench when it's nothing but
- The "RaciallyCharged" plugin, which is completely pointless, as there is already a Conf::feature_blocklist_welcomelist check
- Both of the above were committed before all the WL/BL changes were made, so we can't even use them as things will break anyway

Only way it's going to work sanely is making all the changes in one go. Then Conf::feature_blocklist_welcomelist can probably be renamed to Conf::feature_welcomelist_blocklist and that should be used in the future.

Dunno if we can call things for vote again, but I'd suggest either of these:

- Completely roll back all the changes and forget this whole debacle
- Massive rush to rename all the stuff, do it this time in a branch and test properly before committing to trunk. I will pledge to waste^H^H^H^H^Hdonate one day of my life to do this.

As it requires no permission or votes, I've created a branch now anyway:
https://svn.apache.org/repos/asf/spamassassin/branches/trunk-welcomelist

Will work on it a bit to see what can be done.

I think the whole approach of this rewording is suboptimal. I would strongly suggest to separate the renaming from the aliases management. The aliases should be managed by an extra plugin. For this, the plugin must provide two new commands:

add_eval_alias ALIAS SOURCE
add_command_alias ALIAS SOURCE

Example:
add_eval_alias check_from_in_whitelist check_from_in_welcomelist
add_command_alias whitelist_from welcomelist_from

These alias commands must be written in a central .pre file or in the respective .pre file of the plugin after the loadplugin statement.

This approach has a number of advantages:
- After renaming, the plugins' code does not need to be touched.
- Depending on how the management of the .pre files is done, the alias commands could be omitted in a new installation.
- If someone wants to do without the aliases after an upgrade, they could just delete them.
- If someone would rather use allowlist/denylist or acceptlist/rejectlist for their rules instead of welcomelist/blocklist, they could simply set aliases accordingly.

If you want to make it clearer in a next release of SA that the aliases will soon no longer be supported, you can extend the command for the eval functions to

add_eval_alias check_from_in_whitelist check_from_in_welcomelist warn="rules: eval function check_from_in_whitelist deprecated, use check_from_in_welcomelist instead"

Instead of warn, you could use dbg or info.

If you don't want to go this way, then at least the stubs should be coded better. Instead of 

sub check_from_in_whitelist {
  return check_from_in_welcomelist(@_);
}

an alias should simply be set in the symbol table:
*check_from_in_whitelist = \&check_from_in_welcomelist;
Then the two functions behave identically.

Or if you want to have the function with warn, then

sub check_from_in_whitelist {
  warn "rules: eval function check_from_in_whitelist deprecated, use check_from_in_welcomelist instead"
  &check_from_in_welcomelist;
}

Important: Call with & and without arguments so that @_ remains visible for the called function.

(In reply to Henrik Krohns from comment #69)
> 
> Dunno if we can call things for vote again, but I'd suggest either of these:
> 
> - Completely roll back all the changes and forget this whole debacle
> - Massive rush to rename all the stuff, do it this time in a branch and test
> properly before committing to trunk. I will pledge to waste^H^H^H^H^Hdonate
> one day of my life to do this.

I'll also add this for voting table:

- Roll back all the changes from trunk, and work on them in the just created trunk-whitelist branch. This would mean actually getting 4.0 out now and not spending eternity here debating. Racial stuff can be in 4.1 or any other imaginary version, if we ever have resources to do it properly, that's up to anyone that wants to actually work on it..

This is my preferred way, would vote +1

(In reply to Henrik Krohns from comment #72)
> - Roll back all the changes from trunk, and work on them in the just created
> trunk-whitelist branch.

To what degree are there people running production systems off trunk and how will this affect them? Is it the case that everyone who contributes to the mass-check system must be running trunk? I'm confused by earlier comments that seem to imply that. I know that rules are kept in trunk and that's what goes into the update channels, but is it the case that we have gotten ourselves into a situation in which trunk has to always be stable and even backwards compatible to a certain degree?

My above questions are verging into a more general conversation that would be better on a mailing list, but the part that is specific to this bugzilla issue has to do with whether there would be problems resulting from rolling back trunk that would make it easier to simply push forward.

(In reply to Henrik Krohns from comment #72)
> 
> This is my preferred way, would vote +1

I don't have a vote, but I'll vote +1 for this anyway.


Sidney, I *think* that the changes can be rolled back in trunk, including the rules. However, there are probably a few people running trunk that are not developers/committers that may have used some of the new terminology. These would break with the rollback. 

However, I think trunk should be treated as somewhat unstable, so we could just announce what is going to happen (and the implications on own rules) a week or two before the actual drop, and let it go at that.

If there was worry over some probably small percentage of the probably few people running trunk suddenly getting lint errors, some work could be done to put some aliasing into trunk, aliasing the new names to the old names. I (again having no vote) would vote against this; it is extra work and likely to destabilize things.

As a completely separate longer-term discussion, perhaps rules should be in their own tree and not part of trunk.

My rolling back trunk refers specifically to:

- Rules (sa-update)
- RaciallyCharged plugin (which I'd really like to see go away anyway)
- Conf::feature_blocklist_welcomelist (as commented before)
- Above mean frankly any committed code, which should be reviewed again..

Rolling back would offer a clean slate and not confuse people by ending up in 4.0.

Of course there is a slight chance that someone using trunk during these few years is actually using the newly named features/functions directly. However, this seems quite unlikely and a small heads up on the list would probably be enough.

If you like that things should be left as is in the 4.0 release and rules, I'm fine with that too, as I only really care about getting 4.0 out. In my professional opinion it just seems dumb.

I'm not saying that trunk isn't "unstable" per se, but it's already communicated numerous times that 3.4 is pretty much deprecated and trunk is considered somewhat "stable" as it's used for mass-checking also. I know people are using trunk in production, and I've even advocated it personally. It really should not be a generic testing bed for this scale of change, this feature should have never been committed to trunk and rules for testing purposes as KAM claimed. It would have been trivial to develop and test locally against different 3.4 configs.

Actually rolling back rules wouldn't be that simple, as for example 60_whitelist.cf already renamed things for 3.4 users, these both hit:

USER_IN_BLOCKLIST    From: user is listed in the block-list
USER_IN_BLACKLIST    DEPRECATED: See USER_IN_BLOCKLIST

So in some capacity we are already committed to it, and people will see both variants of rules hitting for a long time..

Since there are no more issues with the 4.0 milestone other than this one, perhaps now is the time to create a 4.0 branch. The next steps would be to create two roadmaps, 1) steps that would rollback commits from this issue that are necessary to remove to get to a stable and coherent 4.0.0 release; and an alternate 2) steps that would complete this issue to get to a stable and coherent 4.0.0 release.

That way we would not conflate the issue of masscheck having to be run from trunk (bug 7837) with anything having to do with the 4.0.0 release and how this issue will or will not be part of it.

Once we know what we would have to do to get a 4.0.0 released with or without the nomenclature change, we can more rationally decide on which one to do. And having the roadmaps will make it easier to plan out and split up the work to getting this done in a more transparent manner with more consensus.

It may be that we have to put more urgency into solving bug 7837 in a way that leaves the process of masscheck less fragile in its dependency on bleeding edge code.

Anyway, I think the action item out of this that is most likely to get agreement is to create a 4.0 branch. It doesn't have to formally be R-T-C yet, but I would like to have a consensus on what we are going to do with this issue for 4.0.0 before anyone actually takes the step of reverting the commits, i.e., let's do it with a plan and a roadmap this time.

I've committed massive update to trunk-welcomelist (revision 1899925), which renames pretty much all of white->welcome, black->block. You can see how many changes were missing. Backwards compatibility was applied to things like checking .spamassassin/auto-welcomelist vs existing .spamassassin/auto-whitelist etc. Most likely something is missing, so if someone wants to go with this option, get the branch and test things out.

(In reply to Michael Storz from comment #71)
> I think the whole approach of this rewording is suboptimal. I would strongly
> suggest to separate the renaming from the aliases management. The aliases
> should be managed by an extra plugin. For this, the plugin must provide two
> new commands:
> 
> add_eval_alias ALIAS SOURCE
> add_command_alias ALIAS SOURCE
> 
> Example:
> add_eval_alias check_from_in_whitelist check_from_in_welcomelist
> add_command_alias whitelist_from welcomelist_from

Thanks for the ideas, though I assume the code and inner workings of this would get wayy to complicated for any benefits it would give. The set_default_commands/aliases method already exists and is more than sufficient for this purpose.

> These alias commands must be written in a central .pre file or in the
> respective .pre file of the plugin after the loadplugin statement.
> 
> This approach has a number of advantages:
> - After renaming, the plugins' code does not need to be touched.

It's no problem to touch distributed plugins. And remember that we are only talking about backwards compatible aliases for some functions, it doesn't make much difference when they are actually physically removed (if at all).

> - If someone would rather use allowlist/denylist or acceptlist/rejectlist
> for their rules instead of welcomelist/blocklist, they could simply set
> aliases accordingly.

Would would we want to compilicate things further by allowing people to rename things randomly? They can do what they want with their own plugins, but what we distribute is what they should use.

> an alias should simply be set in the symbol table:
> *check_from_in_whitelist = \&check_from_in_welcomelist;
> Then the two functions behave identically.

This was good hint and I used it, cheers.

By the way I'm actually pretty happy with the state of trunk-welcomelist now, I'm running it on production too. And it only took some hours of determined hard grinding.

I would not object merging it into trunk, if few people try it out first.

(In reply to Sidney Markowitz from comment #77)
> 2) steps that would complete this issue to get to a stable
> and coherent 4.0.0 release.

It could go something like this:

- Test trunk-welcomelist

- Disable automatic updates per this comment

# If https://svn.apache.org/repos/asf/spamassassin/trunk/rulesrc/scores/DISABLE-AUTOMATIC-UPDATES
# exists then DNS updates will be skipped so that update publishing is
# effectively disabled

- Merge trunk-welcomelist into trunk

- Wait for rule updates to run, then manually get the new rule package and test it works ok on few 3.4 and trunk systems

- Enable automatic updates

- Create 4.0 branch if needed. There was discussion before if we really need one.. I think agreement was that development is so marginal that it's fine to do things in trunk directly. As long as commits are "complete and tested", and not some bits and pieces of large update like this bug - those should be done in a branch.. or simply don't commit clearly unfinished things which you never get back working on.

(In reply to Henrik Krohns from comment #81)
> There was discussion before if we really need
> one.. I think agreement was that development is so marginal that it's fine
> to do things in trunk directly.

I agree. I realized that soon after I posted my comment.

That's great news that trunk-welcomelist is in such good shape. I'm +1 about merging it back into trunk if we get enough testing for you to be comfortable with it. With trunk still in C-T-R it doesn't even need a vote, though it would be good to confirm that we have a sense of agreement about it.

(In reply to Henrik Krohns from comment #80)
> By the way I'm actually pretty happy with the state of trunk-welcomelist
> now, I'm running it on production too. And it only took some hours of
> determined hard grinding.

Thank you very much. I am very appreciative of your hard work on this and all the other issues that have been needed for 4.0. 

> I would not object merging it into trunk, if few people try it out first.

I will be deploying trunk-welcomelist on my personal system today. Absent significant problems, I am all for merging it into trunk before a 4.0 release branch is created.

Assuming we want to go with the welcomelist route, I'd still like to figure out a more generic solution for defining "version compatibility". So people can turn on some flag when they have checked their local configs and removed old incompatibilities, so old duplicate USER_IN_BLACKLIST rules stop beeing seen. A bit like postfix "compatibility_level".

Previous solution was loading/unloading module and "ifplugin Mail::SpamAssassin::Plugin::RaciallyCharged". A bit awkward and way too hardcoded.

I'm visioning something like "enable_compat welcomelist_blocklist" that one can insert into init.pre. It would create "Mail::SpamAssassin::Conf::compat_welcomelist_blacklist" function dynamically, so we can do this check to skip rules:

if can(Mail::SpamAssassin::Conf::compat_welcomelist_blacklist)

(Yeah this could be in it's own bug, but it's quite tied to this bug at this time)

(In reply to Henrik Krohns from comment #84)
> if can(Mail::SpamAssassin::Conf::compat_welcomelist_blacklist)
> 
> (Yeah this could be in it's own bug, but it's quite tied to this bug at this
> time)

This can be continued in Bug 7972

I think an RC makes sense. I've been using SA 4.0/trunk in production for eons though I have not started using the new aliases for configuration options.  I think if we release 4.0 with those options as in beta, it might work out very well.  We would get more testers, keep working on this bug, and get 4.0 in usage.

This good in init.pre?

# Version compatibility - Welcomelist/Blocklist
# In SpamAssassin 4.0, rules containing "whitelist" or "blacklist" have been
# renamed to contain more racially neutral "welcomelist" and "blocklist"
# terms.  When this compatibility flag is enabled, old rule names from stock
# rules will not hit anymore alongside the new ones.  For more information,
# see: https://wiki.apache.org/spamassassin/WelcomelistBlocklist
#
enable_compat welcomelist_blocklist

Maybe someone write up more details on such wiki page.

Things should be pretty good for merging to trunk already.

Trunk-welcomelist branch is now merged back to trunk (Revision 1900215) and deleted.

Automatic updates has been stopped, tomorrow we see if everything is ok and a new tarball is generated. Then we can manually wget the tarballs and test on some installations. Again, note all the 60_welcomelist*.cf etc changes, everything should be now if can()'d and "enable_compat welcomelist_blocklist" should disable old rules from running.

(In reply to Henrik Krohns from comment #88)
> Trunk-welcomelist branch is now merged back to trunk (Revision 1900215) and
> deleted.
> 
> Automatic updates has been stopped, tomorrow we see if everything is ok and
> a new tarball is generated. Then we can manually wget the tarballs and test
> on some installations. Again, note all the 60_welcomelist*.cf etc changes,
> everything should be now if can()'d and "enable_compat
> welcomelist_blocklist" should disable old rules from running.

Good job!

I found a few whitelist entries, but they might be intentional:

lib/Mail/SpamAssassin/Plugin/DCC.pm
See http://www.rhyolite.com/dcc/reputations.html You C<must> whitelist your

lib/Mail/SpamAssassin/Plugin/Pyzor.pm
whitelisted to the Pyzor server for SpamAssassin to consider ignoring the
result.  Final decision is made by pyzor_whitelist_factor.
Ignore Pyzor result if REPORTCOUNT x NUMBER >= pyzor_whitelist_min.
For default setting this means: 50 reports requires 10 whitelistings.

lib/Mail/SpamAssassin/Plugin/Razor2.pm
      # if mail isn't whitelisted, check it out

lib/Mail/SpamAssassin/Plugin/FromNameSpoof.pm
    foreach my $white_addr (keys %{$list_refs->{$owner}}) {
      if ($check =~ $list_refs->{$owner}{$white_addr}) {

rules/60_welcomelist_auth.cf
# SpamAssassin rules file: default SPF and DKIM whitelists
# SPF and DKIM whitelist rules

rules/60_welcomelist_spf.cf
# Whitelist and blacklist addresses are now file-glob-style patterns, so

./t/body_str.t:      use_auto_whitelist 0
./t/db_awl_perms.t:  use_auto_whitelist 1
./t/dnsbl_subtests.t:# use_auto_whitelist 0
./t/dkim.t:    use_auto_whitelist 0
./t/spamd_kill_restart.t:  use_auto_whitelist 0
./t/spamd_kill_restart_rr.t:  use_auto_whitelist 0
./t/spamd_unix.t:  use_auto_whitelist 0
./t/sql_based_whitelist.t:    use_auto_whitelist 1
./t/sql_based_whitelist.t:    use_auto_whitelist 1
./t/whitelist_subject.t:  use_auto_whitelist 0
./masses/bayes-testing/benchmark/tests/db_file/site/local.cf:use_auto_whitelist 0
./masses/bayes-testing/benchmark/tests/mysql/site/local.cf:use_auto_whitelist 0
./masses/bayes-testing/benchmark/tests/sdbm/site/local.cf:use_auto_whitelist 0
./masses/bayes-testing/benchmark/tests/pgsql/site/local.cf:use_auto_whitelist 0
./masses/bayes-testing/benchmark/tests/bdb/site/local.cf:use_auto_whitelist     0
./build/automc/run_preflight:  use_auto_whitelist 0
./t.rules/run:      post_config_text    => "use_learner 0\nuse_auto_whitelist 0\n".$configtext,
backend/nitemc/run_one_nitemc:  use_auto_whitelist 0

./t/db_awl_path.t:  auto_whitelist_path ./$workdir/awl/shouldbeinaccessible
./t/db_awl_path.t:  auto_whitelist_file_mode 0755
./t/db_awl_perms.t:  auto_whitelist_path ./$userstate/awl
./t/db_awl_perms.t:  auto_whitelist_file_mode 0755
./t/sa_awl.t:  auto_whitelist_path ./$userstate/awltest
./t/sa_awl.t:  auto_whitelist_file_mode 0755
./t/sql_based_whitelist.t:    auto_whitelist_factory Mail::SpamAssassin::SQLBasedAddrList
./t/sql_based_whitelist.t:    auto_whitelist_factory Mail::SpamAssassin::SQLBasedAddrList
./sql/README.awl:This is done with the auto_whitelist_factory config variable, like
./sql/README.awl:auto_whitelist_factory Mail::SpamAssassin::SQLBasedAddrList
./sql/README.awl:if auto_whitelist_distinguish_signed is true, e.g. (in local.cf):
./sql/README.awl:  auto_whitelist_distinguish_signed 1
./sql/README.awl:  auto_whitelist_distinguish_signed 1

./UPGRADE
- FreeMail: new options freemail_import_whitelist_auth,
  freemail_import_def_whitelist_auth added (Bug 6451)
  pyzor_max setting to pyzor_count_min.  Added pyzor_whitelist_min and
  pyzor_whitelist_factor setting.  Also try to ignore "empty body" FPs.

./INSTALL
    will not be suitable for DNS queries and white/blacklisting.

New update tarball available with welcomelist:

wget http://sa-update.spamassassin.org/1900236.tar.gz
wget http://sa-update.spamassassin.org/1900236.tar.gz.sha256
wget http://sa-update.spamassassin.org/1900236.tar.gz.sha512
wget http://sa-update.spamassassin.org/1900236.tar.gz.asc
sa-update --install 1900236.tar.gz

Please try it on many 3.x systems.

Works fine on 3.4.6 and 3.4.2, 3.3 is unsupported.

Tested some 3.4 versions too, I've now re-enabled automatic updates.

(In reply to Henrik Krohns from comment #90)
> New update tarball available with welcomelist:
> 
> wget http://sa-update.spamassassin.org/1900236.tar.gz
> wget http://sa-update.spamassassin.org/1900236.tar.gz.sha256
> wget http://sa-update.spamassassin.org/1900236.tar.gz.sha512
> wget http://sa-update.spamassassin.org/1900236.tar.gz.asc
> sa-update --install 1900236.tar.gz
> 
> Please try it on many 3.x systems. 

Tested with 3.4.6 as patched & built by MacPorts. No anomalies detected in retesting my last 1900 received messages.

Changed few more missing strings, I'm going to call this bug as fixed enough.