module-build-general — General discussion about Module::Build project

Re: [Module-build-general] RFC: Build test test_dirs=(list)

[Ken W. <ke...@ma...>]

On Tuesday, August 12, 2003, at 07:02  PM, Michael G Schwern wrote:
> What might be a lot simpler is semantics like this:
>
>     # run only foo.t
>     Build test test_files=t/foo.t
>
>     # run all .t files in t/
>     Build test test_files=t/*.t
>
> so rather than having "test_files" and "matching" as seperate beasts, 
> just
> run the test_files arguments through glob().

Okay, here's what I'll do.  I like running 'test_files' through glob(), 
for several reasons.  First, it means the default changes from a bunch 
of code that finds the files, to the string "t/*.t".  Second, I like 
the ease of specifying sets of stuff this way, and I think it scales 
well.

I think the semantics should work like this:

  * Run through split_like_shell().

  * Expand with glob().

  * Pass through files unaltered, and expand directories using the likes 
of glob("$_/*.t").

This will match people's expectations from the shell a little more 
closely, I think.

Here's how I'll apply the first patch.  The 'recursive_test_files' 
property remains unimplemented and I'm not sure about the name yet.  
Steve's 'matching' argument can still be implemented later if we want 
regexes.

  -Ken

--- lib/Module/Build/Base.pm    12 Aug 2003 15:52:39 -0000      1.165
+++ lib/Module/Build/Base.pm    13 Aug 2003 15:24:25 -0000
@@ -1012,13 +1012,21 @@

    my @tests;
    if ($self->{args}{test_files}) {  # XXX use 'properties'
-    @tests = $self->split_like_shell($self->{args}{test_files});
+    @tests = (map { -d $_ ? $self->expand_test_dir($_) : $_ }
+             map glob,
+             $self->split_like_shell($self->{args}{test_files}));
    } else {
      # Find all possible tests in t/ or test.pl
      push @tests, 'test.pl'                          if -e 'test.pl';
-    push @tests, @{$self->rscan_dir('t', qr{\.t$})} if -e 't' and -d _;
+    push @tests, $self->expand_test_dir('t')        if -e 't' and -d _;
    }
    return [sort @tests];
+}
+
+sub expand_test_dir {
+  my ($self, $dir) = @_;
+  return @{$self->rscan_dir($dir, qr{\.t$})} if 
$self->{properties}{recursive_test_files};
+  return glob File::Spec->catfile($dir, "*.t");
  }

  sub ACTION_testdb {

Re: [Module-build-general] RFC: Build test test_dirs=(list)

[Steve P. <sp...@qu...>]

On Wednesday, August 13, 2003, at 01:02  am, Michael G Schwern wrote:

> On Tue, Aug 12, 2003 at 11:51:16PM +0100, Steve Purkis wrote:
>>> If I want to test only the methods in the parent class Foo::Bar::One,
>>> I run "Build test test_dirs=t/Foo/Bar/One/t".   If I want to run
>>> the tests for that class and all it's children, I run
>>> "Build test test_dirs=t/Foo/Bar/One".  If I'm fixing a bug in
>>> Foo::Bar::One::B,
>>> I run "Build test test_dirs=t/Foo/Bar/One/B/t" over and over, and 
>>> then
>>> when I'm
>>> satisfied I run a full regression test with "Build test".
>>
>> That works fine if you organize tests by directory, but a lot of
>> authors like to group their tests by function in the /t directory -
>
> This may be a habit formed from MakeMaker's limitation on directory
> depth.

Maybe, but it's also something that's not going to go away anytime 
soon.  And I'm not convinced it's a bad habit (especially not for small 
projects).


>> Using regexps on the path to the test script would let you specify a
>> subset of tests to run in all of these scenarios:
>>
>> 	./Build test matching=t/Foo/Bar/One
>> 	./Build test matching=runthrough
>> 	./Build test matching=t/0*
>>
>> Which could be handy.
>
> I presume you mean globs, not regexes?

Nope, I meant regexes, that's just a typo in my example:

	./Build test matching=t/0.*

Here's some better examples:

	# run all tests found that have 'foo' in their names:
	./Build test matching=Foo

	# for David's example, same as this (recursive) glob:
	./Build test test_files=t/Foo/

	# ... but might also include:
	./Build test test_files=t/Bar/Foo/

Us numbered-test freaks could run sets of tests like this:

	# run tests 50-99:
	./Build test matching=[5-9]\\d

And 'matching' can co-exist with globs, which are also a good idea:

	# run all tests in t/Foo ending in "oo.t"
	./Build test test_files=t/Foo/* matching=oo\\.t

I've attached a patch based on your glob one that implements this.

-Steve

Re: [Module-build-general] RFC: Build test test_dirs=(list)

[Michael G S. <sc...@po...>]

On Tue, Aug 12, 2003 at 11:51:16PM +0100, Steve Purkis wrote:
> >If I want to test only the methods in the parent class Foo::Bar::One,
> >I run "Build test test_dirs=t/Foo/Bar/One/t".   If I want to run
> >the tests for that class and all it's children, I run
> >"Build test test_dirs=t/Foo/Bar/One".  If I'm fixing a bug in 
> >Foo::Bar::One::B,
> >I run "Build test test_dirs=t/Foo/Bar/One/B/t" over and over, and then 
> >when I'm
> >satisfied I run a full regression test with "Build test".
> 
> That works fine if you organize tests by directory, but a lot of 
> authors like to group their tests by function in the /t directory - 

This may be a habit formed from MakeMaker's limitation on directory
depth.


> Using regexps on the path to the test script would let you specify a 
> subset of tests to run in all of these scenarios:
> 
> 	./Build test matching=t/Foo/Bar/One
> 	./Build test matching=runthrough
> 	./Build test matching=t/0*
> 
> Which could be handy.

I presume you mean globs, not regexes?

What might be a lot simpler is semantics like this:

    # run only foo.t
    Build test test_files=t/foo.t

    # run all .t files in t/
    Build test test_files=t/*.t

so rather than having "test_files" and "matching" as seperate beasts, just
run the test_files arguments through glob().

Additionally, one can add this shorthand.

    # equivalent to t/*.t
    build test test_files=t/

This is handy because if/when we get the "look for test files recursively"
option, the above would go recursive.  Otherwise there's no easy
way to say "test everything in this directory and what's below it".

This patch implements both.

--- lib/Module/Build/Base.pm	2003/08/12 23:53:00	1.2
+++ lib/Module/Build/Base.pm	2003/08/12 23:58:27
@@ -875,13 +875,27 @@
   
   my @tests;
   if ($self->{args}{test_files}) {  # XXX use 'properties'
-    @tests = $self->split_like_shell($self->{args}{test_files});
+    @tests = map { glob $_ }
+             map { -d $_ ? $self->expand_test_dir($_) : $_ }
+                 $self->split_like_shell($self->{args}{test_files});
   } else {
     # Find all possible tests in t/ or test.pl
     push @tests, 'test.pl'                          if -e 'test.pl';
     push @tests, @{$self->rscan_dir('t', qr{\.t$})} if -e 't' and -d _;
   }
   return [sort @tests];
+}
+
+sub expand_test_dir {
+    my($self, $dir) = @_;
+
+    opendir(DIR, $dir) or die "Can't open $dir: $!";
+    my @tests = map  { File::Spec->catfile($dir, $_) }
+                grep { /\.t$/ } 
+                     readdir DIR;
+    closedir DIR;
+
+    return @tests;
 }
 
 sub ACTION_testdb {


-- 
Michael G Schwern        sc...@po...  http://www.pobox.com/~schwern/
Do not try comedy at home!  Milk & Cheese are advanced experts!  Attempts at
comedy can be dangerously unfunny!

Re: [Module-build-general] Re: [PATCH] Better Makefile.PLs. Property methods.

[Ken W. <ke...@ma...>]

On Tuesday, August 12, 2003, at 03:48  PM, Steve Purkis wrote:
>
> Is it worth me looking into replacing instances of 
> $self->{property}{foo} w/ $self->foo, or would you prefer to keep 
> using direct variable access?

I think for at least the time being it's fine using direct access.

  -Ken

Re: [Module-build-general] RFC: Build test test_dirs=(list)

[Steve P. <sp...@qu...>]

On Tuesday, August 12, 2003, at 10:35  pm, David Carmean wrote:

> If I want to test only the methods in the parent class Foo::Bar::One,
> I run "Build test test_dirs=t/Foo/Bar/One/t".   If I want to run
> the tests for that class and all it's children, I run
> "Build test test_dirs=t/Foo/Bar/One".  If I'm fixing a bug in 
> Foo::Bar::One::B,
> I run "Build test test_dirs=t/Foo/Bar/One/B/t" over and over, and then 
> when I'm
> satisfied I run a full regression test with "Build test".

That works fine if you organize tests by directory, but a lot of 
authors like to group their tests by function in the /t directory - 
M::B's suite is a good example of that:

	t/basic.t
	t/compat.t
	t/runthrough.t
	...

And I know a lot of people (myself included) who also like to number 
them according to priority.  So for example, you might see something 
like:

	t/01..basic.t
	t/02..framework.t
	t/03..foo-bar.t
	...

Using regexps on the path to the test script would let you specify a 
subset of tests to run in all of these scenarios:

	./Build test matching=t/Foo/Bar/One
	./Build test matching=runthrough
	./Build test matching=t/0*

Which could be handy.

-Steve

Re: [Module-build-general] RFC: Build test test_dirs=(list)

[Michael G S. <sc...@po...>]

On Tue, Aug 12, 2003 at 11:25:14AM -0700, David Carmean wrote:
> It would just make vim recognize them as Perl files by default :o).   
> Speaking of magic, where did the .t convention come from anway?

Most of the Deep Testing Conventions date back to perl1.


-- 
Michael G Schwern        sc...@po...  http://www.pobox.com/~schwern/
It's Ecstacy time!

[Module-build-general] Re: [PATCH] Better Makefile.PLs. Property methods.

[Ken W. <ke...@ma...>]

On Tuesday, August 12, 2003, at 04:54  PM, Michael G Schwern wrote:

> On Tue, Aug 12, 2003 at 10:58:47AM -0500, Ken Williams wrote:
>>
>> I don't actually understand the precise semantics of NAME & DISTNAME 
>> in
>> MakeMaker - the docs say that DISTNAME defaults to NAME, but it's
>> actually adapted with at least s/::/-/ translation.  It also says that
>> NAME will default to "the directory name", but doesn't say which
>> directory.
>
> Neither do I, which means its best not to tempt fate.
>
> How about use NAME if module_name exists and DISTNAME if it doesn't?

Sounds good to me - applied.

  -Ken

Re: [Module-build-general] RFC: Build test test_dirs=(list)

[Steve P. <sp...@qu...>]

On Tuesday, August 12, 2003, at 08:31  pm, Ken Williams wrote:

> On Tuesday, August 12, 2003, at 01:25  PM, David Carmean wrote:
>>
>> I was suggesting a new option, not a change to the default behavior: 
>> An
>> argument named "test_dirs".  But unless I've missed a patch since 
>> 0.19 was
>> posted to CPAN, M::B::Base::test_dirs already *does* recursively find 
>> ".t" files
>> in t/ .
>
> Ooh, you're right!  I remember that setting off a flag in my head when 
> I wrote it a long time ago, but then forgot about it.  It shouldn't 
> scan recursively, for the reason Schwern mentions - as well as for 
> compatibility reasons with MakeMaker layouts.
>
> In some cases it might be handy to scan recursively though.  We'll 
> have to think about a set of options that gives people flexibility in 
> a few different ways.

A flag passed to new() would be a good idea, methinks.  Perhaps 
'test_recursive', 'rscan_tests' or 'scan_test_subdirs'?

Speaking of flexibility, a common request I've heard is to be able to 
run a subset of tests.  So what about letting the user run test scripts 
that match a regex:

	./Build tests "/.*foo.*/"
	./Build tests matching=".*foo.*"

