| Message ID | 20170810123022.6DB7B439942E1@oldenburg.str.redhat.com |
|---|---|
| State | New |
| Headers | show |
On 08/10/2017 02:30 PM, Florian Weimer wrote: > The warning looked like this: > > Use of uninitialized value in string ne at > …/scripts/test-installation.pl line 184, <LDD> line 24. > > It is triggered by this line of ldd output: > > libfreebl3.so => /lib64/libfreebl3.so (0x00007f055003c000) > > The other lines have a version in the soname: > > libanl.so.1 => /lib64/libanl.so.1 (0x00007f055023f000) > > 2017-08-10 Florian Weimer<fweimer@redhat.com> > > [BZ #21940] > * scripts/test-installation.pl: Handle NSS crypto libaries in ldd > output. Ping? <https://sourceware.org/ml/libc-alpha/2017-08/msg00391.html> Thanks, Florian
On 08/10/2017 05:30 AM, Florian Weimer wrote: > The warning looked like this: > > Use of uninitialized value in string ne at > …/scripts/test-installation.pl line 184, <LDD> line 24. > > It is triggered by this line of ldd output: > > libfreebl3.so => /lib64/libfreebl3.so (0x00007f055003c000) > > The other lines have a version in the soname: > > libanl.so.1 => /lib64/libanl.so.1 (0x00007f055023f000) > > 2017-08-10 Florian Weimer <fweimer@redhat.com> > > [BZ #21940] > * scripts/test-installation.pl: Handle NSS crypto libaries in ldd > output. > > diff --git a/scripts/test-installation.pl b/scripts/test-installation.pl > index 4b0e9f3c4a..466c526cc9 100755 > --- a/scripts/test-installation.pl > +++ b/scripts/test-installation.pl > @@ -177,10 +177,15 @@ open LDD, "ldd /tmp/test-prg$$ |" > or die ("Couldn't execute ldd"); > while (<LDD>) { > if (/^\s*lib/) { > + # When libcrypt is linked against NSS, some of the referenced > + # libraries do not have a trailing version in their soname. > ($name, $version1, $version2) = > - /^\s*lib(\w*)\.so\.([0-9\.]*)\s*=>.*\.so\.([0-9\.]*)/; > + /^\s*lib(\w*)\.so(?:\.([0-9\.]*))?\s*=>.*\.so(?:\.([0-9\.]*))?/; I would use: /^\s*lib(\w+)\.so(?:\.([0-9\.]+))?\s*=>.*\.so(?:\.([0-9\.]+))?/ ^ ^ ^ to avoid matching "lib.so.". > $found{$name} = 1; > - if ($versions{$name} ne $version1 || $version1 ne $version2) { > + if (defined($version1) != defined($version2) > + || defined($version1) != defined($versions{$name}) > + || (defined($versions{$name}) > + && ($versions{$name} ne $version1 || $version1 ne $version2))) { > print "Library lib$name is not correctly installed.\n"; > print "Please check your installation!\n"; > print "Offending line of ldd output: $_\n"; It might help readability to follow up the match with something like: next if ! (defined($name) && defined($version1) && defined($version2)); next if ! (exists $versions{$name}) && defined($versions{$name})); (I seem to recall tests on hash entries creating them when they didn't previously exist, but that may not matter here. Something other than `next' may also be desirable, and the opportunity for more fine-grained error messages is introduced, if useful.) Then you could retain the simpler conditional: if ($versions{$name} ne $version1 || $version1 ne $version2) { That also continues using string comparisons, which would be better in case there are multiple "."'s. NSS installs *.so files on my system, but I do have programs that link against libraries with multiple version points; e.g.: $ ldd /usr/bin/avprobe | grep bz2 libbz2.so.1.0 => /usr/lib/libbz2.so.1.0 (0x00007f869fab8000) Rical
On 10/07/2017 12:50 PM, Rical Jasan wrote: > I would use: > > /^\s*lib(\w+)\.so(?:\.([0-9\.]+))?\s*=>.*\.so(?:\.([0-9\.]+))?/ > ^ ^ ^ > to avoid matching "lib.so.". I made the change in the attached patch. >> $found{$name} = 1; >> - if ($versions{$name} ne $version1 || $version1 ne $version2) { >> + if (defined($version1) != defined($version2) >> + || defined($version1) != defined($versions{$name}) >> + || (defined($versions{$name}) >> + && ($versions{$name} ne $version1 || $version1 ne $version2))) { >> print "Library lib$name is not correctly installed.\n"; >> print "Please check your installation!\n"; >> print "Offending line of ldd output: $_\n"; > > It might help readability to follow up the match with something like: > > next if ! (defined($name) && defined($version1) && defined($version2)); > next if ! (exists $versions{$name}) && defined($versions{$name})); Is this really clearer? What I wanted to express is “error if the defined-ness state is different across the three value, and if $versions{$name} is defined, it must much both versions”. The original condition captures this fairly succinctly. > (I seem to recall tests on hash entries creating them when they didn't > previously exist, but that may not matter here. Something other than > `next' may also be desirable, and the opportunity for more fine-grained > error messages is introduced, if useful.) I suspect the original script was written for Perl 4, which did not have the exists operator. I instinctively stuck to that baseline. Anyway, I came up with something simpler, which sidesteps these issues. > Then you could retain the simpler conditional: > > if ($versions{$name} ne $version1 || $version1 ne $version2) { > > That also continues using string comparisons, which would be better in > case there are multiple "."'s. I only used != as the Boolean XOR operator on the numeric result from the defined operator. Thanks, Florian The warning looked like this: Use of uninitialized value in string ne at …/scripts/test-installation.pl line 184, <LDD> line 24. It is triggered by this line of ldd output: libfreebl3.so => /lib64/libfreebl3.so (0x00007f055003c000) The other lines have a version in the soname: libanl.so.1 => /lib64/libanl.so.1 (0x00007f055023f000) 2017-10-10 Florian Weimer <fweimer@redhat.com> [BZ #21940] * scripts/test-installation.pl: Handle NSS crypto libaries in ldd output. diff --git a/scripts/test-installation.pl b/scripts/test-installation.pl index 74d25e1c8d..7eaeb1ae06 100755 --- a/scripts/test-installation.pl +++ b/scripts/test-installation.pl @@ -176,10 +176,17 @@ open LDD, "ldd /tmp/test-prg$$ |" or die ("Couldn't execute ldd"); while (<LDD>) { if (/^\s*lib/) { - ($name, $version1, $version2) = - /^\s*lib(\w*)\.so\.([0-9\.]*)\s*=>.*\.so\.([0-9\.]*)/; + # When libcrypt is linked against NSS, some of the referenced + # libraries do not have a trailing version in their soname. + my ($name, $version1, $version2) = + /^\s*lib(\w+)\.so(?:\.([0-9\.]+))?\s*=>.*\.so(?:\.([0-9\.]+))?/; $found{$name} = 1; - if ($versions{$name} ne $version1 || $version1 ne $version2) { + # Version strings are either undefined or non-empty. + $version1 = '' unless defined $version1; + $version2 = '' unless defined $version2; + my $vername = $versions{$name}; + $vername = '' unless defined $vername; + if ($version1 ne $version2 || $vername ne $version1) { print "Library lib$name is not correctly installed.\n"; print "Please check your installation!\n"; print "Offending line of ldd output: $_\n";
On 10/10/2017 02:26 AM, Florian Weimer wrote: > On 10/07/2017 12:50 PM, Rical Jasan wrote: > >> I would use: >> >> /^\s*lib(\w+)\.so(?:\.([0-9\.]+))?\s*=>.*\.so(?:\.([0-9\.]+))?/ >> ^ ^ ^ >> to avoid matching "lib.so.". > > I made the change in the attached patch. > >>> $found{$name} = 1; >>> - if ($versions{$name} ne $version1 || $version1 ne $version2) { >>> + if (defined($version1) != defined($version2) >>> + || defined($version1) != defined($versions{$name}) >>> + || (defined($versions{$name}) >>> + && ($versions{$name} ne $version1 || $version1 ne $version2))) { >>> >>> print "Library lib$name is not correctly installed.\n"; >>> print "Please check your installation!\n"; >>> print "Offending line of ldd output: $_\n"; >> >> It might help readability to follow up the match with something like: >> >> next if ! (defined($name) && defined($version1) && defined($version2)); >> next if ! (exists $versions{$name}) && defined($versions{$name})); > > Is this really clearer? What I wanted to express is “error if the > defined-ness state is different across the three value, and if > $versions{$name} is defined, it must much both versions”. The original > condition captures this fairly succinctly. Converting to a natural language expression was exactly the trouble I had. I've attached an alternative proposal. I'm not sure if the "Unexpected dependency" message is correct, but the existence/defined-ness check on $versions{$name} seems the important bit, which makes the subsequent transitive version tests safe. My system has unversioned NSS libraries, but I don't have a sandbox handy to run a `make install' (which is when it looks like this script is run), so it's untested. >> (I seem to recall tests on hash entries creating them when they didn't >> previously exist, but that may not matter here. Something other than >> `next' may also be desirable, and the opportunity for more fine-grained >> error messages is introduced, if useful.) > > I suspect the original script was written for Perl 4, which did not have > the exists operator. I instinctively stuck to that baseline. This script definitely wouldn't pass a compilation check using the strict pragma; I noticed you slipped in a `my', but the whole thing suffers from the same scoping issue. I see install.texi says Perl 5 is recommended, and I noticed perl(1) says: Using the "use strict" pragma ensures that all variables are properly declared and prevents other misuses of legacy Perl features. I can update the script (and others) to specifically address such "miuses" in a separate patchset, if it's considered worthwhile. Rical > Anyway, I came up with something simpler, which sidesteps these issues. > >> Then you could retain the simpler conditional: >> >> if ($versions{$name} ne $version1 || $version1 ne $version2) { >> >> That also continues using string comparisons, which would be better in >> case there are multiple "."'s. > > I only used != as the Boolean XOR operator on the numeric result from > the defined operator. > > Thanks, > Florian > > bug21940.patch > > > The warning looked like this: > > Use of uninitialized value in string ne at > …/scripts/test-installation.pl line 184, <LDD> line 24. > > It is triggered by this line of ldd output: > > libfreebl3.so => /lib64/libfreebl3.so (0x00007f055003c000) > > The other lines have a version in the soname: > > libanl.so.1 => /lib64/libanl.so.1 (0x00007f055023f000) > > 2017-10-10 Florian Weimer <fweimer@redhat.com> > > [BZ #21940] > * scripts/test-installation.pl: Handle NSS crypto libaries in ldd > output. > > diff --git a/scripts/test-installation.pl b/scripts/test-installation.pl > index 74d25e1c8d..7eaeb1ae06 100755 > --- a/scripts/test-installation.pl > +++ b/scripts/test-installation.pl > @@ -176,10 +176,17 @@ open LDD, "ldd /tmp/test-prg$$ |" > or die ("Couldn't execute ldd"); > while (<LDD>) { > if (/^\s*lib/) { > - ($name, $version1, $version2) = > - /^\s*lib(\w*)\.so\.([0-9\.]*)\s*=>.*\.so\.([0-9\.]*)/; > + # When libcrypt is linked against NSS, some of the referenced > + # libraries do not have a trailing version in their soname. > + my ($name, $version1, $version2) = > + /^\s*lib(\w+)\.so(?:\.([0-9\.]+))?\s*=>.*\.so(?:\.([0-9\.]+))?/; > $found{$name} = 1; > - if ($versions{$name} ne $version1 || $version1 ne $version2) { > + # Version strings are either undefined or non-empty. > + $version1 = '' unless defined $version1; > + $version2 = '' unless defined $version2; > + my $vername = $versions{$name}; > + $vername = '' unless defined $vername; > + if ($version1 ne $version2 || $vername ne $version1) { > print "Library lib$name is not correctly installed.\n"; > print "Please check your installation!\n"; > print "Offending line of ldd output: $_\n"; diff --git a/scripts/test-installation.pl b/scripts/test-installation.pl index 45c666b0a2..fcb7707889 100755 --- a/scripts/test-installation.pl +++ b/scripts/test-installation.pl @@ -171,14 +171,20 @@ if ($?) { $ok = 1; %found = (); +$librgx = qr/lib(\w+)\.so(?:\.([0-9\.]+))?/; open LDD, "ldd /tmp/test-prg$$ |" or die ("Couldn't execute ldd"); while (<LDD>) { - if (/^\s*lib/) { + if (m,^\s*${librgx}\s*=>\s*(?:/[^/]+)*/${librgx},) { ($name, $version1, $version2) = - /^\s*lib(\w*)\.so\.([0-9\.]*)\s*=>.*\.so\.([0-9\.]*)/; + ($1, defined($2) ? $2 : '', defined($4) ? $4 : ''); $found{$name} = 1; + if (!exists $versions{$name}) { + print "Unexpected dependency: lib$name\n"; + $ok = 0; + next; + } if ($versions{$name} ne $version1 || $version1 ne $version2) { print "Library lib$name is not correctly installed.\n"; print "Please check your installation!\n";
On 10/18/2017 07:06 AM, Rical Jasan wrote: > diff --git a/scripts/test-installation.pl b/scripts/test-installation.pl > index 45c666b0a2..fcb7707889 100755 > --- a/scripts/test-installation.pl > +++ b/scripts/test-installation.pl > @@ -171,14 +171,20 @@ if ($?) { > > $ok = 1; > %found = (); > +$librgx = qr/lib(\w+)\.so(?:\.([0-9\.]+))?/; > > open LDD, "ldd /tmp/test-prg$$ |" > or die ("Couldn't execute ldd"); > while (<LDD>) { > - if (/^\s*lib/) { > + if (m,^\s*${librgx}\s*=>\s*(?:/[^/]+)*/${librgx},) { > ($name, $version1, $version2) = > - /^\s*lib(\w*)\.so\.([0-9\.]*)\s*=>.*\.so\.([0-9\.]*)/; > + ($1, defined($2) ? $2 : '', defined($4) ? $4 : ''); > $found{$name} = 1; > + if (!exists $versions{$name}) { > + print "Unexpected dependency: lib$name\n"; > + $ok = 0; > + next; > + } This looks fine to me. Could you commit it with a ChangeLog entry? Thanks, Florian
On 12/14/2017 07:10 AM, Florian Weimer wrote: > On 10/18/2017 07:06 AM, Rical Jasan wrote: >> diff --git a/scripts/test-installation.pl b/scripts/test-installation.pl >> index 45c666b0a2..fcb7707889 100755 >> --- a/scripts/test-installation.pl >> +++ b/scripts/test-installation.pl >> @@ -171,14 +171,20 @@ if ($?) { >> $ok = 1; >> %found = (); >> +$librgx = qr/lib(\w+)\.so(?:\.([0-9\.]+))?/; >> open LDD, "ldd /tmp/test-prg$$ |" >> or die ("Couldn't execute ldd"); >> while (<LDD>) { >> - if (/^\s*lib/) { >> + if (m,^\s*${librgx}\s*=>\s*(?:/[^/]+)*/${librgx},) { >> ($name, $version1, $version2) = >> - /^\s*lib(\w*)\.so\.([0-9\.]*)\s*=>.*\.so\.([0-9\.]*)/; >> + ($1, defined($2) ? $2 : '', defined($4) ? $4 : ''); >> $found{$name} = 1; >> + if (!exists $versions{$name}) { >> + print "Unexpected dependency: lib$name\n"; >> + $ok = 0; >> + next; >> + } > > This looks fine to me. Could you commit it with a ChangeLog entry? I have the resources available to set up burner systems now, so after reacquainting myself with this patch, I set about testing it. Not only is test-installation.pl the very last thing run during `make install', but it _only_ happens when prefix=/usr and DESTDIR is not set, so even testing the patch requires bombing the system libc. The above tidbit actually makes the situation worse because instead of having some "uninitialized value" warnings printed to stderr by Perl with an otherwise successful installation, that "Unexpected dependency" check will cause `make install' to fail, at the very end, after everything has already been installed. What needs to happen is to make libfreebl3 an expected dependency, as well as allowing for unversioned sonames. I've attached a patch that works for me, but it definitely stems from hacking just enough to make it work. There are some design choices to be made, so others need to weigh in on whether they think what I did is appropriate. The process starts with shlib-versions, which defines valid versions of shared libraries, both installed by glibc as well as external dependencies. Note that in this file, versions are not prefixed by a dot (important later). The choice I made here was to simply not define a version. scripts/soversions.awk uses shlib-versions.v to create soversions.i in the build root. I'm not sure where shlib-versions.v comes from, but it is a generated file as well. I chose to tag the special case of no version with an arbitrary version of "undef" because if it is left blank at this point, the whitespace-delimited fields in soversions.i will be parsed incorrectly when generating soversions.mk in Makeconfig. Correspondingly, Makeconfig is given a special case for the "undef" version, which generates soversions.mk lines absent of any versioning. There is some interesting voodoo here with what appears to be a superfluous dot preceding the version, but I tried stripping it to condense things and wound up with a broken installation with programs that couldn't find "libc.so6" at runtime, so I was convinced I should keep my unversioned soname propagation as an independent, exceptional case. Lastly, scripts/test-installation.pl populates its %versions hash by matching lines in soversions.mk with versions prefixed with a dot, so I made the dot optional. This results in an empty string for the version of unversioned libraries, but that is specific to the library name, and should be OK. This is where we meet the above diff, which will match an unversioned library, so those empty strings will result in equality for a given library, and since an entry was defined for the library in the %versions hash, it isn't an unexpected dependency either. There is some ordering in soversions.awk that I don't quite understand the purpose of, but just appending libfreebl3 to shlib-versions seems to have worked. I'm concerned about the fact test-installation.pl is only run at the end of `make install'; if it fails at that point, you're screwed anyway, so it seems to be of limited utility. I wonder if it's possible to adapt it to run after a `make install' with prefix!=/usr or DESTDIR set. That can be a separate patch, though, and I haven't played with it. I can resubmit this to the list as an RFC if you think that would help visibility. Testing requires a special setup, and I'm not sure how much of the automated testing infrastructure out there actually exercises this script, or would be affected by the attached patch's changes. The length of the path from shlib-versions to test-installation.pl suggests to me there are other things that depend on the framework. Rical diff --git a/Makeconfig b/Makeconfig index 80c498e33b..d9e9003c4b 100644 --- a/Makeconfig +++ b/Makeconfig @@ -1110,6 +1110,8 @@ $(common-objpfx)soversions.mk: $(common-objpfx)soversions.i $(..)Makeconfig case $$number in \ [0-9]*) echo "$$lib.so-version=.$$number"; \ echo "all-sonames+=$$lib=$$lib.so\$$($$lib.so-version)";;\ + undef) echo "$$lib.so-version="; \ + echo "all-sonames+=$$lib=$$lib.so";;\ *) echo "$$lib.so-version=$$number"; \ echo "all-sonames+=$$lib=\$$($$lib.so-version)";;\ esac; \ diff --git a/scripts/soversions.awk b/scripts/soversions.awk index 247f061bc3..676c8692f6 100644 --- a/scripts/soversions.awk +++ b/scripts/soversions.awk @@ -14,6 +14,8 @@ $1 == "DEFAULT" { sub(/=.*$/, "", lib); sub(/^.*=/, "", number); if (lib in numbers) next; + if (number == "") + number = "undef"; numbers[lib] = number; order[lib] = ++order_n; if (NF > 1) { diff --git a/scripts/test-installation.pl b/scripts/test-installation.pl index 45c666b0a2..b77d7c4020 100755 --- a/scripts/test-installation.pl +++ b/scripts/test-installation.pl @@ -112,7 +112,8 @@ while (<SOVERSIONS>) { next if (/^all-sonames/); chop; if (/^lib/) { - ($name, $version)= /^lib(.*)\.so-version=\.(.*)$/; + ($name, $version)= /^lib(.*)\.so-version=\.?(.*)$/; + $version = "" if ! defined $version; # Filter out some libraries we don't want to link: # - nss_ldap since it's not yet available # - libdb1 since it conflicts with libdb @@ -123,7 +124,8 @@ while (<SOVERSIONS>) { next if ($build_mathvec == 0 && $name eq "mvec"); if ($name ne "nss_ldap" && $name ne "db1" && $name ne "thread_db" - && $name ne "nss_test1" && $name ne "libgcc_s") { + && $name ne "nss_test1" && $name ne "nss_test2" + && $name ne "libgcc_s") { $link_libs .= " -l$name"; $versions{$name} = $version; } @@ -171,14 +173,20 @@ if ($?) { $ok = 1; %found = (); +$librgx = qr/lib(\w+)\.so(?:\.([0-9\.]+))?/; open LDD, "ldd /tmp/test-prg$$ |" or die ("Couldn't execute ldd"); while (<LDD>) { - if (/^\s*lib/) { + if (m,^\s*${librgx}\s*=>\s*(?:/[^/]+)*/${librgx},) { ($name, $version1, $version2) = - /^\s*lib(\w*)\.so\.([0-9\.]*)\s*=>.*\.so\.([0-9\.]*)/; + ($1, defined($2) ? $2 : '', defined($4) ? $4 : ''); $found{$name} = 1; + if (!exists $versions{$name}) { + print "Unexpected dependency: lib$name\n"; + $ok = 0; + next; + } if ($versions{$name} ne $version1 || $version1 ne $version2) { print "Library lib$name is not correctly installed.\n"; print "Please check your installation!\n"; diff --git a/shlib-versions b/shlib-versions index b9cb99d2fb..e73d4270ac 100644 --- a/shlib-versions +++ b/shlib-versions @@ -75,3 +75,7 @@ libgcc_s=1 # The vector math library libmvec=1 + +# libcrypt is linked against this when built with --enable-nss-crypt. +# It does not use a versioned soname. +libfreebl3=
Ping. On 12/19/2017 01:16 AM, Rical Jasan wrote: > On 12/14/2017 07:10 AM, Florian Weimer wrote: >> On 10/18/2017 07:06 AM, Rical Jasan wrote: >>> diff --git a/scripts/test-installation.pl b/scripts/test-installation.pl >>> index 45c666b0a2..fcb7707889 100755 >>> --- a/scripts/test-installation.pl >>> +++ b/scripts/test-installation.pl >>> @@ -171,14 +171,20 @@ if ($?) { >>> $ok = 1; >>> %found = (); >>> +$librgx = qr/lib(\w+)\.so(?:\.([0-9\.]+))?/; >>> open LDD, "ldd /tmp/test-prg$$ |" >>> or die ("Couldn't execute ldd"); >>> while (<LDD>) { >>> - if (/^\s*lib/) { >>> + if (m,^\s*${librgx}\s*=>\s*(?:/[^/]+)*/${librgx},) { >>> ($name, $version1, $version2) = >>> - /^\s*lib(\w*)\.so\.([0-9\.]*)\s*=>.*\.so\.([0-9\.]*)/; >>> + ($1, defined($2) ? $2 : '', defined($4) ? $4 : ''); >>> $found{$name} = 1; >>> + if (!exists $versions{$name}) { >>> + print "Unexpected dependency: lib$name\n"; >>> + $ok = 0; >>> + next; >>> + } >> >> This looks fine to me. Could you commit it with a ChangeLog entry? > > I have the resources available to set up burner systems now, so after > reacquainting myself with this patch, I set about testing it. Not only > is test-installation.pl the very last thing run during `make install', > but it _only_ happens when prefix=/usr and DESTDIR is not set, so even > testing the patch requires bombing the system libc. > > The above tidbit actually makes the situation worse because instead of > having some "uninitialized value" warnings printed to stderr by Perl > with an otherwise successful installation, that "Unexpected dependency" > check will cause `make install' to fail, at the very end, after > everything has already been installed. > > What needs to happen is to make libfreebl3 an expected dependency, as > well as allowing for unversioned sonames. I've attached a patch that > works for me, but it definitely stems from hacking just enough to make > it work. There are some design choices to be made, so others need to > weigh in on whether they think what I did is appropriate. > > The process starts with shlib-versions, which defines valid versions of > shared libraries, both installed by glibc as well as external > dependencies. Note that in this file, versions are not prefixed by a > dot (important later). The choice I made here was to simply not define > a version. > > scripts/soversions.awk uses shlib-versions.v to create soversions.i in > the build root. I'm not sure where shlib-versions.v comes from, but it > is a generated file as well. I chose to tag the special case of no > version with an arbitrary version of "undef" because if it is left blank > at this point, the whitespace-delimited fields in soversions.i will be > parsed incorrectly when generating soversions.mk in Makeconfig. > > Correspondingly, Makeconfig is given a special case for the "undef" > version, which generates soversions.mk lines absent of any versioning. > There is some interesting voodoo here with what appears to be a > superfluous dot preceding the version, but I tried stripping it to > condense things and wound up with a broken installation with programs > that couldn't find "libc.so6" at runtime, so I was convinced I should > keep my unversioned soname propagation as an independent, exceptional case. > > Lastly, scripts/test-installation.pl populates its %versions hash by > matching lines in soversions.mk with versions prefixed with a dot, so I > made the dot optional. This results in an empty string for the version > of unversioned libraries, but that is specific to the library name, and > should be OK. This is where we meet the above diff, which will match an > unversioned library, so those empty strings will result in equality for > a given library, and since an entry was defined for the library in the > %versions hash, it isn't an unexpected dependency either. > > There is some ordering in soversions.awk that I don't quite understand > the purpose of, but just appending libfreebl3 to shlib-versions seems to > have worked. > > I'm concerned about the fact test-installation.pl is only run at the end > of `make install'; if it fails at that point, you're screwed anyway, so > it seems to be of limited utility. I wonder if it's possible to adapt > it to run after a `make install' with prefix!=/usr or DESTDIR set. That > can be a separate patch, though, and I haven't played with it. > > I can resubmit this to the list as an RFC if you think that would help > visibility. Testing requires a special setup, and I'm not sure how much > of the automated testing infrastructure out there actually exercises > this script, or would be affected by the attached patch's changes. The > length of the path from shlib-versions to test-installation.pl suggests > to me there are other things that depend on the framework. > > Rical
On 12/19/2017 10:16 AM, Rical Jasan wrote: > diff --git a/Makeconfig b/Makeconfig > index 80c498e33b..d9e9003c4b 100644 > --- a/Makeconfig > +++ b/Makeconfig > @@ -1110,6 +1110,8 @@ $(common-objpfx)soversions.mk: $(common-objpfx)soversions.i $(..)Makeconfig > case $$number in \ > [0-9]*) echo "$$lib.so-version=.$$number"; \ > echo "all-sonames+=$$lib=$$lib.so\$$($$lib.so-version)";;\ > + undef) echo "$$lib.so-version="; \ > + echo "all-sonames+=$$lib=$$lib.so";;\ > *) echo "$$lib.so-version=$$number"; \ > echo "all-sonames+=$$lib=\$$($$lib.so-version)";;\ > esac; \ > diff --git a/scripts/soversions.awk b/scripts/soversions.awk > index 247f061bc3..676c8692f6 100644 > --- a/scripts/soversions.awk > +++ b/scripts/soversions.awk > @@ -14,6 +14,8 @@ $1 == "DEFAULT" { > sub(/=.*$/, "", lib); > sub(/^.*=/, "", number); > if (lib in numbers) next; > + if (number == "") > + number = "undef"; > numbers[lib] = number; > order[lib] = ++order_n; > if (NF > 1) { I'm not sure if want to put libfrebl3.so into soversions.mk anyway. It results in gnu/lib-names-64.h:#define LIBFREEBL3_SO "libfreebl3.so" which is unexpected. So perhaps it is better to stick libfreebl3.so explicitly into test-installation.pl. > diff --git a/scripts/test-installation.pl b/scripts/test-installation.pl > index 45c666b0a2..b77d7c4020 100755 > --- a/scripts/test-installation.pl > +++ b/scripts/test-installation.pl > @@ -112,7 +112,8 @@ while (<SOVERSIONS>) { > next if (/^all-sonames/); > chop; > if (/^lib/) { > - ($name, $version)= /^lib(.*)\.so-version=\.(.*)$/; > + ($name, $version)= /^lib(.*)\.so-version=\.?(.*)$/; > + $version = "" if ! defined $version; I believe $version will always be defined at this point, although it can be the empty string. > # Filter out some libraries we don't want to link: > # - nss_ldap since it's not yet available > # - libdb1 since it conflicts with libdb > @@ -123,7 +124,8 @@ while (<SOVERSIONS>) { > next if ($build_mathvec == 0 && $name eq "mvec"); > if ($name ne "nss_ldap" && $name ne "db1" > && $name ne "thread_db" > - && $name ne "nss_test1" && $name ne "libgcc_s") { > + && $name ne "nss_test1" && $name ne "nss_test2" > + && $name ne "libgcc_s") { I think you need to filter out libfreebl3 at this point as well because it is not necessarily available. It should not be subject to link tests, just like libgcc_s. > $link_libs .= " -l$name"; > $versions{$name} = $version; > } > @@ -171,14 +173,20 @@ if ($?) { > > $ok = 1; > %found = (); > +$librgx = qr/lib(\w+)\.so(?:\.([0-9\.]+))?/; > > open LDD, "ldd /tmp/test-prg$$ |" > or die ("Couldn't execute ldd"); > while (<LDD>) { > - if (/^\s*lib/) { > + if (m,^\s*${librgx}\s*=>\s*(?:/[^/]+)*/${librgx},) { > ($name, $version1, $version2) = > - /^\s*lib(\w*)\.so\.([0-9\.]*)\s*=>.*\.so\.([0-9\.]*)/; > + ($1, defined($2) ? $2 : '', defined($4) ? $4 : ''); > $found{$name} = 1; > + if (!exists $versions{$name}) { > + print "Unexpected dependency: lib$name\n"; > + $ok = 0; > + next; > + } > if ($versions{$name} ne $version1 || $version1 ne $version2) { > print "Library lib$name is not correctly installed.\n"; > print "Please check your installation!\n"; > diff --git a/shlib-versions b/shlib-versions > index b9cb99d2fb..e73d4270ac 100644 > --- a/shlib-versions > +++ b/shlib-versions > @@ -75,3 +75,7 @@ libgcc_s=1 > > # The vector math library > libmvec=1 > + > +# libcrypt is linked against this when built with --enable-nss-crypt. > +# It does not use a versioned soname. > +libfreebl3= See above, this won't be necessary if libfreebl3.so is included in test-installation.pl. Thanks, Florian
diff --git a/scripts/test-installation.pl b/scripts/test-installation.pl index 4b0e9f3c4a..466c526cc9 100755 --- a/scripts/test-installation.pl +++ b/scripts/test-installation.pl @@ -177,10 +177,15 @@ open LDD, "ldd /tmp/test-prg$$ |" or die ("Couldn't execute ldd"); while (<LDD>) { if (/^\s*lib/) { + # When libcrypt is linked against NSS, some of the referenced + # libraries do not have a trailing version in their soname. ($name, $version1, $version2) = - /^\s*lib(\w*)\.so\.([0-9\.]*)\s*=>.*\.so\.([0-9\.]*)/; + /^\s*lib(\w*)\.so(?:\.([0-9\.]*))?\s*=>.*\.so(?:\.([0-9\.]*))?/; $found{$name} = 1; - if ($versions{$name} ne $version1 || $version1 ne $version2) { + if (defined($version1) != defined($version2) + || defined($version1) != defined($versions{$name}) + || (defined($versions{$name}) + && ($versions{$name} ne $version1 || $version1 ne $version2))) { print "Library lib$name is not correctly installed.\n"; print "Please check your installation!\n"; print "Offending line of ldd output: $_\n";