Re: [PATCH 0/4] Fix tests with missing iconv(1) executable

From: Christian Couder <christian.couder@gmail.com>
Date: Feb 16, 2026, 08:57 UTC
Message-ID: <CAP8UFD0zja_P7fOuCtLt46ubit+QTOME2K4+M9N=CQNceevMBQ@mail.gmail.com>
In-Reply-To: <20260209-b4-pks-ci-msvc-iconv-fixes-v1-0-1e3167cd8828@pks.im>

Hi,

On Mon, Feb 9, 2026 at 1:42 PM Patrick Steinhardt <ps@pks.im> wrote:

Show 12 quoted lines

>
> Hi,
>
> I recently noticed that th MSVC-based tests in GitLab CI started to
> fail. The root cause is that the iconv(1) executable cannot be found on
> this platform anymore. This isn't entirely surprising: we depend on the
> Git for Windows environment to provide necessary shell tools, and that
> environment of course is not a fully fledged MSYS2 installation.
>
> In any case, this patch series fixes those issues by building on top of
> the ICONV prerequisite. If the prereq isn't found, then we also don't
> assume that the iconv(1) executable exists.

I think it's reasonable to assume that iconv isn't available if the ICONV prereq isn't satisfied.

> An alternative strategy would be to introduce a new ICONV_EXECUTABLE
> prereq. But given that Git doesn't perform any kind of reencoding itself
> in case the ICONV support isn't built into it I found it to not be worth
> the additional hassle.

I agree that it's better to not add a new ICONV_EXECUTABLE prereq if possible, as it keeps things simple.

> In any case, this patch series causes the MSVC jobs to pass again on
> GitLab CI.

I think it would be nice if this could talk a bit about the NO_ICONV build knob and how it still relates to the ICONV prereq though.

Before this series, for example, the Makefile says:

  # Define NO_ICONV if your libc does not properly support iconv.

while t/test-lib.sh has:

  test -z "$NO_ICONV" && test_set_prereq ICONV

Unfortunately the diffstat below:

Show 7 quoted lines

>  t/t4041-diff-submodule-option.sh             |  8 +++--
>  t/t4059-diff-submodule-not-initialized.sh    |  8 +++--
>  t/t4060-diff-submodule-option-diff-format.sh |  8 +++--
>  t/t4205-log-pretty-formats.sh                | 50 ++++++++++++++++------------
>  t/t5550-http-fetch-dumb.sh                   | 20 +++++------
>  t/t6006-rev-list-format.sh                   | 29 +++++++++++-----
>  6 files changed, 77 insertions(+), 46 deletions(-)

shows no change in the Makefile, or any build infrastructure file, despite the fact that the series changes the one-to-one relationship between the NO_ICONV build knob and the ICONV prereq.

In the Makefile, for example, I think something like the following would be nice:

diff --git a/Makefile b/Makefile
index 47ed9fa7fd..ed54071fd7 100644
--- a/Makefile
+++ b/Makefile
@@ -182,7 +182,9 @@ include shared.mak
# Define NO_SOCKADDR_STORAGE if your platform does not have struct
# sockaddr_storage.
#
-# Define NO_ICONV if your libc does not properly support iconv.
+# Define NO_ICONV if your libc does not properly support iconv. Note that for
+# simplicity the test suite assumes that iconv(1) is available if and only if
+# NO_ICONV is not defined.
#
# Define OLD_ICONV if your library has an old iconv(), where the second
# (input buffer pointer) parameter is declared with type (const char **).

Not sure how a similar update should be done in configure.ac or
meson.build but maybe it might be worth clarifying things there too.

On the other hand maybe we can say that the situation regarding the
documentation of iconv(1) and the build knobs was quite bad before
this series already and that a full separate patch series would be
required to improve on that. But I think at least this cover letter
should say that.