Hello,
sekmadienis 07 Gruodis 2008, Raphael Hertzog rašė:
> > +sub merge_from_symfile {
> > + my ($self, $src) = @_;
> > + while (($soname, $srcobj) = each(%{$src->{objects}})) {
> > + if (exists $self->{objects}{$soname}) {
> > + # Update/override infos only
> > + $self->{objects}{$soname}{deps} = $srcobj->{deps};
>
> I don't think it makes sense to update the dependency if we already have
> the object in the symfile. If the library is described in 2 symbols files,
> either it's wrong or one of the two should take precedence, using the
> symbol list of one and the dependencies of the second seems wrong.
>
> I don't think that we will encounter such cases in practice, hence I would
> suggest to just display a warning stating that we have two conflicting
> objects for the same library. Or it means that we're trying to merge
> several times the same symfile.
>
> > + } else {
> > + # Shallow copy the soname object (because deps can be replaced
> > later) + my %obj;
> > + while (($key, $val) = each(%$srcobj)) {
> > + $obj{$key} = $val;
> > + }
> > + $self->{objects}{$soname} = \%obj;
> > + }
> > + }
>
> Taking into account the previous comment, this "shallow copy" is probably
> not needed.
I can't tell for sure because it has been so long since I wrote the patch, but
I think I wrote this part due to {$pkg} key which I used in the previous patch
version ($symfile is created before the 'foreach $pkg (@{$file2pkg->{$lib}}'
loop at line 196, so this merging thing sort of made sense back then. Actually
this loop confused and is still confusing me a lot). When I fixed the key to
{$dpkg_symfile}, I simply forgot to remove this. So I agree it does not make
sense now.
> There's a possibility we're going to merge the same file multiple times
> but it's not big deal (except if we display an ugly warning). To avoid
> doing too much we could however restrict the merge to only what is of
> interest to us: the infos describing the library of soname $soname.
I disagree about the warning. It is possible to merge the same symbols file
multiple time in a very normal case, i.e. when the library package contains >2
libraries and the executable depends on both of them. There should be no such
warning. However, I agree that it is fine to do something like:
$symfile->{objects}{$soname} = $dpkg_symfile_cache{$dpkg_symfile}{objects}
{$soname}
instead of
$symfile->merge_from_symfile($dpkg_symfile_cache{$dpkg_symfile})
> $dpkg_objdump_cache{$lib} => $libobj
Sure.
> > sub symfile_has_soname {
> > my ($file, $soname) = @_;
> > +
> > + return 1 if (exists $symfile_has_soname_cache{$file}{$soname});
> > +
> > open(SYM_FILE, "<", $file) ||
> > syserr(_g("cannot open file %s"), $file);
> > my $result = 0;
> > while (<SYM_FILE>) {
> > if (/^\Q$soname\E /) {
> > $result = 1;
> > + $symfile_has_soname_cache{$file}{$soname} = 1;
> > last;
> > }
> > }
>
> Why are you not caching a negative result as well? it's not more
> complicated…
Stupid me. While writing this part right before I sent this new version of the
patch, I was thinking that symbols file could reappear in the process (i.e. I
was thinking about dpkg-gensymbols). So, yeah, caching negative result is fine
and should be done.
> Otherwise it's fine and I will happily apply it to the master branch. Tell
> me if you plan to submit an updated patch or if I should finish the work
> myself.
Feel free to fix the problems with the patch and commit it. Then me know and
I'll post my comments if I have any.
--
Modestas Vainius <modestas@vainius.eu>
Attachment:
signature.asc
Description: This is a digitally signed message part.