On Mon, Mar 17, 2014 at 09:34:04AM +0100, Lukas Slebodnik wrote:
On (17/03/14 07:53), Lukas Slebodnik wrote:
>On (15/03/14 17:57), Lukas Slebodnik wrote:
>>On (12/11/13 11:15), Simo Sorce wrote:
>>>On Tue, 2013-11-12 at 11:03 -0500, Simo Sorce wrote:
>>>> On Tue, 2013-11-12 at 16:23 +0100, Lukas Slebodnik wrote:
>>>
>>>> > Updated patches are attached.
>>>>
>>>> This is not the right way to fix the failure.
>>>>
>>>> That library *knowingly* has missing symbols because it is an internal
>>>> library.
>>>>
>>>> You need to build or link to an outer program that have only the
>>>> necessary symbols you know are going to be available.
>>>>
>>>> In this case those symbols are in libsss_krb5.so, so all you should nee
>>>> to do is to have this in tests:
>>>>
>>>> + { "libsss_krb5_common.so", {
LIBPFX"libdlopen_test_providers.so",
>>>> +
LIBPFX"libsss_krb5_common.so",
>>>> + LIBPFX"libsss_krb5.so",
>>>> + NULL } },
>>>>
>>>> Adding c files to librraies to "make them pass" the dl-open
test is
>>>> wrong in 99% of the cases. you are just making the test pass not
testing
>>>> that the libraries have the right dependencies. In particular for
>>>> internal modules you really need to understand the hierarchy of the
>>>> modules and link in the test all the necessary dependencies before
>>>> thinking of adding any file to the module itself.
>>>>
>>>> In this case what you did was to duplicate symblos in the resulting
Text
>>>> file, and that can potentially cause very serious consequencies if any
>>>> of those symbols is a static symbols that is meant to have a share de
>>>> value all code rely on.
>>>
>>>Oh let me add.
>>>I did my check very quickly so it is possible that my version is not the
>>>final correct approach, I was just describing how you should pass in
>>>multiple libs for dependency.
>>>
>>>It is possible that you need those symbols directly in
>>>libsss_krb5_common.so, but if that is actually the case, then it means
>>>you have to remove those symbols from libsss_krb5.so and add them
>>>libsss_krb5_common.so as dependency for libsss_krb5.so in makefile and
>>>tests. This may even require moving functions from one file to another
>>>in the extreme case.
>>>
>>
>>zombie patches are returning :-)
>>
>>I hope all commit messages are self descripting.
>>
>>They were lot of changes in Makefile.am
>>sh-4.2$ git diff --stat HEAD~6..HEAD Makefile.am
>> Makefile.am | 99 +++++++++++++++++++------------------------------------------
>> 1 file changed, 31 insertions(+), 68 deletions(-)
>>
>>LS
>
>>From 457eaee99fb5afba5f6dd3752efcf78ce19d723c Mon Sep 17 00:00:00 2001
>>From: Lukas Slebodnik <lslebodn(a)redhat.com>
>>Date: Sat, 15 Mar 2014 17:29:25 +0100
>>Subject: [PATCH 5/6] BUILD: Move duplicated files from providers to
>> libsss_ldap_common.so
>>
>
>self-NACK
>
>make distcheck fails with 5th patch.
>
Interdiff for 5th patch:
diff --git a/Makefile.am b/Makefile.am
index 2d0a295..a65369b 100644
--- a/Makefile.am
+++ b/Makefile.am
@@ -1742,6 +1742,10 @@ endif
# Plugin Libraries #
####################
+# libsss_krb5_common must be installed before libsss_ldap_common
+# because libtool tries to relink libsss_ldap_common when installing
+# libsss_ldap_common and therefore make distcheck fails
+pkglib_LTLIBRARIES += libsss_krb5_common.la
pkglib_LTLIBRARIES += libsss_ldap_common.la
libsss_ldap_common_la_SOURCES = \
src/providers/ldap/ldap_id.c \
@@ -1778,6 +1782,8 @@ libsss_ldap_common_la_SOURCES = \
src/providers/ldap/sdap.c \
src/util/user_info_msg.c \
src/util/sss_ldap.c
libsss_ldap_common_la_LIBADD = \
$(KRB5_LIBS) \
libsss_krb5_common.la \
@@ -1800,11 +1806,7 @@ libsss_ldap_common_la_SOURCES += \
src/providers/ldap/sdap_async_autofs.c
endif
libsss_ldap_common_la_CFLAGS = \
$(KRB5_CFLAGS)
-pkglib_LTLIBRARIES += libsss_krb5_common.la
libsss_krb5_common_la_SOURCES = \
src/providers/krb5/krb5_utils.c \
src/providers/krb5/krb5_become_user.c \
Updated patches are attached
LS
Sorry for the delay in reviewing the packages.
I think these packages are good and I'd like to ACK them.
The testing consisted of:
* visual inspection of the patches. The commit messages help,
thanks!
* applying the dlopen-test patch on master. The dlopen test then
didn't pass. After aplying the other patches, the dlopen test
passed as well.
* starting IPA, AD and LDAP (with and without ID mapping to be sure)
went fine