Skip Menu |

This queue is for tickets about the Module-Build CPAN distribution.

Report information
The Basics
Id: 41166
Status: resolved
Priority: 0/
Queue: Module-Build

People
Owner: Nobody in particular
Requestors: blazej_cegielka [...] o2.pl
jleader [...] alumni.caltech.edu
ntyni [...] iki.fi
Cc: TREVELYAN [...] cpan.org
AdminCc:

Bug Information
Severity: Normal
Broken in: 0.30
Fixed in: 0.34_06



Subject: svn.perl.org r11816 broke zim: install_sets modification not preserved anymore
Hi, as seen in http://bugs.debian.org/506696 Module::Build 0.30 breaks the zim (http://zim-wiki.org/) build system. Specifically, zim's Build.PL, as seen at http://bazaar.launchpad.net/~pardus-cpan/zim/trunk/files has this: # Figure out where to install the share directory and put it in each # install set. Just setting 'install_path' breaks --install_base. my $sets = $$build{properties}{install_sets}; for my $dirs (values %$sets) { my @dirs = File::Spec->splitdir($$dirs{bin}); pop @dirs; # loose 'bin' $$dirs{share} = File::Spec->catdir(@dirs, 'share'); } This breaks with Module::Build 0.30, because modifying the install_sets property isn't preserved anymore to the Build phase. Module::Build::Base::resume() now explicitly resets install_sets by calling the _set_install_paths() method. Please let us know if this should be considered a bug in Module::Build 0.30 or Zim itself. Thanks for your work on Module::Build, -- Niko Tyni (Debian Perl Group) ntyni@debian.org
RT-Send-CC: mschwern [...] cpan.org, module-build [...] perl.org
On Sun Nov 23 16:18:50 2008, ntyni@iki.fi wrote: Show quoted text
> http://bazaar.launchpad.net/~pardus-cpan/zim/trunk/files > > has this: > > my $sets = $$build{properties}{install_sets}; > for my $dirs (values %$sets) { > my @dirs = File::Spec->splitdir($$dirs{bin}); > pop @dirs; # loose 'bin' > $$dirs{share} = File::Spec->catdir(@dirs, 'share'); > } > > This breaks with Module::Build 0.30, because modifying the > install_sets property isn't preserved anymore to the Build phase. > Module::Build::Base::resume() now explicitly resets install_sets by > calling the _set_install_paths() method. > > Please let us know if this should be considered a bug in
Module::Build Show quoted text
> 0.30 or Zim itself.
Hi Niko, I think that code is out-of-bounds by modifying the install_sets element directly. It would possibly be less breakable if it were done local() in one of the overridden methods, but probably Module::Build should offer some "official" 'share' directory as part of the API. That does, however, raise the issue of whether it behaves ala File::ShareDir or not. Schwern: does this call to $self->_set_install_paths in resume() have to happen only because it has not yet been set via some other entry point? If so, we could probably find somewhere else from which to call it or simply let it return if the key is already set. Thanks, Eric
On Tue Jan 13 00:35:49 2009, EWILHELM wrote: Show quoted text
> I think that code is out-of-bounds by modifying the install_sets > element directly. It would possibly be less breakable if it were done > local() in one of the overridden methods, but probably Module::Build > should offer some "official" 'share' directory as part of the API. > That does, however, raise the issue of whether it behaves ala > File::ShareDir or not.
Hi Eric, I see the debian maintainers filed a bug based on my hacks to get Module::Build do what I want. As far as I am aware there is no standard way to add directory types to the install structure - is that correct ? Do you happen to know of other projects dealing with this particular of installing a full application instead of just a module ? Any hints are appreciated, will need to find a new hack to hook into Module::Build ... Cheers! Jaap
Implemented a temporary fix in M:B subclass for my package. Could be used as the base for a new interface for adding new data types. ============================= __PACKAGE__->add_property(data_files => {}); sub install_types { my $self = shift; my @types = $self->SUPER::install_types; push @types, keys %{$$self{properties}{data_files}}; return sort @types; } sub install_destination { my ($self, $type) = @_; return $self->SUPER::install_destination($type) unless exists $$self{properties}{data_files}{$type}; return $self->install_path($type) if $self->install_path($type); my $relpath = $$self{properties}{data_files}{$type}; return File::Spec->catdir($self->install_base, $relpath) if $self->install_base; return File::Spec->catdir($self->prefix, $relpath) if $self->prefix; # Fall back to heuristic determination of prefix based on # the 'bin' directory. my $bindir = $self->install_sets($self->installdirs)->{'bin'}; my @prefix = File::Spec->splitdir($bindir); pop @prefix; # loose 'bin' return File::Spec->catdir(@prefix, $relpath); } ====================================== What remains is to decide how blib/share is build. In my case I already had a method "process_share_files" that does this and I call "$build->add_build_element('share')" to set it up. Probably there should be a default for data_files which just copies a directory tree from the source one to one to blib.
Subject: _set_install_paths on resumed builder
Date: Sun, 22 Feb 2009 22:59:32 +0100
To: bug-Module-Build [...] rt.cpan.org
From: Błażej Cegiełka <blazej_cegielka [...] o2.pl>
let me describe smth if i understand right some ideas of Module::Build. if i want to add some config files to my app i create builder using kwarg conf_files => {'conf/config.ini' => 'conf/config.ini'}, and then call $build->add_build_element('conf'). build process works. next i want to say where my conf files should be installed. i can use install_path => {'conf' => '/etc/myapp'} but this need to be an absolute path. so it will not work if i wanted install myapp in my /home/mydir. not exactly. it work, but not as i expected. if i call <code> ./Build install --install_base /home/mydir </code> i expect that my config.ini will be installed in /home/mydir/etc/myapp/config.ini. it didn't. i need to use: <code> ./Buld install --install_base /home/mydir --install_path conf=/home/mydir/etc/myapp </code> this can stay if i want to install my app by hand. but what if i want to distribute myapp by CPAN? how CPAN will know that myapp need to have set --install_path conf? this is why install_base_relpaths() exists? if i use it, $builder->{'properties'}{'install_base_relpaths'} contains: 'conf' => [ 'etc', 'mypkg' ], sounds good. but when i create Build script from Build.PL and after Module::Build->resume() call $build->install_base_relpaths() i will not see my 'conf' anymore. so the state of $builder created in Build.PL is not eq with $builder resumed in Build. i checked resume() func in Module::Build::Base and i saw that before return you call $self->_set_install_paths(). in _set_install_paths() line 265 you overwrite old $builder->{properties}{install_base_relpaths} with hardcoded values and this is why my 'conf' type install_base_relpath disappear. the question is why you call _set_install_paths() on resumed $builder which allready have this set? in my opinion besides it is unnecessary, it create a bug extra. am i right? burb
Subject: install_sets modification not preserved on resume (solution)
Hi, I can confirm this bug. It use to be possible to add install targets, this then got broken, but works again with the following fix: in Module/Build/Base.pm (v 0.34): delete line 89: $self->_set_install_paths; this is safe since new() called it already, before user the user made config changes! or move it before line 54: $self->read_config so that it does not overwrite the restored config. (ie $self->{properties}) How is this bug seen? In both the submitters and my case its because we have added new build and install targets with code like: " #New build types $build->add_build_element('etc'); $build->add_build_element('share'); my $distdir = lc $build->dist_name(); foreach my $id ('core', 'site', 'vendor') { #Where to install these build types when using prefix symantics $build->prefix_relpaths($id, 'share' => "share/$distdir"); $build->prefix_relpaths($id, 'etc' => "etc/$distdir"); #Where to install these build types when using default symantics my $set = $build->install_sets($id); $set->{'share'} = '/usr/'.($id eq 'site' ? 'local/':'')."share/$distdir"; $set->{'etc'} = ($id eq 'site' ? '/usr/local/etc/':'/etc/').$distdir; } #Where to install these types when using install_base symantics $build->install_base_relpaths('share' => "share/$distdir"); $build->install_base_relpaths('etc' => "etc/$distdir"); " A Module::Build subclass might have something like this: sub copy_files { my $self = shift; my $dir = shift; my $files = $self->rscan_dir($dir, sub {-f $_ and not m!/\.|[#~]$!}); foreach my $file (@$files) { $self->copy_if_modified(from => $file, to_dir => "blib"); } } #Copy etc files to blib sub process_etc_files { my $self = shift; copy_files($self, "etc"); } #Copy share files to blib sub process_share_files { my $self = shift; copy_files($self, "share"); } Before the bug was introduced this all worked. Once said line it deleted it works again. Symptom of the bug is that the build works and dirs/files are created in blib/, but the install then ignore them and does not copy them to their final destination. I hope this helps. Thorben
Looking at this quickly, I don't think it's quite so simple. _set_install_paths() depends on values in $self->{config} and sets values in $self->{properties}. So it needs to run after config is ready (in case any Config.pm value are overridden) but before any further customization of properties. I suspect the right place is probably in _construct, but I'll want to look more closely and trace out the code. The monolithic "bless" call in _construct probably needs to be broken up so that "defaults" are set before user inputs are applied. If someone could provide a test file that demonstrates the expected behavior, that would be a big help to getting this fixed.
Hi, Here a somewhat ugly/quickly written, but effective test. Without line 89 of Base.pm commented it fails as follows: " ok 1 - use Module::Build; ok 2 - Make sure Module::Build was loaded from blib/ # Dist is in /var/folders/qH/qHUigC3eGoGgT7PM4aqJfU+++yQ/-Tmp-/MB-zbbX6obH # Creating custom builder _build/lib/MyModuleBuilder.pm in _build/lib # Checking whether your kit is complete... # Looks good # # Checking prerequisites... # Looks good # # Creating new 'Build' script for 'Simple' version '0.01' Warning: Build.PL has been altered. You may need to run 'perl Build.PL' again. # Copying lib/Simple.pm -> blib/lib/Simple.pm # Copying etc/config -> blib/etc/config # Copying share/data -> blib/share/data # Copying share/html/index.html -> blib/share/html/index.html # Manifying blib/lib/Simple.pm -> blib/libdoc/Simple.3pm ok 3 - Built etc/config ok 4 - Built share/data ok 5 - Built share/html Subroutine magic_number_matches redefined at Build line 8. Warning: Build.PL has been altered. You may need to run 'perl Build.PL' again. # Installing /var/folders/qH/qHUigC3eGoGgT7PM4aqJfU+++yQ/-Tmp-/MB-zbbX6obH/t/install_extra_targets-45366/lib/perl5/Simple.pm # Installing /var/folders/qH/qHUigC3eGoGgT7PM4aqJfU+++yQ/-Tmp-/MB-zbbX6obH/t/install_extra_targets-45366/man/man3/Simple.3pm # Writing /var/folders/qH/qHUigC3eGoGgT7PM4aqJfU+++yQ/-Tmp-/MB-zbbX6obH/t/install_extra_targets-45366/lib/perl5/darwin-thread-multi-2level/auto/Simple/.packlist not ok 6 - installed etc/config # Failed test 'installed etc/config' # at t/install_extra_target.t line 131. not ok 7 - installed share/data # Failed test 'installed share/data' # at t/install_extra_target.t line 132. not ok 8 - installed share/html # Failed test 'installed share/html' # at t/install_extra_target.t line 133. 1..8 # Looks like you failed 3 tests of 8. " With line 89 of Base.pm commented out it succeeds as follows: " ok 1 - use Module::Build; ok 2 - Make sure Module::Build was loaded from blib/ # Dist is in /var/folders/qH/qHUigC3eGoGgT7PM4aqJfU+++yQ/-Tmp-/MB-MLjQO9PJ # Creating custom builder _build/lib/MyModuleBuilder.pm in _build/lib # Checking whether your kit is complete... # Looks good # # Checking prerequisites... # Looks good # # Creating new 'Build' script for 'Simple' version '0.01' Warning: Build.PL has been altered. You may need to run 'perl Build.PL' again. # Copying lib/Simple.pm -> blib/lib/Simple.pm # Copying etc/config -> blib/etc/config # Copying share/data -> blib/share/data # Copying share/html/index.html -> blib/share/html/index.html # Manifying blib/lib/Simple.pm -> blib/libdoc/Simple.3pm ok 3 - Built etc/config ok 4 - Built share/data ok 5 - Built share/html Subroutine magic_number_matches redefined at Build line 8. Warning: Build.PL has been altered. You may need to run 'perl Build.PL' again. # Installing /var/folders/qH/qHUigC3eGoGgT7PM4aqJfU+++yQ/-Tmp-/MB-MLjQO9PJ/t/install_extra_targets-45186/etc/simple/config # Installing /var/folders/qH/qHUigC3eGoGgT7PM4aqJfU+++yQ/-Tmp-/MB-MLjQO9PJ/t/install_extra_targets-45186/lib/perl5/Simple.pm # Installing /var/folders/qH/qHUigC3eGoGgT7PM4aqJfU+++yQ/-Tmp-/MB-MLjQO9PJ/t/install_extra_targets-45186/man/man3/Simple.3pm # Installing /var/folders/qH/qHUigC3eGoGgT7PM4aqJfU+++yQ/-Tmp-/MB-MLjQO9PJ/t/install_extra_targets-45186/share/simple/data # Installing /var/folders/qH/qHUigC3eGoGgT7PM4aqJfU+++yQ/-Tmp-/MB-MLjQO9PJ/t/install_extra_targets-45186/share/simple/html/index.html # Writing /var/folders/qH/qHUigC3eGoGgT7PM4aqJfU+++yQ/-Tmp-/MB-MLjQO9PJ/t/install_extra_targets-45186/lib/perl5/darwin-thread-multi-2level/auto/Simple/.packlist ok 6 - installed etc/config ok 7 - installed share/data ok 8 - installed share/html 1..8 " I hope this helps. Thorben
#!perl -w use strict; use lib $ENV{PERL_CORE} ? '../lib/Module/Build/t/lib' : 't/lib'; use MBTest; use_ok 'Module::Build'; ensure_blib('Module::Build'); use File::Spec::Functions qw( catdir ); use Config; use Cwd (); my $cwd = Cwd::cwd; my $tmp = MBTest->tmpdir; use DistGen; my $dist = DistGen->new( dir => $tmp ); note("Dist is in $tmp\n"); $dist->add_file("Build.PL", <<'===EOF==='); #!perl -w use strict; use Module::Build; my $subclass = Module::Build->subclass(code => <<'=EOF='); sub copy_files { my $self = shift; my $dir = shift; my $files = $self->rscan_dir($dir, sub {-f $_ and not m!/\.|[#~]$!}); foreach my $file (@$files) { $self->copy_if_modified(from => $file, to_dir => "blib"); } } #Copy etc files to blib sub process_etc_files { my $self = shift; $self->copy_files("etc"); } #Copy share files to blib sub process_share_files { my $self = shift; $self->copy_files("share"); } 1; =EOF= my $build = $subclass->new( module_name => 'Simple', license => 'perl' ); $build->add_build_element('etc'); $build->add_build_element('share'); my $distdir = lc $build->dist_name(); foreach my $id ('core', 'site', 'vendor') { #Where to install these build types when using prefix symantics $build->prefix_relpaths($id, 'share' => "share/$distdir"); $build->prefix_relpaths($id, 'etc' => "etc/$distdir"); #Where to install these build types when using default symantics my $set = $build->install_sets($id); $set->{'share'} = '/usr/'.($id eq 'site' ? 'local/':'')."share/$distdir"; $set->{'etc'} = ($id eq 'site' ? '/usr/local/etc/':'/etc/').$distdir; } #Where to install these types when using install_base symantics $build->install_base_relpaths('share' => "share/$distdir"); $build->install_base_relpaths('etc' => "etc/$distdir"); $build->create_build_script(); ===EOF=== #Test Build.PL exists ok? $dist->add_file("etc/config", <<'===EOF==='); [main] Foo = bar Jim = bob [supplemental] stardate = 1234344 ===EOF=== $dist->add_file("share/data", <<'===EOF==='); 7 * 9 = 42? ===EOF=== $dist->add_file("share/html/index.html", <<'===EOF==='); <HTML> <BODY> <H1>Hello World!</H1> </BODY> </HTML> ===EOF=== $dist->regen; $dist->chdir_in; my $installdest = catdir($tmp, 't', "install_extra_targets-$$"); diag(stdout_of(sub { local @ARGV = ("--install_base=$installdest"); do "Build.PL"; } )); diag(stdout_of( sub { local @ARGV = (); do "Build"; } )); ok(-e "blib/etc/config", "Built etc/config"); ok(-e "blib/share/data", "Built share/data"); ok(-e "blib/share/html/index.html", "Built share/html"); diag(stdout_of( sub { local @ARGV = ('install'); do "Build"; } )); ok(-e "$installdest/etc/simple/config", "installed etc/config"); ok(-e "$installdest/share/simple/data", "installed share/data"); ok(-e "$installdest/share/simple/html/index.html", "installed share/html"); $dist->remove(); done_testing();
Thank you for the test case. I've modified it slightly, marked the failures as TODO and added it to the repository trunk. That will definitely help in considering the right fix. -- David
Patched in trunk. It was quite an extensive patch, making generation of default install paths lazy rather than on object construction. Everyone following this bug, please check out trunk and test. Subversion repo: http://svn.perl.org/modules/Module-Build/trunk Thank you. -- David