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

[Andreas Herrmann <aherrmann@suse.de>]

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)