Re: [PATCH] Makefile: make NO_ICONV really mean "no iconv"

From: Jeff King <peff@peff.net>
To: Eric Sunshine <sunshine@sunshineco.com>
Cc: git@vger.kernel.org, Mahmoud Al-Qudsi <mqudsi@neosmart.net>
Subject: Re: [PATCH] Makefile: make NO_ICONV really mean "no iconv"
Date: Fri, 15 Jun 2018 00:20:23 -0400	[thread overview]
Message-ID: <20180615042023.GA31294@sigill.intra.peff.net> (raw)
In-Reply-To: <20180615022503.34111-1-sunshine@sunshineco.com>

On Thu, Jun 14, 2018 at 10:25:03PM -0400, Eric Sunshine wrote:

> The Makefile tweak NO_ICONV is meant to allow Git to be built without
> iconv in case iconv is not installed or is otherwise dysfunctional.
> However, NO_ICONV's disabling of iconv is incomplete and can incorrectly
> allow "-liconv" to slip into the linker flags when NEEDS_LIBICONV is
> defined, which breaks the build when iconv is not installed.
> 
> On some platforms, iconv lives directly in libc, whereas, on others it
> resides in libiconv. For the latter case, NEEDS_LIBICONV instructs the
> Makefile to add "-liconv" to the linker flags. config.mak.uname
> automatically defines NEEDS_LIBICONV for platforms which require it.
> The adding of "-liconv" is done unconditionally, despite NO_ICONV.
> 
> Work around this problem by making NO_ICONV take precedence over
> NEEDS_LIBICONV.

Nicely explained.

We have OLD_ICONV, too, which should probably do nothing if NO_ICONV is
set. I think that works OK. We end up setting -DOLD_ICONV on the command
line, but that's only consider inside "#ifndef NO_ICONV" within the
code.

> This patch is extra noisy due to the indentation change. Viewing it with
> "git diff -w" helps. An alternative to re-indenting would have been to
> "undefine NEEDS_LIBICONV", however, 'undefine' was added to GNU make in
> 3.82 but MacOS is stuck on 3.81 (from 2006) so 'undefine' was avoided.

Yeah, with "-w" it looks pretty obviously correct.

-Peff