]> git.donarmstrong.com Git - debhelper.git/commitdiff
update and remove XXX comments
authorJoey Hess <joey@gnu.kitenet.net>
Tue, 14 Apr 2009 18:51:34 +0000 (14:51 -0400)
committerJoey Hess <joey@gnu.kitenet.net>
Tue, 14 Apr 2009 18:51:34 +0000 (14:51 -0400)
Debian/Debhelper/Buildsystem/autotools.pm
Debian/Debhelper/Buildsystem/cmake.pm
Debian/Debhelper/Buildsystem/makefile.pm
Debian/Debhelper/Buildsystem/perl_makemaker.pm
Debian/Debhelper/Dh_Buildsystem_Basic.pm
Debian/Debhelper/Dh_Buildsystems.pm

index 1ebc93846329f3a0247293e339712b9c54e39c4c..088516779b6d9e1c9be67e611368dffeeb2bceaa 100644 (file)
@@ -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, @_);
 }
index 026004a0704145f5adfd698d331ca3dc74083939..ff24824902494137ce262161b42d6e5cf490b8cd 100644 (file)
@@ -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);
 }
index 7ffb048ed56c0981aa1861ed39fedf38d582df61..36081ecc3f2015334c72ae81aeb34f3dfaa076b2 100644 (file)
@@ -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();
index 15539588be22ac303aaf2471b28bc60873950060..91a0da3d771cdeec8f5d0b34910160899a4b93dd 100644 (file)
@@ -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, @_);
        }
 }
index cd4e448f7ad05e5018e24ffb0b4fa5ade50812d2..76163bcb5f5f1112a18ef795db21c5a772dc4ff1 100644 (file)
@@ -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;
index 676551b9dce1ccf41ff1ccdb7f5c5ac31b124b4b..34108caa2e21bf32b5eb45dccad29e7e6056ae33 100644 (file)
@@ -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;