or maybe shell-style wild cards:

	./Build tests "*foo*"

-Steve

Re: [Module-build-general] RFC: Build test test_dirs=(list)

[David C. <dl...@ha...>]

On Tue, Aug 12, 2003 at 10:03:03PM +0100, Steve Purkis wrote:

> A flag passed to new() would be a good idea, methinks.  Perhaps 
> 'test_recursive', 'rscan_tests' or 'scan_test_subdirs'?
> 
> Speaking of flexibility, a common request I've heard is to be able to 
> run a subset of tests.  So what about letting the user run test scripts 
> that match a regex:
> 
> 	./Build tests "/.*foo.*/"
> 	./Build tests matching=".*foo.*"

Easily running subsets of tests is exactly what prompted me to start this 
discussion.  With my test_dirs hack to my subclassed builders, I accomplish 
this by making stubs off of a directory tree.  For example, if I have the following 
class heirarchy in my package:

    Foo::Bar
    Foo::Bar::One
    Foo::Bar::One::A
    Foo::Bar::One::B

Then tests for Foo::Bar::One::A live in t/Foo/Bar/One/A/t, 
tests for private methods of Foo::Bar::One live in t/Foo/Bar/One/t, 
and high-level tests for the entire distribution live in t/Foo/Bar.

If I want to test only the methods in the parent class Foo::Bar::One, 
I run "Build test test_dirs=t/Foo/Bar/One/t".   If I want to run 
the tests for that class and all it's children, I run 
"Build test test_dirs=t/Foo/Bar/One".  If I'm fixing a bug in Foo::Bar::One::B, 
I run "Build test test_dirs=t/Foo/Bar/One/B/t" over and over, and then when I'm 
satisfied I run a full regression test with "Build test".

