summaryrefslogtreecommitdiff
path: root/Debian
diff options
context:
space:
mode:
authorJoey Hess <joey@gnu.kitenet.net>2009-04-14 14:51:34 -0400
committerJoey Hess <joey@gnu.kitenet.net>2009-04-14 14:51:34 -0400
commit20411481c9b97c9a7d1927eb61004e4360d77259 (patch)
tree8753f190e40d868a86b81fc05e0954912f16f13a /Debian
parent683f6060d8304d6d4e83bd76e5ac624a35b43442 (diff)
update and remove XXX comments
Diffstat (limited to 'Debian')
-rw-r--r--Debian/Debhelper/Buildsystem/autotools.pm5
-rw-r--r--Debian/Debhelper/Buildsystem/cmake.pm3
-rw-r--r--Debian/Debhelper/Buildsystem/makefile.pm11
-rw-r--r--Debian/Debhelper/Buildsystem/perl_makemaker.pm18
-rw-r--r--Debian/Debhelper/Dh_Buildsystem_Basic.pm17
-rw-r--r--Debian/Debhelper/Dh_Buildsystems.pm25
6 files changed, 19 insertions, 60 deletions
diff --git a/Debian/Debhelper/Buildsystem/autotools.pm b/Debian/Debhelper/Buildsystem/autotools.pm
index 1ebc938..0885167 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 026004a..ff24824 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 7ffb048..36081ec 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 1553958..91a0da3 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, @_);
}
}
diff --git a/Debian/Debhelper/Dh_Buildsystem_Basic.pm b/Debian/Debhelper/Dh_Buildsystem_Basic.pm
index cd4e448..76163bc 100644
--- a/Debian/Debhelper/Dh_Buildsystem_Basic.pm
+++ b/Debian/Debhelper/Dh_Buildsystem_Basic.pm
@@ -36,12 +36,6 @@ sub DEFAULT_BUILD_DIRECTORY {
"obj-" . dpkg_architecture_value("DEB_BUILD_GNU_TYPE");
}
-# XXX JEH 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.
-# XXX MDX See comment below. is_auto is currently unused but I think it
-# is a good addition to the API for future cases.
-
# Constructs a new build system object. Named parameters:
# - builddir - specifies build directory to use. If not specified,
# in-source build will be performed. If undef or empty,
@@ -144,12 +138,6 @@ sub get_rel2builddir_path {
return $path;
}
-# 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.
-# XXX MDX implemented.
-
sub _mkdir {
my ($cls, $dir)=@_;
if (-e $dir && ! -d $dir) {
@@ -260,11 +248,6 @@ sub test {
}
# 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.
-# XXX MDX destdir is used in the dh_auto_install tool.
sub install {
my $self=shift;
my $destdir=shift;
diff --git a/Debian/Debhelper/Dh_Buildsystems.pm b/Debian/Debhelper/Dh_Buildsystems.pm
index 676551b..34108ca 100644
--- a/Debian/Debhelper/Dh_Buildsystems.pm
+++ b/Debian/Debhelper/Dh_Buildsystems.pm
@@ -27,6 +27,7 @@ our @EXPORT=qw(&buildsystems_init &buildsystems_do &load_buildsystem);
# perl_makemaker (+configure +install (special hack); the rest - next class)
# makefile (+build +test +install +clean; configure - next class)
# perl_build (handles everything)
+# XXX JEH I think that makes sense..
# Historical order must be kept for backwards compatibility. New
# buildsystems MUST be added to the END of the list.
@@ -56,9 +57,6 @@ sub create_buildsystem_instance {
}
sub load_buildsystem {
- # XXX JEH the $system param is never passed
- # by any call to this function
- # XXX MDX Yes, it was sort of redudant. But see buildsystems_do() now.
my ($action, $system)=@_;
if (defined $system) {
my $inst = create_buildsystem_instance($system);
@@ -88,10 +86,11 @@ sub list_buildsystems {
sub buildsystems_init {
my %args=@_;
- # XXX JEH AFAICS, these 2 env variables are never used or documented
- # XXX MDX They are used (see below), not documented though.
# TODO: Not documented in the manual pages yet.
# Initialize options from environment variables
+ # XXX JEH I think these should be my variables, they are only used
+ # inside this one file so putting them in the global %dh hash seems
+ # unnecessary.
if (exists $ENV{DH_AUTO_BUILDDIRECTORY}) {
$dh{BUILDDIR} = $ENV{DH_AUTO_BUILDDIRECTORY};
}
@@ -126,10 +125,6 @@ sub buildsystems_do {
$action =~ s/^dh_auto_//;
}
- # XXX JEH does this if ever not fire?
- # XXX MDX See dh_auto_install. I'm removing this anyway
- # and making buildsystem_init() call in dh_auto_* mandatory.
-
if (grep(/^\Q$action\E$/, qw{configure build test install clean}) == 0) {
error("unrecognized auto action: ".basename($0));
}
@@ -143,16 +138,4 @@ sub buildsystems_do {
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.
-# XXX I refactored this into a module rather than OO class. I do not agree
-# about a single sub though as it is more complicated than that and
-# I think it is more clear to have the code segmented a bit. See also
-# dh_auto_install why both buildsystems_init() and buildsystems_do()
-# are needed.
-
1;