Re: Google Code In, Task Implementing a dpkg-dev feature (instance #01)
Hi,
sorry for the delay of my answer. I also put debian-dpkg@lists.debian.org
in copy as that's the list we use to review proposed changes.
On Mon, 06 Dec 2010, Chris Baines wrote:
> I recently requested the task listed in the subject for the Google Code
> In. My request has been accepted by one of your colleagues. I would like
> to if possible complete this bug 596841.
OK, looks a good choice.
> I have attached an initial proposal that works, though as my Perl coding
> experience is very low it might not be the best way to approach the
> problem.
Next time, please submit a patch generated with "diff -u" (or directly git
diff), it's easier to review.
There are several problems with your patch. Let's go through them:
> diff --git a/scripts/dpkg-shlibdeps.pl b/scripts/dpkg-shlibdeps.pl
> index 91ea4e5..f96669d 100755
> --- a/scripts/dpkg-shlibdeps.pl
> +++ b/scripts/dpkg-shlibdeps.pl
> @@ -163,29 +163,38 @@ foreach my $file (keys %exec) {
> my %altlibfiles;
> my %soname_notfound;
> my %alt_soname;
> +
> + # Used to remember to quit if an error
> + my $soname_error_found;
First of all, there are indentations problems. A tabulations is always
equivalent to 8 spaces. (And you seem to use a setting where a tab means
4 spaces). And due to this, you completely broke the indentation. I don't
know what editors you use, but be sure to fix this.
You might better name this variable $error_count and you need to
initialize to 0 (and this probably must be done outside the main loop
(see comments about where you return the failure).
> foreach my $soname (@sonames) {
> - my $lib = my_find_library($soname, $obj->{RPATH}, $obj->{format}, $file);
This line has not been modified except for the indentation. Always avoid
spurious spacing changes.
> - unless (defined $lib) {
> - $soname_notfound{$soname} = 1;
> - $global_soname_notfound{$soname} = 1;
> - my $msg = _g("couldn't find library %s needed by %s (ELF format: '%s'; RPATH: '%s').\n" .
> - "Note: libraries are not searched in other binary packages " .
> - "that do not have any shlibs or symbols file.\nTo help dpkg-shlibdeps " .
> - "find private libraries, you might need to set LD_LIBRARY_PATH.");
> - if (scalar(split_soname($soname))) {
> - error($msg, $soname, $file, $obj->{format}, join(":", @{$obj->{RPATH}}));
> - } else {
> - warning($msg, $soname, $file, $obj->{format}, join(":", @{$obj->{RPATH}}));
> + my $lib = my_find_library($soname, $obj->{RPATH}, $obj->{format}, $file);
> + if (defined $lib) {
You changed an "unless" test in a "if" test. You reordered two
blocks of code without a good reason. It makes it difficult to see what is
the core of your change. If you want to make miscellaneous improvements
while fixing the bug, please create a dedicated git branch and commit each
change separately so that it can be better reviewed.
What I mean is that we had roughly:
unless (foo) {
do_a;
next;
}
do_b;
And you turned it into:
if (!foo) {
do_b;
} else {
do_a;
}
They are equivalent (in terms of result and what's executed) but those
changes are useless. Please do the minimal changes.
> + $libfiles{$lib} = $soname;
> + my $reallib = realpath($lib);
> + if ($reallib ne $lib) {
> + $altlibfiles{$reallib} = $soname;
> + }
> + print "Library $soname found in $lib\n" if $debug;
> + } else {
> + $soname_notfound{$soname} = 1;
> + $global_soname_notfound{$soname} = 1;
> + my $msg = _g("couldn't find library %s needed by %s (ELF format: '%s'; RPATH: '%s').\n" .
> + "Note: libraries are not searched in other binary packages " .
> + "that do not have any shlibs or symbols file.\nTo help dpkg-shlibdeps " .
> + "find private libraries, you might need to set LD_LIBRARY_PATH.");
> + if (scalar(split_soname($soname))) {
> + errormsg($msg, $soname, $file, $obj->{format}, join(":", @{$obj->{RPATH}}));
> + $soname_error_found = $soname_error_found + 1;
I prefer one of the shorter forms:
$soname_error_found++;
$soname_error_found += 1;
> + } else {
> + warning($msg, $soname, $file, $obj->{format}, join(":", @{$obj->{RPATH}}));
> + }
> }
> - next;
> - }
> - $libfiles{$lib} = $soname;
> - my $reallib = realpath($lib);
> - if ($reallib ne $lib) {
> - $altlibfiles{$reallib} = $soname;
> - }
> - print "Library $soname found in $lib\n" if $debug;
> }
> + if ($soname_error_found == 1) {
> + error("Cannot continue due to the error above.");
> + } elsif ($soname_error_found > 1) {
> + error("Cannot continue due to the errors listed above.");
> + }
Failing here is not the best place. You are still in the main loop
(foreach my $file (keys %exec)) and it means you still have other files to
analyze... so you will not achieve the goal of #596841 which is to see
all errors before failing. You should really move the failure outside of
the main loop (just after it) and so you need to move the variable
counting the errors outside of the loop scope as well.
The error message here must be translated and you should use ngettext
to retrieve the correct error message (ngettext is wrapped in P_()
by Dpkg::Gettext):
P_("Cannot continue due to the error above.",
"Cannot continue due to the errors listed above.",
$soname_error_found);
> my $file2pkg = find_packages(keys %libfiles, keys %altlibfiles);
> my $symfile = Dpkg::Shlibs::SymbolFile->new();
> my $dumplibs_wo_symfile = Dpkg::Shlibs::Objdump->new();
I hope you can provide an updated patch based on my comments.
Did you try your change by the way?
Cheers,
--
Raphaël Hertzog ◈ Debian Developer
Follow my Debian News ▶ http://RaphaelHertzog.com (English)
▶ http://RaphaelHertzog.fr (Français)
Reply to: