[PATCH] Check for existence of asprintf and vasprintf

[PATCH] Check for existence of asprintf and vasprintf

[Ulf Hermann]

Add replacements to libeu.a if they don't exist. Include system.h
and link against libeu.a where they are used.

Signed-off-by: Ulf Hermann <ulf.hermann@qt.io>
---
 ChangeLog              |  4 ++++
 configure.ac           | 10 ++++++++++
 lib/ChangeLog          | 10 ++++++++++
 lib/Makefile.am        |  8 ++++++++
 lib/asprintf.c         | 46 ++++++++++++++++++++++++++++++++++++++++++++++
 lib/color.c            |  1 +
 lib/system.h           |  9 +++++++++
 lib/vasprintf.c        | 49 +++++++++++++++++++++++++++++++++++++++++++++++++
 libdwfl/ChangeLog      |  4 ++++
 libdwfl/offline.c      |  1 +
 src/ChangeLog          |  5 +++++
 src/arlib-argp.c       |  1 +
 src/unstrip.c          |  1 +
 tests/ChangeLog        |  7 +++++++
 tests/Makefile.am      |  4 ++--
 tests/backtrace-data.c |  2 ++
 tests/backtrace.c      |  2 ++
 17 files changed, 162 insertions(+), 2 deletions(-)
 create mode 100644 lib/asprintf.c
 create mode 100644 lib/vasprintf.c

diff --git a/ChangeLog b/ChangeLog
index e97e02b..8e01ce2 100644
--- a/ChangeLog
+++ b/ChangeLog
@@ -1,3 +1,7 @@
+2017-02-22  Ulf Hermann  <ulf.hermann@qt.io>
+
+	* configure.ac: Add checks for asprintf and vasprintf.
+
 2017-02-20  Ulf Hermann  <ulf.hermann@qt.io>
 
 	* configure.ac: Track which components are enabled and output
