Hi, I have now subscribed to the debian-dpkg list so we can communicate directly through the list.
> 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.
Sorry, using gedit, I think I have fixed this now.
> 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).
Done.
> > - 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.
I have removed my changes to the flow. I just like using stacked if
commands instead of "next;" to perform flow control, as I think its
neater and clearer.
> > + $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;
Done.
> > + } 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.
I half achieved it, I was testing my changes using one binary, so I saw
more errors, but not as many as I should have. I have now moved the
$error_count variable to before the loop, and the failure to after the
main loop, and after the package level warning code.
> 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();
Done.
> I hope you can provide an updated patch based on my comments.
>
> Did you try your change by the way?
Yes, I tested it by using a binary for the fgrun program, with and
without the required simgear libraries.
I have attached the updated patch file.
Thanks,
Chris
--- dpkg-shlibdeps-old.pl 2010-12-08 17:12:10.000000000 +0000
+++ dpkg-shlibdeps.pl 2010-12-08 17:11:03.000000000 +0000
@@ -150,6 +150,9 @@
my %objdump_cache;
my %symfile_has_soname_cache;
+# Used to count errors due to missing libraries
+my $error_count = 0;
+
my $cur_field;
foreach my $file (keys %exec) {
$cur_field = $exec{$file};
@@ -173,7 +176,8 @@
"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}}));
+ errormsg($msg, $soname, $file, $obj->{format}, join(":", @{$obj->{RPATH}}));
+ $error_count++;
} else {
warning($msg, $soname, $file, $obj->{format}, join(":", @{$obj->{RPATH}}));
}
@@ -423,6 +427,13 @@
}
}
+# Quit now if any missing libraries
+if ($error_count >= 1) {
+ error(P_("Cannot continue due to the error above.",
+ "Cannot continue due to the errors listed above.",
+ $error_count));
+}
+
# Open substvars file
my $fh;
if ($stdout) {
signature.asc
Description: This is a digitally signed message part