Re: [Module-build-general] RFC: Build test test_dirs=(list)

[Ken W. <ke...@ma...>]

On Tuesday, August 12, 2003, at 01:25  PM, David Carmean wrote:
>
> I was suggesting a new option, not a change to the default behavior: An
> argument named "test_dirs".  But unless I've missed a patch since 0.19 
> was
> posted to CPAN, M::B::Base::test_dirs already *does* recursively find 
> ".t" files
> in t/ .

Ooh, you're right!  I remember that setting off a flag in my head when 
I wrote it a long time ago, but then forgot about it.  It shouldn't 
scan recursively, for the reason Schwern mentions - as well as for 
compatibility reasons with MakeMaker layouts.

In some cases it might be handy to scan recursively though.  We'll have 
to think about a set of options that gives people flexibility in a few 
different ways.

> It would just make vim recognize them as Perl files by default :o) .   
> Speaking of magic, where did the .t convention come from anway?

It probably first appeared in the core, for the regression test scripts.

I agree with Schwern that the amount of magic in this area of M::B (and 
in most other areas) needs to stay very limited.  This can already be 
overridden by the user/subclasser anyway.  Plus, Test::Harness output 
doesn't/didn't look nice with .pl files.  PLUS, lots of people use .pl 
files to hold functions they share among their various test scripts.

  -Ken

