Bug 6392 - [review] Test suite fails with perl 5.12.0
[review] Test suite fails with perl 5.12.0
Status: RESOLVED FIXED
Product: Spamassassin
Classification: Unclassified
Component: Regression Tests
3.3.1
Other All
: P5 enhancement
: 3.3.2
Assigned To: SpamAssassin Developer Mailing List
ready to apply
:
: 6399 6463 6474 6493 6590 (view as bug list)
Depends on:
Blocks:
  Show dependency tree
 
Reported: 2010-03-29 16:03 UTC by eserte12
Modified: 2011-06-14 22:55 UTC (History)
10 users (show)



Attachment Type Modified Status Actions Submitter/CLA Status
avoid goto run_compiled_method in Plugin::Check patch None Mark Martinec [HasCLA]
Proposed combo patch for 3.3. patch None Mark Martinec [HasCLA]
Changes made to release a CPAN dev version of SA patch None Todd Rinaldo [NoCLA]
a backport of trunk's r1027163 applicable to 3.2 patch None Mark Martinec [HasCLA]
the r1027163 applicable to 3.3 patch None Mark Martinec [HasCLA]

Note You need to log in before you can comment on or make changes to this bug.
Description eserte12 2010-03-29 16:03:37 UTC
Most tests show the "defined(%hash)" and "goto ... into a construct" warning. In one case this warning leads to a failing test:

defined(%hash) is deprecated at ../blib/lib/Mail/SpamAssassin/Dns.pm line 757.
        (Maybe you should just omit the defined()?)
Use of "goto" to jump into a construct is deprecated at ../blib/lib/Mail/SpamAssassin/Plugin/Check.pm line 409.
Use of "goto" to jump into a construct is deprecated at ../blib/lib/Mail/SpamAssassin/Plugin/Check.pm line 409.
Use of "goto" to jump into a construct is deprecated at ../blib/lib/Mail/SpamAssassin/Plugin/Check.pm line 409.
Use of "goto" to jump into a construct is deprecated at ../blib/lib/Mail/SpamAssassin/Plugin/Check.pm line 409.
Use of "goto" to jump into a construct is deprecated at ../blib/lib/Mail/SpamAssassin/Plugin/Check.pm line 409.
Use of "goto" to jump into a construct is deprecated at ../blib/lib/Mail/SpamAssassin/Plugin/Check.pm line 409.
Use of "goto" to jump into a construct is deprecated at ../blib/lib/Mail/SpamAssassin/Plugin/Check.pm line 409.
Use of "goto" to jump into a construct is deprecated at ../blib/lib/Mail/SpamAssassin/Plugin/Check.pm line 409.
Use of "goto" to jump into a construct is deprecated at ../blib/lib/Mail/SpamAssassin/Plugin/Check.pm line 409.
Use of "goto" to jump into a construct is deprecated at ../blib/lib/Mail/SpamAssassin/Plugin/Check.pm line 409.
Use of "goto" to jump into a construct is deprecated at ../blib/lib/Mail/SpamAssassin/Plugin/Check.pm line 409.
Use of "goto" to jump into a construct is deprecated at ../blib/lib/Mail/SpamAssassin/Plugin/Check.pm line 409.
Use of "goto" to jump into a construct is deprecated at ../blib/lib/Mail/SpamAssassin/Plugin/Check.pm line 409.
Use of "goto" to jump into a construct is deprecated at ../blib/lib/Mail/SpamAssassin/Plugin/Check.pm line 409.
Use of "goto" to jump into a construct is deprecated at ../blib/lib/Mail/SpamAssassin/Plugin/Check.pm line 409.
Use of "goto" to jump into a construct is deprecated at ../blib/lib/Mail/SpamAssassin/Plugin/Check.pm line 409.
Use of "goto" to jump into a construct is deprecated at ../blib/lib/Mail/SpamAssassin/Plugin/Check.pm line 409.
Use of "goto" to jump into a construct is deprecated at ../blib/lib/Mail/SpamAssassin/Plugin/Check.pm line 409.
# Failed test 9 in t/recreate.t at line 86
t/recreate.t ......................
Failed 1/9 subtests

Line 86 is: ok($warning == 0);

Regards,
    Slaven
