4 files changed, 15 insertions, 22 deletions

diff --git a/Debian/Debhelper/Buildsystem/autotools.pm b/Debian/Debhelper/Buildsystem/autotools.pm
index 1ebc9384..08851677 100644
--- a/Debian/Debhelper/Buildsystem/autotools.pm
+++ b/Debian/Debhelper/Buildsystem/autotools.pm
@@ -49,11 +49,6 @@ sub configure {
 		push @opts, "--host=" . dpkg_architecture_value("DEB_HOST_GNU_TYPE");
 	}
 
-	# XXX JEH the reason it needs to use get_toppath here,
-	# but does not need to in the is_buildable method is not clear,
-	# unless one is familiar with the implementation of its parent
-	# class. I think that speaks to a bad design..
-	# XXX MDX It should be more explicit now.
 	$self->mkdir_builddir();
 	$self->doit_in_builddir($self->get_rel2builddir_path("configure"), @opts, @_);
 }
diff --git a/Debian/Debhelper/Buildsystem/cmake.pm b/Debian/Debhelper/Buildsystem/cmake.pm
index 026004a0..ff248249 100644
--- a/Debian/Debhelper/Buildsystem/cmake.pm
+++ b/Debian/Debhelper/Buildsystem/cmake.pm
@@ -42,9 +42,6 @@ sub configure {
 	push @flags, "-DCMAKE_SKIP_RPATH=ON";
 	push @flags, "-DCMAKE_VERBOSE_MAKEFILE=ON";
 
-	# XXX JEH again a non-sequitor get_topdir.
-	# XXX MDX I cannot avoid it as I need to pass the path to the sourcedir
-	# to cmake which is relative to the builddir.
 	$self->mkdir_builddir();
 	$self->doit_in_builddir("cmake", $self->get_rel2builddir_path(), @flags);
 }
diff --git a/Debian/Debhelper/Buildsystem/makefile.pm b/Debian/Debhelper/Buildsystem/makefile.pm
index 7ffb048e..36081ecc 100644
--- a/Debian/Debhelper/Buildsystem/makefile.pm
+++ b/Debian/Debhelper/Buildsystem/makefile.pm
@@ -10,11 +10,6 @@ use strict;
 use Debian::Debhelper::Dh_Lib;
 use base 'Debian::Debhelper::Dh_Buildsystem_Basic';
 
-# XXX JEH setting this env var is dodgy,
-# probably better to test if it exists with a default value.
-# (Factor out to helper function?)
-# XXX MDX Done. See new().
-
 sub get_makecmd_C {
 	my $self=shift;
 	if ($self->get_builddir()) {
@@ -25,6 +20,12 @@ sub get_makecmd_C {
 
 # XXX JEH I *like* this. Yay for factoring out ugly ugly stuff!
 # XXX MDX TODO: this could use dh debian/rules parser.
+# XXX JEH That one checks for explicit only targets, while we want
+#         implicit targets here too. I think the current code is ok;
+#         it's a bonus that it checks if the target it empty.
+#         Hmm, one problem is that if a target exists but will run no
+#         commands since it's already built, the approach below will return
+#         nothing and assume it doesn't exist.
 sub exists_make_target {
 	my ($self, $target) = @_;
 	my $makecmd=$self->get_makecmd_C();
diff --git a/Debian/Debhelper/Buildsystem/perl_makemaker.pm b/Debian/Debhelper/Buildsystem/perl_makemaker.pm
index 15539588..91a0da3d 100644
--- a/Debian/Debhelper/Buildsystem/perl_makemaker.pm
+++ b/Debian/Debhelper/Buildsystem/perl_makemaker.pm
@@ -21,11 +21,16 @@ sub is_auto_buildable {
 	if ($action eq "install") {
 		# This hack is needed to keep full 100% compatibility with previous
 		# debhelper versions.
+		# XXX JEH perl_makemaker comes before makefile, so
+		# couldn't it instead just test for Makefile.PL?
 		if (-e "Makefile" &&
 		    system('grep -q "generated automatically by MakeMaker" Makefile') == 0) {
 			return 1;
 		}
 	}
+	# XXX JEH why test for configure here? If building or cleaning, and
+	# a Makefile.PL exists, we know this class can handle those
+	# actions -- it does so by inheriting from the makefile class.
 	elsif ($action eq "configure") {
 		return -e "Makefile.PL";
 	}
@@ -52,18 +57,13 @@ sub configure {
 sub install {
 	my $self=shift;
 	my $destdir=shift;
-	# 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?
-	# XXX MDX Solved. perl_makemaker will need come before makefile in
-	# @BUILDSYSTEMS. See also hack in is_auto_buildable().
-	# This is a safety check needed to keep 100% compatibility with
-	# earlier debhelper behaviour. This if is very unlikely to be false.
+	# XXX JEH this test seems redundant with the one in
+	# is_auto_buildable, if we get here we know that one succeeded.
 	if (-e "Makefile" &&
 	    system('grep -q "generated automatically by MakeMaker" Makefile') == 0) {
 		$self->SUPER::install($destdir, "PREFIX=/usr", @_);
-	} else {
+	}
+	else {
 		$self->SUPER::install($destdir, @_);
 	}
 }