Re: [Module-build-general] RFC: Build test test_dirs=(list)

[David C. <dl...@ha...>]

On Mon, Aug 11, 2003 at 06:29:26PM -0700, Michael G Schwern wrote:
> On Mon, Aug 11, 2003 at 03:18:10PM -0700, David Carmean wrote:
> > Just today I've created a builder subclass and have overridden the
> > test_files() method to look for $self->{args}{test_dirs} and add all the
> > *.t files found in or beneath those directories;  IMO much more convenient
> > than typing a long list for $self->{args}{test_files}.  So for instance
> > when I'm creating the Foo::Bar::Baz::Feh module in package Foo::Bar,
> > all the tests for Feh live in t/Foo/Bar/Baz/Feh, and I run "Build test
> > test_dirs=t/Foo/Bar/Baz/Feh".
> 
> The trouble with doing this by default is it makes this sort of testing
> very difficult:
> 
>     t/foo.t
>     t/Dummy-Module/lib/Dummy/Module.pm
>     t/Dummy-Module/t/Foo.t
> 
> where foo.t needs a little module to test with.  If you go recursive in
> scanning for .t files you'll pick up Foo.t.  There are workarounds but
> they're similar to yours above so you're just shifting the problem.
> 
> What might be better is a simple flag you can pass to Module::Build->new()
> which says "check recursively for tests".  Whether or not this should be
> the default is a matter to be settled by a steel cage match.


I was suggesting a new option, not a change to the default behavior: An 
argument named "test_dirs".  But unless I've missed a patch since 0.19 was 
posted to CPAN, M::B::Base::test_dirs already *does* recursively find ".t" files 
in t/ .

> > I'm trying to think of a reason not to have it add *.pl as well....?
> 
> The more things you look for as tests the harder it will be to have testing 
> data in t/.  Its desirable to keep the number of magical files to a minimum 
> else you magic it difficult to shake off that magic.
> 
> Since there's no compelling use-case for making *.pl magical other than "Look, 
> we can name it something else!" I'd say no.

It would just make vim recognize them as Perl files by default :o) .   Speaking of 
magic, where did the .t convention come from anway?

[Module-build-general] Re: [PATCH] Better Makefile.PLs. Property methods.

[Ken W. <ke...@ma...>]

On Sunday, August 10, 2003, at 10:49  PM, Michael G Schwern wrote:
> Instead of DISTNAME, the NAME of the module is taken from module_name.
> MakeMaker needs the NAME of the module more than it needs the DISTNAME.
> It can figure out the DISTNAME from the NAME but its pretty bad about
> going the other way around.

This is kind of tricky - the problem is that there may not be a 
'module_name' property, as in the case of LWP or the like.

I don't actually understand the precise semantics of NAME & DISTNAME in 
MakeMaker - the docs say that DISTNAME defaults to NAME, but it's 
actually adapted with at least s/::/-/ translation.  It also says that 
NAME will default to "the directory name", but doesn't say which 
directory.

> That's part one.  Part two stems from when I tried to replace
> $build->dist_name with $build->module_name I got an error, no such 
> method.
> Since it seems rather arbitrary whether or not one accesses a property
> as a method or as a hash element, I threw in a little bit of code to
> automaticly generate simple accessor methods for all the properties 
> which
> don't already have one.  So $build->module_name DWIM now.

Looks good, applied - though it should be testing $self->{properties}, 
not $self:

@@ -250,6 +250,17 @@
        );

    sub valid_property { exists $valid_properties{$_[1]} }
