[PATCH] Re: Version object support

[John Peacock]

Michael G Schwern wrote:
 The code looks ok to me.  Stick it on RT as a patch so I won't forget it.

Complete patch (plus tests) for version objects as initializer to VERSION
parameter.  I'm still going to try and get a patch prepared to include a cutdown
version object inside EU::MM itself, and I'll take a look at closing several of
the other $VERSION-related bug reports in RT at the same time...

John

Re: Version object support

[Andreas J. Koenig]

 On Thu, 29 Mar 2007 13:33:01 -0700, Michael G Schwern [EMAIL PROTECTED] 
 said:

   Well, just because PAUSE isn't doing it quite right doesn't mean everyone 
else
   has to.

Well said. Besides, PAUSE honours META.yml and so hopefully is future proof.

-- 
andreas

Re: Version object support

[John Peacock]

Michael G Schwern wrote:

Hold up.  Why won't isa() work on base version objects?  What's a base
version object?  Did you instead mean so it will work on non-object references
(ie. hash refs, etc...)?  Or did you mean normal string/number versions?


Oh, isa() will work fine with base version objects (meaning not a 
version subclass), but rather than completely restructure the code, I'm 
still taking a ref($val) (which will be the class name for objects).  If 
the class name matches exactly, the first half of the test will succeed 
and the eval will never fire.  If not, we have the second half which 
will test if an object is a child of the class we are testing.  This 
should be future proof above and beyond the initial 'version' case.



{ $_  ($given eq $_ || eval{$val-isa($_)) }



If you eliminate that how will it display the right message when it accepts a
non-reference?


Yeah, ignore that; I realize that I can't cheat at that point in the 
testing.


As soon as I get some tests, I'll put the patch into RT...

John

Re: Version object support

[John Peacock]

Michael G Schwern wrote:
 That's where it falls down.  As mentioned before, two reasons.
 
 1) _verify_att() is currently case insensitive.  This is because I was lazy
 and found hash easier to write than HASH by hand.  This means if you just
 add in a key of 'version' it will happily accept VERSION, version and VerSioN
 objects.  So that has to be fixed.

This turns out to be a trivial change, so a revised patch is attached.  I'll
generate a new test file (since it needs to skip all if version.pm is not
installed).

 2) _verify_att() currently uses ref() because it has only had to deal with
 references so far.  This means any subclass instances of 'version' will be
 rejected and that would be impolite.  It should be fixed to use isa().

isa() would normally be correct from an OO point of view, but there are
limitations that mean that the more strict ref() is the correct choice.  The
_only_ non-numeric initializers for $VERSION that are supported by PAUSE and
CPAN are version.pm objects.  version.pm objects *always* return a ref() of
'version' (whether implemented by XS or pure Perl code).  Even though I uploaded
version::AlphaBeta to CPAN (as a proof of concept), that subclass of version.pm
is not supported by PAUSE, so any module using that class would not be 
indexable.

To this end, it would be entirely appropriate that EU::MM *would* complain about
a subclass of version.pm being used instead of version.pm itself.  No subclass
of version.pm is supported by the entire toolchain, which is all we are trying
to accomplish here.  isa() is too general for this case; we really do want to
use ref() and only accept 'version' objects (in addition to scalars).

Does this make any more sense?  I'm trying to make the most minimal extension to
the allowed signatures, to prevent mistakes from being made.  If there was a way
to extend the basic types of Perl (e.g. ARRAY, CODE, HASH) other than through
object classes, I would have gone that route with version.pm objects and created
a first class 'version' type.  We do not want to support any random subclass of
version.pm - we only want to support true version.pm objects.  And those will
always return ref() eq 'version'.

John

-- 
John Peacock
Director of Information Research and Technology
Rowman  Littlefield Publishing Group
4501 Forbes Blvd
Suite H
Lanham, MD 20706
301-459-3366 x.5010
fax 301-429-5747
Index: t/writemakefile_args.t
===
--- t/writemakefile_args.t	(revision 3955)
+++ t/writemakefile_args.t	(working copy)
@@ -53,7 +53,7 @@
 };
 
 is( $warnings, VERIFY );
