diff options
Diffstat (limited to 'Debian')
| -rw-r--r-- | Debian/Debhelper/Buildsystem/autotools.pm | 14 | ||||
| -rw-r--r-- | Debian/Debhelper/Buildsystem/cmake.pm | 5 | ||||
| -rw-r--r-- | Debian/Debhelper/Buildsystem/makefile.pm | 13 | ||||
| -rw-r--r-- | Debian/Debhelper/Buildsystem/perl_build.pm | 2 | ||||
| -rw-r--r-- | Debian/Debhelper/Buildsystem/python_distutils.pm | 3 | ||||
| -rw-r--r-- | Debian/Debhelper/Dh_Buildsystem_Bases.pm | 44 | ||||
| -rw-r--r-- | Debian/Debhelper/Dh_Buildsystems.pm | 15 |
7 files changed, 96 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; } diff --git a/Debian/Debhelper/Dh_Buildsystem_Bases.pm b/Debian/Debhelper/Dh_Buildsystem_Bases.pm index ab24829b..24164537 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 7e8ca29d..aa2dff99 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; |