+
+  # Create an accessor for each property that doesn't already have one
+  foreach my $property (keys %valid_properties) {
+      next if __PACKAGE__->can($property);
+      no strict 'refs';
+      *{$property} = sub {
+          my $self = shift;
+          $self->{properties}{$property} = shift if @_;
+          return $self->{properties}{$property};
+      };
+  }
  }


  -Ken

Re: [Module-build-general] [PATCH] Better Makefile.PLs. Property methods.

[Ken W. <ke...@ma...>]

On Monday, August 11, 2003, at 03:35  AM, Michael G Schwern wrote:
> The odd indentation was removed, might look good inside Compat.pm but 
> it
> looks odd in the resulting Makefile.PL and that's what really matters.
>
> The unnecessary quotes around the key names has been removed for purely
> aesthetic reasons.
>
> Hash values are lined up for easier reading.
>
> Added a comment about this being generated by Module::Build::Compat so
> people don't go patching the Makefile.PL and sending it off to the 
> author
> like I was about to do.

I've applied (in a modified form) these parts of the patch.

  -Ken

[Module-build-general] Re: Module::Build & autosplit

[Ken W. <ke...@ma...>]

On Tuesday, August 12, 2003, at 09:39  AM, Martyn J. Pearce wrote:

> Greetings Ken,
>
> In the docs for Module::Build (thanks!), you mention that you are 
> uncertain as
> to the use of the autosplit function, and to let you know if users 
> would like
> you not to remove it.
>
> Well, I'm not sure of the basis of your doubt about it (but if you 
> were to
> explain your qualms, I could possibly apply them to my situation & see 
> if they
> fit); however, I'm just processing changes to apply Module::Build to
> Class::MethodMaker (as one of three ways of building it), and for 
> that, modulo
> any changes you convince me of, autosplit definately is needed.  So 
> please
> don't remove it!
>

Hi Martyn,

Yeah, I suppose I shouldn't remove it - I wrote that part of the docs 
(and code) a long long time ago, right near the beginning of the M::B 
project.

The only reservations I have about autosplit is that I haven't been 
convinced that it's very effectual - it seems like in general modules 
load up fast enough without it, and it can be inefficient in mod_perl 
situations when you want to load as much of the module initially as 
possible.

However, I don't need to impose my opinions on the rest of the world, 
especially since autosplit isn't doing any actual damage.  Consider it 
kept, and I'll change the wording in the docs. =)

  -Ken

Re: [Module-build-general] RFC: Build test test_dirs=(list)

[Michael G S. <sc...@po...>]

On Mon, Aug 11, 2003 at 03:18:10PM -0700, David Carmean wrote:
> Just today I've created a builder subclass and have overridden the
> test_files() method to look for $self->{args}{test_dirs} and add all the
> *.t files found in or beneath those directories;  IMO much more convenient
> than typing a long list for $self->{args}{test_files}.  So for instance
> when I'm creating the Foo::Bar::Baz::Feh module in package Foo::Bar,
> all the tests for Feh live in t/Foo/Bar/Baz/Feh, and I run "Build test
> test_dirs=t/Foo/Bar/Baz/Feh".

The trouble with doing this by default is it makes this sort of testing
very difficult:

    t/foo.t
    t/Dummy-Module/lib/Dummy/Module.pm
    t/Dummy-Module/t/Foo.t

where foo.t needs a little module to test with.  If you go recursive in
scanning for .t files you'll pick up Foo.t.  There are workarounds but
they're similar to yours above so you're just shifting the problem.

What might be better is a simple flag you can pass to Module::Build->new()
which says "check recursively for tests".  Whether or not this should be
the default is a matter to be settled by a steel cage match.


> I'm trying to think of a reason not to have it add *.pl as well....?

The more things you look for as tests the harder it will be to have testing 
data in t/.  Its desirable to keep the number of magical files to a minimum 
else you magic it difficult to shake off that magic.

Since there's no compelling use-case for making *.pl magical other than "Look, 
we can name it something else!" I'd say no.


-- 
Michael G Schwern        sc...@po...  http://www.pobox.com/~schwern/
Stupid am I?  Stupid like a fox!

[Module-build-general] RFC: Build test test_dirs=(list)

[David C. <dl...@ha...>]

When working on a large distribution with many packages/modules and
tests, I often don't want to wait for all tests in the suite to run when
I'm working deep in the heirarchy.  Especially when I'm pretending to
practice Extreme Programming.

Just today I've created a builder subclass and have overridden the
test_files() method to look for $self->{args}{test_dirs} and add all the
*.t files found in or beneath those directories;  IMO much more convenient
than typing a long list for $self->{args}{test_files}.  So for instance
when I'm creating the Foo::Bar::Baz::Feh module in package Foo::Bar,
all the tests for Feh live in t/Foo/Bar/Baz/Feh, and I run "Build test
test_dirs=t/Foo/Bar/Baz/Feh".

