[SSSD] [PATCH] BUILD: Add version symbol files for public libraries.

Jakub Hrozek jhrozek at redhat.com
Wed Jul 9 11:15:17 UTC 2014


On Tue, Jul 08, 2014 at 11:04:27PM +0200, Lukas Slebodnik wrote:
> On (08/07/14 15:19), Sumit Bose wrote:
> >On Tue, Jul 08, 2014 at 12:51:48PM +0200, Jakub Hrozek wrote:
> >> On Tue, Jul 08, 2014 at 11:21:09AM +0200, Lukas Slebodnik wrote:
> >> > On (08/07/14 09:27), Jakub Hrozek wrote:
> >> > >On Thu, Jun 26, 2014 at 10:31:27AM +0200, Lukas Slebodnik wrote:
> >> > >> ehlo,
> >> > >> 
> >> > >> attached patch fixes ticket #2194.
> >> > >> 
> >> > >> If you wan to know more about version script (version maps) here are links:
> >> > >> 
> >> > >> http://people.redhat.com/drepper/dsohowto.pdf
> >> > >>     (sections 2.2.5 .. 2.2.7, 3.4, 3.5)
> >> > >> https://www.gnu.org/software/gnulib/manual/html_node/LD-Version-Scripts.html
> >> > >> ftp://ftp.gnu.org/old-gnu/Manuals/ld-2.9.1/html_node/ld_25.html
> >> > >> 
> >> > >> LS
> >> > >
> >> > >I verified with:
> >> > >    nm -D $libname | awk '$2 == "T" {print $3";"}' | sort
> >> > >that the functions exported by the library match the functions defined
> >> > >in the version file.
> >> > >
> >> > >The versions also match the SONAMEs.
> >> > >
> >> > >One question inline:
> >> > >
> >> > >> From 0779624cf96480fe3b62f5c881fb9b123d24a965 Mon Sep 17 00:00:00 2001
> >> > >> From: Lukas Slebodnik <lslebodn at redhat.com>
> >> > >> Date: Mon, 9 Jun 2014 14:38:31 +0200
> >> > >> Subject: [PATCH] BUILD: Add version symbol files for public libraries.
> >> > >> 
> >> > >> Version symbol files will help package systems to catch backward compatible
> >> > >> changes (newly added functions) into library.
> >> > >> 
> >> > >> Resolves:
> >> > >> https://fedorahosted.org/sssd/ticket/2194
> >> > >
> >> > >[...]
> >> > >
> >> > >> new file mode 100644
> >> > >> index 0000000000000000000000000000000000000000..bd3abb211120784494fe366ddd19b94d9b982657
> >> > >> --- /dev/null
> >> > >> +++ b/src/sss_client/idmap/sss_nss_idmap.exports
> >> > >> @@ -0,0 +1,17 @@
> >> > >> +SSS_NSS_IDMAP_0.0.1 {
> >> > >> +
> >> > >> +    # public functions
> >> > >> +    global:
> >> > >> +
> >> > >> +        sss_nss_getsidbyname;
> >> > >> +        sss_nss_getsidbyid;
> >> > >> +        sss_nss_getnamebysid;
> >> > >> +        sss_nss_getidbysid;
> >> > >> +
> >> > >> +    # exported for mocking
> >> > >
> >> > >I don't think the comment is right. The symbol is exported because
> >> > >libsss_nss_idmap links with the client code:
> >> > >740 libsss_nss_idmap_la_SOURCES = \
> >> > >741     src/sss_client/idmap/sss_nss_idmap.c \
> >> > >742     src/sss_client/common.c \
> >> > >743     src/util/strtonum.c
> >> > >
> >> > >And libsss_nss_idmap uses the function to call out to the NSS responder
> >> > >(see src/sss_client/idmap/sss_nss_idmap.c:100)
> >> > >
> >> > >Can we not mark the function as global? It's not part of the sss_nss_idmap's
> >> > >API.
> >> > Try to comment out this symbol from file
> >> > --- a/src/sss_client/idmap/sss_nss_idmap.exports
> >> > +++ b/src/sss_client/idmap/sss_nss_idmap.exports
> >> > @@ -9,7 +9,7 @@ SSS_NSS_IDMAP_0.0.1 {
> >> >          sss_nss_getidbysid;
> >> >  
> >> >      # exported for mocking
> >> > -        sss_nss_make_request;
> >> > +    #    sss_nss_make_request;
> >> >  
> >> >      # everything else is local
> >> >      local:
> >> > 
> >> > sss_nss_idmap will fail, because function sss_nss_make_request cannot be mocked
> >> > sh-4.2$ cat sss_nss_idmap-tests.log
> >> > [==========] Running 1 test(s).
> >> > [ RUN      ] test_getsidbyname
> >> > 0x2 != 0
> >> > ../sssd/src/tests/cmocka/sss_nss_idmap-tests.c:102: error: Failure!
> >> > [  FAILED  ] test_getsidbyname
> >> > [==========] 1 test(s) run.
> >> > [  PASSED  ] 0 test(s).
> >> > [  FAILED  ] 1 test(s), listed below:
> >> > [  FAILED  ] test_getsidbyname
> >> > 
> >> > We can build libsss_nss_idmap twice.
> >> >     1) real library with version symbol file
> >> >     2) library without version symbol file only for tests (sss_nss_idmap-tests)
> >> > 
> >> > I was not able to find better solution.
> >> > 
> >> > LS
> >> 
> >> I tried playing a bit with Makefile.am and the only viable solution I
> >> see is to hardcode the list of libsss_nss_idmap sources in Makefile.am's
> >> sss_nss_idmap_tests_SOURCES except sss_client/common.c and mock
> >> sss_nss_lock and sss_nss_unlock..
> >> 
> >> But I'm not sure it's worth it, the rist is that a 3rd party links with
> >> sss_nss_make_request (which has no declaration in the public headers).
> >> 
> >> So now I prefer to push your original patch, but I would also like to
> >> hear other opinions, I'm not the biggest packaging expert around..
> >
> >While discussing this on irc with Andreas (cmocka author) and Jakub we
> >came up with
> >
> >SSS_NSS_IDMAP_0.0.1 {
> >
> >    # public functions
> >    global:
> >
> >        sss_nss_getsidbyname;
> >        sss_nss_getsidbyid;
> >        sss_nss_getnamebysid;
> >        sss_nss_getidbysid;
> >
> >    # everything else is local
> >    local:
> >        *;
> >};
> >
> >UNIT_TESTING_ONLY {
> >    global:
> >        sss_nss_make_request;
> >};
> >
> >which would move the sss_nss_make_request into a separate version which
> >is not for public use. RPMs will still show a Provides like:
> >
> >libsss_nss_idmap.so.0(UNIT_TESTING_ONLY)
> >
> >which can be filtered out (at least on recent Fedora version) with
> >
> >%global __provides_exclude ^.*FOR_CMOCKA.*$
> >
> >in the spec file.
> This change would only hide provides from rpm, which is not platform
> independent. I tried build libsss_nss_idmap.so in different way for
> test and for installation with custom make targets (check-local,
> install-exec-local, install-exec-hook). It was not possible because
> I would need targets which are executed before check or install and int the
> middle or after targets (check or install).
> 
> I decided to build library libsss_nss_idmap twice.
>     1) libsss_nss_idmap_test.so --will be used for sss_nss_idmap-test
>     2) libsss_nss_idmap.so --will be installed
> 
> Two version script files were combined for libsss_nss_idmap_test.so
> defualt sss_nss_idmap.exports and extra sss_nss_idmap.unit_tests, which
> contains function sss_nss_make_request.
> 
> LS

Originally I was going to say this breaks the purpose of unit tests that
we should stay as close to the production binaries as possible, but the
different is only in the version file (and rpath) and the
libsss_nss_idmap_la_* variables are used as a basis of the test library.

Provides also look sane,  nm output does, too.

I'm giving my ACK now, but I'll wait a bit before I push the patch in
case other developers didn't agree.



More information about the sssd-devel mailing list