Comment 1 Mark Martinec 2010-03-30 01:30:36 UTC
> defined(%hash) is deprecated at ../blib/lib/Mail/SpamAssassin/Dns.pm line 757.
> (Maybe you should just omit the defined()?)

A bug indeed.

trunk:
  defined(%hash) is not the way to test a hash for being empty
Sending lib/Mail/SpamAssassin/Dns.pm
Committed revision 928958.
Comment 2 Todd Rinaldo 2010-03-30 05:52:12 UTC
Hi, 5.12 will be out within the next 30 days most likely. The fix is to replace all occurrences of:

defined %

with

%

I believe this is considered the most efficient check for if a hash is empty
Comment 3 Todd Rinaldo 2010-03-30 05:53:35 UTC
sorry, I missed the commit in this ticket. looks good.
Comment 4 Mark Martinec 2010-03-30 11:23:51 UTC
> The fix is to replace all occurrences
> sorry, I missed the commit in this ticket. looks good.

I believe that was the only one. Thanks for spotting it.

But I found a similar case in rulesrc/sandbox/jm/EmailBL.pm
-    return 0 unless defined @$emails;
+    return 0 unless @$emails;

Also found two cases of 'if/unless keys %hash', which is suboptimal.
Under perl 5.12 the 'if/unless %hash' supposedly runs much faster
(according to release notes), semantics is the same.


trunk:
  Bug 6392: Test suite fails with perl 5.12.0;
  some more cleanups according to Bug 6392 comment 4
Sending lib/Mail/SpamAssassin/HTML.pm
Sending lib/Mail/SpamAssassin/Plugin/BodyEval.pm
Sending rulesrc/sandbox/jm/EmailBL.pm
Committed revision 929084.


The goto into a construct still needs fixing,
then all of this is to be backported to 3.3.2.
Comment 5 Todd Rinaldo 2010-03-30 14:32:18 UTC
Actually it looks like there's more than that I found doing the test suite from CPAN last night. Should I supply a patch from there? or is there a better source for me to do so?

I'm guess that this link should build as a dev version of the CPAN module?

http://svn.apache.org/viewvc/spamassassin/trunk/
Comment 6 Mark Martinec 2010-03-30 15:34:15 UTC
> Actually it looks like there's more than that I found doing the test suite from
> CPAN last night. Should I supply a patch from there? or is there a better
> source for me to do so?

A problem description would do, a patch is even better.
Either against 3.3.1, or better still, against trunk.

> I'm guess that this link should build as a dev version of the CPAN module?
> http://svn.apache.org/viewvc/spamassassin/trunk/

My preferred choice is just to fetch/update directly from svn:
  $ svn checkout https://svn.apache.org/repos/asf/spamassassin/trunk
Build/make within such directory should work just fine
(perl Makefile.PL; make; make test; make install).
Comment 7 Mark Martinec 2010-03-30 15:54:37 UTC
Created attachment 4726 [details]
avoid goto run_compiled_method in Plugin::Check

Proposed patch to fix 'Use of "goto" to jump into a construct is deprecated'.

For the sake of patch compactness for review purposes I did not make
indentation change - which would be made when a patch is applied.
Comment 8 Mark Martinec 2010-03-30 16:05:39 UTC
trunk:
  Bug 6392: Test suite fails with perl 5.12.0:
  avoid goto run_compiled_method in Plugin::Check, getting rid
  of 'Use of "goto" to jump into a construct is deprecated'
Sending lib/Mail/SpamAssassin/Plugin/Check.pm
Committed revision 929182.

(the r929182 includes indentation change; for easier review
please see the patch attached to previous comment)
Comment 9 Mark Martinec 2010-03-31 00:22:50 UTC
It appears we caught all perl 5.12 warnings now (including Bug 6396).

There is another bunch of undef warnings while running a t/dkim2.t
test. I'll send a patch to the author of a Mail::DKIM module, will
probably end up in Mail-DKIM-0.37_8 and 0.38. Should be harmless though,
it's just ugly. I can attach it here if anyone is interested.