I'm trying to think of a reason not to have it add *.pl as well....?

I'd like to see something like this incorporated into M::B.  It's a
trivial patch that I'd be happy to submit.

Comments....?

Re: [Module-build-general] Default MANIFEST.SKIP enhancement?

[Ken W. <ke...@ma...>]

On Monday, August 11, 2003, at 04:12  AM, Michael G Schwern wrote:

> So Module::Build uses ExtUtils::Manifest.  ExtUtils::Manifest provides
> a default MANIFEST.SKIP to avoid the usual junk around at build time.
> blib, Makefiles, etc...  But it doesn't know about Module::Build so
> it doesn't know about Build and _build/.
>
> I've added those two in to the default MANIFEST.SKIP.  Anything else
> needed?

That should do it.  I always add ^Foo-Bar to the list too (in module 
Foo::Bar) to skip the distdir & tarballs, but I guess the default 
MANIFEST.SKIP isn't dynamically built.

>
>
> PS  Ken, let me know if this never makes it to the list.

Finally made it there this morning.

  -Ken

Re: [Module-build-general] [PATCH] Better Makefile.PLs. Property methods.

[Steve P. <sp...@qu...>]

On Monday, August 11, 2003, at 01:21  pm, Michael G Schwern wrote:

> On Mon, Aug 11, 2003 at 11:02:29AM +0100, Steve Purkis wrote:
>> Can I also suggest a slight variation on the accessor methods to 
>> return
>> $self in 'set' context, ie:
>>
>>       *{$property} = sub {
>>           my $self = shift;
>>           if (@_) { $self->{$property} = $_[0]; return $self; }
>>           else    { return $self->{$property}; }
>>       };
>>
>>       # means I can do this:
>>       $self->module_name( 'Foo' )
>>            ->install_base( '/home/foo' );
>
>         $self->module_name( 'Foo' );
>         $self->install_base( '/home/foo' );
>
> It doesn't buy much.

Yeah, I just find it easier to read, especially when you get into the 
habit of doing it by default.  In spurkis-land, I imagine using M::B 
like this:

	my $m = Module::Build->new
		->module_name( 'Foo' )
		->install_base( 'bar' )
		->...etc...
		->create_build_script
		->dispatch( 'config' );

And internally, something like this:

   sub ACTION_code {
     my $self = shift;
     $self->create_blib_arch
          ->autosplit_blib
          ->process_PL_files
          ->compile_support_files
          ->process_pm_files
          ->process_xs_files
          ->process_pod_files
          ->process_script_files
          ->_call_action( 'docs' );
   }

It's great - at a high level it's almost like writing a todo list.  But 
maybe I'm dreaming in agfa-colour ;).


> And it means you can't simultaneously set and get.
>
>     do_this if $self->module_bleh( $foo );

True, but I'd try to avoid that by checking $foo first anyway.  Or 
maybe do:

	$self->module_bleh( $foo ) and do_this if $foo;

But I didn't mean to turn this into a debate about style - I really am 
easy either way.  Having accessor methods is the important thing.

-Steve

Re: [Module-build-general] [PATCH] Better Makefile.PLs. Property methods.

[Michael G S. <sc...@po...>]

On Mon, Aug 11, 2003 at 11:02:29AM +0100, Steve Purkis wrote:
> Can I also suggest a slight variation on the accessor methods to return 
> $self in 'set' context, ie:
> 
>       *{$property} = sub {
>           my $self = shift;
>           if (@_) { $self->{$property} = $_[0]; return $self; }
>           else    { return $self->{$property}; }
>       };
> 
>       # means I can do this:
>       $self->module_name( 'Foo' )
>            ->install_base( '/home/foo' );

        $self->module_name( 'Foo' );
        $self->install_base( '/home/foo' );

It doesn't buy much.

And it means you can't simultaneously set and get.

    do_this if $self->module_bleh( $foo );


-- 
Michael G Schwern        sc...@po...  http://www.pobox.com/~schwern/
Hold on while I slip into something a little more naked.

Re: [Module-build-general] [PATCH] Better Makefile.PLs. Property methods.

[Steve P. <sp...@qu...>]

On Monday, August 11, 2003, at 09:35  am, Michael G Schwern wrote:

> [snip]
> Part two stems from when I tried to replace
> $build->dist_name with $build->module_name I got an error, no such 
> method.
> Since it seems rather arbitrary whether or not one accesses a property
> as a method or as a hash element, I threw in a little bit of code to
> automaticly generate simple accessor methods for all the properties 
> which
> don't already have one.  So $build->module_name DWIM now.