diff --git a/configure.ac b/configure.ac
index ed60be5..3d4bb70 100644
--- a/configure.ac
+++ b/configure.ac
@@ -290,6 +290,16 @@ AC_CHECK_DECLS([mempcpy],[],[],
                 #include <string.h>])
 AM_CONDITIONAL(HAVE_MEMPCPY, [test "x$ac_cv_have_decl_mempcpy" = "xyes"])
 
+AC_CHECK_DECLS([asprintf],[],[],
+               [#define _GNU_SOURCE
+                #include <stdio.h>])
+AM_CONDITIONAL(HAVE_ASPRINTF, [test "x$ac_cv_have_decl_asprintf" = "xyes"])
+
+AC_CHECK_DECLS([vasprintf],[],[],
+               [#define _GNU_SOURCE
+                #include <stdio.h>])
+AM_CONDITIONAL(HAVE_VASPRINTF, [test "x$ac_cv_have_decl_vasprintf" = "xyes"])
+
 AC_CHECK_LIB([stdc++], [__cxa_demangle], [dnl
 AC_DEFINE([USE_DEMANGLE], [1], [Defined if demangling is enabled])])
 AM_CONDITIONAL(DEMANGLE, test "x$ac_cv_lib_stdcpp___cxa_demangle" = "xyes")
diff --git a/lib/ChangeLog b/lib/ChangeLog
index c97537e..d0229ec 100644
--- a/lib/ChangeLog
+++ b/lib/ChangeLog
@@ -1,3 +1,13 @@
+2017-02-22  Ulf Hermann  <ulf.hermann@qt.io>
+
+	* Makefile.am (libeu_a_SOURCES): Add asprintf.c and vasprintf.c
+	if !HAVE_ASPRINTF and !HAVE_VASPRINTF, respectively.
+	* asprintf.c: New file.
+	* vasprintf.c: New file.
+	* color.c: Include system.h.
+	* system.h: Include stdarg.h, add declarations for asprintf and
+	vasprintf if they are not available from libc.
+
 2017-02-20  Ulf Hermann  <ulf.hermann@qt.io>
 
 	* Makefile.am (libeu_a_SOURCES): Skip printversion.c and
diff --git a/lib/Makefile.am b/lib/Makefile.am
index 93a965c..02e6bd9 100644
--- a/lib/Makefile.am
+++ b/lib/Makefile.am
@@ -44,6 +44,14 @@ if !HAVE_MEMPCPY
 libeu_a_SOURCES += mempcpy.c
 endif
 
+if !HAVE_ASPRINTF
+libeu_a_SOURCES += asprintf.c
+endif
+
+if !HAVE_VASPRINTF
+libeu_a_SOURCES += vasprintf.c
+endif
+
 if ARGP
 libeu_a_SOURCES += printversion.c color.c
 noinst_HEADERS += printversion.h color.h
diff --git a/lib/asprintf.c b/lib/asprintf.c
new file mode 100644
index 0000000..4e10f5c
--- /dev/null
+++ b/lib/asprintf.c
@@ -0,0 +1,46 @@
+/* Implementation of asprintf, using vasprintf
+   Copyright (C) 2017 The Qt Company Ltd.
+   This file is part of elfutils.
+
+   This file is free software; you can redistribute it and/or modify
+   it under the terms of either
+
+     * the GNU Lesser General Public License as published by the Free
+       Software Foundation; either version 3 of the License, or (at
+       your option) any later version
+
+   or
+
+     * the GNU General Public License as published by the Free
+       Software Foundation; either version 2 of the License, or (at
+       your option) any later version
+
+   or both in parallel, as here.
+
+   elfutils is distributed in the hope that it will be useful, but
+   WITHOUT ANY WARRANTY; without even the implied warranty of
+   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
+   General Public License for more details.
+
+   You should have received copies of the GNU General Public License and
+   the GNU Lesser General Public License along with this program.  If
+   not, see <http://www.gnu.org/licenses/>.  */
+
+#ifdef HAVE_CONFIG_H
+# include <config.h>
+#endif
+
+#include "system.h"
+#include <stdarg.h>
+#include <stdio.h>
+#include <stdlib.h>
+
+int
+asprintf(char **strp, const char *fmt, ...)
+{
+    va_list ap;
+    va_start(ap, fmt);
+    int result = vasprintf(strp, fmt, ap);
+    va_end(ap);
+    return result;
+}
diff --git a/lib/color.c b/lib/color.c
index f62389d..3362f83 100644
--- a/lib/color.c
+++ b/lib/color.c
@@ -39,6 +39,7 @@
 #include <unistd.h>
 #include "libeu.h"
 #include "color.h"
+#include "system.h"
 
 /* Prototype for option handler.  */
 static error_t parse_opt (int key, char *arg, struct argp_state *state);
diff --git a/lib/system.h b/lib/system.h
index 6ddbb2d..b6f2269 100644
--- a/lib/system.h
+++ b/lib/system.h
@@ -40,6 +40,7 @@
 #include <endian.h>
 #include <byteswap.h>
 #include <unistd.h>
+#include <stdarg.h>
 
 #if __BYTE_ORDER == __LITTLE_ENDIAN
 # define LE32(n)	(n)
@@ -71,6 +72,14 @@
 void *mempcpy(void *dest, const void *src, size_t n);
 #endif
 
+#if !HAVE_DECL_ASPRINTF
+int asprintf (char **strp, const char *fmt, ...);
+#endif
+
+#if !HAVE_DECL_VASPRINTF
+int vasprintf (char **strp, const char *fmt, va_list ap);
+#endif
+
 /* A special gettext function we use if the strings are too short.  */
 #define sgettext(Str) \
   ({ const char *__res = strrchr (gettext (Str), '|');			      \
diff --git a/lib/vasprintf.c b/lib/vasprintf.c
new file mode 100644
index 0000000..0f41fbc
--- /dev/null
+++ b/lib/vasprintf.c
@@ -0,0 +1,49 @@
+/* Implementation of vasprintf, using vsnprintf
+   Copyright (C) 2017 The Qt Company Ltd.
+   This file is part of elfutils.
+
+   This file is free software; you can redistribute it and/or modify
+   it under the terms of either
+
+     * the GNU Lesser General Public License as published by the Free
+       Software Foundation; either version 3 of the License, or (at
+       your option) any later version
+
+   or
+
+     * the GNU General Public License as published by the Free
+       Software Foundation; either version 2 of the License, or (at
+       your option) any later version
+
+   or both in parallel, as here.
+
+   elfutils is distributed in the hope that it will be useful, but
+   WITHOUT ANY WARRANTY; without even the implied warranty of
+   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
+   General Public License for more details.
+
+   You should have received copies of the GNU General Public License and
+   the GNU Lesser General Public License along with this program.  If
+   not, see <http://www.gnu.org/licenses/>.  */
+
+#ifdef HAVE_CONFIG_H
+# include <config.h>
+#endif
+
+#include "system.h"
+#include <stdarg.h>
+#include <stdio.h>
+#include <stdlib.h>
+
+int
+vasprintf(char **strp, const char *fmt, va_list ap)
+{
+    va_list test;
+    va_copy(test, ap);
+    int length = vsnprintf(NULL, 0, fmt, test);
+    va_end(test);
+    *strp = malloc(length + 1);
+    if (*strp == NULL)
+        return -1;
+    return vsnprintf(*strp, length + 1, fmt, ap);
+}
diff --git a/libdwfl/ChangeLog b/libdwfl/ChangeLog
index 570c66f..75358dc 100644
--- a/libdwfl/ChangeLog
+++ b/libdwfl/ChangeLog
@@ -1,3 +1,7 @@
+2017-02-22  Ulf Hermann  <ulf.hermann@qt.io>
+
+	* offline.c: Include system.h.
+
 2017-02-20  Ulf Hermann  <ulf.hermann@qt.io>
 
 	* argp-dummy.c: New file.
diff --git a/libdwfl/offline.c b/libdwfl/offline.c
index c0a2599..24a9b6c 100644
--- a/libdwfl/offline.c
+++ b/libdwfl/offline.c
@@ -27,6 +27,7 @@
    not, see <http://www.gnu.org/licenses/>.  */
 
 #include "libdwflP.h"
+#include <system.h>
 #include <fcntl.h>
 #include <unistd.h>
 
diff --git a/src/ChangeLog b/src/ChangeLog
index d26169b..bdf04cc 100644
--- a/src/ChangeLog
+++ b/src/ChangeLog
@@ -1,3 +1,8 @@
+2017-02-22  Ulf Hermann  <ulf.hermann@qt.io>
+
+	* arlib-argp.c: Include system.h.
+	* unstrip.c: Likewise.
+
 2017-02-17  Ulf Hermann  <ulf.hermann@qt.io>
 
 	* elfcompress.c: Include system.h.
diff --git a/src/arlib-argp.c b/src/arlib-argp.c
index 1bdd8d0..b2847ac 100644
--- a/src/arlib-argp.c
+++ b/src/arlib-argp.c
@@ -21,6 +21,7 @@
 
 #include <argp.h>
 #include <libintl.h>
+#include <system.h>
 
 #include "arlib.h"
 
diff --git a/src/unstrip.c b/src/unstrip.c
index 6e57a6b..7a86ead 100644
--- a/src/unstrip.c
+++ b/src/unstrip.c
@@ -51,6 +51,7 @@
 #include "libdwelf.h"
 #include "libeu.h"
 #include "printversion.h"
+#include "system.h"
 
 #ifndef _
 # define _(str) gettext (str)
diff --git a/tests/ChangeLog b/tests/ChangeLog
index 7b15ec5..0e7e28c 100644
--- a/tests/ChangeLog
+++ b/tests/ChangeLog
@@ -1,3 +1,10 @@
+2017-02-22  Ulf Hermann  <ulf.hermann@qt.io>
+
+	* Makefile.am (backtrace_LDADD): Link against libeu.a.
+	(backtrace_data_LDADD): Likewise.
+	* backtrace.c: Include system.h.
+	* backtrace-data.c: Likewise.
+
 2017-02-15  Ulf Hermann  <ulf.hermann@qt.io>
 
 	* Makefile.am: Link asm tests and elfstrmerge against libeu.a.
diff --git a/tests/Makefile.am b/tests/Makefile.am
index 1dc85f9..5826703 100644
--- a/tests/Makefile.am
+++ b/tests/Makefile.am
@@ -465,12 +465,12 @@ dwflsyms_LDADD = $(libdw) $(libelf) $(argp_LDADD)
 dwfllines_LDADD = $(libdw) $(libelf) $(argp_LDADD)
 dwfl_report_elf_align_LDADD = $(libdw)
 varlocs_LDADD = $(libdw) $(libelf) $(argp_LDADD)
-backtrace_LDADD = $(libdw) $(libelf) $(argp_LDADD)
+backtrace_LDADD = $(libdw) $(libelf) $(libeu) $(argp_LDADD)
 # backtrace-child-biarch also uses those *_CFLAGS and *_LDLAGS variables:
 backtrace_child_CFLAGS = -fPIE
 backtrace_child_LDFLAGS = -pie -pthread
 backtrace_child_biarch_SOURCES = backtrace-child.c
-backtrace_data_LDADD = $(libdw) $(libelf)
+backtrace_data_LDADD = $(libdw) $(libelf) $(libeu)
 backtrace_dwarf_CFLAGS = -Wno-unused-parameter
 backtrace_dwarf_LDADD = $(libdw) $(libelf)
 debuglink_LDADD = $(libdw) $(libelf)
diff --git a/tests/backtrace-data.c b/tests/backtrace-data.c
index a387d8f..68508c0 100644
--- a/tests/backtrace-data.c
+++ b/tests/backtrace-data.c
@@ -42,6 +42,8 @@
 #include ELFUTILS_HEADER(dwfl)
 #endif
 
+#include <system.h>
+
 #if !defined(__x86_64__) || !defined(__linux__)
 
 int
diff --git a/tests/backtrace.c b/tests/backtrace.c
index 34a2ab0..e35563b 100644
--- a/tests/backtrace.c
+++ b/tests/backtrace.c
@@ -40,6 +40,8 @@
 #include ELFUTILS_HEADER(dwfl)
 #endif
 
+#include <system.h>
+
 #ifndef __linux__
 
 int
-- 
2.1.4

Re: [PATCH] Check for existence of asprintf and vasprintf

[Mike Frysinger]

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

On 22 Feb 2017 13:50, Ulf Hermann wrote:
> Add replacements to libeu.a if they don't exist. Include system.h
> and link against libeu.a where they are used.

these portability replacements are starting to get out of hand
-mike

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH] Check for existence of asprintf and vasprintf

[Ulf Hermann]

> these portability replacements are starting to get out of hand
> -mike
To what extent should elfutils be portable to non-GNU systems? My goal here is to port it to windows while minimizing the amount of external dependencies I have to add. The functions I have replaced so far are so trivial that I didn't consider it worthwhile to import things from gnulib in order to provide them.

Ulf

Re: [PATCH] Check for existence of asprintf and vasprintf

[Mike Frysinger]

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

On 22 Feb 2017 17:01, Ulf Hermann wrote:
> > these portability replacements are starting to get out of hand
> 
> To what extent should elfutils be portable to non-GNU systems? My goal here is to port it to windows while minimizing the amount of external dependencies I have to add. The functions I have replaced so far are so trivial that I didn't consider it worthwhile to import things from gnulib in order to provide them.

imo, elfutils shouldn't be growing these fallback implementations itself.
if you want to do this stuff, use gnulib instead.

then there is no ifdef hell in the source files, and you don't have to
worry about testing whether the ifdef's are correct because gnulib did
it all for you.
-mike

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH] Check for existence of asprintf and vasprintf

[Mark Wielaard]

On Wed, Feb 22, 2017 at 05:01:51PM +0100, Ulf Hermann wrote:
> > these portability replacements are starting to get out of hand
> To what extent should elfutils be portable to non-GNU systems?

That has never really been a goal. elfutils is specifically for
handling ELF files and DWARF data on a GNU/Linux systems. We try to
accommodate other systems that use ELF and DWARF though if it is easy
to do.

> My goal here is to port it to windows while minimizing the amount of
> external dependencies I have to add. The functions I have replaced so
> far are so trivial that I didn't consider it worthwhile to import
> things from gnulib in order to provide them.

I haven't had time to review your patches yet. Sorry.
But I would prefer to not have to maintain a whole portability
layer. gnulib was specifically made for this kind of portability.
So if we can just import modules from gnulib that would be my
preference. That way all we have to do is refresh the modules
with gnulib-tool --update around release time.

Cheers,

Mark

Re: [PATCH] Check for existence of asprintf and vasprintf

[Ulf Hermann]

> imo, elfutils shouldn't be growing these fallback implementations itself.
> if you want to do this stuff, use gnulib instead.
> 
> then there is no ifdef hell in the source files, and you don't have to
> worry about testing whether the ifdef's are correct because gnulib did
> it all for you.

OK, so I won't post any more patches with replacement functions. I don't like the idea of using gnulib because the gnulib implementations are generally more complex and in order to maintain them I would have to track gnulib in addition to elfutils. Also I would have to review the licenses for each import. This is not something I'm excited to do for a few 3-line functions. But OK, I will consider it.

What about switching off functionality if certain conditions aren't met? For example the patch that allows us to build libelf and libdw if argp is not available. Are such changes acceptable? I have something similar for obstack, and possibly for fts.

Ulf

Re: [PATCH] Check for existence of asprintf and vasprintf

[Mike Frysinger]

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

On 22 Feb 2017 17:45, Ulf Hermann wrote:
> > imo, elfutils shouldn't be growing these fallback implementations itself.
> > if you want to do this stuff, use gnulib instead.
> > 
> > then there is no ifdef hell in the source files, and you don't have to
> > worry about testing whether the ifdef's are correct because gnulib did
> > it all for you.
> 
> OK, so I won't post any more patches with replacement functions. I don't like the idea of using gnulib because the gnulib implementations are generally more complex and in order to maintain them I would have to track gnulib in addition to elfutils.

sorry, but i don't know what you're talking about.  you don't read the
gnulib code/modules directly, you just run gnulib-tool and tell it which
modules to import.  it does all the rest for you.

you want asprintf ?  then add it to the list.
modules=(
	asprintf
	glob
	vasprintf
	...whatever else you want...
)
gnulib-tool --import "${modules[@]}"

stick that in an autogen.sh script.

> Also I would have to review the licenses for each import. This is not something I'm excited to do for a few 3-line functions. But OK, I will consider it.

gnulib-tool has a --lgpl=[...] flag so you can automatically abort if
the desired license compatibility level isn't met.  so you don't have
to directly review every module if it isn't aborting.
-mike

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH] Check for existence of asprintf and vasprintf

[Ulf Hermann]

> sorry, but i don't know what you're talking about.  you don't read the
> gnulib code/modules directly, you just run gnulib-tool and tell it which
> modules to import.  it does all the rest for you.
> 
> you want asprintf ?  then add it to the list.
> modules=(
> 	asprintf
> 	glob
> 	vasprintf
> 	...whatever else you want...
> )
> gnulib-tool --import "${modules[@]}"

I see. Some of the functions are not available from gnulib, though. In particular, GNU-style basename abd GNU-style strerror_r. There might be more.

Ulf

Re: [PATCH] Check for existence of asprintf and vasprintf

[Mike Frysinger]

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

On 22 Feb 2017 18:43, Ulf Hermann wrote:
> > sorry, but i don't know what you're talking about.  you don't read the
> > gnulib code/modules directly, you just run gnulib-tool and tell it which
> > modules to import.  it does all the rest for you.
> > 
> > you want asprintf ?  then add it to the list.
> > modules=(
> > 	asprintf
> > 	glob
> > 	vasprintf
> > 	...whatever else you want...
> > )
> > gnulib-tool --import "${modules[@]}"
> 
> I see. Some of the functions are not available from gnulib, though. In particular, GNU-style basename abd GNU-style strerror_r. There might be more.

you're probably looking at the raw module list.
you'll want to look at the full documentation instead:
	https://www.gnu.org/software/gnulib/MODULES.html

basename() is in the dirname module

you're correct that GNU strerror_r is not handled by gnulib.
that doesn't look like it's too hard to deal with, but it is
something that'd have to be considered.
-mike

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH] Check for existence of asprintf and vasprintf

[John Ogness]

On 2017-02-22, Mike Frysinger <vapier@gentoo.org> wrote:
> you're correct that GNU strerror_r is not handled by gnulib.
> that doesn't look like it's too hard to deal with, but it is
> something that'd have to be considered.

And if someone is going to take the time to implement strerror_r, really
the patch should go to gnulib, not here.

John Ogness

Re: [PATCH] Check for existence of asprintf and vasprintf

[Ulf Hermann]

> basename() is in the dirname module

That's the POSIX variant. We're using the GNU variant everywhere and the GNU variant is a whopping two lines of code:

char *base = strrchr(path, '/');
return base ? base + 1 : (char *)path;

> you're correct that GNU strerror_r is not handled by gnulib.
> that doesn't look like it's too hard to deal with, but it is
> something that'd have to be considered.

We're using strerror_r in exactly one place and my proposal is to just return a fixed string if we hit that on platforms where GNU strerror_r is not available.

Ulf

Re: [PATCH] Check for existence of asprintf and vasprintf

[Ulf Hermann]

First, I'm not sure if we want to import the respective gnulib modules directly into the elfutils code base or if you want me to do this in my fork. In the latter case the issue is settled as there is no value for me in jumping through hoops if the code is not going to be upstreamed anyway. So, for now I'm assuming we're talking about importing gnulib modules into the elfutils code base.

> gnulib-tool has a --lgpl=[...] flag so you can automatically abort if
> the desired license compatibility level isn't met.  so you don't have
> to directly review every module if it isn't aborting.

Are you aware that for most of those modules, building them into elfutils restricts the license choices for the resulting combination? Any non-trivial combination of the required modules with elfutils makes the result de facto GPLv3 only. GPLv3 is fine for me as perfparser is also GPLv3, but as elfutils so far is LGPLv3+/GPLv2+ I'm wondering if we want to do this. In fact, when just doing the usual "configure/make/make install" procedure without reviewing the intermediate results, a user would have no way of knowing what license applies to the binaries. IMO that is bad and will certainly lead to problems somewhere down the line.

Ulf

Re: [PATCH] Check for existence of asprintf and vasprintf

[Ulf Hermann]

Please compare the following code with asprintf.c/vasprintf.c/vasnprintf.c from gnulib. asprintf depends on vasprintf which in turn depends on vasnprintf there. vasnprintf is non-standard and not even available on glibc, while vsnprintf as used by my implementation is standardized by POSIX and widely available (even on windows). Granted, we have two calls to vsnprintf in my code vs one call to vasnprintf in the gnulib code. However, if that becomes an issue, I would rather go for some platform-specific optimization (windows also has a number of non-standard and possibly useful *printf functions) rather than importing such a monster.

> +int
> +asprintf(char **strp, const char *fmt, ...)
> +{
> +    va_list ap;
> +    va_start(ap, fmt);
> +    int result = vasprintf(strp, fmt, ap);
> +    va_end(ap);
> +    return result;
> +}
> +
> +int
> +vasprintf(char **strp, const char *fmt, va_list ap)
> +{
> +    va_list test;
> +    va_copy(test, ap);
> +    int length = vsnprintf(NULL, 0, fmt, test);
> +    va_end(test);
> +    *strp = malloc(length + 1);
> +    if (*strp == NULL)
> +        return -1;
> +    return vsnprintf(*strp, length + 1, fmt, ap);
> +}

Re: [PATCH] Check for existence of asprintf and vasprintf

[Mark Wielaard]

On Thu, Feb 23, 2017 at 11:18:53AM +0100, Ulf Hermann wrote:
> First, I'm not sure if we want to import the respective gnulib modules
> directly into the elfutils code base or if you want me to do this in
> my fork. In the latter case the issue is settled as there is no value
> for me in jumping through hoops if the code is not going to be
> upstreamed anyway. So, for now I'm assuming we're talking about
> importing gnulib modules into the elfutils code base.

I would be fine with integrating gnulib code upstream.
Since we already have a lib and m4 directory it would probably
be easiest if we use a new gnulib and gnulib-m4 top-level dir
for that. And we'll have to figure out what the best way to handle
the sources in the repository is. I think I would prefer option 3
https://www.gnu.org/software/gnulib/manual/html_node/VCS-Issues.html
So normal developers don't need to have gnulib itself installed.
The release manager would run gnulib-tool --update before release.
But maybe there are other ways that are more natural to support?

> > gnulib-tool has a --lgpl=[...] flag so you can automatically abort if
> > the desired license compatibility level isn't met.  so you don't have
> > to directly review every module if it isn't aborting.
> 
> Are you aware that for most of those modules, building them into elfutils
> restricts the license choices for the resulting combination? Any
> non-trivial combination of the required modules with elfutils makes the
> result de facto GPLv3 only. GPLv3 is fine for me as perfparser is also
> GPLv3, but as elfutils so far is LGPLv3+/GPLv2+ I'm wondering if we want
> to do this. In fact, when just doing the usual
> "configure/make/make install" procedure without reviewing the
> intermediate results, a user would have no way of knowing what license
> applies to the binaries. IMO that is bad and will certainly lead to
> problems somewhere down the line.

As long as everything is upward compatible with GPLv3+ I think that is
fine. It will only be an issue on non-GNU/Linux setups. But we should
indeed make clear during configure time when extra license requirements
would apply because gnulib code is being used/imported. Maybe we can
just have configure warn/error out "Your setup requires importing of
GPLv3+ gnulib code, please configure with --enable-gplv3-gnulib".
Or something like that.

Cheers,

Mark

Re: [PATCH] Check for existence of asprintf and vasprintf

[Mike Frysinger]

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

On 23 Feb 2017 10:39, Ulf Hermann wrote:
> > basename() is in the dirname module
> 
> That's the POSIX variant. We're using the GNU variant everywhere

looking closer, it's not either.  POSIX doesn't guarantee that the input
is not modified, while GNU guarantees that it is left alone.  the gnulib
one makes the same guarantee as GNU, but it also returns a new buffer.

seems like getting a new basename module into gnulib wouldn't be *too*
difficult to pull off.

> and the GNU variant is a whopping two lines of code:
> 
> char *base = strrchr(path, '/');
> return base ? base + 1 : (char *)path;

and we get straight to an example of why your solutions don't scale.
your replacement is wrong.  and ironically, it's wrong on Windows,
which is the whole point of your work.  '/' is not the path sep used
on every system out there.  it also does not properly handle systems
(such as Windows) that have filesystem prefixes like C:\.

> > you're correct that GNU strerror_r is not handled by gnulib.
> > that doesn't look like it's too hard to deal with, but it is
> > something that'd have to be considered.
> 
> We're using strerror_r in exactly one place and my proposal is to just return a fixed string if we hit that on platforms where GNU strerror_r is not available.

i don't have an opinion on that
-mike

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH] Check for existence of asprintf and vasprintf

[Ulf Hermann]

>> and the GNU variant is a whopping two lines of code:
>>
>> char *base = strrchr(path, '/');
>> return base ? base + 1 : (char *)path;
>
> and we get straight to an example of why your solutions don't scale.
> your replacement is wrong.  and ironically, it's wrong on Windows,
> which is the whole point of your work.  '/' is not the path sep used
> on every system out there.  it also does not properly handle systems
> (such as Windows) that have filesystem prefixes like C:\.

Both basename variants' documentations only talk about '/'. This is not 
the place where we should handle windows directory separators. We should 
either already expect forward slashes as input (with drive names as 
first path component, as e.g. msys does it), or convert them early on. 
And the implementation I've given above is the same code as in glibc.

However, I will actually prepare some patches to replace missing 
functions using gnulib rather than my own implementations.

Ulf

Re: [PATCH] Check for existence of asprintf and vasprintf

[Mike Frysinger]

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

On 24 Feb 2017 10:42, Ulf Hermann wrote:
> >> and the GNU variant is a whopping two lines of code:
> >>
> >> char *base = strrchr(path, '/');
> >> return base ? base + 1 : (char *)path;
> >
> > and we get straight to an example of why your solutions don't scale.
> > your replacement is wrong.  and ironically, it's wrong on Windows,
> > which is the whole point of your work.  '/' is not the path sep used
> > on every system out there.  it also does not properly handle systems
> > (such as Windows) that have filesystem prefixes like C:\.
> 
> Both basename variants' documentations only talk about '/'. This is not 
> the place where we should handle windows directory separators.

umm, sure it is.  if dirname/basename don't handle the OS separator, then
where do you think it should be handled ?  every place that calls basename ?

GNU/POSIX not talking about it isn't terribly relevant -- they don't care
about systems like Windows that have diff path name semantics.

although it's a moot point if you move everything to gnulib as they handle
the issue properly.
-mike

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 833 bytes --]