code review, added comments

[debhelper.git] / Debian / Debhelper / Buildsystem / makefile.pm

diff --git a/Debian/Debhelper/Buildsystem/makefile.pm b/Debian/Debhelper/Buildsystem/makefile.pm
index 91a6341cfc6b35e3eab4eed912837862eba3ca86..cbd9e3c38b213465a6f421b2160f61f3364c3840 100644 (file)
--- a/Debian/Debhelper/Buildsystem/makefile.pm
+++ b/Debian/Debhelper/Buildsystem/makefile.pm
@@ -11,6 +11,7 @@ use Debian::Debhelper::Dh_Lib;
 use Debian::Debhelper::Dh_Buildsystem_Bases;
 use base 'Debian::Debhelper::Dh_Buildsystem_Chdir';
 
+# XXX JEH I *like* this. Yay for factoring out ugly ugly stuff!
 sub _exists_make_target {
        my ($cls, $target) = @_;
        # Use make -n to check to see if the target would do
@@ -24,6 +25,9 @@ sub _make_first_existing_target {
        my $cls = shift;
        my $targets = shift;
 
+       # XXX JEH setting this env var is dodgy,
+       # probably better to test if it exists with a default value.
+       # (Factor out to helper function?)
        $ENV{MAKE}="make" unless exists $ENV{MAKE};
        foreach my $target (@$targets) {
                if ($cls->_exists_make_target($target)) {
@@ -42,10 +46,14 @@ sub is_buildable {
        my $self=shift;
        my ($action) = @_;
        if (grep /^\Q$action\E$/, qw{build test install clean}) {
+               # XXX JEH why does get_buildpath need to be used 
+               # here? is_buildable is run at the top of the source
+               # directory, so -e 'Makefile' should be the same
                return -e $self->get_buildpath("Makefile") ||
                       -e $self->get_buildpath("makefile") ||
                       -e $self->get_buildpath("GNUmakefile");
        } else {
+               # XXX JEH why return 1 here?
                return 1;
        }
 }
@@ -64,10 +72,15 @@ sub install_impl {
        my $self=shift;
        my $destdir=shift;
 
+       # XXX JEH again with the setting the env var, see above..
        $ENV{MAKE}="make" unless exists $ENV{MAKE};
        my @params="DESTDIR=$destdir";
 
        # Special case for MakeMaker generated Makefiles.
+       # XXX JEH This is a really unfortunate breaking of the
+       # encapsulation of the perl_makefile module. Perhaps it would be
+       # better for that module to contain some hack that injects that
+       # test into this one?
        if (-e "Makefile" &&
            system('grep -q "generated automatically by MakeMaker" Makefile') == 0) {
                push @params, "PREFIX=/usr";