Excellent idea - I've been wondering why there's so much direct access 
of instance variables in a module that's designed to be sub-classed.  
If there's no reason, maybe the next step is to replace all instances 
of $self->{<valid property>} ?

Can I also suggest a slight variation on the accessor methods to return 
$self in 'set' context, ie:

       *{$property} = sub {
           my $self = shift;
           if (@_) { $self->{$property} = $_[0]; return $self; }
           else    { return $self->{$property}; }
       };

       # means I can do this:
       $self->module_name( 'Foo' )
            ->install_base( '/home/foo' );

-Steve

[Module-build-general] [PATCH] Better Makefile.PLs. Property methods.

[Michael G S. <sc...@po...>]

The following patch does two things.  First, the primary purpose, is to
make the Makefile.PLs generated by Module::Build::Compat a bit cleaner.

Instead of DISTNAME, the NAME of the module is taken from module_name.
MakeMaker needs the NAME of the module more than it needs the DISTNAME.
It can figure out the DISTNAME from the NAME but its pretty bad about
going the other way around.

The odd indentation was removed, might look good inside Compat.pm but it
looks odd in the resulting Makefile.PL and that's what really matters.

The unnecessary quotes around the key names has been removed for purely
aesthetic reasons.

Hash values are lined up for easier reading.

Added a comment about this being generated by Module::Build::Compat so
people don't go patching the Makefile.PL and sending it off to the author
like I was about to do.


That's part one.  Part two stems from when I tried to replace
$build->dist_name with $build->module_name I got an error, no such method.
Since it seems rather arbitrary whether or not one accesses a property
as a method or as a hash element, I threw in a little bit of code to
automaticly generate simple accessor methods for all the properties which
don't already have one.  So $build->module_name DWIM now.


-- 
Michael G Schwern        sc...@po...  http://www.pobox.com/~schwern/
I can't give you any knowledge, but who wants candy??
	-- http://www.angryflower.com/drawin.gif

--- ./lib/Module/Build/Base.pm	2003/08/11 03:40:24	1.1
+++ ./lib/Module/Build/Base.pm	2003/08/11 03:41:24
@@ -235,6 +235,17 @@
       );
 
   sub valid_property { exists $valid_properties{$_[1]} }
+
+  # Create an accessor for each property that doesn't already have one
+  no strict 'refs';
+  foreach my $property (keys %valid_properties) {
+      next if __PACKAGE__->can($property);
+      *{$property} = sub {
+          my $self = shift;
+          $self->{$property} = $_[0] if @_;
+          return $self->{$property};
+      };
+  }
 }
 
 # XXX Problem - if Module::Build is loaded from a different directory,
@@ -1298,7 +1309,7 @@
   }
   return $self->{properties}{script_files};
 }
-*scripts = \&script_files;
+BEGIN { *scripts = \&script_files; }
 
 sub valid_licenses {
   return { map {$_, 1} qw(perl gpl artistic lgpl bsd open_source unrestricted restrictive unknown) };
--- ./lib/Module/Build/Compat.pm	2003/08/11 03:30:57	1.1
+++ ./lib/Module/Build/Compat.pm	2003/08/11 03:46:20
@@ -68,16 +68,20 @@
     delete $prereq{perl};
     my $prereq = join '', map "\t\t\t'$_' => '$prereq{$_}',\n", keys %prereq;
 
-    printf {$fh} <<'EOF', $build->dist_name, $build->dist_version, $prereq;
-    use ExtUtils::MakeMaker;
-    WriteMakefile
-      ('DISTNAME' => '%s',
-       'VERSION' => '%s',
-       'PL_FILES' => {},
-       'PREREQ_PM' => {
+    printf {$fh} <<'EOF', $build->module_name, $build->dist_version, $prereq;
+# Generated by Module::Build::Compat->create_makefile_pl
+
+use ExtUtils::MakeMaker;
+
+WriteMakefile
+  (
+   NAME           => '%s',
+   VERSION        => '%s',
+   PL_FILES       => {},
+   PREREQ_PM      => {
 %s
 		      },
-      );
+  );
 EOF
   }
 }

[Module-build-general] Default MANIFEST.SKIP enhancement?

[Michael G S. <sc...@po...>]

So Module::Build uses ExtUtils::Manifest.  ExtUtils::Manifest provides
a default MANIFEST.SKIP to avoid the usual junk around at build time.
blib, Makefiles, etc...  But it doesn't know about Module::Build so
it doesn't know about Build and _build/.

I've added those two in to the default MANIFEST.SKIP.  Anything else
needed?


PS  Ken, let me know if this never makes it to the list.

-- 
Michael G Schwern        sc...@po...  http://www.pobox.com/~schwern/
That which stirs me, stirs everything.
	-- Squonk Opera, "Spoon"

