Re: [PATCH v2 1/5] cmake: make it easier to diagnose regressions in CTest runs

From: "Ævar Arnfjörð Bjarmason" <avarab@gmail.com>
To: Johannes Schindelin via GitGitGadget <gitgitgadget@gmail.com>
Cc: git@vger.kernel.org, Phillip Wood <phillip.wood123@gmail.com>,
	Johannes Schindelin <johannes.schindelin@gmx.de>,
	Eric Sunshine <ericsunshine@gmail.com>
Subject: Re: [PATCH v2 1/5] cmake: make it easier to diagnose regressions in CTest runs
Date: Thu, 08 Sep 2022 09:22:10 +0200	[thread overview]
Message-ID: <220908.86illyl3yo.gmgdl@evledraar.gmail.com> (raw)
In-Reply-To: <e00cb37b98ac09cff010e843ef19eeec761f8985.1661243463.git.gitgitgadget@gmail.com>

On Tue, Aug 23 2022, Johannes Schindelin via GitGitGadget wrote:

> From: Johannes Schindelin <johannes.schindelin@gmx.de>
>
> When a test script fails in Git's test suite, the usual course of action
> is to re-run it using options to increase the verbosity of the output,
> e.g. `-v` and `-x`.
>
> Like in Git's CI runs, when running the tests in Visual Studio via the
> CTest route, it is cumbersome or at least requires a very unintuitive
> approach to pass options to the test scripts.
>
> So let's just pass those options by default: This will not clutter any
> output window but the log that is written to a log file will have
> information necessary to figure out test failures.
>
> While at it, also imitate what the Windows jobs in Git's CI runs do to
> accelerate running the test scripts: pass the `--no-bin-wrappers` and
> `--no-chain-lint` options. This makes the test runs noticeably faster
> because the `bin-wrappers/` scripts as well as the `chain-lint` code
> make heavy use of POSIX shell scripting, which is really, really slow on
> Windows due to the need to emulate POSIX behavior via the MSYS2 runtime.
>
> Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
> ---
>  contrib/buildsystems/CMakeLists.txt | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/contrib/buildsystems/CMakeLists.txt b/contrib/buildsystems/CMakeLists.txt
> index 1b23f2440d8..4aee1e24342 100644
> --- a/contrib/buildsystems/CMakeLists.txt
> +++ b/contrib/buildsystems/CMakeLists.txt
> @@ -1088,7 +1088,7 @@ file(GLOB test_scipts "${CMAKE_SOURCE_DIR}/t/t[0-9]*.sh")
>  #test
>  foreach(tsh ${test_scipts})
>  	add_test(NAME ${tsh}
> -		COMMAND ${SH_EXE} ${tsh}
> +		COMMAND ${SH_EXE} ${tsh} --no-bin-wrappers --no-chain-lint -vx
>  		WORKING_DIRECTORY ${CMAKE_SOURCE_DIR}/t)
>  endforeach()

The -vx part of this looks sensible, and matches the commit message
$subject.

I think the "--no-bin-wrappers --no-chain-lint" and the "while at it"
here should be stripped out, and put into its own commit.

Which, as I commented on in the v1[1] is making the implicit assumption
that this cmake file is only used on Windows, but since 561962479c
(cmake: fix CMakeLists.txt on Linux, 2022-05-24) that isn't the case.

So, perhaps we should have a performance hack due to Windows's slowness,
but:

 A. It should be guarded by some "is windows?" check. Your commit
    message justifies why this is a good idea on Windows, but completely
    elides over the fact that this isn't Windows-specific code anymore.

 B. We can still build wind "make" or "cmake", I don't see a reason for
    why such a perf hack (if we decide to keep it) should depend on the
    build system.

 C. Since I sent [1] we've had submitted chainlint.pl in-flight
    series. It's partly trying to take special considerations to be fast
    on Windows. I don't think it's the case with that series that this
    needs to be skipped on Windows anymore (and if it is, Eric would
    like to know).

1.  https://lore.kernel.org/git/220811.861qtnqb5p.gmgdl@evledraar.gmail.com/