On Mon, 10 Aug 2009 07:50 +0200, "Thomas Klausner" <d...@cpan.org>
wrote:
> Hi!
> 
> On Sat, Aug 08, 2009 at 09:07:55AM -0600, Curtis Jewell wrote:
> > This should be close to executable, (I pulled it out of another project
> > of mine) and once cleaned up, should do the trick:
> 
> Would it be ok for you if I tried to integrate your code into 
> Module::Build alongside Davids suggestions? 

Sure. Not a problem.

One thing I oopsed on:

This:
        if (0 == scalar (%files)) {
                croak 'Empty or no MANIFEST file';
        }

should be this:

        if (0 == scalar (%{$files})) {
                croak 'Empty or no MANIFEST file';
        }

> One more short question:
>  
> >   FILE:
> >     foreach my $direntry ( readdir $dir_handle ) {
> 
> Why are you not using File::Find or similar?

Probably because I would have:

1) Had to go back to 5.00503's File::Find and seen if it did what I
wanted it to. [This code was supposed to be runnable on 5.00503 without
any additional modules (other than Module::Build and its requirements)
being installed. Of course, the last version of Module::Build to be
installable on that low a version of perl was 0.2808_04 - but I was
willing to support doing that. Since newer versions are requiring 5.6.1,
you can delete the gensym call and the code around it. ]
2) Had to decide to not use it, since I couldn't pass parameters
(specifically, the $files hash and the destination directory) into the
subroutine it calls on each file. I would need File::Find::find to
return a list instead, and then loop over the list to check it against
the manifest. Easier to just readdir the directories and process file
files myself [I want to keep use of global or package-level 'my'
variables to a minimum. I'm already using too many for my comfort.]
Maybe I could have put the wanted subroutine within the install_share
subroutine, so it could have used its 'my' variables... but that looks
even uglier than just doing a recursive foreach loop against the
directory contents.
3) Also, keeping track of the "3 locations" each file has to go would be
even harder (and consume more CPU time, because I would have had to make
some File::Spec calls per-file, rather than per-directory) than it is
now. [The "3 locations" being referred to are the source location in
local format, the source location in Unix format (in order to compare to
the MANIFEST file,) and the destination (in blib) location in local
format.]

Also, for your information, as you do the integration:

1) The global variable %ARGS is supposed to be the Module::Build options
hash - M::B::Functions calls Module::Build->new (or
Module::Build::Subclass->new) at the end of processing all its routines
with that as a parameter. Of course, since the goal is to integrate this
into Module::Build now, you can just call its routines instead of
setting (and accessing) keys in a multi-level argument hash.

2: The package-level 'my' variable @install_types is the list of install
types I need to add to the Module::Build object after I create it, like
so:

        foreach my $type (@install_types) {
                $object->add_build_element($type);
        }

Since again, we're working with an already created Module::Build object,
we can just call add_build_element() on it immediately, instead of
having to store a list and waiting to do it at the end. 

3: You'll see calls to _installdir() twice in there. I should have shown
what that did to you, as well.

sub _installdir {
        return $Config{'sitelibexp'} unless ( defined $ARGS{install_type} );
        return $Config{'sitelibexp'}   if ( 'site'   eq $ARGS{install_type} );
        return $Config{'privlibexp'}   if ( 'perl'   eq $ARGS{install_type} );
        return $Config{'vendorlibexp'} if ( 'vendor' eq $ARGS{install_type} );
        croak 'Invalid install type';
}

--Curtis
--
Curtis Jewell
swords...@csjewell.fastmail.us

%DCL-E-MEM-BAD, bad memory
-VMS-F-PDGERS, pudding between the ears

[I use PC-Alpine, which deliberately does not display colors and pictures in 
HTML mail]

Reply via email to