summaryrefslogtreecommitdiff
path: root/Debian
diff options
context:
space:
mode:
authorJoey Hess <joey@gnu.kitenet.net>2009-04-10 17:05:23 -0400
committerJoey Hess <joey@gnu.kitenet.net>2009-04-10 17:05:23 -0400
commit2b7f42f9ef70c08bb7bc138fb8b24dc993da54ac (patch)
treeb215c75636fc08ed9e2571effdbd8854e441fcd4 /Debian
parent776abbb23a899ebba954774c8b3740706a21f72b (diff)
code review, added comments
I went through every line of the buildsystem implementation, and added numerous comments. Search for "XXX JEH" to find them.
Diffstat (limited to 'Debian')
-rw-r--r--Debian/Debhelper/Buildsystem/autotools.pm14
-rw-r--r--Debian/Debhelper/Buildsystem/cmake.pm5
-rw-r--r--Debian/Debhelper/Buildsystem/makefile.pm13
-rw-r--r--Debian/Debhelper/Buildsystem/perl_build.pm2
-rw-r--r--Debian/Debhelper/Buildsystem/python_distutils.pm3
-rw-r--r--Debian/Debhelper/Dh_Buildsystem_Bases.pm44
-rw-r--r--Debian/Debhelper/Dh_Buildsystems.pm15
7 files changed, 96 insertions, 0 deletions
diff --git a/Debian/Debhelper/Buildsystem/autotools.pm b/Debian/Debhelper/Buildsystem/autotools.pm
index 65694b3..945ca40 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 d7504d1..00f6be4 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 91a6341..cbd9e3c 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 74106d9..a43d65d 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 2a6df37..2e7eacb 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;
}
diff --git a/Debian/Debhelper/Dh_Buildsystem_Bases.pm b/Debian/Debhelper/Dh_Buildsystem_Bases.pm
index ab24829..2416453 100644
--- a/Debian/Debhelper/Dh_Buildsystem_Bases.pm
+++ b/Debian/Debhelper/Dh_Buildsystem_Bases.pm
@@ -3,6 +3,8 @@
# Copyright: © 2008-2009 Modestas Vainius
# License: GPL-2+
+# XXX JEH file name does not match package name (Bases vs Basic)
+# That will cause problems when using 'use' to load the module.
package Debian::Debhelper::Dh_Buildsystem_Basic;
use Cwd;
@@ -12,10 +14,13 @@ use Debian::Debhelper::Dh_Lib;
# Build system name. Defaults to the last component of the package
# name. Do not override this method unless you know what you are
# doing.
+# XXX JEH s/package/class/ in above comment for clarity..
sub NAME {
my $self = shift;
my $cls = ref($self) || $self;
return ($cls =~ m/^.+::([^:]+)$/) ? $1 : "[invalid package name]";
+ # XXX JEH s/package/class/ in above message for clarity..
+ # (maybe dying on this unusual error would be better tho?)
}
# Description of the build system to be shown to the users.
@@ -25,6 +30,11 @@ sub DESCRIPTION {
sub new {
my ($cls, $builddir) = @_;
+ # XXX JEH probably would be better to use named parameters here
+ # Also, if the builddir value is not specified, it could use
+ # DEFAULT_BUILD_DIRECTORY here. Which would allow moving that
+ # sub from Dh_Buildsystems to here, since it would no longer need
+ # to be used there.
my $self = bless({ builddir => $builddir }, $cls);
if (!defined($builddir) || $builddir eq ".") {
$self->{builddir} = undef;
@@ -73,6 +83,20 @@ sub get_buildpath {
}
}
+# XXX JEH this method seems entirely useless.
+# $self->invoke_impl('foo', @_);
+# $self->foo(@_);
+# The second is easier to both read and write.
+#
+# Turns out that one build system overrides this method,
+# perl_build uses it to set an env vatiable before each method
+# call. But that's unnecessary; it could just set it in its constructor.
+#
+# And there's another version of this method in the derifed class below,
+# which just adds another parameter to the method call. That is used only
+# by the python_distutils build system, to add a --build-base parameter
+# to two calls to python. It could be manually added to those two calls
+# with less fuss.
sub invoke_impl {
my $self=shift;
my $method=shift;
@@ -85,6 +109,10 @@ sub invoke_impl {
# These methods are wrappers around respective *_impl() methods
# which are supposed to do real buildsystem specific work.
+# XXX JEH if invoke_impl is done away with, these can be replaced
+# with the bodies of the foo_impl methods they call. That layer of
+# indirection is not really needed.
+
sub configure {
my $self=shift;
return $self->invoke_impl('configure_impl', @_);
@@ -115,6 +143,8 @@ sub clean {
# source. Arbitary number of custom action arguments might be passed.
# Default implementations do nothing.
+
+
sub configure_impl {
my $self=shift;
1;
@@ -131,6 +161,10 @@ sub test_impl {
}
# destdir parameter specifies where to install files.
+# XXX JEH I don't see where this destdir parameter is passed in
+# to a call to $object->install ? In Dh_Buildsystems it does:
+# return $buildsystem->$toolname(@_, @{$dh{U_PARAMS}});
+# Which passes a different parameter, to ALL these methods.
sub install_impl {
my $self=shift;
my $destdir=shift;
@@ -149,6 +183,9 @@ use base 'Debian::Debhelper::Dh_Buildsystem_Basic';
# Derived class can call this method in its constructor to enforce
# outside-source building even if the user didn't request it.
+#
+# XXX JEH is there a reason for this to be in
+# Dh_Buildsystem_Option instead of the base class?
sub enforce_outside_source_building {
my ($self, $builddir) = @_;
if (!defined $self->{builddir}) {
@@ -163,6 +200,8 @@ sub get_builddir_option {
return;
}
+# XXX JEH if the reasoning for removing invoke_impl from above is followed,
+# then this whole derived class ends up not being needed.
sub invoke_impl {
my $self=shift;
my $method=shift;
@@ -177,6 +216,11 @@ sub invoke_impl {
package Debian::Debhelper::Dh_Buildsystem_Chdir;
+# XXX JEH I'm very leery of code that chdirs, it can be very hard to follow
+# and cause a lot of mess. (As we'll see in the buildsystem modules that
+# use this class.) It seems to me that this entire class could be
+# basically replaced by a doit_in_builddir() function.
+
use Cwd;
use File::Spec;
use Debian::Debhelper::Dh_Lib;
diff --git a/Debian/Debhelper/Dh_Buildsystems.pm b/Debian/Debhelper/Dh_Buildsystems.pm
index 7e8ca29..aa2dff9 100644
--- a/Debian/Debhelper/Dh_Buildsystems.pm
+++ b/Debian/Debhelper/Dh_Buildsystems.pm
@@ -13,6 +13,7 @@ use Exporter qw( import );
our @EXPORT_OK = qw( DEFAULT_BUILD_DIRECTORY );
# IMPORTANT: more specific buildsystems should go first
+# XXX JEH as noted, this has to match historical order for back-compat
my @BUILDSYSTEMS = (
"autotools",
"cmake",
@@ -34,6 +35,7 @@ sub new {
'o_system' => undef,
'loaded_buildsystems' => [] }, $cls);
+ # XXX JEH AFAICS, these 2 env variables are never used or documented
if (!exists $opts{noenv}) {
if (exists $ENV{DH_AUTO_BUILDDIRECTORY}) {
$self->_set_build_directory_option("env", $ENV{DH_AUTO_BUILDDIRECTORY});
@@ -69,6 +71,8 @@ sub get_options {
}
sub _set_build_directory_option {
+ # XXX JEH option argument is not used, would be less confusing to
+ # not pass extra getopt value in
my ($self, $option, $value) = @_;
if (!$value || $value eq "auto") {
# Autogenerate build directory name
@@ -79,6 +83,7 @@ sub _set_build_directory_option {
}
}
+# XXX JEH this sub is not used
sub _dump_options {
my $self=shift;
for my $opt (qw(o_dir o_system)) {
@@ -103,6 +108,8 @@ sub _get_buildsystem_module {
}
sub load_buildsystem {
+ # XXX JEH the $system param is never passed
+ # by any call to this function
my ($self, $action, $system) = @_;
if (!defined $system) {
@@ -155,6 +162,7 @@ sub run_dh_auto_tool {
my $toolname = basename($0);
my $buildsystem;
+ # XXX JEH does this if ever not fire?
if (!exists $self->{initialized}) {
$self->init_dh_auto_tool();
}
@@ -172,4 +180,11 @@ sub run_dh_auto_tool {
return 0;
}
+# XXX JEH generally, why does this need to be an OO object at all?
+# The entire state stored in this object is o_dir and o_system;
+# and the object is used as a singleton. So why not have a single sub
+# that parses the command line, loads the specified system, and uses it,
+# passing it the build directory. It would be both shorter and easier to
+# understand.
+
1;