[2/3] configure.ac: define ICONV_OMITS_BOM if necessary
diff mbox series

Message ID 20191031092618.29073-3-congdanhqx@gmail.com
State New
Headers show
Series
  • Linux with musl libc improvement
Related show

Commit Message

Danh Doan Oct. 31, 2019, 9:26 a.m. UTC
From commit 79444c9294, ("utf8: handle systems that don't write BOM for
UTF-16", 2019-02-12), we're supporting those systems with iconv that
omits BOM with:

    make ICONV_OMITS_BOM=Yes

However, typing the flag all the time is cumbersome and error-prone.

Add a checking into configure script to detect this flag automatically.

Signed-off-by: Doan Tran Cong Danh <congdanhqx@gmail.com>
---

Notes:
    We deliberately fail for ac_cv_iconv_omits_bom on cross-compiling,
    in order to ask builder provide the value for the target.
    
    We're relied on this technik for ac_cv_fread_reads_directories and
    ac_cv_snprintf_returns_bogus.
    
    Adding one more failure for configuring on cross-compiling
    is not going to be a burden for distro.

 configure.ac | 22 ++++++++++++++++++++++
 1 file changed, 22 insertions(+)

Comments

Jeff King Oct. 31, 2019, 6:11 p.m. UTC | #1
On Thu, Oct 31, 2019 at 04:26:17PM +0700, Doan Tran Cong Danh wrote:

> From commit 79444c9294, ("utf8: handle systems that don't write BOM for
> UTF-16", 2019-02-12), we're supporting those systems with iconv that
> omits BOM with:
> 
>     make ICONV_OMITS_BOM=Yes
> 
> However, typing the flag all the time is cumbersome and error-prone.
> 
> Add a checking into configure script to detect this flag automatically.

I think it's worth adding this to the configure script, but note that
you can also do:

  echo ICONV_OMITS_BOM=Yes >config.mak

and then "make" by itself will do what you want (it's still annoying and
error-prone to realize you have to specify the flag in the first place).

> diff --git a/configure.ac b/configure.ac
> index a43b476402..790b53bbdc 100644
> --- a/configure.ac
> +++ b/configure.ac
> @@ -690,6 +690,28 @@ fi
>  
>  fi
>  
> +#
> +# Define ICONV_OMITS_BOM if you are on a system which
> +# iconv omits bom for utf-{16,32}
> +if test -z "$NO_ICONV"; then
> +AC_CACHE_CHECK([whether iconv omits bom for utf-16 and utf-32],
> + [ac_cv_iconv_omits_bom],
> +[
> +if test "x$cross_compiling" = xyes; then
> +	AC_MSG_FAILURE([please provide ac_cv_iconv_omits_bom])
> +elif test `printf a | iconv -f utf-8 -t utf-16 | wc -c` = 2; then

The ICONV_OMITS_BOM flag is about the libc iconv that Git will be linked
against. But this is checking the iconv tool. For a system that is using
musl across the board, that would work. But it might not always be the
case (in particular, I don't know if people statically link some
binaries against musl; certainly I've seen people do it with dietlibc).

I think we should be test-compiling a small program, similar to the way
OLD_ICONV works (though I guess we may even need to run the result to
see what happens).

-Peff
brian m. carlson Oct. 31, 2019, 8:02 p.m. UTC | #2
On 2019-10-31 at 18:11:16, Jeff King wrote:
> On Thu, Oct 31, 2019 at 04:26:17PM +0700, Doan Tran Cong Danh wrote:
> > diff --git a/configure.ac b/configure.ac
> > index a43b476402..790b53bbdc 100644
> > --- a/configure.ac
> > +++ b/configure.ac
> > @@ -690,6 +690,28 @@ fi
> >  
> >  fi
> >  
> > +#
> > +# Define ICONV_OMITS_BOM if you are on a system which
> > +# iconv omits bom for utf-{16,32}
> > +if test -z "$NO_ICONV"; then
> > +AC_CACHE_CHECK([whether iconv omits bom for utf-16 and utf-32],
> > + [ac_cv_iconv_omits_bom],
> > +[
> > +if test "x$cross_compiling" = xyes; then
> > +	AC_MSG_FAILURE([please provide ac_cv_iconv_omits_bom])
> > +elif test `printf a | iconv -f utf-8 -t utf-16 | wc -c` = 2; then
> 
> The ICONV_OMITS_BOM flag is about the libc iconv that Git will be linked
> against. But this is checking the iconv tool. For a system that is using
> musl across the board, that would work. But it might not always be the
> case (in particular, I don't know if people statically link some
> binaries against musl; certainly I've seen people do it with dietlibc).

Yeah, I think we need to do that, and not only for musl.  There are
folks for whom the iconv in libc is missing or inadequate and they use
an additional iconv(3) implementation which may not be the same as
iconv(1) (or vice versa).

Granted, as far as we know this option is only needed for musl, but that
doesn't mean there aren't other environments where this is a problem,
only that we don't yet know about them.
Danh Doan Nov. 1, 2019, 1:40 a.m. UTC | #3
On 2019-10-31 14:11:16 -0400, Jeff King wrote:
> The ICONV_OMITS_BOM flag is about the libc iconv that Git will be linked
> against. But this is checking the iconv tool. For a system that is using
> musl across the board, that would work. But it might not always be the
> case (in particular, I don't know if people statically link some
> binaries against musl; certainly I've seen people do it with dietlibc).
> 
> I think we should be test-compiling a small program, similar to the way
> OLD_ICONV works (though I guess we may even need to run the result to
> see what happens).

Originally, I wrote a C program for AC_RUN_IFELSE, but I found out the
complicated of OLD_ICONV and NEEDS_LIBICONV, then I switch to this
approach.

But, your reasoning makes sense. I think people could build a static-linked
git binary for a clean-room build system.

I'll pursue the C program in the re-roll.

Patch
diff mbox series

diff --git a/configure.ac b/configure.ac
index a43b476402..790b53bbdc 100644
--- a/configure.ac
+++ b/configure.ac
@@ -690,6 +690,28 @@  fi
 
 fi
 
+#
+# Define ICONV_OMITS_BOM if you are on a system which
+# iconv omits bom for utf-{16,32}
+if test -z "$NO_ICONV"; then
+AC_CACHE_CHECK([whether iconv omits bom for utf-16 and utf-32],
+ [ac_cv_iconv_omits_bom],
+[
+if test "x$cross_compiling" = xyes; then
+	AC_MSG_FAILURE([please provide ac_cv_iconv_omits_bom])
+elif test `printf a | iconv -f utf-8 -t utf-16 | wc -c` = 2; then
+	ac_cv_iconv_omits_bom=yes
+else
+	ac_cv_iconv_omits_bom=no
+fi
+])
+if test "x$ac_cv_iconv_omits_bom" = xyes; then
+	ICONV_OMITS_BOM=Yes
+else
+	ICONV_OMITS_BOM=
+fi
+GIT_CONF_SUBST([ICONV_OMITS_BOM])
+fi
 #
 # Define NO_DEFLATE_BOUND if deflateBound is missing from zlib.