[PATCH] config: Conditionalize on LIBDEBUGINFOD instead of DEBUGINFOD

[PATCH] config: Conditionalize on LIBDEBUGINFOD instead of DEBUGINFOD

[Dmitry V. Levin]

When elfutils is configured using --enable-libdebuginfod
--disable-debuginfod, that is, when the library is built and installed
but the server is not, it makes sense to install libdebuginfod.pc
because the latter complements the library rather than the server.

Likewise, it makes sense to install profile.d/debuginfod.*sh files
along with libdebuginfod because the library can use DEBUGINFOD_URLS
environment variable as well as the server.

This change does not affect --enable-debuginfod mode as the latter
requires --enable-libdebuginfod.

Fixes: fed3c3ceeaa6 ("Do not install libdebuginfod.pc unless debuginfod is enabled")
Fixes: b503c358dde8 ("Do not install profile.d/debuginfod.*sh files unless debuginfod is enabled")
Signed-off-by: Dmitry V. Levin <ldv@altlinux.org>
---
 config/ChangeLog   | 5 +++++
 config/Makefile.am | 2 +-
 2 files changed, 6 insertions(+), 1 deletion(-)

diff --git a/config/ChangeLog b/config/ChangeLog
index c532f7e6..e69e7e21 100644
--- a/config/ChangeLog
+++ b/config/ChangeLog
@@ -1,3 +1,8 @@
+2020-11-02  Dmitry V. Levin  <ldv@altlinux.org>
+
+	* Makefile.am (pkgconfig_DATA, install-data-local, uninstall-local):
+	Conditionalize on LIBDEBUGINFOD instead of DEBUGINFOD.
+
 2020-10-31  Dmitry V. Levin  <ldv@altlinux.org>
 
 	* Makefile.am (install-data-local, uninstall-local): Conditionalize
diff --git a/config/Makefile.am b/config/Makefile.am
index ef28dab6..a66f5490 100644
--- a/config/Makefile.am
+++ b/config/Makefile.am
@@ -34,7 +34,7 @@ EXTRA_DIST = elfutils.spec.in known-dwarf.awk 10-default-yama-scope.conf \
 
 pkgconfigdir = $(libdir)/pkgconfig
 pkgconfig_DATA = libelf.pc libdw.pc
-if DEBUGINFOD
+if LIBDEBUGINFOD
 pkgconfig_DATA += libdebuginfod.pc
 
 install-data-local:

-- 
ldv

Re: [PATCH] config: Conditionalize on LIBDEBUGINFOD instead of DEBUGINFOD

[Mark Wielaard]