-WARNING: MAN3PODS takes a hash reference not a string/number.
+WARNING: MAN3PODS takes a HASH reference not a string/number.
  Please inform the author.
 VERIFY
 
@@ -67,7 +67,7 @@
 };
 
 is( $warnings, VERIFY );
-WARNING: AUTHOR takes a string/number not a code reference.
+WARNING: AUTHOR takes a string/number not a CODE reference.
  Please inform the author.
 VERIFY
 
@@ -105,7 +105,7 @@
 };
 
 # We'll get warnings about the bogus libs, that's ok.
-like( $warnings, qr{^WARNING: LIBS takes a array reference or string/number not a hash reference}m );
+like( $warnings, qr{^WARNING: LIBS takes a ARRAY reference or string/number not a HASH reference}m );
 
 
 $warnings = '';
Index: lib/ExtUtils/MakeMaker.pm
===
--- lib/ExtUtils/MakeMaker.pm	(revision 3955)
+++ lib/ExtUtils/MakeMaker.pm	(working copy)
@@ -68,41 +68,42 @@
 # scalar.
 my %Att_Sigs;
 my %Special_Sigs = (
- C  = 'array',
- CONFIG = 'array',
- CONFIGURE  = 'code',
- DIR= 'array',
- DL_FUNCS   = 'hash',
- DL_VARS= 'array',
- EXCLUDE_EXT= 'array',
- EXE_FILES  = 'array',
- FUNCLIST   = 'array',
- H  = 'array',
- IMPORTS= 'hash',
- INCLUDE_EXT= 'array',
- LIBS   = ['array',''],
- MAN1PODS   = 'hash',
- MAN3PODS   = 'hash',
- PL_FILES   = 'hash',
- PM = 'hash',
- PMLIBDIRS  = 'array',
- PMLIBPARENTDIRS= 'array',
- PREREQ_PM  = 'hash',
- SKIP   = 'array',
- TYPEMAPS   = 'array',
- XS = 'hash',
+ C  = 'ARRAY',
+ CONFIG = 'ARRAY',
+ CONFIGURE  = 'CODE',
+ DIR= 'ARRAY',
+ DL_FUNCS   = 'HASH',
+ DL_VARS= 'ARRAY',
+ EXCLUDE_EXT= 'ARRAY',
+ EXE_FILES  = 'ARRAY',
+ FUNCLIST   = 'ARRAY',
+ H  = 'ARRAY',
+ IMPORTS= 'HASH',
+ INCLUDE_EXT= 'ARRAY',
+ LIBS   = ['ARRAY',''],
+ MAN1PODS   = 'HASH',
+ MAN3PODS   = 'HASH',
+ PL_FILES   = 'HASH',
+ PM = 'HASH',
+ PMLIBDIRS  = 'ARRAY',
+ PMLIBPARENTDIRS= 'ARRAY',
+ PREREQ_PM  = 'HASH',
+ SKIP 

Re: Version object support

[Michael G Schwern]

John Peacock wrote:
 2) _verify_att() currently uses ref() because it has only had to deal with
 references so far.  This means any subclass instances of 'version' will be
 rejected and that would be impolite.  It should be fixed to use isa().
 
 isa() would normally be correct from an OO point of view, but there are
 limitations that mean that the more strict ref() is the correct choice.  The
 _only_ non-numeric initializers for $VERSION that are supported by PAUSE and
 CPAN are version.pm objects.  version.pm objects *always* return a ref() of
 'version' (whether implemented by XS or pure Perl code).  Even though I 
 uploaded
 version::AlphaBeta to CPAN (as a proof of concept), that subclass of 
 version.pm
 is not supported by PAUSE, so any module using that class would not be 
 indexable.
 
 To this end, it would be entirely appropriate that EU::MM *would* complain 
 about
 a subclass of version.pm being used instead of version.pm itself.  No subclass
 of version.pm is supported by the entire toolchain

