[PATCH] configure.ac: Don't overwrite NO_EXPAT option

[PATCH] configure.ac: Don't overwrite NO_EXPAT option

[Andreas Herrmann]

Even if 'configure --with-expat=no' was run, expat support is used,
because library detection overwrites it. Avoid this overwrite.
(I think, configure should obey what the user has specified.)

Signed-off-by: Andreas Herrmann <aherrmann@suse.de>
---
 configure.ac | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/configure.ac b/configure.ac
index 38ff86678a..62cc8197f8 100644
--- a/configure.ac
+++ b/configure.ac
@@ -581,6 +581,8 @@ fi
 # Define NO_EXPAT if you do not have expat installed.  git-http-push is
 # not built, and you cannot push using http:// and https:// transports.
 
+if test -z "$NO_EXPAT"; then
+
 GIT_STASH_FLAGS($EXPATDIR)
 
 AC_CHECK_LIB([expat], [XML_ParserCreate],
@@ -589,6 +591,8 @@ AC_CHECK_LIB([expat], [XML_ParserCreate],
 
 GIT_UNSTASH_FLAGS($EXPATDIR)
 
+fi
+
 GIT_CONF_SUBST([NO_EXPAT])
 
 #
-- 
2.41.0

Re: [PATCH] configure.ac: Don't overwrite NO_EXPAT option

[Junio C Hamano]

Andreas Herrmann <aherrmann@suse.de> writes:

> Subject: Re: [PATCH] configure.ac: Don't overwrite NO_EXPAT option

Downcase "D" in "Don't" (cf. "git log --oneline --no-merges
--since=2.months"), please.

> Even if 'configure --with-expat=no' was run, expat support is used,
> because library detection overwrites it. Avoid this overwrite.

As I suspect that "configure" is not used a part of any experienced
Git developers' daily development cycle, it is unfortunately very
understandable how this left unnoticed for so long, ever since
424adc50 (autoconf: Move variables which we always set to
config.mak.in, 2006-08-08), if I am reading the patch correctly, as
a part of the very beginning of autoconf support.

Thanks for spotting it, and the fix look straight-forward.

> (I think, configure should obey what the user has specified.)

Sure, but please rephrase this parenthesized sentence.

 * if you are unsure, and leaning to negative, say that below the
   three-dash line.  No need for (parentheses).

 * otherwise, lose the (parentheses), "I think", and in general be
   more assertive.

The latter is more preferrable.  The sentence with "I think" is your
proposal to make it the project policy to make sure that ./configure
script honors what the user gave it.  If other developers disagree
with the policy you propose there, they will object and give their
reason during their reviews.  If we adopt it as the project policy,
which I personally think is very sensible, it is good to have it
stated not as a mere personal opinion of the author of a commit, but
more as a general rule.

Having said all that, I have some further observation.

The NO_EXPAT support does not look *so* special.  It is split into
two parts:

 * Use AC_ARG_WITH(expat) to handle "--with-expat"

 * AC_CHECK_LIB([expat]) to auto-detect NO_EXPAT

Aren't there other symbols that share the same pattern?  For
example, how well do NO_CURL or NO_ICONV (which I picked because
AC_ARG_WITH(curl) comes before and AC_ARG_WITH(iconv) comes after
AC_ARG_WITH(expat)) work?  Do they share the same issue?

A quick "git grep AC_ARG_WITH" finds openssl, libpcre*, and tcltk
also share the same pattern, in addition to curl and iconv.  I may
be misreading the script but USE_LIBPCRE2 support looks OK.
NO_OPENSSL looks iffy, though not wrong per-se.  SHA1_Init is probed
in libcrypto and in libssl regardless of the value of NO_OPENSSL.

I do not think it is a good idea to address the same issue for some
of these other symbols, in addition to expat, in a single patch.
Let's have people review the current patch for NO_EXPAT first, and
leave the other symbols for possible follow-up patches.

Thanks.

Re: [PATCH] configure.ac: Don't overwrite NO_EXPAT option

[Andreas Herrmann]

On Thu, Jul 13, 2023 at 10:02:17AM -0700, Junio C Hamano wrote:
> Andreas Herrmann <aherrmann@suse.de> writes:
> 
> > Subject: Re: [PATCH] configure.ac: Don't overwrite NO_EXPAT option
> 
> Downcase "D" in "Don't" (cf. "git log --oneline --no-merges
> --since=2.months"), please.

Oops, ok.

> > Even if 'configure --with-expat=no' was run, expat support is used,
> > because library detection overwrites it. Avoid this overwrite.
> 
> As I suspect that "configure" is not used a part of any experienced
> Git developers' daily development cycle, it is unfortunately very
> understandable how this left unnoticed for so long, ever since
> 424adc50 (autoconf: Move variables which we always set to
> config.mak.in, 2006-08-08), if I am reading the patch correctly, as
> a part of the very beginning of autoconf support.
> 
> Thanks for spotting it, and the fix look straight-forward.
> 
> > (I think, configure should obey what the user has specified.)
> 
> Sure, but please rephrase this parenthesized sentence.

I'll rephrase it.

>  * if you are unsure, and leaning to negative, say that below the
>    three-dash line.  No need for (parentheses).
> 
>  * otherwise, lose the (parentheses), "I think", and in general be
>    more assertive.
> 
> The latter is more preferrable.  The sentence with "I think" is your
> proposal to make it the project policy to make sure that ./configure
> script honors what the user gave it.  If other developers disagree
> with the policy you propose there, they will object and give their
> reason during their reviews.  If we adopt it as the project policy,
> which I personally think is very sensible, it is good to have it
> stated not as a mere personal opinion of the author of a commit, but
> more as a general rule.
> 
> Having said all that, I have some further observation.
> 
> The NO_EXPAT support does not look *so* special.  It is split into
> two parts:
> 
>  * Use AC_ARG_WITH(expat) to handle "--with-expat"
> 
>  * AC_CHECK_LIB([expat]) to auto-detect NO_EXPAT
> 
> Aren't there other symbols that share the same pattern?

I'll have a look at this.

> For example, how well do NO_CURL or NO_ICONV (which I picked because
> AC_ARG_WITH(curl) comes before and AC_ARG_WITH(iconv) comes after
> AC_ARG_WITH(expat)) work?  Do they share the same issue?

> A quick "git grep AC_ARG_WITH" finds openssl, libpcre*, and tcltk
> also share the same pattern, in addition to curl and iconv.  I may
> be misreading the script but USE_LIBPCRE2 support looks OK.
> NO_OPENSSL looks iffy, though not wrong per-se.  SHA1_Init is probed
> in libcrypto and in libssl regardless of the value of NO_OPENSSL.
> 
> I do not think it is a good idea to address the same issue for some
> of these other symbols, in addition to expat, in a single patch.
> Let's have people review the current patch for NO_EXPAT first, and
> leave the other symbols for possible follow-up patches.

I agree, separate patches is the preferred way if there is more to
fix.

> Thanks.

Thanks for your comments.

-- 
Regards,
Andreas

SUSE Software Solutions Germany GmbH
Frankenstrasse 146, 90461 Nürnberg, Germany
GF: Ivo Totev, Andrew Myers, Andrew McDonald, Martje Boudien Moerman
(HRB 36809, AG Nürnberg)