From 20411481c9b97c9a7d1927eb61004e4360d77259 Mon Sep 17 00:00:00 2001 From: Joey Hess Date: Tue, 14 Apr 2009 14:51:34 -0400 Subject: update and remove XXX comments --- Debian/Debhelper/Buildsystem/autotools.pm | 5 ----- Debian/Debhelper/Buildsystem/cmake.pm | 3 --- Debian/Debhelper/Buildsystem/makefile.pm | 11 ++++++----- Debian/Debhelper/Buildsystem/perl_makemaker.pm | 18 +++++++++--------- Debian/Debhelper/Dh_Buildsystem_Basic.pm | 17 ----------------- Debian/Debhelper/Dh_Buildsystems.pm | 25 ++++--------------------- 6 files changed, 19 insertions(+), 60 deletions(-) (limited to 'Debian') 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, @_); } } diff --git a/Debian/Debhelper/Dh_Buildsystem_Basic.pm b/Debian/Debhelper/Dh_Buildsystem_Basic.pm index cd4e448f..76163bcb 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 676551b9..34108caa 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; -- cgit v1.2.3