[PATCH] configure.ac: enable systemd socket activation in libxtrans

[Hans de Goede]

Hi,

On 12/02/2013 02:33 PM, Łukasz Stelmach wrote:
> It was <2013-11-30 sob 01:42>, when Peter Hutterer wrote:
>> On 29/11/2013 19:36 , Łukasz Stelmach wrote:
>>> It was <2013-11-29 pią 06:00>, when Peter Hutterer wrote:
>>>> On Thu, Nov 28, 2013 at 04:23:07PM +0100, Hans de Goede wrote:
>>>>> From: Łukasz Stelmach <l.stelmach at samsung.com>
>>>>>
>>>>> Signed-off-by: Łukasz Stelmach <l.stelmach at samsung.com>
>>>>> Signed-off-by: Hans de Goede <hdegoede at redhat.com>
>>>>> ---
>>>>>    configure.ac | 28 ++++++++++++++++++++++++++++
>>>>>    1 file changed, 28 insertions(+)
>>>>>
>>>>> diff --git a/configure.ac b/configure.ac
>>>>> index 6c4a609..ab246d6 100644
>>>>> --- a/configure.ac
>>>>> +++ b/configure.ac
>>>>> @@ -832,6 +832,34 @@ AC_SUBST(SDK_REQUIRED_MODULES)
>>>>>
>>>>>    REQUIRED_MODULES="$FIXESPROTO $DAMAGEPROTO $XCMISCPROTO $XTRANS
>>>>> $BIGREQSPROTO $SDK_REQUIRED_MODULES"
>>>>>
>>>>> +#
>>>>> +# systemd socket activation
>>>>> +#
>>>>> +# activate the code in libxtrans that grabs sockets' file-descriptors
>>>>> +# instead of creating them.
>>>>> +#
>>>>> +AC_ARG_WITH([systemd],
>>>>> +	AS_HELP_STRING([--with-systemd], [support systemd socket activation]),
>>>>> +	[], [with_systemd=check])
>>>>> +have_systemd=check
>>>>> +if test "x$with_systemd" != "xno"; then
>>>>> +	PKG_CHECK_MODULES([systemd], [libsystemd-daemon],
>>>>> +		[AC_DEFINE(HAVE_SYSTEMD, 1, [Define if libsystemd-daemon is available])
>>>>> +		have_systemd=yes;],
>>>>> +		[have_systemd=no])
>>>>> +	if test "x$with_systemd" = "xyes" -a "x$have_systemd" = "xno"; then
>>>>> +		AC_MSG_ERROR([systemd support requested but no library has been found])
>>>>> +	fi
>>>>> +fi
>>>>> +AM_CONDITIONAL([HAVE_SYSTEMD], [test "x$have_systemd" = "xyes"])
>>>>> +if test "x$have_systemd" = "xyes"; then
>>>>
>>>> while the above is fine, I'd prefer if base logic of the check is the same
>>>> as all the other checks for misc "auto" stuff. which seems to be
>>>>
>>>> PKG_CHECK_MODULES(foo, foo, [have_foo=yes], [have_foo=no])
>>>> if test "x$foo" = xauto; then
>>>>      foo=$have_foo
>>>> fi
>>>> if test "x$foo" = xyes; then
>>>>        if "xhave_foo" = xno; then
>>>>           error
>>>>        fi
>>>>        AC_DEFINE(...)
>>>>        other stuff
>>>> fi
>>>> AM_CONDITIONAL()
>>>>
>>>> there are multiple versions of this floating around in the configure file
>>>> but the majority seems to go about it this way. it also helps re-using the
>>>> have_systemd if it's needed elsewhere in the future.
>>>>
>>>>
>>>>> +	SAVE_LIBS=$LIBS
>>>>> +	LIBS="$systemd_LIBS"
>>>>> + AC_CHECK_FUNCS([sd_listen_fds], [], [AC_MSG_ERROR([sd_listen_fds()
>>>>> is missing from libsystemd-daemon])])
>>>>
>>>> can this even happen? quick check of the systemd code doesn't seem like it's
>>>> easily disabled. and the history of it is back long enough that we don't
>>>> need to require a specific version
>>>
>>>
>>> This is not the latest version of the patch. The latest version[1]
>>> has the message changed to
>>>
>>>       sd_listen_fds() is missing or libsystemd-daemon is not available
>>>
>>> It may happen if there is something wrong with paths and for some reason
>>> the library is not available to the linker. Personally I encountered
>>> this when I was developing the patches with the systemd libraries not
>>> installed in the system but rather laying in the source directory.
>>
>> Hans has already answered this, but nonetheless I recommend: never
>> ever write patches to accommodate for a local custom setup. That may
>> be acceptable if you plan to ship the local setup as a fixed image,
>> but otherwise you're just spending time better spent elsewhere
>> (specifically, fixing the cause of the bug :)
>
> +1 to my experience points ;)
>
>> Doubly so for a system (pkgconfig) which has the sole purpose of
>> making such custom local configurations invisible to the user of
>> pkgconfig.
>>
>> In your case as simple prefix change, make install into somewhere and
>> adjusting PKG_CONFIG_PATH should've fixed the issue.
>
>
> Who should rework the patch?

I'm going to respin the patch-set addressing all review comments,
I should have a v5 in a few hours.

Regards,

Hans