Does anyone see anything else? I'm running SpamAssassin trunk
under amavisd-new and Perl 5.12 now on our mailer, looks normal,
and all tests pass cleanly, debug log does not reveal any surprises.
Comment 10 Justin Mason 2010-03-31 09:25:17 UTC
if anyone has copious tuits, it'd be great to install perl 5.12 on hudson-solaris.zones.apache.org so we can run a Hudson job to regularly run the test suite with it.  (we should really add perl 5.10 too while we're at it ;)
Comment 11 Mark Martinec 2010-04-01 15:58:13 UTC
*** Bug 6399 has been marked as a duplicate of this bug. ***
Comment 12 Mark Martinec 2010-04-01 16:07:00 UTC
Created attachment 4732 [details]
Proposed combo patch for 3.3.

The attached patch combines all changes already committed to trunk
mentioned in this problem report: the 'defined @' and 'defined %'
cases, the goto jump into a construct, unnecessary use of keys()
in a scalar context testing for empty hash.
Comment 13 Todd Rinaldo 2010-04-01 16:09:38 UTC
FYI, Bug 6396 is technically a 5.12 regression too. I believe it's already in trunk.
Comment 14 Mark Martinec 2010-04-01 16:18:40 UTC
> FYI, Bug 6396 is technically a 5.12 regression too.
> I believe it's already in trunk.

