From 14d936391e07454c7936a28c512e8b42b26cd05e Mon Sep 17 00:00:00 2001 From: Modestas Vainius Date: Wed, 15 Apr 2009 17:39:06 +0300 Subject: [PATCH 1/1] debhelper modular buildsystems (try 3). * New feature - when listing buildsystems, list their status too (auto/specified). * Dh_Buildsystem_Basic.pm renamed to Dh_Buildsystem.pm * Addressed a few issues expressed in the comments, answered a few comments. * Cache DEB_BUILD_GNU_TYPE value. Performance hit is noticable when listing build systems. * is_auto_buildable() renamed to check_auto_buildable() (again). Since there is is_buildable() now, I didn't want to use is_ for that method. Signed-off-by: Modestas Vainius --- Debian/Debhelper/Buildsystem/autotools.pm | 8 +- Debian/Debhelper/Buildsystem/cmake.pm | 4 +- Debian/Debhelper/Buildsystem/makefile.pm | 14 +- Debian/Debhelper/Buildsystem/perl_build.pm | 6 +- .../Debhelper/Buildsystem/perl_makemaker.pm | 26 +--- .../Debhelper/Buildsystem/python_distutils.pm | 15 +-- ...Buildsystem_Basic.pm => Dh_Buildsystem.pm} | 122 ++++++++++++------ Debian/Debhelper/Dh_Buildsystems.pm | 83 +++++++----- 8 files changed, 157 insertions(+), 121 deletions(-) rename Debian/Debhelper/{Dh_Buildsystem_Basic.pm => Dh_Buildsystem.pm} (64%) diff --git a/Debian/Debhelper/Buildsystem/autotools.pm b/Debian/Debhelper/Buildsystem/autotools.pm index b94d8e1..74f1652 100644 --- a/Debian/Debhelper/Buildsystem/autotools.pm +++ b/Debian/Debhelper/Buildsystem/autotools.pm @@ -15,9 +15,9 @@ sub DESCRIPTION { "support for building GNU Autotools based packages" } -sub is_auto_buildable { +sub check_auto_buildable { my $self=shift; - my $action=shift; + my ($action)=@_; # Handle configure; the rest - next class # XXX JEH @@ -30,6 +30,10 @@ sub is_auto_buildable { # actions, it would use the methods inherited from its parent # class. In the above example, that will try to run "make" w/o a # Makefile, which prints a useful error. + # XXX MDX I'm all for it but this will differ from current dh_auto_build + # behaviour (which is that dh_auto_build doesn't fail if + # dh_auto_configure was not run). It is your call whether you are + # willing to break this aspect of backwards compatibility. if ($action eq "configure") { return -x "configure"; } diff --git a/Debian/Debhelper/Buildsystem/cmake.pm b/Debian/Debhelper/Buildsystem/cmake.pm index ff24824..1b5dda4 100644 --- a/Debian/Debhelper/Buildsystem/cmake.pm +++ b/Debian/Debhelper/Buildsystem/cmake.pm @@ -14,11 +14,11 @@ sub DESCRIPTION { "support for building CMake based packages (outside-source tree only)" } -sub is_auto_buildable { +sub check_auto_buildable { my $self=shift; my ($action)=@_; my $ret = -e "CMakeLists.txt"; - $ret &&= $self->SUPER::is_auto_buildable(@_) if $action ne "configure"; + $ret &&= $self->SUPER::check_auto_buildable(@_) if $action ne "configure"; return $ret; } diff --git a/Debian/Debhelper/Buildsystem/makefile.pm b/Debian/Debhelper/Buildsystem/makefile.pm index 286f0f6..0595b0f 100644 --- a/Debian/Debhelper/Buildsystem/makefile.pm +++ b/Debian/Debhelper/Buildsystem/makefile.pm @@ -8,7 +8,7 @@ package Debian::Debhelper::Buildsystem::makefile; use strict; use Debian::Debhelper::Dh_Lib; -use base 'Debian::Debhelper::Dh_Buildsystem_Basic'; +use base 'Debian::Debhelper::Dh_Buildsystem'; sub get_makecmd_C { my $self=shift; @@ -18,14 +18,6 @@ sub get_makecmd_C { return $self->{makecmd}; } -# 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(); @@ -61,12 +53,14 @@ sub new { return $self; } -sub is_auto_buildable { +sub check_auto_buildable { my $self=shift; my ($action) = @_; # Handles build, test, install, clean; configure - next class # XXX JEH shouldn't it also handle configure, just as a no-op? + # XXX MDX No, then cmake.mk would have no chance of hitting for + # no good reason. if (grep /^\Q$action\E$/, qw{build test install clean}) { # This is always called in the source directory, but generally # Makefiles are created (or live) in the the build directory. diff --git a/Debian/Debhelper/Buildsystem/perl_build.pm b/Debian/Debhelper/Buildsystem/perl_build.pm index 550f2a5..6365530 100644 --- a/Debian/Debhelper/Buildsystem/perl_build.pm +++ b/Debian/Debhelper/Buildsystem/perl_build.pm @@ -8,13 +8,13 @@ package Debian::Debhelper::Buildsystem::perl_build; use strict; use Debian::Debhelper::Dh_Lib; -use base 'Debian::Debhelper::Dh_Buildsystem_Basic'; +use base 'Debian::Debhelper::Dh_Buildsystem'; sub DESCRIPTION { "support for building Perl Build.PL based packages (in-source only)" } -sub is_auto_buildable { +sub check_auto_buildable { my ($self, $action) = @_; # Handles everything @@ -25,6 +25,8 @@ sub is_auto_buildable { # succeed, silently do nothing. Perhaps it would be better, then, # to omit the test below. Then, it would try to run ./Build # which doesn't exist, which should result in a semi-useful error. + # XXX MDX Agreed. But it would not be fully backwards compatible + # (see comment in autotools.mk why). Your call. if ($action ne "configure") { $ret &&= -e "Build"; } diff --git a/Debian/Debhelper/Buildsystem/perl_makemaker.pm b/Debian/Debhelper/Buildsystem/perl_makemaker.pm index 91a0da3..2957016 100644 --- a/Debian/Debhelper/Buildsystem/perl_makemaker.pm +++ b/Debian/Debhelper/Buildsystem/perl_makemaker.pm @@ -14,23 +14,19 @@ sub DESCRIPTION { "support for building Perl MakeMaker based packages (in-source only)" } -sub is_auto_buildable { - my ($self, $action)=@_; +sub check_auto_buildable { + my $self=shift; + my ($action)=@_; # Handles configure, install; the rest - next class 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; - } + return -e "Makefile.PL"; } # 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. + # XXX MDX Yes. But that's again different behaviour from current + # (see comment in autotools.mk). Your call. elsif ($action eq "configure") { return -e "Makefile.PL"; } @@ -57,15 +53,7 @@ sub configure { sub install { my $self=shift; my $destdir=shift; - # 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 { - $self->SUPER::install($destdir, @_); - } + $self->SUPER::install($destdir, "PREFIX=/usr", @_); } 1; diff --git a/Debian/Debhelper/Buildsystem/python_distutils.pm b/Debian/Debhelper/Buildsystem/python_distutils.pm index 0b8367e..0061a44 100644 --- a/Debian/Debhelper/Buildsystem/python_distutils.pm +++ b/Debian/Debhelper/Buildsystem/python_distutils.pm @@ -9,23 +9,14 @@ package Debian::Debhelper::Buildsystem::python_distutils; use strict; use Debian::Debhelper::Dh_Lib; -use base 'Debian::Debhelper::Dh_Buildsystem_Basic'; +use base 'Debian::Debhelper::Dh_Buildsystem'; sub DESCRIPTION { "support for building Python distutils based packages" } -sub is_auto_buildable { - my $self=shift; - my $action=shift; - - # Handle build install clean; the rest - next class - # XXX JEH shouldn't it also handle configure? It would be handled - # by doing nothing, but that's what's appropriate for python. - if (grep(/^\Q$action\E$/, qw{build install clean})) { - return -e "setup.py"; - } - return 0; +sub check_auto_buildable { + return -e "setup.py"; } sub setup_py { diff --git a/Debian/Debhelper/Dh_Buildsystem_Basic.pm b/Debian/Debhelper/Dh_Buildsystem.pm similarity index 64% rename from Debian/Debhelper/Dh_Buildsystem_Basic.pm rename to Debian/Debhelper/Dh_Buildsystem.pm index b6eec4b..364dbfc 100644 --- a/Debian/Debhelper/Dh_Buildsystem_Basic.pm +++ b/Debian/Debhelper/Dh_Buildsystem.pm @@ -1,12 +1,17 @@ -# Defines basic debhelper buildsystem class interface. +# Defines debhelper buildsystem class interface and implementation +# of common functionality. # # Copyright: © 2008-2009 Modestas Vainius # License: GPL-2+ -# XXX JEH maybe rename this class to Debian::Debhelper::Dh_Buildsystem? # XXX JEH also it seems the functions in Dh_Buildsystems could be merged # into this same file. -package Debian::Debhelper::Dh_Buildsystem_Basic; +# XXX MDX I disagree. I think that mixing OO class and non-OO functions in the +# same file is a bad style. What is more, these two modules have different +# purposes (Dh_Buildsystems is an agregator of Dh_Buildsystem and its +# derivatives). Moreover, we don't want Dh_Buildsystem to inherit from Exporter +# (like Dh_Buildsystems do), do we? +package Debian::Debhelper::Dh_Buildsystem; use strict; use warnings; @@ -14,6 +19,10 @@ use Cwd; use File::Spec; use Debian::Debhelper::Dh_Lib; +# Cache DEB_BUILD_GNU_TYPE value. Performance hit of multiple +# invocations is noticable when listing buildsystems. +our $DEB_BUILD_GNU_TYPE = dpkg_architecture_value("DEB_BUILD_GNU_TYPE"); + # Build system name. Defaults to the last component of the class # name. Do not override this method unless you know what you are # doing. @@ -36,47 +45,68 @@ sub DESCRIPTION { # Default build directory. Can be overriden in the derived # class if really needed. sub DEFAULT_BUILD_DIRECTORY { - "obj-" . dpkg_architecture_value("DEB_BUILD_GNU_TYPE"); + "obj-" . $DEB_BUILD_GNU_TYPE; } # 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, -# default DEFAULT_BUILD_DIRECTORY will be used. -# - is_auto - might be used by the derived classes to determine if -# the build system has been picked up automatically. -# Derived class can override the constructor to initialize common parameters. -# Constructor SHOULD NOT be used to initialize build environment because -# constructed class may not be eventually used to build the package (if e.g. -# is_auto_buildable() returns 0). +# - builddir - specifies build directory to use. If not specified, +# in-source build will be performed. If undef or empty, +# default DEFAULT_BUILD_DIRECTORY will be used. +# - build_action - set this parameter to the name of the build action +# if you want the object to determine its is_buidable +# status automatically (with check_auto_buildable()). +# Do not pass this parameter if is_buildable flag should +# be forced to true or set this parameter to undef if +# is_buildable flag should be false. +# Derived class can override the constructor to initialize common object +# parameters and execute commands to configure build environment if +# is_buildable flag is set on the object. # # XXX JEH the above comment begs the question: Why not test # is_auto_buildable in the constructor, and only have the constructor # succeed if it can handle the source? That would also eliminate the # delayed warning mess in enforce_in_source_building. +# XXX MDX Yes, that warning stuff was a mess. I implemented your +# idea partitially. # -# (In turn that could be used to remove the pre_action, since that's the +# XXX JEH (In turn that could be used to remove the pre_action, since that's the # only use of it -- the post_action is currently unused too. It could be # argued that these should be kept in case later buildsystems need them # though.) +# XXX MDX Well, I think we could keep them (now both empty) for the reason +# you mention. # -# AFAICS, there is only one reason you need an instance of the object +# XXX JEH AFAICS, there is only one reason you need an instance of the object # if it can't build -- to list build systems. But that only needs # DESCRIPTION and NAME, which could be considered to be class methods, # rather than object methods -- no need to construct an instance of the # class before calling those. -# -# I see that if --buildsystem is manually specified to override, +# XXX MDX Well yeah, they used to be (and still can be used) as such. But I +# implemented a new feature to show force/auto_buildable status +# while listing buildsystems. That feature needs an instance. + +# XXX JEH I see that if --buildsystem is manually specified to override, # the is_auto_buildable test is completely skipped. So if this change were # made, you'd not be able to skip the test, and some --buildsystem choices # might cause an error. OTOH, those seem to be cases where it would later # fail anyway. The real use cases for --buildsystem, such as forcing use of # cmake when there are both a CMakeLists.txt and a Makefile, would still # work. +# XXX MDX 1) If buildsystem is forced, there might be a good reason for it. +# What is more, that check as it is now is for *auto* stuff only. +# In general, it cannot be used to reliably check if the source +# will be buildable or not. +# 2) Your last sentence is not entirely true. Backwards compatibility +# is also a huge limitation. The check_auto_buildable() should always +# fail if it is not possible to add a new buildsystem in the backwards +# compatible manner. See also my comments in the makefile.pm. +# 3) What is more, I implemented skipping of the auto buildable check, +# so this is no longer the issue. + sub new { my ($cls, %opts)=@_; - my $self = bless({ builddir => undef, is_auto => $opts{is_auto} }, $cls); + my $self = bless({ builddir => undef, is_buildable => 1 }, $cls); if (exists $opts{builddir}) { if ($opts{builddir}) { $self->{builddir} = $opts{builddir}; @@ -85,9 +115,23 @@ sub new { $self->{builddir} = $self->DEFAULT_BUILD_DIRECTORY(); } } + if (exists $opts{build_action}) { + if (defined $opts{build_action}) { + $self->{is_buildable} = $self->check_auto_buildable($opts{build_action}); + } + else { + $self->{is_buildable} = 0; + } + } return $self; } +# Test is_buildable flag of the object. +sub is_buildable { + my $self=shift; + return $self->{is_buildable}; +} + # This instance method is called to check if the build system is capable # to auto build a source package. Additional argument $action describes # which operation the caller is going to perform (either configure, @@ -98,7 +142,7 @@ sub new { # This method is supposed to be called with source root directory being # working directory. Use $self->get_buildpath($path) method to get full # path to the files in the build directory. -sub is_auto_buildable { +sub check_auto_buildable { my $self=shift; my ($action) = @_; return 0; @@ -109,11 +153,11 @@ sub is_auto_buildable { sub enforce_in_source_building { my $self=shift; if ($self->{builddir}) { - # Since this method is called in the constructor, emitting - # warnings immediatelly may cause too much noise when - # scanning for auto buildsystems or listing them. - push @{$self->{warnings}}, - $self->NAME()." buildsystem does not support building outside-source. In-source build enforced."; + # Do not emit warning unless the object is buildable. + if ($self->is_buildable()) { + warning("warning: " . $self->NAME() . + " does not support building outside-source. In-source build enforced."); + } $self->{builddir} = undef; } } @@ -168,14 +212,14 @@ sub get_rel2builddir_path { sub _mkdir { my ($cls, $dir)=@_; # XXX JEH is there any reason not to just doit("mkdir") ? + # XXX MDX Replaced below part. This call is there to be + # more verbose about errors (if accidently $dir in + # non-dir form and to test for ! -d $dir. if (-e $dir && ! -d $dir) { error("error: unable to create '$dir': object already exists and is not a directory"); } elsif (! -d $dir) { - verbose_print("mkdir '$dir'"); - if (! $dh{NO_ACT}) { - mkdir($dir, 0755) or error("error: unable to create '$dir': $!"); - } + doit("mkdir", $dir); return 1; } return 0; @@ -183,9 +227,8 @@ sub _mkdir { sub _cd { my ($cls, $dir)=@_; - # XXX JEH I think this should verbose_print("cd $dir") - # then the verbose_prints in doit_in_builddir are unnecessary. if (! $dh{NO_ACT}) { + verbose_print("cd $dir"); chdir $dir or error("error: unable to chdir to $dir"); } } @@ -193,7 +236,7 @@ sub _cd { # Creates a build directory. Returns 1 if the directory was created # or 0 if it already exists or there is no need to create it. sub mkdir_builddir { - my $self=shift;a + my $self=shift; if ($self->get_builddir()) { return $self->_mkdir($self->get_builddir()); } @@ -207,10 +250,8 @@ sub doit_in_builddir { if ($self->get_builddir()) { my $builddir = $self->get_builddir(); my $sourcedir = $self->get_rel2builddir_path(); - verbose_print("cd to the build directory: $builddir"); $self->_cd($builddir); doit(@_); - verbose_print("cd back to the source directory: $sourcedir"); $self->_cd($sourcedir); } else { @@ -223,6 +264,10 @@ sub doit_in_builddir { # gets wiped (if it exists) and 1 is returned. Otherwise, nothing # is done and 0 is returned. # XXX JEH only makefile.pm uses this, move it there? +# XXX MDX Well true, but I think this one is good to have for API +# completeness near to mkdir_builddir and doit_in_builddir above. +# I don't have strong feelings about it, but it looks more common +# function than makefile specific to me. sub clean_builddir { my $self=shift; if ($self->get_builddir()) { @@ -240,14 +285,7 @@ sub clean_builddir { # method should also call SUPER implementation of it. sub pre_action { my $self=shift; - my $action=shift; - - # Emit warnings pre action. - if (exists $self->{warnings}) { - for my $msg (@{$self->{warnings}}) { - warning("warning: " . $msg); - } - } + my ($action)=@_; } # Instance method that is called after performing any action (see below). @@ -255,7 +293,7 @@ sub pre_action { # method should also call SUPER implementation of it. sub post_action { my $self=shift; - my $action=shift; + my ($action)=@_; } # The instance methods below provide support for configuring, diff --git a/Debian/Debhelper/Dh_Buildsystems.pm b/Debian/Debhelper/Dh_Buildsystems.pm index 6774511..3d45e13 100644 --- a/Debian/Debhelper/Dh_Buildsystems.pm +++ b/Debian/Debhelper/Dh_Buildsystems.pm @@ -21,11 +21,11 @@ our @EXPORT=qw(&buildsystems_init &buildsystems_do &load_buildsystem); # install: makefile (with perl_makermaker) hack, python_distutils, perl_build # clean: makefile, python_distutils, perl_build # So historical @BUILDSYSTEMS order (as per autodetection, see -# is_auto_buildable() of the respective classes): +# check_auto_buildable() of the respective classes): # autotools (+configure; the rest - next class) -# python_distutils (+build +install +clean; the rest - next class) # perl_makemaker (+configure +install (special hack); the rest - next class) # makefile (+build +test +install +clean; configure - next class) +# python_distutils (handles everything) # perl_build (handles everything) # XXX JEH I think that makes sense.. @@ -33,13 +33,17 @@ our @EXPORT=qw(&buildsystems_init &buildsystems_do &load_buildsystem); # buildsystems MUST be added to the END of the list. our @BUILDSYSTEMS = ( "autotools", - "python_distutils", "perl_makemaker", "makefile", + "python_distutils", "perl_build", "cmake", ); +my $opt_buildsys; +my $opt_builddir; +my $opt_list; + sub create_buildsystem_instance { my $system=shift; my %bsopts=@_; @@ -50,8 +54,8 @@ sub create_buildsystem_instance { error("unable to load buildsystem class '$system': $@"); } - if (!exists $bsopts{builddir} && exists $dh{BUILDDIR}) { - $bsopts{builddir} = $dh{BUILDDIR}; + if (!exists $bsopts{builddir} && defined $opt_builddir) { + $bsopts{builddir} = ($opt_builddir eq "") ? undef : $opt_builddir; } return $module->new(%bsopts); } @@ -66,8 +70,8 @@ sub load_buildsystem { else { # Try to determine build system automatically for $system (@BUILDSYSTEMS) { - my $inst = create_buildsystem_instance($system, is_auto=>1); - if ($inst->is_auto_buildable($action)) { + my $inst = create_buildsystem_instance($system, build_action=>$action); + if ($inst->is_buildable()) { verbose_print("Selected buildsystem (auto): ". $inst->NAME()); return $inst; } @@ -76,43 +80,28 @@ sub load_buildsystem { return; } -sub list_buildsystems { - for my $system (@BUILDSYSTEMS) { - my $inst = create_buildsystem_instance($system); - printf("%s - %s.\n", $inst->NAME(), $inst->DESCRIPTION()); - } -} - sub buildsystems_init { my %args=@_; # 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}; + $opt_builddir = $ENV{DH_AUTO_BUILDDIRECTORY}; } if (exists $ENV{DH_AUTO_BUILDSYSTEM}) { - $dh{BUILDSYS} = $ENV{DH_AUTO_BUILDSYSTEM}; + $opt_buildsys = $ENV{DH_AUTO_BUILDSYSTEM}; } # Available command line options - my $list_bs = sub { list_buildsystems(); exit 0 }; - my $set_builddir = sub { $dh{BUILDDIR} = $_[1] }; my %options = ( - "b:s" => $set_builddir, - "build-directory:s" => $set_builddir, - "builddirectory:s" => $set_builddir, + "b:s" => \$opt_builddir, + "builddirectory:s" => \$opt_builddir, - "m=s" => \$dh{BUILDSYS}, - # XXX JEH Let's only keep one spelling of this. - "build-system=s" => \$dh{BUILDSYS}, - "buildsystem=s" => \$dh{BUILDSYS}, + "m=s" => \$opt_buildsys, + "buildsystem=s" => \$opt_buildsys, - "l" => $list_bs, - "--list" => $list_bs, + "l" => \$opt_list, + "--list" => \$opt_list, ); map { $args{options}{$_} = $options{$_} } keys(%options); Debian::Debhelper::Dh_Lib::init(%args); @@ -127,10 +116,40 @@ sub buildsystems_do { } if (grep(/^\Q$action\E$/, qw{configure build test install clean}) == 0) { - error("unrecognized auto action: ".basename($0)); + error("unrecognized build action: " . $action); + } + + if ($opt_list) { + # List buildsystems (including auto and specified status) + my $auto_found; + my $specified_found; + print "STATUS (* auto, + specified) NAME - DESCRIPTION", "\n"; + for my $system (@BUILDSYSTEMS) { + my $inst = create_buildsystem_instance($system, build_action => undef); + my $is_specified = defined $opt_buildsys && $opt_buildsys eq $inst->NAME(); + my $status; + if ($is_specified) { + $status = "+"; + $specified_found = 1; + } + elsif (!$auto_found && $inst->check_auto_buildable($action)) { + $status = "*"; + $auto_found = 1; + } + else { + $status = " "; + } + printf("%s %s - %s.\n", $status, $inst->NAME(), $inst->DESCRIPTION()); + } + # List a 3rd party buildsystem too. + if (!$specified_found && defined $opt_buildsys) { + my $inst = create_buildsystem_instance($opt_buildsys, build_action => undef); + printf("+ %s - %s.\n", $inst->NAME(), $inst->DESCRIPTION()); + } + exit 0; } - my $buildsystem = load_buildsystem($action, $dh{BUILDSYS}); + my $buildsystem = load_buildsystem($action, $opt_buildsys); if (defined $buildsystem) { $buildsystem->pre_action($action); $buildsystem->$action(@_, @{$dh{U_PARAMS}}); -- 2.39.2