[-- Attachment #1: Type: text/plain, Size: 759 bytes --]

Hi Dmitry,

On Mon, 2020-11-02 at 08:00 +0000, Dmitry V. Levin wrote:
> When elfutils is configured using --enable-libdebuginfod
> --disable-debuginfod, that is, when the library is built and installed
> but the server is not, it makes sense to install libdebuginfod.pc
> because the latter complements the library rather than the server.

Agreed.

> Likewise, it makes sense to install profile.d/debuginfod.*sh files
> along with libdebuginfod because the library can use DEBUGINFOD_URLS
> environment variable as well as the server.

While packaging I noticed that we install the profiles.d files even
when the DEBUGINFOD_URLS is empty. I think we should avoid that.

So what do you think if the attached revised patch?

Thanks,

Mark

[-- Attachment #2: Type: text/x-patch, Size: 3167 bytes --]

From 4c121eeda370f192c0ed35ba35bc6482c02fdac0 Mon Sep 17 00:00:00 2001
From: "Dmitry V. Levin" <ldv@altlinux.org>
Date: Mon, 2 Nov 2020 08:00:00 +0000
Subject: [PATCH] config: Conditionalize on LIBDEBUGINFOD and
 DEFAULT_DEBUGINFOD_URLS

When elfutils is configured using --enable-libdebuginfod
--disable-debuginfod, that is, when the library is built and installed
but the server is not, it makes sense to install libdebuginfod.pc
because the latter complements the library rather than the server.

Likewise, it makes sense to install profile.d/debuginfod.*sh files
only when --enable-debuginfod-urls has been given because otherwise
the environment variables will just be empty.

This change does not affect --enable-debuginfod mode as the latter
requires --enable-libdebuginfod.

Fixes: fed3c3ceeaa6 ("Do not install libdebuginfod.pc unless debuginfod is enabled")
Fixes: b503c358dde8 ("Do not install profile.d/debuginfod.*sh files unless debuginfod is enabled")
Signed-off-by: Dmitry V. Levin <ldv@altlinux.org>
Signed-off-by: Mark Wielaard <mark@klomp.org>
---
 ChangeLog          | 4 ++++
 config/ChangeLog   | 8 ++++++++
 config/Makefile.am | 4 +++-
 configure.ac       | 2 ++
 4 files changed, 17 insertions(+), 1 deletion(-)

diff --git a/ChangeLog b/ChangeLog
index 565d021c..d8cbd9b4 100644
--- a/ChangeLog
+++ b/ChangeLog
@@ -1,3 +1,7 @@
+2020-11-03  Mark Wielaard  <mark@klomp.org>
+
+	* configure.ac (DEFAULT_DEBUGINFOD_URLS): New AM_CONDITIONAL.
+
 2020-11-01  Érico N. Rolim  <erico.erc@gmail.com>
 
 	* configure.ac: Check for fts and obstack from outside libc.
diff --git a/config/ChangeLog b/config/ChangeLog
index c532f7e6..8c25f17d 100644
--- a/config/ChangeLog
+++ b/config/ChangeLog
@@ -1,3 +1,11 @@
+2020-11-02  Dmitry V. Levin  <ldv@altlinux.org>
+	    Mark Wielaard  <mark@klomp.org>
+
+	* Makefile.am (pkgconfig_DATA): Conditionalize on LIBDEBUGINFOD
+	instead of DEBUGINFOD.
+	(install-data-local, uninstall-local): Conditionalize on
+	DEFAULT_DEBUGINFOD_URLS.
+
 2020-10-31  Dmitry V. Levin  <ldv@altlinux.org>
 
 	* Makefile.am (install-data-local, uninstall-local): Conditionalize
diff --git a/config/Makefile.am b/config/Makefile.am
index ef28dab6..1a43dec6 100644
--- a/config/Makefile.am
+++ b/config/Makefile.am
@@ -34,9 +34,11 @@ EXTRA_DIST = elfutils.spec.in known-dwarf.awk 10-default-yama-scope.conf \
 
 pkgconfigdir = $(libdir)/pkgconfig
 pkgconfig_DATA = libelf.pc libdw.pc
-if DEBUGINFOD
+if LIBDEBUGINFOD
 pkgconfig_DATA += libdebuginfod.pc
+endif
 
+if DEFAULT_DEBUGINFOD_URLS
 install-data-local:
 	$(INSTALL_DATA) profile.sh -D $(DESTDIR)$(sysconfdir)/profile.d/debuginfod.sh
 	$(INSTALL_DATA) profile.csh -D $(DESTDIR)$(sysconfdir)/profile.d/debuginfod.csh
diff --git a/configure.ac b/configure.ac
index c1a6954d..a258daa2 100644
--- a/configure.ac
+++ b/configure.ac
@@ -763,6 +763,8 @@ AC_ARG_ENABLE(debuginfod-urls,
             [default_debuginfod_urls=""])
 AC_SUBST(DEBUGINFOD_URLS, $default_debuginfod_urls)                
 AC_CONFIG_FILES([config/profile.sh config/profile.csh])
+AM_CONDITIONAL([DEFAULT_DEBUGINFOD_URLS],
+               [test "x$default_debuginfod_urls" != "x"])
 
 AC_OUTPUT
 
-- 
2.18.4

Re: [PATCH] config: Conditionalize on LIBDEBUGINFOD instead of DEBUGINFOD

[Frank Ch. Eigler]

Hi -

> While packaging I noticed that we install the profiles.d files even
> when the DEBUGINFOD_URLS is empty. I think we should avoid that.

I think we should install them anyway.  They give a sysadmin an easy knob
to set a systemwide default, even if the distro build didn't.

- FChE

Re: [PATCH] config: Conditionalize on LIBDEBUGINFOD instead of DEBUGINFOD

[Mark Wielaard]

Hi Frank,

On Tue, 2020-11-03 at 12:07 -0500, Frank Ch. Eigler via Elfutils-devel
wrote:
> > While packaging I noticed that we install the profiles.d files even
> > when the DEBUGINFOD_URLS is empty. I think we should avoid that.
> 
> I think we should install them anyway.  They give a sysadmin an easy
> knob to set a systemwide default, even if the distro build didn't.

That is a good point. But I think the default setting is wrong if it
isn't actually used/setup. As the profile shell snippets are now they
define the DEBUGINFOD_URLS environment variable as an non-empty string
(although just containing spaces), which makes anything using
libdw/libdebuginfod do extra work because we have to assume there is
some server defined.

Is there a way to make it so that even if the profiles are installed
the DEBUGINFOD_URLS is not defined or is the empty string if --enable-
debuginfod-urls isn't given?

Thanks,

Mark

Re: [PATCH] config: Conditionalize on LIBDEBUGINFOD instead of DEBUGINFOD

[Frank Ch. Eigler]

Hi -

> That is a good point. But I think the default setting is wrong if it
> isn't actually used/setup. As the profile shell snippets are now they
> define the DEBUGINFOD_URLS environment variable as an non-empty string
> (although just containing spaces), [...]

Good point, we should make it empty or unset in that case.  Will send
a shell scripting patch to the profile files.

- FChE

Re: [PATCH] config: Conditionalize on LIBDEBUGINFOD instead of DEBUGINFOD

[Dmitry V. Levin]

Hi,

On Tue, Nov 03, 2020 at 06:26:07PM +0100, Mark Wielaard wrote:
> Hi Frank,
> 
> On Tue, 2020-11-03 at 12:07 -0500, Frank Ch. Eigler via Elfutils-devel
> wrote:
> > > While packaging I noticed that we install the profiles.d files even
> > > when the DEBUGINFOD_URLS is empty. I think we should avoid that.
> > 
> > I think we should install them anyway.  They give a sysadmin an easy
> > knob to set a systemwide default, even if the distro build didn't.
> 
> That is a good point. But I think the default setting is wrong if it
> isn't actually used/setup. As the profile shell snippets are now they
> define the DEBUGINFOD_URLS environment variable as an non-empty string
> (although just containing spaces), which makes anything using
> libdw/libdebuginfod do extra work because we have to assume there is
> some server defined.
> 
> Is there a way to make it so that even if the profiles are installed
> the DEBUGINFOD_URLS is not defined or is the empty string if --enable-
> debuginfod-urls isn't given?

Something like this?

diff --git a/config/profile.sh.in b/config/profile.sh.in
index 8a022489..aa228a0d 100644
--- a/config/profile.sh.in
+++ b/config/profile.sh.in
@@ -1,3 +1,4 @@
-
-DEBUGINFOD_URLS="$DEBUGINFOD_URLS @DEBUGINFOD_URLS@"
-export DEBUGINFOD_URLS
+if [ -n "@DEBUGINFOD_URLS@" ]; then
+	DEBUGINFOD_URLS="${DEBUGINFOD_URLS-}${DEBUGINFOD_URLS:+ }@DEBUGINFOD_URLS@"
+	export DEBUGINFOD_URLS
+fi


-- 
ldv

[PATCH 2/1] config: do not define DEBUGINFOD_URLS environment variable unnecessarily

[Dmitry V. Levin]

Before this change, when elfutils was configured without
--enable-debuginfod-urls, the installed profile.d/debuginfod.sh and
profile.d/debuginfod.csh scripts used to define the DEBUGINFOD_URLS
environment variable as an non-empty string containing spaces, making
all libdebuginfod users do extra work.

Change these scripts to avoid defining the DEBUGINFOD_URLS environment
variable unless configured using --enable-debuginfod-urls.

Signed-off-by: Dmitry V. Levin <ldv@altlinux.org>
---
 config/ChangeLog      |  5 +++++
 config/profile.csh.in | 13 +++++++++++--
 config/profile.sh.in  |  7 ++++---
 3 files changed, 20 insertions(+), 5 deletions(-)

diff --git a/config/ChangeLog b/config/ChangeLog
index e69e7e21..e4da0ebc 100644
--- a/config/ChangeLog
+++ b/config/ChangeLog
@@ -1,3 +1,8 @@
+2020-11-04  Dmitry V. Levin  <ldv@altlinux.org>
+
+	* profile.sh.in, profile.csh.in: Do not define $DEBUGINFOD_URLS unless
+	configured using --enable-debuginfod-urls.
+
 2020-11-02  Dmitry V. Levin  <ldv@altlinux.org>
 
 	* Makefile.am (pkgconfig_DATA, install-data-local, uninstall-local):
diff --git a/config/profile.csh.in b/config/profile.csh.in
index 4f25896d..0a2d6d16 100644
--- a/config/profile.csh.in
+++ b/config/profile.csh.in
@@ -1,2 +1,11 @@
-
-setenv DEBUGINFOD_URLS "$DEBUGINFOD_URLS @DEBUGINFOD_URLS@"
+if ("@DEBUGINFOD_URLS@" != "") then
+	if ($?DEBUGINFOD_URLS) then
+		if ($%DEBUGINFOD_URLS) then
+			setenv DEBUGINFOD_URLS "$DEBUGINFOD_URLS @DEBUGINFOD_URLS@"
+		else
+			setenv DEBUGINFOD_URLS "@DEBUGINFOD_URLS@"
+		endif
+	else
+		setenv DEBUGINFOD_URLS "@DEBUGINFOD_URLS@"
+	endif
+endif
diff --git a/config/profile.sh.in b/config/profile.sh.in
index 8a022489..aa228a0d 100644
--- a/config/profile.sh.in
+++ b/config/profile.sh.in
@@ -1,3 +1,4 @@
-
-DEBUGINFOD_URLS="$DEBUGINFOD_URLS @DEBUGINFOD_URLS@"
-export DEBUGINFOD_URLS
+if [ -n "@DEBUGINFOD_URLS@" ]; then
+	DEBUGINFOD_URLS="${DEBUGINFOD_URLS-}${DEBUGINFOD_URLS:+ }@DEBUGINFOD_URLS@"
+	export DEBUGINFOD_URLS
+fi

-- 
ldv

Re: [PATCH] config: Conditionalize on LIBDEBUGINFOD instead of DEBUGINFOD

[Frank Ch. Eigler]

Hi -

> Something like this?

> -DEBUGINFOD_URLS="$DEBUGINFOD_URLS @DEBUGINFOD_URLS@"
> -export DEBUGINFOD_URLS
> +if [ -n "@DEBUGINFOD_URLS@" ]; then
> +	DEBUGINFOD_URLS="${DEBUGINFOD_URLS-}${DEBUGINFOD_URLS:+ }@DEBUGINFOD_URLS@"
> +	export DEBUGINFOD_URLS
> +fi

You took the words right out of my mouth. :-)

- FChE

Re: [PATCH] config: Conditionalize on LIBDEBUGINFOD instead of DEBUGINFOD

[Mark Wielaard]

Hi Dmitry,

On Mon, 2020-11-02 at 08:00 +0000, Dmitry V. Levin wrote:
> When elfutils is configured using --enable-libdebuginfod
> --disable-debuginfod, that is, when the library is built and
> installed
> but the server is not, it makes sense to install libdebuginfod.pc
> because the latter complements the library rather than the server.
> 
> Likewise, it makes sense to install profile.d/debuginfod.*sh files
> along with libdebuginfod because the library can use DEBUGINFOD_URLS
> environment variable as well as the server.
> 
> This change does not affect --enable-debuginfod mode as the latter
> requires --enable-libdebuginfod.

Given the discussion and your followup patch to not define
DEBUGINFOD_URLS environment variable unnecessarily I pushed your
original patch.

Thanks,

Mark

Re: [PATCH 2/1] config: do not define DEBUGINFOD_URLS environment variable unnecessarily

[Mark Wielaard]

Hi Dmitry,

On Wed, 2020-11-04 at 08:00 +0000, Dmitry V. Levin wrote:
> Before this change, when elfutils was configured without
> --enable-debuginfod-urls, the installed profile.d/debuginfod.sh and
> profile.d/debuginfod.csh scripts used to define the DEBUGINFOD_URLS
> environment variable as an non-empty string containing spaces, making
> all libdebuginfod users do extra work.
> 
> Change these scripts to avoid defining the DEBUGINFOD_URLS environment
> variable unless configured using --enable-debuginfod-urls.

Looks good. Thanks for figuring out the csh variant.
Pushed.

Cheers,

Mark