> re: 3.3.0 beta1, I generally don't like seeing a release go out that has known
> make test failures.
> 
> Currently, I'm failing on this:
> 
> t/dkim2...........................ok 32/123     Not found: 
> DKIM_ADSP_NXDOMAIN =  DKIM_ADSP_NXDOMAIN  at t/dkim2.t line 85.
> # Failed test 120 in t/SATest.pm at line 716
> 
> Turning on verbose tests, I found this output:
>         Checking for anti-pattern DKIM_ADSP_ALL at t/dkim2.t line 85.
> *** Mail::DKIM 0.36, Test 120 is expected to fail with versions
>     older than 0.36_5


> I'd like to either A) see that warning in non-verbose tests,

I do see the explanation line ("...is expected to fail...") even on
plain 'make test' run (not only on prove -v t/dkim2.t):

  t/dcc.t ........................... ok
  t/debug.t ......................... ok
  t/desc_wrap.t ..................... ok
  t/dkim.t .......................... skipped: (no reason given)
  t/dkim2.t ......................... 69/123   Not found: DKIM_ADSP_NXDOMAIN
    =  DKIM_ADSP_NXDOMAIN  at t/dkim2.t line 85.
  # Failed test 120 in t/SATest.pm at line 716


  *** Mail::DKIM 0.31, Tests 105, 109, 113, 117, 120 are expected to fail
  with versions older than 0.34
  t/dkim2.t ......................... Failed 1/123 subtests
  t/dnsbl.t ......................... ok
  t/dnsbl_sc_meta.t ................. ok


> B) Makefile.PL modified on line 185 to say 'Mail::DKIM'    => 0.36_5,
> I suggest solution B and submitting a patch for this purpose.
> Created an attachment (id=4585) [details]
> Very Simple Patch to require a newer DKIM module for install of SA

Making Mail::DKIM 0.37 a required minimal version is indeed the easiest
way out, but perhaps not everybody would appreciate it (even though this
version is out for 3 months now). I went to considerable lengths to make
the DKIM plugin 'mostly usable' even with old versions of Mail::DKIM
(see Bug 6100), and at that time we agreed that 0.31 is the oldest usable.

Making 0.37 a minimal required version would be fine with me,
if we can collect enough votes and no vetoes, which might be tricky.



> or C) see a warning 
> from the Makefile.PL advising they upgrade if not require it before I +1.

The release notes document it:

- CPAN module requirements:
  - minimal version of Mail::DKIM is 0.31 (preferred: 0.36_5 or later)
  [...]
  DKIM PLUGIN
- absolute minimal version of Mail::DKIM is 0.31;
  support for ADSP requires Mail::DKIM 0.34;
  a DNS test (and rule) for NXDOMAIN is operational since Mail::DKIM 0.36_5

Perhaps the Mail/SpamAssassin/Util/DependencyInfo.pm could be enhanced
to report recommended versions of modules, when not met.

> > or C) see a warning 
> > from the Makefile.PL advising they upgrade if not require it before I +1.

Attached is a patch to DependencyInfo.pm. It introduces a 
tag 'recommended_min_version', and issues a warning if a module
does not meet it. Please see if this meets our needs.


In case of an old Mail::DKIM, it would look like:


$ perl Makefile.PL
[...]
***************************************************************************
NOTE: the optional Mail::DKIM (version 0.31) module is installed (0.31),   
but is below the recommended version 0.37, some functionality will not be available.

  If this module is installed and the DKIM plugin is enabled,
  SpamAssassin will perform DKIM signature verification when DKIM-Signature
  header fields are present in the message headers, and check ADSP rules
  (e.g. anti-phishing) when a mail message does not contain a valid author
  domain signature. Version 0.36_5 or later is needed to fully support ADSP.
[...]
optional module missing: Net::Ident
optional module older than recommended: Mail::DKIM
optional module missing: DBI

warning: some functionality may not be available,
please read the above report before continuing!

Created attachment 4587 [details]
a patch to DependencyInfo.pm, introducing recommended_min_version

Created attachment 4588 [details]
fixed patch

take two

Any opinions on that solution?


Here are the diagnostics as issued (with the patch):

1. no Mail::DKIM module at all:
***************************************************************************
NOTE: the optional Mail::DKIM module is not installed.

  If this module is installed and the DKIM plugin is enabled,
  SpamAssassin will perform DKIM signature verification when DKIM-Signature
  header fields are present in the message headers, and check ADSP rules
  (e.g. anti-phishing) when a mail message does not contain a valid author
  domain signature. Version 0.36_5 or later is needed to fully support ADSP.

optional module missing: Net::Ident
optional module missing: Mail::DKIM

