Thanks for the additional details. Find attached a patch which splits the functionality into the new dh_missing helper.
Aside from the fact that documentation is still missing, what are your thoughts on that patch? Once documentation is added, could we merge it, then change dh_installman and dh_installdocs? Thanks! On Sun, Mar 26, 2017 at 10:13 AM, Niels Thykier <[email protected]> wrote: > Michael Stapelberg: > > On Thu, Mar 23, 2017 at 9:43 PM, Niels Thykier <[email protected]> > wrote: > > > >> Michael Stapelberg: > >>> Sorry for the late reply. > >>> > >> > >> Hi again, :) > >> > >>> [...] > >>> > >>> By “defer it”, what exactly do you mean? Checking for files which were > >> not > >>> installed? > >>> > >> > >> Put the logic into a separate helper, which has - as sole purpose - the > >> responsibility for doing "list/fail-on-missing" checks. > >> > > > > Thanks for clarifying. Would dh_missing be an okay name? > > > > I believe it would. I did think of a couple of alternatives as well: > * dh_missing > * dh_check-missing > * dh_check-not-installed > > (The latter due to the "config" file being called "debian/not-installed"). > > If you write the tool, you get to decide the name :) (within reason :P) > > > > >> > >>> > >>> [...] > >>> > >> > >> To begin with, I think I would just log the original pattern (for source > >> files) and let this new helper "re-expand" it. If performance becomes > >> an issue, we can revisit that decision. The simpler it is to retro-fit > >> this new functionality into an existing helper, the easier we can have > >> "third-party" tools adopt it. :) > >> > > > > Makes sense. > > > > I looked at dh_installdocs and it’s actually way more complicated than I > > imagined. > > > > The current check is only looking at "SOURCEDIR" (with "." converted > into "debian/tmp"). As I recall, dh_installdocs pulls its source files > from "." and "debian" so it is should not be affected (at lot at least). > > > It’s too bad we don’t have a point through which all files must pass > (like > > install_file), otherwise the change would be simple. > > > > Indeed. In fact, invented install_file (et al) is a recent invention to > prepare for things like this. Although it had less coverage than I had > hoped. > > > Would it be okay to convert the helpers one-by-one to use the new > > mechanism? If not, I don’t think I have enough motivation to actually > > update all dh_* programs to log their files. > > > > [...] > > Yes, provided that the conversion is backwards compatible. > > I think dh_install and dh_installman would be the two starting points. > Once we have those, we can enable the new tool in compat 11 and see how > it works (plus do some incremental improvements from there). > > Thanks, > ~Niels > > > > -- Best regards, Michael
From e41b275eb7e2f5ccd49886570edebd5d4c4b4c40 Mon Sep 17 00:00:00 2001 From: Michael Stapelberg <[email protected]> Date: Sun, 26 Mar 2017 14:20:02 +0200 Subject: [PATCH] =?UTF-8?q?Move=20dh=5Finstall=E2=80=99s=20--{list,fail}-m?= =?UTF-8?q?issing=20to=20dh=5Fmissing?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit This commit splits dh_install’s --list-missing and --fail-missing functionality into the separate helper dh_missing. To make this work, dh_install logs its source patterns and dh_missing reads these patterns, treating them as installed. This allows us to address #415396, i.e. recognize files installed by other helpers (e.g. dh_installman) as installed. --- Debian/Debhelper/Dh_Lib.pm | 14 ++++++++ dh_install | 47 ++---------------------- dh_missing | 80 +++++++++++++++++++++++++++++++++++++++++ t/dh_missing/Makefile | 6 ++++ t/dh_missing/debian/changelog | 5 +++ t/dh_missing/debian/compat | 1 + t/dh_missing/debian/control | 20 +++++++++++ t/dh_missing/debian/foo.install | 1 + t/dh_missing/dh_missing.t | 50 ++++++++++++++++++++++++++ t/dh_missing/file-for-foo | 1 + 10 files changed, 181 insertions(+), 44 deletions(-) create mode 100755 dh_missing create mode 100644 t/dh_missing/Makefile create mode 100644 t/dh_missing/debian/changelog create mode 100644 t/dh_missing/debian/compat create mode 100644 t/dh_missing/debian/control create mode 100644 t/dh_missing/debian/foo.install create mode 100755 t/dh_missing/dh_missing.t create mode 100644 t/dh_missing/file-for-foo diff --git a/Debian/Debhelper/Dh_Lib.pm b/Debian/Debhelper/Dh_Lib.pm index f4b9ca26..72448058 100644 --- a/Debian/Debhelper/Dh_Lib.pm +++ b/Debian/Debhelper/Dh_Lib.pm @@ -50,6 +50,7 @@ use vars qw(@EXPORT %dh); &generated_file &autotrigger &package_section &restore_file_on_clean &restore_all_files &open_gz &reset_perm_and_owner &deprecated_functionality + &log_installed_files ); # The Makefile changes this if debhelper is installed in a PREFIX. @@ -1455,6 +1456,19 @@ sub deprecated_functionality { return 1; } +sub log_installed_files { + my ($name, $package, @patterns) = @_; + + my $log = generated_file($package, 'installed-by-' . $name); + open(my $fh, '>', $log); + for my $src (@patterns) { + print $fh "$src\n"; + } + close($fh); + + return 1; +} + 1 # Local Variables: diff --git a/dh_install b/dh_install index 205d4701..a062f9cc 100755 --- a/dh_install +++ b/dh_install @@ -133,8 +133,6 @@ init(options => { "sourcedir=s" => \$dh{SOURCEDIR}, }); -my @installed; - my $srcdir = '.'; $srcdir = $dh{SOURCEDIR} if defined $dh{SOURCEDIR}; @@ -143,6 +141,8 @@ my $missing_files = 0; # PROMISE: DH NOOP WITHOUT install foreach my $package (getpackages()) { + my @installed; + # Look at the install files for all packages to handle # list-missing/fail-missing, but skip really installing for # packages that are not being acted on. @@ -252,51 +252,10 @@ foreach my $package (getpackages()) { } } } -} -if ($dh{LIST_MISSING} || $dh{FAIL_MISSING}) { - # . as srcdir makes no sense, so this is a special case. - if ($srcdir eq '.') { - $srcdir='debian/tmp'; - } - - my @missing; - if ( -f 'debian/not-installed') { - my @not_installed = filearray('debian/not-installed'); - foreach (@not_installed) { - s:^\s*:debian/tmp/: unless m:^\s*debian/tmp/:; - } - # Pretend that these are also installed. - push(@installed, @not_installed); - } - my $installed=join("|", map { - # Kill any extra slashes, for robustness. - y:/:/:s; - s:/+$::; - s:^(\./)*::; - "\Q$_\E\/.*|\Q$_\E"; - } @installed); - $installed=qr{^($installed)$}; - find(sub { - -f || -l || return; - $_="$File::Find::dir/$_"; - if (! /$installed/ && ! excludefile($_)) { - my $file=$_; - $file=~s/^\Q$srcdir\E\///; - push @missing, $file; - } - }, $srcdir); - if (@missing) { - warning "$_ exists in $srcdir but is not installed to anywhere" foreach @missing; - if ($dh{FAIL_MISSING}) { - error("missing files, aborting"); - } - } + log_installed_files('dh_install', $package, @installed); } -if ($missing_files) { - error("missing files, aborting"); -} =head1 LIMITATIONS diff --git a/dh_missing b/dh_missing new file mode 100755 index 00000000..ed3d547e --- /dev/null +++ b/dh_missing @@ -0,0 +1,80 @@ +#!/usr/bin/perl + +use strict; +use warnings; +use File::Find; +use Debian::Debhelper::Dh_Lib; + +init(options => { + "list-missing" => \$dh{LIST_MISSING}, + "fail-missing" => \$dh{FAIL_MISSING}, +}); + +my @installed; + +my $srcdir = '.'; +$srcdir = $dh{SOURCEDIR} if defined $dh{SOURCEDIR}; + +my $missing_files = 0; + +if (!$dh{LIST_MISSING} && !$dh{FAIL_MISSING}) { + exit 0; +} + +# . as srcdir makes no sense, so this is a special case. +if ($srcdir eq '.') { + $srcdir='debian/tmp'; +} + +for my $file (<debian/.debhelper/generated/*/installed-by-*>) { + local $/ = undef; + open(my $fh, '<', $file) or die "could not open $file: $!"; + for my $line (split(/\n/, <$fh>)) { + push(@installed, $line); + } + close($fh); +} + +my @missing; +if ( -f 'debian/not-installed') { + my @not_installed = filearray('debian/not-installed'); + foreach (@not_installed) { + s:^\s*:debian/tmp/: unless m:^\s*debian/tmp/:; + } + # Pretend that these are also installed. + push(@installed, @not_installed); +} +my $installed=join("|", map { + # Kill any extra slashes, for robustness. + y:/:/:s; + s:/+$::; + s:^(\./)*::; + "\Q$_\E\/.*|\Q$_\E"; +} @installed); +$installed=qr{^($installed)$}; +find(sub { + -f || -l || return; + $_="$File::Find::dir/$_"; + if (! /$installed/ && ! excludefile($_)) { + my $file=$_; + $file=~s/^\Q$srcdir\E\///; + push @missing, $file; + } +}, $srcdir); +if (@missing) { + warning "$_ exists in $srcdir but is not installed to anywhere" foreach @missing; + if ($dh{FAIL_MISSING}) { + error("missing files, aborting"); + } +} + +if ($missing_files) { + error("missing files, aborting"); +} + + +# Local Variables: +# indent-tabs-mode: t +# tab-width: 4 +# cperl-indent-level: 4 +# End: diff --git a/t/dh_missing/Makefile b/t/dh_missing/Makefile new file mode 100644 index 00000000..679592a7 --- /dev/null +++ b/t/dh_missing/Makefile @@ -0,0 +1,6 @@ +install: + install -m 755 -d debian/tmp/usr/bin + install -m 644 file-for-foo debian/tmp/usr/bin/file-for-foo + +installmore: install + install -m 644 file-for-foo debian/tmp/usr/bin/file-for-foo-more diff --git a/t/dh_missing/debian/changelog b/t/dh_missing/debian/changelog new file mode 100644 index 00000000..5850f0e2 --- /dev/null +++ b/t/dh_missing/debian/changelog @@ -0,0 +1,5 @@ +foo (1.0-1) unstable; urgency=low + + * Initial release. (Closes: #XXXXXX) + + -- Test <testing@nowhere> Mon, 11 Jul 2016 18:10:59 +0200 diff --git a/t/dh_missing/debian/compat b/t/dh_missing/debian/compat new file mode 100644 index 00000000..f599e28b --- /dev/null +++ b/t/dh_missing/debian/compat @@ -0,0 +1 @@ +10 diff --git a/t/dh_missing/debian/control b/t/dh_missing/debian/control new file mode 100644 index 00000000..48d4de2f --- /dev/null +++ b/t/dh_missing/debian/control @@ -0,0 +1,20 @@ +Source: foo +Section: misc +Priority: optional +Maintainer: Test <testing@nowhere> +Standards-Version: 3.9.8 + +Package: foo +Architecture: all +Description: package foo + Package foo + +Package: bar +Architecture: all +Description: package bar + Package bar + +Package: baz +Architecture: all +Description: package baz + Package baz diff --git a/t/dh_missing/debian/foo.install b/t/dh_missing/debian/foo.install new file mode 100644 index 00000000..eddea57c --- /dev/null +++ b/t/dh_missing/debian/foo.install @@ -0,0 +1 @@ +usr/bin/*-for-foo \ No newline at end of file diff --git a/t/dh_missing/dh_missing.t b/t/dh_missing/dh_missing.t new file mode 100755 index 00000000..aa59ec67 --- /dev/null +++ b/t/dh_missing/dh_missing.t @@ -0,0 +1,50 @@ +#!/usr/bin/perl +use strict; +use Test::More; +use File::Basename (); + +# Let the tests be run from anywhere, but current directory +# is expected to be the one where this test lives in. +chdir File::Basename::dirname($0) or die "Unable to chdir to ".File::Basename::dirname($0); + +my $TOPDIR = "../.."; +my $rootcmd; + +if ($< == 0) { + $rootcmd = ''; +} +else { + system("fakeroot true 2>/dev/null"); + $rootcmd = $? ? undef : 'fakeroot'; +} + +if (not defined($rootcmd)) { + plan skip_all => 'fakeroot required'; +} +else { + plan(tests => 4); +} + +# Verify dh_missing does not fail when all files are installed. +system("$TOPDIR/dh_clean"); +system("$rootcmd make install"); +system("$rootcmd $TOPDIR/dh_install"); +is(system("$rootcmd $TOPDIR/dh_missing --fail-missing"), 0, 'dh_missing failed'); + +# Verify dh_missing does fail when not all files are installed. +system("$rootcmd $TOPDIR/dh_clean"); +system("$rootcmd make installmore"); +system("$rootcmd $TOPDIR/dh_install"); +system("$rootcmd $TOPDIR/dh_missing --fail-missing"); +isnt($?, -1, 'dh_missing was executed'); +ok(! ($? & 127), 'dh_missing did not die due to a signal'); +my $exitcode = ($? >> 8); +is($exitcode, 2, 'dh_missing exited with exit code 2'); + +system("$rootcmd $TOPDIR/dh_clean"); + +# Local Variables: +# indent-tabs-mode: t +# tab-width: 4 +# cperl-indent-level: 4 +# End: diff --git a/t/dh_missing/file-for-foo b/t/dh_missing/file-for-foo new file mode 100644 index 00000000..8773f398 --- /dev/null +++ b/t/dh_missing/file-for-foo @@ -0,0 +1 @@ +file content \ No newline at end of file -- 2.11.0
