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(a)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.
bye,
Sumit
_______________________________________________
sssd-devel mailing list
sssd-devel(a)lists.fedorahosted.org
https://lists.fedorahosted.org/mailman/listinfo/sssd-devel