Bug 7107 - RFE: if() preprocessor directive should support a test for perl version
Summary: RFE: if() preprocessor directive should support a test for perl version
Status: RESOLVED FIXED
Alias: None
Product: Spamassassin
Classification: Unclassified
Component: Rules (show other bugs)
Version: SVN Trunk (Latest Devel Version)
Hardware: PC All
: P2 enhancement
Target Milestone: 3.4.1
Assignee: SpamAssassin Developer Mailing List
URL:
Whiteboard:
Keywords:
Depends on:
Blocks:
 
Reported: 2014-11-26 20:27 UTC by John Hardin
Modified: 2015-02-20 18:20 UTC (History)
3 users (show)



Attachment Type Modified Status Actions Submitter/CLA Status
Proposed patch (no documentation) patch None John Hardin [HasCLA]
Add Mail::SpamAssassin::Conf::perl_min_version_5010000() patch None John Hardin [HasCLA]

Note You need to log in before you can comment on or make changes to this bug.
Description John Hardin 2014-11-26 20:27:49 UTC
SA 3.4 supports perl 5.8.8 and above.

There are perl RE syntax elements (like ++) that were introduced after that version, so if a rule includes such syntax the rule will fail lint on older perl (e.g. during sa-update) even though it passes on newer perl (e.g. on RuleQA).

Unfortunately the SA preprocessor parser allows only very limited elements of perl, and neither $] (old) nor $^V (current) are included in that subset, so this can't be done:

if ($^V ge v5.10)
  body POST_588_RE  /\w++/
endif

That causes:

config: unparseable chars in 'if ($^V ge v5.10)': 'V'
config: unparseable chars in 'if ($^V ge v5.10)': '.10'
lint: bad 'if' line, in "rules/72_active.cf" at build/mkrules line 255.

Request:

Have the preprocessor parse/permit $^V and version numbers so that such checks can be performed, or define a "perl_version" preprocessor var similar to the existing "version" so that we can do this:

if perl_version >= 5.010000
  body POST_588_RE  /\w++/
endif

perl_version seems a better option, as it doesn't cause a lint failure, and if() fails to "false" if the expression syntax is bad, so the risky RE would be disabled by default in older SA.
Comment 1 John Hardin 2014-11-26 20:30:10 UTC
See also Bug 6207
Comment 2 Kevin A. McGrail 2014-11-26 20:36:30 UTC
Might be easier and safer to include a function that takes a parameter of min version perl and then returns 1/0 if version is equal or greater.

Not sre off-hand if can will take a parameter such as:

if can(Mail::SpamAssassin::Conf::perl_version_required(5.10.0))