That strikes me as odd.  Why not?


 We do not want to support any random subclass of
 version.pm - we only want to support true version.pm objects.

This, too, strikes me as odd.  Why not?

Re: Version object support

[John Peacock]

Michael G Schwern wrote:
 We do not want to support any random subclass of
 version.pm - we only want to support true version.pm objects.
 
 This, too, strikes me as odd.  Why not?

Because the PAUSE indexer runs everything in a safe compartment, with a very
restrictive methodology, and every additional class of version objects would
have to be added manually.  As I said, I would rather have implemented version
objects as a first class type, rather than as a class.  I'm just trying to keep
the abstraction as tight as possible.

That's not to say that version objects are a dead end once it hits 1.0.  I
already have version::Limit (allows a module author to strictly define what
future releases are compatible with what earlier API's).  At some point, I will
release version::Math (to make it easier to create scripts to increment $VERSION
when releasing modules to CPAN).  But those things don't need to be supported
during the release/build/install cycle.  Only version.pm objects meet that
particular need, because of all of the work I took to make sure it DTRT under
all circumstances (e.g. it works like a number when it needs to and works
like a version when it needs to).

John

-- 
John Peacock
Director of Information Research and Technology
Rowman  Littlefield Publishing Group
4501 Forbes Blvd
Suite H
Lanham, MD 20706
301-459-3366 x.5010
fax 301-429-5747

Re: Version object support

[Andy Armstrong]

On 29 Mar 2007, at 12:53, John Peacock wrote:

At some point, I will
release version::Math (to make it easier to create scripts to  
increment $VERSION

when releasing modules to CPAN).


Feel free to use Perl::Version[1] in any way you fancy. It isn't a  
subclass of version because - for my application - I couldn't work  
with some of the odd things version does with numeric versions.


Perl::Version includes a utility called perl-reversion for managing  
module version numbers (incrementing, setting, reporting)


[1] http://search.cpan.org/~andya/Perl-Version-v0.0.7

--
Andy Armstrong, hexten.net

Re: Version object support

[Michael G Schwern]

John Peacock wrote:
 Michael G Schwern wrote:
 We do not want to support any random subclass of
 version.pm - we only want to support true version.pm objects.
 This, too, strikes me as odd.  Why not?
 
 Because the PAUSE indexer runs everything in a safe compartment, with a very
 restrictive methodology, and every additional class of version objects would
 have to be added manually.  As I said, I would rather have implemented version
 objects as a first class type, rather than as a class.  I'm just trying to 
 keep
 the abstraction as tight as possible.

Well, just because PAUSE isn't doing it quite right doesn't mean everyone else
has to.  If every piece of the build chain hard codes version then when it
comes time to allow subclasses of it we'll have to fix every piece again.

Go ahead and use isa() to check the class of the version object.  Can't hurt.

Re: Version object support

[John Peacock]

Michael G Schwern wrote:
 Go ahead and use isa() to check the class of the version object.  Can't hurt.

...said the man who wasn't working on the patch. ;-)

OK, here's my compromise:  we still use ref for the first pass through (so it
will handle the base version objects), and we try real hard to use isa() safely
if that fails.  Here's that block of the diff:

@@ -120,8 +121,8 @@
 }

 my @sigs   = ref $sig ? @$sig : $sig;