warning: some functionality may not be available,
please read the above report before continuing!

Warning: prerequisite Mail::DKIM 0.31 not found.


2. Mail::DKIM older than required (0.30):
***************************************************************************
NOTE: the optional Mail::DKIM module is installed (0.30),
but is below the minimum required version 0.31, some functionality will not be available.
[...]
optional module missing: Net::Ident
optional module out of date: Mail::DKIM

warning: some functionality may not be available,
please read the above report before continuing!

Warning: prerequisite Mail::DKIM 0.31 not found. We have 0.30.


3. some interim version installed (0.36):
***************************************************************************
NOTE: the optional Mail::DKIM module is installed (0.36),
but is below the recommended version 0.37, some functionality may not be available.
[...]
optional module missing: Net::Ident
optional module older than recommended: Mail::DKIM

warning: some functionality may not be available,
please read the above report before continuing!


4. fresh version installed (0.37):
(module not mentioned in the perl Makefile.PL report)

Created attachment 4589 [details]
final proposed patch

say 'may' instead of 'will'; for consistency do a 'use' instead of 'require';
print one empty line after diagnostics in dkim2.t to let the warning stand out
more clearly

(In reply to comment #0)
> Turning on verbose tests, I found this output:
> 
>         Checking for anti-pattern DKIM_ADSP_ALL at t/dkim2.t line 85.
> 
> 
> *** Mail::DKIM 0.36, Test 120 is expected to fail with versions older than 
> 0.36_5
> ok 121
> 
> 
> I'd like to either A) see that warning in non-verbose tests, B) Makefile.PL 
> modified on line 185 to say 'Mail::DKIM'    => 0.36_5, or C) see a warning 
> from the Makefile.PL advising they upgrade if not require it before I +1.


I don't think C) will actually avoid the confused users and mails to the list about the test failing.

what about D) skip the test if Mail::DKIM version < 0.36_5?   is that not viable?

I like this idea a lot.  One minor note is that I installed 0.28 for testing and it still came up with the 0.31 message though later on it did say:

Warning: prerequisite Mail::DKIM 0.31 not found. We have 0.28.

In general though, I'm +1 for certain!

(In reply to comment #8)
> I like this idea a lot.  One minor note is that I installed 0.28 for testing
> and it still came up with the 0.31 message though later on it did say:

Which one, my improved reporting by Makefile.PL, or Justin's
choice D suggestion?

> Warning: prerequisite Mail::DKIM 0.31 not found. We have 0.28.

That is correct, although it can be improved to say something like:
... 0.31 not found (0.37 recommended). We have 0.28.


(In reply to comment #7)
> what about D) skip the test if Mail::DKIM version < 0.36_5?
> is that not viable?

It is certainly possible. This was my first idea, but I dismissed it,
because if all tests pass, people may be misled into believing that
the complete DKIM functionality is available and working.

Sorry, I was approving your DependencyInfo.pm patch.  I don't like the option D as it leads to limited functionality but I do agree that the work you put into supporting older versions is good and valuable.

However, the DependencyInfo patch has a small logic issue somewhere:

Here's the output from perl Makefile.PL with DKIM 0.28 installed for testing purposes:

***************************************************************************
NOTE: the optional Mail::DKIM (version 0.31) module is installed,
but is not an up-to-date version.

  If this module is installed and the DKIM plugin is enabled,
  SpamAssassin will perform DKIM signature verification when DKIM-Signature
  header fields are present in the message headers, and check ADSP rules
  (e.g. anti-phishing) when a mail message does not contain a valid author
  domain signature. Version 0.36_5 or later is needed to fully support ADSP.


Warning: prerequisite Mail::DKIM 0.31 not found. We have 0.28.

If below 0.31 (required) I woudln't show the optional installation message at all as 0.31 is the minimum.

> (In reply to comment #7)
> > what about D) skip the test if Mail::DKIM version < 0.36_5?
> > is that not viable?
> 
> It is certainly possible. This was my first idea, but I dismissed it,
> because if all tests pass, people may be misled into believing that
> the complete DKIM functionality is available and working.

ok.
+1 for option C).  Minor nit, could we amend that message to say

'NOTE: the optional Mail::DKIM module is installed (0.36),
but is below the recommended version 0.37, some functionality may not be
available, and some of the DKIM tests in the SpamAssassin test suite
may fail.'


ie. be clear about the test failures.

I wonder why the STDERR was being hidden in Kevin's case? odd.

Ok, here is the output of the new DependencyInfo.pm (to be attached).
Playing with modules Archive::Tar (required) and Mail::DKIM (optional):



1)======================

$ perl Makefile.PL 
[...]
checking module dependencies and their versions...