Regards,
KAM
Comment 3 Kevin A. McGrail 2014-11-26 20:42:58 UTC
See also bug 7108 to revert the commenting of the rule.  This will make the rule only work with trunk or >=3.4.1 once released but should be safe for sa-update even on old versions.
Comment 4 Benny Pedersen 2014-11-26 21:36:57 UTC
(In reply to Kevin A. McGrail from comment #2)
> Might be easier and safer to include a function that takes a parameter of
> min version perl and then returns 1/0 if version is equal or greater.

was it not max perl version here ?
Comment 5 Kevin A. McGrail 2014-11-26 22:13:17 UTC
(In reply to Benny Pedersen from comment #4)
> (In reply to Kevin A. McGrail from comment #2)
> > Might be easier and safer to include a function that takes a parameter of
> > min version perl and then returns 1/0 if version is equal or greater.
> 
> was it not max perl version here ?

I do not think so. I'm thinking the variable is the minimum perl version that works for the if loop.

So if I am running 5.10.1 and I do if can(Mail::SpamAssassin::Conf::perl_version_required(5.8.8)) then the function will return 1 but If I pass if can(Mail::SpamAssassin::Conf::perl_version_required(5.10.2)), the the function will return 0.

By using the "can" functionality, this should work for old versions of SA seamlessly to ignore the functionality as well as new versions.

Theory is that old versions of SA won't have the function and will always fail.  New versions of SA will have the function and will perform a test to see if they fail or not.

It's a theory at this point.
Comment 6 John Hardin 2014-11-26 22:33:20 UTC
(In reply to Kevin A. McGrail from comment #5)
> (In reply to Benny Pedersen from comment #4)
> > (In reply to Kevin A. McGrail from comment #2)
> > > Might be easier and safer to include a function that takes a parameter of
> > > min version perl and then returns 1/0 if version is equal or greater.
> > 
> > was it not max perl version here ?
> 
> I do not think so. I'm thinking the variable is the minimum perl version
> that works for the if loop.

Correct.

> By using the "can" functionality, this should work for old versions of SA
> seamlessly to ignore the functionality as well as new versions.

Using "perl_version" does the same at the cost of a warning at startup, and I think "perl_version" is much neater (and simpler, but I haven't looked at the code yet) than a can() as you suggest.
 
> Theory is that old versions of SA won't have the function and will always
> fail. 

I tested that as I was composing this bug. "IF perl_version >= 5.010000" emits a warning about perl_version and skips the block, so it should be fail-safe for old releases of SA and should start working automatically once perl_version gets defined.

The can() approach would be preferred if we really don't want the warning, but I prefer perl_version even with the warning because it's terse, unambiguous and complements the existing "version" var.
Comment 7 John Hardin 2014-11-27 02:47:56 UTC
Created attachment 5254 [details]
Proposed patch (no documentation)

Proposed patch. Implementing "perl_version" for if() is as simple as I thought it would be.

I'll add documentation and such if this is acceptable. Comments solicited.
Comment 8 John Hardin 2014-11-27 18:55:28 UTC
Fixed.

svn commit -m 'bug 7107: add "perl_version" to preprocessor conditionals' Conf*
Sending        Conf/Parser.pm
Sending        Conf.pm
Transmitting file data ..
Committed revision 1642207.
Comment 9 Mark Martinec 2014-11-30 00:14:10 UTC
I'd prefer something like:

  can(Mail::SpamAssassin::Conf::feature_possessive_quantifier)
Comment 10 John Hardin 2014-12-02 02:09:16 UTC
Created attachment 5255 [details]
Add Mail::SpamAssassin::Conf::perl_min_version_5010000()

Okay, perl_version is an attractive solution, but the warning it generates on older SA causes administrative heartburn.

That can be avoided by doing this:

if version > 3.004.001 && perl_version >= 5.010000
 ...
endif

UNLESS the perl version is < 5.10, in which case that STILL generates a type warning.

So:

Patch adds Mail::SpamAssassin::Conf::perl_min_version_5010000() so that we can do this:

if can(Mail::SpamAssassin::Conf::perl_min_version_5010000)
 ...
endif

Patch includes documentation, but it seems clumsy to me. Comments solicited. Simply not mention perl_version at all? Revert it completely?
Comment 11 John Hardin 2014-12-02 02:14:35 UTC
Thought I changed status when attaching patch2. Bah.
Comment 12 Kevin A. McGrail 2014-12-02 02:35:51 UTC
For me, I would leave perl_version because it will be useful eventually and it works.
Comment 13 John Hardin 2014-12-04 20:47:10 UTC
Should this all be backported to the 3.3.x branch? It's really simple and, I believe, risk-free.
Comment 14 Kevin A. McGrail 2014-12-04 21:09:47 UTC
(In reply to John Hardin from comment #13)
> Should this all be backported to the 3.3.x branch? It's really simple and, I
> believe, risk-free.

No, we aren't releasing or maintaining the 3.3.x branch.
Comment 15 John Hardin 2014-12-10 15:07:13 UTC
Fixed (again).

svn commit -m 'bug 7107: add "perl_min_version_5010000" for preprocessor conditionals' Conf*
Sending        Conf.pm
Transmitting file data .
Committed revision 1644448.