SA Bugzilla – Bug 7107
RFE: if() preprocessor directive should support a test for perl version
Last modified: 2015-02-20 18:20:05 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.
See also Bug 6207
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
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.
(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 ?
(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.
(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.
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.
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.
I'd prefer something like: can(Mail::SpamAssassin::Conf::feature_possessive_quantifier)
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?
Thought I changed status when attaching patch2. Bah.
For me, I would leave perl_version because it will be useful eventually and it works.
Should this all be backported to the 3.3.x branch? It's really simple and, I believe, risk-free.
(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.
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.