Linux Archive

Linux Archive (http://www.linux-archive.org/)
-   Debian dpkg (http://www.linux-archive.org/debian-dpkg/)
-   -   Google Code In, Task Implementing a dpkg-dev feature (instance #01) (http://www.linux-archive.org/debian-dpkg/463001-google-code-task-implementing-dpkg-dev-feature-instance-01-a.html)

Raphael Hertzog 12-08-2010 01:13 PM

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').
" .
> - "Note: libraries are not searched in other binary packages " .
> - "that do not have any shlibs or symbols file.
To 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
" 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').
" .
> + "Note: libraries are not searched in other binary packages " .
> + "that do not have any shlibs or symbols file.
To 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
" 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)


--
To UNSUBSCRIBE, email to debian-dpkg-REQUEST@lists.debian.org
with a subject of "unsubscribe". Trouble? Contact listmaster@lists.debian.org
Archive: 20101208141307.GD8445@rivendell.home.ouaza.com">ht tp://lists.debian.org/20101208141307.GD8445@rivendell.home.ouaza.com

Chris Baines 12-08-2010 04:22 PM

Google Code In, Task Implementing a dpkg-dev feature (instance #01)
 
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').
" .
> > - "Note: libraries are not searched in other binary packages " .
> > - "that do not have any shlibs or symbols file.
To 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
" 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').
" .
> > + "Note: libraries are not searched in other binary packages " .
> > + "that do not have any shlibs or symbols file.
To 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
" 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

Raphael Hertzog 12-09-2010 02:23 PM

Google Code In, Task Implementing a dpkg-dev feature (instance #01)
 
Hi,

On Wed, 08 Dec 2010, Chris Baines wrote:
> > 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 I merged it!

http://git.debian.org/?p=dpkg/dpkg.git;a=commitdiff;h=4105379e2532d68579cfb1d95e b49205414507db

I made a supplementary change to prettify the output with multiple errors:
http://git.debian.org/?p=dpkg/dpkg.git;a=commitdiff;h=f3fa58a3317e3456bcfb490aae 66fa4371949fae

Cheers,
--
Raphaël Hertzog ◈ Debian Developer

Follow my Debian News ▶ http://RaphaelHertzog.com (English)
▶ http://RaphaelHertzog.fr (Français)


--
To UNSUBSCRIBE, email to debian-dpkg-REQUEST@lists.debian.org
with a subject of "unsubscribe". Trouble? Contact listmaster@lists.debian.org
Archive: 20101209152318.GA22953@rivendell.home.ouaza.com">h ttp://lists.debian.org/20101209152318.GA22953@rivendell.home.ouaza.com


All times are GMT. The time now is 03:29 AM.

VBulletin, Copyright ©2000 - 2014, Jelsoft Enterprises Ltd.
Content Relevant URLs by vBSEO ©2007, Crawlability, Inc.