-my $given = lc ref $val;
-unless( grep $given eq $_, @sigs ) {
+my $given  = ref $val;
+unless( grep { $given eq $_ || ($_  eval{$val-isa($_)}) } @sigs ) {
 my $takes = join  or , map { $_ ne '' ? $_ reference
 : string/number
  } @sigs;

I'm going to write a test that exercises this code, but I already confirmed that
a subclass of version will fail the first || term and pass the second.  Now that
I look at it again, I'd like to rewrite that grep block to be

{ $_  ($given eq $_ || eval{$val-isa($_)) }

which is (to my mind) a little clearer about the intent of the tests.  Of course
I personally would also edit the next line to eliminate the useless ne ''
(which isn't clearer to my eyes).

Does this work for you?

John

-- 
John Peacock
Director of Information Research and Technology
Rowman  Littlefield Publishing Group
4501 Forbes Blvd
Suite H
Lanham, MD 20706
301-459-3366 x.5010
fax 301-429-5747

Re: Version object support

[Michael G Schwern]

John Peacock wrote:
 Michael G Schwern wrote:
 Go ahead and use isa() to check the class of the version object.  Can't hurt.
 
 ...said the man who wasn't working on the patch. ;-)

...said the man who doesn't really want it in the first place. :P  But if
we're going to do it we're going to do it right.

Don't worry, I'll stop torturing you soon.


 OK, here's my compromise:  we still use ref for the first pass through (so it
 will handle the base version objects)

Hold up.  Why won't isa() work on base version objects?  What's a base
version object?  Did you instead mean so it will work on non-object references
(ie. hash refs, etc...)?  Or did you mean normal string/number versions?


 and we try real hard to use isa() safely
 if that fails.  Here's that block of the diff:
 
 @@ -120,8 +121,8 @@
  }
 
  my @sigs   = ref $sig ? @$sig : $sig;
 -my $given = lc ref $val;
 -unless( grep $given eq $_, @sigs ) {
 +my $given  = ref $val;
 +unless( grep { $given eq $_ || ($_  eval{$val-isa($_)}) } @sigs ) 
 {
  my $takes = join  or , map { $_ ne '' ? $_ reference
  : string/number
   } @sigs;
 
 I'm going to write a test that exercises this code, but I already confirmed 
 that
 a subclass of version will fail the first || term and pass the second.  Now 
 that
 I look at it again, I'd like to rewrite that grep block to be
 
   { $_  ($given eq $_ || eval{$val-isa($_)) }
 
 which is (to my mind) a little clearer about the intent of the tests.  Of 
 course
 I personally would also edit the next line to eliminate the useless ne ''
 (which isn't clearer to my eyes).

If you eliminate that how will it display the right message when it accepts a
non-reference?

The code looks ok to me.  Stick it on RT as a patch so I won't forget it.

Re: Version object support

[John Peacock]

Michael G Schwern wrote:

As that subroutine is only called in one place, its the very first
thing in WriteMakefile() even before a MakeMaker object is created, I
can say its fairly impossible to get that from a $VERSION
declaration.


I wish you had taken me at my word, rather than try to explain it away:


*** Module::AutoInstall configuration finished.
WARNING: VERSION takes a string/number not a version reference.
 Please inform the author.
Writing Makefile for SVK



What you failed to remember is that not all code that uses 
ExtUtil::MakeMaker is code *you* control.  The patch, to refresh your 
memory, was trivial:


--- MakeMaker.pm.orig   2006-12-25 13:10:21.0 -0500
+++ MakeMaker.pm2006-12-25 13:13:59.0 -0500
@@ -90,6 +90,7 @@
  SKIP   = 'array',
  TYPEMAPS   = 'array',
  XS = 'hash',
+ VERSION= ['version',''],
  _KEEP_AFTER_FLUSH  = '',

  clean  = 'hash',


Thanks

John

Re: Version object support

[Michael G Schwern]

John Peacock wrote:
 Michael G Schwern wrote:
 As that subroutine is only called in one place, its the very first
 thing in WriteMakefile() even before a MakeMaker object is created, I
 can say its fairly impossible to get that from a $VERSION
 declaration.
 
 I wish you had taken me at my word, rather than try to explain it away:

 What you failed to remember is that not all code that uses
 ExtUtil::MakeMaker is code *you* control.

 The patch, to refresh your memory, was trivial:

When it comes to MakeMaker I am very conservative and very skeptical.  I don't
like to take bug reports and patches without throughly understanding them.
You sent a patch about a bug with a single line of code to trigger it.  I was
unable to replicate that bug and asked for more information.
http://www.nntp.perl.org/group/perl.makemaker/2007/02/msg2694.html

Furthermore, you said you'd come back with the full bug report.
http://www.nntp.perl.org/group/perl.makemaker/2007/02/msg2699.html

I never got that report.

A user says When I do A, B happens.  By my figuring of the code and testing
that's impossible.  There must be something missing from the report.
Something else is going on.  I can't reproduce the problem.  I'm waiting for
more information from the reporter.  You're damn right I'm not going to just
apply the patch!  Don't get snippy, if I took every patch at its word CPAN
would be breaking twice a month.

Then I went and dug out an actual case where the warning does happen!
http://www.nntp.perl.org/group/perl.makemaker/2007/02/msg2699.html

Maybe its not your case, but you didn't give me much to go on.  Either way,
the problem is acknowledged as a valid one however its getting triggered.

I also noted what will have to happen in order for the problem to really be
fixed.  Your trivial fix is too trivial, I'm sorry, and had to be rejected.  I
await the slightly less trivial patch.  If you have questions about how to
implement that I'll be happy to help.


 *** Module::AutoInstall configuration finished.
 WARNING: VERSION takes a string/number not a version reference.
  Please inform the author.
 Writing Makefile for SVK

And finally, only now is it shown that this isn't vanilla MakeMaker at all but
Module::AutoInstall which is part of Module::Install which breaks the
MakeMaker warranty nine ways to Sunday!  I have no idea what it might be doing
or what you're doing to trigger this code.  I still lack an explanation about
how to trigger this bug and I'm not going to spend time guessing when you can
just attach a tarball with the failing code instead of feeding me a few
out-of-context lines at a time.

Re: Version object support

[John Peacock]

Michael G Schwern wrote:
 I never got that report.

I never found the original report that *I* got, which is why I never sent
anything more.  When I got that original report, I know I spent hours in a
debugger trying to figure out where it happened.  I then patched my copy of
MakeMaker and forgot the details (but I did remember what needed fixing).  But
see the next paragraph for why my trivial fix is still likely to be the correct 
one.

 I also noted what will have to happen in order for the problem to really be
 fixed.  Your trivial fix is too trivial, I'm sorry, and had to be rejected.  I
 await the slightly less trivial patch.  If you have questions about how to
 implement that I'll be happy to help.

OK, here's the problem.  ExtUtils::MakeMaker is written to assume only certain
hash values (for the keys that EU::MM accepts as parameters) can be anything but
scalars.  That's what %Special_Sigs is used for.  EU::MM has decided /a priori/
that anything not in that hash must be a scalar or a warning is thrown.  EU::MM
is validating hash values based on certain assumptions, which in this case are
no longer true.

Once upon a time, $VERSION contained only a number (or a string that looked like
a number with underscores).  That isn't true any more.  $VERSION can now contain
a version object.  So my trivial patch simply adds the VERSION key and the
possible values of ['version',''] (version object or scalar).

 I still lack an explanation about
 how to trigger this bug and I'm not going to spend time guessing when you can
 just attach a tarball with the failing code instead of feeding me a few
 out-of-context lines at a time.
 

This doesn't have anything really to do with Module::Install.  Here's how to
reproduce it.  Download the SVK tarball.  Unpack it and type

perl Makefile.PL

After you answer any questions about installing missing (or optional) modules,
you get the warning.  Here's the backtrace from that point:

. = ExtUtils::MakeMaker::_verify_att(ref(HASH)) called from file
`/usr/lib/perl5/5.8.8/ExtUtils/MakeMaker.pm' line 57
$ = ExtUtils::MakeMaker::WriteMakefile('test', ref(HASH), 'NAME', 'SVK',
'DISTNAME', 'SVK', 'AUTHOR', 'Chia-liang Kao [EMAIL PROTECTED]', 'DIR',
ref(ARRAY), 'dist', ref(HASH), 'clean', ref(HASH), 'NO_META', 1, 'ABSTRACT', 'A
decentralized version control system', 'PL_FILES', ref(HASH), 'EXE_FILES',
ref(ARRAY), 'VERSION', ref(version), 'PREREQ_PM', ref(HASH)) called from file
`inc/Module/Install/Makefile.pm' line 147
. = Module::Install::Makefile::write(ref(Module::Install::Makefile)) called from
file `inc/Module/Install/WriteAll.pm' line 37
. = Module::Install::WriteAll::WriteAll(undef, undef, undef, 'sign', 1) called
from file `Makefile.PL' line 142

I *should* be able to use the exact code you cited:

use ExtUtils::MakeMaker;
use version;

WriteMakefile(
NAME= Foo,
VERSION = qv(2.0.0),
);

and not have EU::MM throw a warning.  I think you are overthinking this into
imagining you need to generically support random objects everywhere.  I'm just
saying that there is now a legitimate way to initialize $VERSION that uses an
object, instead of a scalar, and it would be reasonable for EU::MM to support
that...

John

Re: Version object support

[Michael G Schwern]

John Peacock wrote:
 I also noted what will have to happen in order for the problem to really be
 fixed.  Your trivial fix is too trivial, I'm sorry, and had to be rejected.  
 I
 await the slightly less trivial patch.  If you have questions about how to
 implement that I'll be happy to help.
 
 OK, here's the problem.  ExtUtils::MakeMaker is written to assume only certain
 hash values (for the keys that EU::MM accepts as parameters) can be anything 
 but
 scalars.  That's what %Special_Sigs is used for.  EU::MM has decided /a 
 priori/
 that anything not in that hash must be a scalar or a warning is thrown.  
 EU::MM
 is validating hash values based on certain assumptions, which in this case are
 no longer true.

I agree with all that, no question.  It sure would be handy if there was a way
to extend %Special_Sigs.  That would also allay my misgivings about special
casing a particular module.


 Once upon a time, $VERSION contained only a number (or a string that looked 
 like
 a number with underscores).  That isn't true any more.  $VERSION can now 
 contain
 a version object.  So my trivial patch simply adds the VERSION key and the
 possible values of ['version',''] (version object or scalar).

That's where it falls down.  As mentioned before, two reasons.

1) _verify_att() is currently case insensitive.  This is because I was lazy
and found hash easier to write than HASH by hand.  This means if you just
add in a key of 'version' it will happily accept VERSION, version and VerSioN
objects.  So that has to be fixed.

2) _verify_att() currently uses ref() because it has only had to deal with
references so far.  This means any subclass instances of 'version' will be
rejected and that would be impolite.  It should be fixed to use isa().

A patch for version objects would have to make those two changes.


 I still lack an explanation about
 how to trigger this bug and I'm not going to spend time guessing when you can
 just attach a tarball with the failing code instead of feeding me a few
 out-of-context lines at a time.

 
 This doesn't have anything really to do with Module::Install.  Here's how to
 reproduce it.  Download the SVK tarball.  Unpack it and type
 
   perl Makefile.PL
 
 After you answer any questions about installing missing (or optional) modules,
 you get the warning.  Here's the backtrace from that point:
 
 . = ExtUtils::MakeMaker::_verify_att(ref(HASH)) called from file
 `/usr/lib/perl5/5.8.8/ExtUtils/MakeMaker.pm' line 57
 $ = ExtUtils::MakeMaker::WriteMakefile('test', ref(HASH), 'NAME', 'SVK',
 'DISTNAME', 'SVK', 'AUTHOR', 'Chia-liang Kao [EMAIL PROTECTED]', 'DIR',
 ref(ARRAY), 'dist', ref(HASH), 'clean', ref(HASH), 'NO_META', 1, 'ABSTRACT', 
 'A
 decentralized version control system', 'PL_FILES', ref(HASH), 'EXE_FILES',
 ref(ARRAY), 'VERSION', ref(version), 'PREREQ_PM', ref(HASH)) called from file
 `inc/Module/Install/Makefile.pm' line 147
 . = Module::Install::Makefile::write(ref(Module::Install::Makefile)) called 
 from
 file `inc/Module/Install/WriteAll.pm' line 37

That explains why it happens with a version_from() in Module::Install and not
with a VERSION_FROM in MakeMaker.  Module::Install is pulling the version out
itself and passing it into WriteMakefile() as VERSION.

Now all is clear.


 . = Module::Install::WriteAll::WriteAll(undef, undef, undef, 'sign', 1) called
 from file `Makefile.PL' line 142
 
 I *should* be able to use the exact code you cited:
 
 use ExtUtils::MakeMaker;
 use version;
 
 WriteMakefile(
 NAME= Foo,
 VERSION = qv(2.0.0),
 );
 
 and not have EU::MM throw a warning.

We're having a violent agreement.


 I think you are overthinking this into
 imagining you need to generically support random objects everywhere.  I'm just
 saying that there is now a legitimate way to initialize $VERSION that uses an
 object, instead of a scalar, and it would be reasonable for EU::MM to support
 that...

I'm only vaguely worried about making a single class special.  However, as it
will be special in 5.10 I'm not so worried.  Allowing the ability to extend
what keys and types WriteMakefile() will take would be nice, but I'm not
asking you to do that.

I'll take a patch to support version objects, but it has to be done right and
there's two things in _verify_att() that have to change for that to happen.
Some tests would be spiffy.

Re: Version object support

[John Peacock]

Michael G Schwern wrote:
 So what's the warning you're seeing?

WARNING: VERSION takes a string/number not a version reference.
 Please inform the author.

From this chunk here (in _verify_att):

warn WARNING: $key takes a $takes not a $has.\n.
  Please inform the author.\n;

but I'm having a brainfart trying to make it happen again.  I'll check through
my various collections of e-mails to see when it got reported to me...

John

Re: Version object support

[Michael G Schwern]

John Peacock wrote:
 Michael G Schwern wrote:
 So what's the warning you're seeing?
 
 WARNING: VERSION takes a string/number not a version reference.
  Please inform the author.
 
From this chunk here (in _verify_att):
 
 warn WARNING: $key takes a $takes not a $has.\n.
   Please inform the author.\n;
 
 but I'm having a brainfart trying to make it happen again.  I'll check through
 my various collections of e-mails to see when it got reported to me...

As that subroutine is only called in one place, its the very first thing in 
WriteMakefile() even before a MakeMaker object is created, I can say its fairly 
impossible to get that from a $VERSION declaration.

What probably happened is this:

use ExtUtils::MakeMaker;
use version;

WriteMakefile(
NAME= Foo,
VERSION = qv(2.0.0),
);

If the _verify_att() code is going to start working with objects then the code 
has to
A) Start using -isa().
B) Stop doing the reference check case insensitively.

Re: Version object support

[Michael G Schwern]

John Peacock wrote:
 It was pointed out to me that ExtUtils::MakeMaker is needlessly warning when
 someone uses a line like this:
 
 use version; our $VERSION = qv('2.0.0');
 
 The warning can be safely ignored, but it would be better if EU::MM were to be
 less whiny about this.  A trivial patch follows:
 
 --- MakeMaker.pm.orig   2006-12-25 13:10:21.0 -0500
 +++ MakeMaker.pm2006-12-25 13:13:59.0 -0500
 @@ -90,6 +90,7 @@
   SKIP   = 'array',
   TYPEMAPS   = 'array',
   XS = 'hash',
 + VERSION= ['version',''],
   _KEEP_AFTER_FLUSH  = '',
 
   clean  = 'hash',

If I'm not mistaken that's the Makefile.PL argument handling code which should 
not be related to the $VERSION checking code.  Furthermore, I see no warning.

0 windhund ~/tmp/Foo$ cat Makefile.PL 
#!/usr/bin/perl -w

use ExtUtils::MakeMaker;

WriteMakefile(
NAME = Foo,
VERSION_FROM = lib/Foo.pm
);


0 windhund ~/tmp/Foo$ cat lib/Foo.pm 
package Foo;

use version;  our $VERSION = qv('2.0.0');


0 windhund ~/tmp/Foo$ perl Makefile.PL 
Writing Makefile for Foo
0 windhund ~/tmp/Foo$ mod_version_check ExtUtils::MakeMaker
6.31


So what's the warning you're seeing?