code review, added comments

I went through every line of the buildsystem implementation,
and added numerous comments. Search for "XXX JEH" to find them.

5 files changed, 37 insertions, 0 deletions

diff --git a/Debian/Debhelper/Buildsystem/autotools.pm b/Debian/Debhelper/Buildsystem/autotools.pm
index 65694b36..945ca408 100644
--- a/Debian/Debhelper/Buildsystem/autotools.pm
+++ b/Debian/Debhelper/Buildsystem/autotools.pm
@@ -37,6 +37,16 @@ sub configure_impl {
 	push @opts, "--infodir=\${prefix}/share/info";
 	push @opts, "--sysconfdir=/etc";
 	push @opts, "--localstatedir=/var";
+	# XXX JEH this is where the sheer evil of Dh_Buildsystem_Chdir
+	# becomes evident. Why is exec_in_topdir needed here?
+	# Because:
+	# - The parent class happens to be derived from Dh_Buildsystem_Chdir.
+	# - sourcepage() happens to, like many other parts of debhelper's
+	#   library, assume it's being run in the top of the source tree,
+	#   and fails if it's not.
+	# Having to worry about interactions like that for every line of
+	# every derived method is simply not acceptable.
+	# Dh_Buildsystem_Chdir must die! -- JEH
 	push @opts, "--libexecdir=\${prefix}/lib/" . $self->exec_in_topdir(\&sourcepackage);
 	push @opts, "--disable-maintainer-mode";
 	push @opts, "--disable-dependency-tracking";
@@ -48,6 +58,10 @@ sub configure_impl {
 		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..
 	doit($self->get_toppath("configure"), @opts, @_);
 }
 
diff --git a/Debian/Debhelper/Buildsystem/cmake.pm b/Debian/Debhelper/Buildsystem/cmake.pm
index d7504d18..00f6be4d 100644
--- a/Debian/Debhelper/Buildsystem/cmake.pm
+++ b/Debian/Debhelper/Buildsystem/cmake.pm
@@ -42,7 +42,12 @@ sub configure_impl {
 	$self->_add_cmake_flag("CMAKE_SKIP_RPATH", "ON");
 	$self->_add_cmake_flag("CMAKE_VERBOSE_MAKEFILE", "ON");
 	# TODO: LDFLAGS
+	# XXX JEH why are we using a method and an object
+	# field to build up a simple one-time-use list?
+	# 	my @flags;
+	# 	push @flags, ... if $foo
 
+	# XXX JEH again a non-sequitor get_topdir. 
 	doit("cmake", $self->get_topdir(), @{$self->{cmake_flags}}, @_);
 }
 
diff --git a/Debian/Debhelper/Buildsystem/makefile.pm b/Debian/Debhelper/Buildsystem/makefile.pm
index 91a6341c..cbd9e3c3 100644
--- 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";
diff --git a/Debian/Debhelper/Buildsystem/perl_build.pm b/Debian/Debhelper/Buildsystem/perl_build.pm
index 74106d9b..a43d65da 100644
--- a/Debian/Debhelper/Buildsystem/perl_build.pm
+++ b/Debian/Debhelper/Buildsystem/perl_build.pm
@@ -39,6 +39,8 @@ sub new {
 
 sub configure_impl {
 	my $self=shift;
+	# XXX JEH I think the below comment is inherited from elsewhere;
+	# doesn't really make sense now.
 	$ENV{PERL_MM_USE_DEFAULT}=1; # Module::Build can also use this.
 	doit("perl", "Build.PL", "installdirs=vendor", @_);
 }
diff --git a/Debian/Debhelper/Buildsystem/python_distutils.pm b/Debian/Debhelper/Buildsystem/python_distutils.pm
index 2a6df37b..2e7eacbb 100644
--- a/Debian/Debhelper/Buildsystem/python_distutils.pm
+++ b/Debian/Debhelper/Buildsystem/python_distutils.pm
@@ -28,6 +28,8 @@ sub get_builddir_option {
 	return;
 }
 
+# XXX JEH the default for all these methods is to do nothing successfully.
+# So either this, or those default stubs, need to be removed.
 sub configure_impl {
 	# Do nothing
 	1;
@@ -38,6 +40,7 @@ sub build_impl {
 	doit("python", "setup.py", "build", @_);
 }
 
+# XXX JEH see anove comment
 sub test_impl {
 	1;
 }