[Module-build-general] Re: [PATCH] make doesn't create man pages

[Steve P. <sp...@qu...>]

On Friday, August 8, 2003, at 05:53  pm, Ken Williams wrote:

>
> On Friday, August 8, 2003, at 10:02  AM, Steve Purkis wrote:
>>
>> Anyways - here's a patch to fix the docs/build dependency bug, and 
>> make M::B create man pages at 'build' time.
>
> Looks good, Steve - below is the patch I'll commit.  It creates a new 
> action 'code', which does what 'build' used to do, and now 'build' 
> creates both code & docs.

Good idea, and its much more obvious what's happening that way.  Wish 
I'd thought of it :)


> Hmm, I guess this doesn't actually need the loop-avoidance patch 
> anymore, does it!  Well, I guess it's still good to have that.

Yeah, but hopefully to be used sparingly..

-Steve

[Module-build-general] Re: [PATCH] make doesn't create man pages

[Ken W. <ke...@ma...>]

On Friday, August 8, 2003, at 10:02  AM, Steve Purkis wrote:
>
> Anyways - here's a patch to fix the docs/build dependency bug, and 
> make M::B create man pages at 'build' time.

Looks good, Steve - below is the patch I'll commit.  It creates a new 
action 'code', which does what 'build' used to do, and now 'build' 
creates both code & docs.

Hmm, I guess this doesn't actually need the loop-avoidance patch 
anymore, does it!  Well, I guess it's still good to have that.

  -Ken

Index: lib/Module/Build/Base.pm
===================================================================
RCS file: /cvsroot/module-build/Module-Build/lib/Module/Build/Base.pm,v
retrieving revision 1.160
diff -u -r1.160 Base.pm
--- lib/Module/Build/Base.pm	5 Aug 2003 17:28:35 -0000	1.160
+++ lib/Module/Build/Base.pm	8 Aug 2003 16:50:58 -0000
@@ -913,7 +913,7 @@
    my $p = $self->{properties};
    require Test::Harness;

-  $self->depends_on('build');
+  $self->depends_on('code');

    # Do everything in our power to work with all versions of 
Test::Harness
    local ($Test::Harness::switches,
@@ -967,7 +967,7 @@
    $self->depends_on('test');
  }

-sub ACTION_build {
+sub ACTION_code {
    my ($self) = @_;

    # All installable stuff gets created in blib/ .
@@ -990,6 +990,12 @@
    $self->process_script_files;
  }

+sub ACTION_build {
+  my $self = shift;
+  $self->depends_on('code');
+  $self->depends_on('docs');
+}
+
  sub compile_support_files {
    my $self = shift;
    my $p = $self->{properties};
@@ -1167,6 +1173,7 @@

  sub ACTION_docs {
    my $self = shift;
+  $self->depends_on('code');
    require Pod::Man;
    $self->manify_bin_pods() if $self->install_destination('bindoc');
    $self->manify_lib_pods() if $self->install_destination('libdoc');
@@ -1307,14 +1314,14 @@
  sub ACTION_install {
    my ($self) = @_;
    require ExtUtils::Install;
-  $self->depends_on('build', 'docs');
+  $self->depends_on('build');
    ExtUtils::Install::install($self->install_map('blib'), 1, 0, 
$self->{args}{uninst}||0);
  }

  sub ACTION_fakeinstall {
    my ($self) = @_;
    require ExtUtils::Install;
-  $self->depends_on('build', 'docs');
+  $self->depends_on('build');
    ExtUtils::Install::install($self->install_map('blib'), 1, 1, 
$self->{args}{uninst}||0);
  }

@@ -1324,7 +1331,7 @@
    die "You must have only.pm 0.25 or greater installed for this 
operation: $@\n"
      unless eval { require only; 'only'->VERSION(0.25); 1 };

-  $self->depends_on('build', 'docs');
+  $self->depends_on('build');

    my %onlyargs = map {exists($self->{args}{$_}) ? ($_ => 
$self->{args}{$_}) : ()}
      qw(version versionlib);

Re: [Module-build-general] Couple of code changes

[Dave R. <au...@ur...>]

On Fri, 8 Aug 2003, Steve Purkis wrote:

> On Wednesday, August 6, 2003, at 07:25  pm, Ken Williams wrote:
>
> > ...
> >  1) Actions now run only once per dispatch() invocation.  Dave, maybe
> > this means the 'distmeta' interaction with the 'distdir' stuff could
> > be cleaned up some?  I suppose there's no real reason to fix things
> > that aren't broken, though.
>
> Au contraire - consistency is one good reason to refactor.

As long as there are tests, it doesn't hurt to change it.


-dave

/*=======================
House Absolute Consulting
www.houseabsolute.com
=======================*/