Yes. It's also already in the 3.3 branch too, so that ticket is
only still left open for considering the Test::NoWarnings.
Comment 15 Mark Martinec 2010-04-06 14:15:10 UTC
(In reply to comment #9)
> There is another bunch of undef warnings while running a t/dkim2.t
> test. I'll send a patch to the author of a Mail::DKIM module, will
> probably end up in Mail-DKIM-0.37_8 and 0.38. Should be harmless though,
> it's just ugly.

Now fixed with Mail-DKIM-0.38  (already in FreeBSD ports now).
Comment 16 Justin Mason 2010-04-21 11:46:04 UTC
+1 on 4732
Comment 17 Todd Rinaldo 2010-05-05 15:08:48 UTC
I just ran make on branch 3.3 and got the below errors. Was this missed in trunk? or just not backmerged into 3.3?

rulesrc/sandbox/wtogami/20_vanity.cf: 1 active rules, 22 other
Use of "goto" to jump into a construct is deprecated at lib/Mail/SpamAssassin/Plugin/Check.pm line 409.
Comment 18 Mark Martinec 2010-05-13 12:01:22 UTC
> rulesrc/sandbox/wtogami/20_vanity.cf: 1 active rules, 22 other
> Use of "goto" to jump into a construct is deprecated at
> lib/Mail/SpamAssassin/Plugin/Check.pm line 409.

> I just ran make on branch 3.3 and got the below errors.
> Was this missed in trunk?

No, it *is* in the trunk.

> or just not backmerged into 3.3?

Indeed. As the whiteboard on the top of this bug report indicates,
the proposed patch is waiting one more vote for applying to 3.3 branch.
Comment 19 Kevin A. McGrail 2010-05-13 12:36:22 UTC
(In reply to comment #18)
> > rulesrc/sandbox/wtogami/20_vanity.cf: 1 active rules, 22 other
> > Use of "goto" to jump into a construct is deprecated at
> > lib/Mail/SpamAssassin/Plugin/Check.pm line 409.
> 
> > I just ran make on branch 3.3 and got the below errors.
> > Was this missed in trunk?
> 
> No, it *is* in the trunk.
> 
> > or just not backmerged into 3.3?
> 
> Indeed. As the whiteboard on the top of this bug report indicates,
> the proposed patch is waiting one more vote for applying to 3.3 branch.

+1 to apply combo patch. KAM
Comment 20 Mark Martinec 2010-05-13 12:48:30 UTC
Thanks!


3.3:
  Bug 6392: Test suite fails with perl 5.12.0
Sending lib/Mail/SpamAssassin/Dns.pm
Sending lib/Mail/SpamAssassin/HTML.pm
Sending lib/Mail/SpamAssassin/Plugin/BodyEval.pm
Sending lib/Mail/SpamAssassin/Plugin/Check.pm
Sending rulesrc/sandbox/jm/EmailBL.pm
Committed revision 943933.
Comment 21 Mark Martinec 2010-07-17 05:51:59 UTC
*** Bug 6463 has been marked as a duplicate of this bug. ***
Comment 22 Kevin A. McGrail 2010-07-31 22:16:46 UTC
*** Bug 6474 has been marked as a duplicate of this bug. ***
Comment 23 Michael Scheidell 2010-08-12 09:25:28 UTC
has anyone regressed test this on perl 5.8.8 5.8.9, and 5.10?
Comment 24 Todd Rinaldo 2010-08-12 10:58:49 UTC
(In reply to comment #23)
> has anyone regressed test this on perl 5.8.8 5.8.9, and 5.10?

I suggest letting CPAN testers do it for you.

Bump the version to have an underscore and it'll show up on CPAN as a dev version. This would not show up in the cpan client as available for install.

$VERSION = "3.003001_1"

Once it's up there, you would have access to all the various platforms and versions running the test suite: http://www.cpantesters.org/show/Mail-SpamAssassin.html#Mail-SpamAssassin-3.3.1
Comment 25 Todd Rinaldo 2010-09-08 17:32:02 UTC
Created attachment 4805 [details]
Changes made to release a CPAN dev version of SA

I just released 3.3.1_01 to CPAN. This will not be indexed by PAUSE since it's a dev version, however this will give CPAN testers access to it so we can see if there are currently any regressions with the 3.3 branch. I'll post the results in a couple of days.

The link doesn't exist yet but I believe it will show up here:
http://search.cpan.org/~toddr/Mail-SpamAssassin-3.3.1_01/
Comment 26 Todd Rinaldo 2010-09-09 21:02:20 UTC
This link has test reports from the 3.3.2 candidate. So far 5.10 and 5.12 pass without issue.

http://goo.gl/Y3K7
Comment 27 Karsten Bräckelmann 2010-09-19 15:55:03 UTC
*** Bug 6493 has been marked as a duplicate of this bug. ***
Comment 28 Shlomi Fish 2010-09-19 16:22:38 UTC
OK, now that I can comment on this bug - please release a new version on CPAN. A bug should not be marked as fixed until it is fixed in a stable release. "It's not over until the fat lady sings.". We don't have to suffer through this warning ever since perl-5.12.x came out on what was invalid syntax.

Please release a new version.

Regards,

-- Shlomi Fish
Comment 29 Karsten Bräckelmann 2010-09-19 17:08:43 UTC
(In reply to comment #28)
> OK, now that I can comment on this bug - please release a new version on CPAN.

Why would you not have been able to comment on this bug before I closed your duplicate report?

> A bug should not be marked as fixed until it is fixed in a stable release.

Incorrect. As I just explained to you, Status RESOLVED FIXED means that the issue has been fixed in SVN. Status CLOSED would indicate a release, but we don't usually use anything beyond RESOLVED in this bug tracker.
Comment 30 Todd Rinaldo 2010-09-20 11:56:37 UTC
(In reply to comment #23)
> has anyone regressed test this on perl 5.8.8 5.8.9, and 5.10?

So far 5.10 and 5.12 are relativeley green per CPAN testers. There seems to be a limited amount of 5.8 and below automated testers these days.

http://www.cpantesters.org/distro/M/Mail-SpamAssassin.html#Mail-SpamAssassin-3.3.1_01?grade=1&perlmat=1&patches=1&oncpan=2&distmat=3&perlver=ALL&osname=ALL&version=3.3.1_01
Comment 31 Michael Scheidell 2010-09-20 12:00:22 UTC
(In reply to comment #30)
> (In reply to comment #23)
> > has anyone regressed test this on perl 5.8.8 5.8.9, and 5.10?
> 
> So far 5.10 and 5.12 are relativeley green per CPAN testers. There seems to be
> a limited amount of 5.8 and below automated testers these days.
> 
> http://www.cpantesters.org/distro/M/Mail-SpamAssassin.html#Mail-SpamAssassin-3.3.1_01?grade=1&perlmat=1&patches=1&oncpan=2&distmat=3&perlver=ALL&osname=ALL&version=3.3.1_01

us FreeBsd guys seem to be on the ball :-)
Comment 32 kd6lvw+software 2010-10-20 15:57:45 UTC
I'm really getting sick of seeing these messages in my logs, especially when they have been fixed in the mainline trunk.  Please push out version 3.3.2 to CPAN so this lameness may disappear from everyone's syslog.

[This is a follow-up request.  Such was originally requested in July 2010, yet no one has done it.]
Comment 33 Shlomi Fish 2010-10-20 16:13:06 UTC
(In reply to comment #32)
> I'm really getting sick of seeing these messages in my logs, especially when
> they have been fixed in the mainline trunk.  Please push out version 3.3.2 to
> CPAN so this lameness may disappear from everyone's syslog.
> 
> [This is a follow-up request.  Such was originally requested in July 2010, yet
> no one has done it.]

I second that.

Regards,

-- Shlomi Fish
Comment 34 Todd Rinaldo 2010-10-20 16:58:22 UTC
I think it would be helpful for us to know what's holding up the release of 3.3.2 from the 3.3 branch (not trunk). From what I can tell, no commits have gone into it since July 2010

http://svn.apache.org/repos/asf/spamassassin/branches/3.3
Comment 35 Mark Martinec 2010-10-25 11:17:39 UTC
trunk:
  Bug 6392: fix one more case of a 'goto into a construct',
  this one occured with sa-compile
Sending lib/Mail/SpamAssassin/Plugin/BodyRuleBaseExtractor.pm
Committed revision 1027163.
Comment 36 Mark Martinec 2010-10-25 11:47:10 UTC
Created attachment 4815 [details]
a backport of  trunk's r1027163 applicable to 3.2

The attachment is a backported BodyRuleBaseExtractor.pm patch r1027163
from trunk, applicable to 3.2 and proposed for inclusion into 3.2.
A corresponding indentation change makes the patch appear more
obfuscated than it really is.

As this 'goto into a construct' is just another instance
of similar cases already covered and fixed here, I'm just
reopening it - I hope it's alright.
Comment 37 Mark Martinec 2010-10-25 11:49:07 UTC
one more case of a 'goto into a construct' to be fixed
Comment 38 Mark Martinec 2010-10-25 13:34:38 UTC
> Created an attachment (id=4815) [details]
> a backport of  trunk's r1027163 applicable to 3.2

Oops, sorry, that was indeed for 3.2, please disregards the attached patch.
Will provide one for 3.3.
Comment 39 Mark Martinec 2010-10-25 13:40:36 UTC
Created attachment 4816 [details]
the r1027163 applicable to 3.3

Ok, here is the correct one for 3.3.
In fact the patch for 3.3 is identical to trunk's r1027163.
Comment 40 D. Stussy 2010-12-08 22:00:06 UTC
Still waiting for you all to push SA 3.3.2 out to CPAN so that EVERYONE can eliminate these warnings....
Comment 41 Shlomi Fish 2010-12-10 11:45:44 UTC
Hi all,

please release a new version already.

Regards,

-- Shlomi Fish
Comment 42 D. Stussy 2011-03-11 17:48:41 UTC
Yet another 3 months later....  What's the cause of the delay?
Comment 43 Mark Martinec 2011-05-04 15:07:45 UTC
(In reply to comment #35)
> trunk:
>   Bug 6392: fix one more case of a 'goto into a construct',
>   this one occured with sa-compile
> Sending lib/Mail/SpamAssassin/Plugin/BodyRuleBaseExtractor.pm
> Committed revision 1027163.

Applying this one to 3.3 as well:

Sending lib/Mail/SpamAssassin/Plugin/BodyRuleBaseExtractor.pm
Committed revision 1099478.
Comment 44 Mark Martinec 2011-05-04 15:09:48 UTC
I think we have a go on this one, all 'goto' fixes are now backported
from trunk to 3.3.  Closing.
Comment 45 Mark Martinec 2011-05-18 10:20:09 UTC
*** Bug 6590 has been marked as a duplicate of this bug. ***
Comment 46 K Frost 2011-06-14 22:48:07 UTC
Hi all.

I appreciate the patches, even that will save me the trouble of
patching it myself, but a new CPAN release would be very useful.

Any idea when that should be expected, and/or what the holdup is?

thanks

-- frosty
Comment 47 Kevin A. McGrail 2011-06-14 22:55:38 UTC
> I appreciate the patches, even that will save me the trouble of
> patching it myself, but a new CPAN release would be very useful.
> 
> Any idea when that should be expected, and/or what the holdup is?

3.3.2 has received enough votes to be released.  I have currently uploaded a trial version to CPAN at http://cpan.perl.org/authors/id/K/KM/KMCGRAIL/Mail-SpamAssassin-3.3.2-TRIAL.tar.gz

We are working on all the rest of the bits of the release process.

Regards,
KAM