***************************************************************************
ERROR: the required Archive::Tar module is not installed,                  
minimum required version is 1.23.                                          

  The "sa-update" program requires this module to access tar update
  archive files.                                                   


***************************************************************************
NOTE: the optional Net::Ident module is not installed.

  If you plan to use the --auth-ident option to spamd, you will need
  to install this module.


***************************************************************************
NOTE: the optional Mail::DKIM module is not installed,
minimum required version is 0.31, recommended version is 0.37 or higher.

  If this module is installed and the DKIM plugin is enabled,
  SpamAssassin will perform DKIM signature verification when DKIM-Signature
  header fields are present in the message headers, and check ADSP rules
  (e.g. anti-phishing) when a mail message does not contain a valid author
  domain signature. Version 0.36_5 or later is needed to fully support ADSP.

REQUIRED module missing: Archive::Tar
optional module missing: Net::Ident
optional module missing: Mail::DKIM

warning: some functionality may not be available,
please read the above report before continuing!


2)======================

$ perl Makefile.PL
[...]
checking module dependencies and their versions...

***************************************************************************
ERROR: the required Archive::Tar module is installed (1.22),               
but is below the minimum required version 1.23,                            
some functionality will not be available.                                  

  The "sa-update" program requires this module to access tar update
  archive files.                                                   


***************************************************************************
NOTE: the optional Net::Ident module is not installed.                     

  If you plan to use the --auth-ident option to spamd, you will need
  to install this module.


***************************************************************************
NOTE: the optional Mail::DKIM module is installed (0.29),
but is below the minimum required version 0.31,
some functionality will not be available.
Recommended version is 0.37 or higher.

  If this module is installed and the DKIM plugin is enabled,
  SpamAssassin will perform DKIM signature verification when DKIM-Signature
  header fields are present in the message headers, and check ADSP rules
  (e.g. anti-phishing) when a mail message does not contain a valid author
  domain signature. Version 0.36_5 or later is needed to fully support ADSP.

REQUIRED module out of date: Archive::Tar
optional module missing: Net::Ident
optional module out of date: Mail::DKIM

warning: some functionality may not be available,
please read the above report before continuing!


3)======================

$ perl Makefile.PL
[...]
checking module dependencies and their versions...

***************************************************************************
NOTE: the optional Net::Ident module is not installed.                     

  If you plan to use the --auth-ident option to spamd, you will need
  to install this module.


***************************************************************************
NOTE: the optional Mail::DKIM module is installed (0.35),
but is below the recommended version 0.37,
some functionality may not be available,
and some of the tests in the SpamAssassin test suite may fail.

  If this module is installed and the DKIM plugin is enabled,
  SpamAssassin will perform DKIM signature verification when DKIM-Signature
  header fields are present in the message headers, and check ADSP rules
  (e.g. anti-phishing) when a mail message does not contain a valid author
  domain signature. Version 0.36_5 or later is needed to fully support ADSP.

optional module missing: Net::Ident
optional module older than recommended: Mail::DKIM

warning: some functionality may not be available,
please read the above report before continuing!

Writing Makefile for Mail::SpamAssassin
Makefile written by ExtUtils::MakeMaker 6.55

Created attachment 4597 [details]
new DependencyInfo.pm

new here is a replacement DependencyInfo.pm, which produces
the above results

Mark, can you attach that as a patch, for review?  it's hard to tell what's changed with a totally new replacement file.

Created attachment 4599 [details]
proposed patch

Here it is as a patch.  Btw, changes of a version '0.00' to 0
is because a string '0.00' is treated as true in perl.

Can we say 0.37 instead of 0.36_5 everywhere?  0.36_5 didn't exist as the newer version for very long, and it doesn't universally work in all version comparison algorithms.  We can confuse users a less to just say 0.37.

> Can we say 0.37 instead of 0.36_5 everywhere?  0.36_5 didn't exist as the newer
> version for very long, and it doesn't universally work in all version
> comparison algorithms.  We can confuse users a less to just say 0.37.

Agreed. The 0.37 has now been available sufficiently long,
so that there is no longer a need to refer to its beta versions.

+1 looks great

One more vote please.

(assuming the:
  domain signature. Version 0.36_5 or later is needed to fully support ADSP.
will be replaced by:
  domain signature. Version 0.37 or later is needed to fully support ADSP.
as per comment Comment 16 and 17)

+1

Thanks for a review!


  Bug 6246: let DependencyInfo.pm understand a concept of
  recommended module version, besides a required version;
  polish reporting, esp. in view of Mail::DKIM dependencies
Sending lib/Mail/SpamAssassin/Util/DependencyInfo.pm
Sending t/dkim2.t
Committed revision 891207.

resolved, closing