Re: [PATCH v3 09/12] cmake: support GIT_TEST_OPTS, abstract away WIN32 defaults

["Ævar Arnfjörð Bjarmason" <avarab@gmail.com>]

On Thu, Nov 03 2022, Phillip Wood wrote:

> On 01/11/2022 22:51, Ævar Arnfjörð Bjarmason wrote:
>> The rationale for adding "--no-bin-wrappers" and "--no-chain-lint" in
>> 2ea1d8b5563 (cmake: make it easier to diagnose regressions in CTest
>> runs, 2022-10-18) was those options slowed down the tests considerably
>> on Windows.
>> But since f31b6244950 (Merge branch 'yw/cmake-updates', 2022-06-07)
>> and with the preceding commits cmake and ctest are not
>> Windows-specific anymore.
>> So let's set those same options by default on Windows, but do so
>> with
>> the set() facility. As noted in cmake's documentation[1] this
>> integrates nicely with e.g. cmake-gui.
>
> Shouldn't there a documentation string for the variable if you want to
> support cmake-gui?

Yeah, I missed that. Now I've actually tested it locally with cmake-gui,
and it works.

>> On *nix we don't set any custom options. The change in 2ea1d8b5563
>> didn't discuss why Windows should have divergent defaults with "cmake"
>> and "make", but such reasons presumably don't apply on *nix. I for one
>> am happy with the same defaults as the tests have when running via the
>> Makefile.
>> With the "message()" addition we'll emit this when running cmake:
>> 	Generating hook-list.h
>> 	-- Using user-selected test options: -vixd
>> 	-- Configuring done
>> 	-- Generating done
>> 	-- Build files have been written to: /home/avar/g/git/contrib/buildsystems/out
>> Unfortunately cmake doesn't support a non-hacky way to pass
>> variables
>> to ctest without re-running cmake itself, so when re-running tests via
>> cmake and wanting to change the test defaults we'll need:
>> 	GIT_TEST_OPTS=-i cmake -S contrib/buildsystems -B
>> contrib/buildsystems/out &&
>> 	ctest --jobs=$(nproc) --test-dir contrib/buildsystems/out -R t0071 --verbose
>
> Rather than having to rerun cmake I think it would be nicer to use the
> shell to pass the test options when the tests are run so the user can 
> set their preferred defaults when running cmake but override them with
> GIT_TEST_OPTIONS when running ctest as I showed previously.

Yeah, I saw that, sorry about not directly addressing it. I tried it,
but in the end I think I'd rather narrow down the scope in this series.

I.e. before it's hardcoded with no way to change it, after you can set
it at build time.

cmake's model of viewing the world seems to really dislike the notion of
dynamically setting test options for whatever reason, it's a FAQ about
cmake/ctest. Your sh -c workaround is clever, and there's similar
workarounds e.g. on stackoverflow.

But let's pursue that separately, and just go for the "way cmake likes
it" in this series.

>> 1. https://cmake.org/cmake/help/latest/command/set.html#set-cache-entry
>> Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
>> ---
>>   contrib/buildsystems/CMakeLists.txt | 46 +++++++++++++++++++++++++++--
>>   1 file changed, 44 insertions(+), 2 deletions(-)
>> diff --git a/contrib/buildsystems/CMakeLists.txt
>> b/contrib/buildsystems/CMakeLists.txt
>> index f0de37b35a1..6a3240d4ffa 100644
>> --- a/contrib/buildsystems/CMakeLists.txt
>> +++ b/contrib/buildsystems/CMakeLists.txt
>> @@ -49,7 +49,7 @@ To use this in Visual Studio:
>>     Open the worktree as a folder. Visual Studio 2019 and later will
>> detect
>>   the CMake configuration automatically and set everything up for you,
>> -ready to build. You can then run the tests in `t/` via a regular Git Bash.
>> +ready to build. See "== Running the tests ==" below for running the tests.
>>     Note: Visual Studio also has the option of opening
>> `CMakeLists.txt`
>>   directly; Using this option, Visual Studio will not find the source code,
>> @@ -74,6 +74,35 @@ empty(default) :
>>     NOTE: -DCMAKE_BUILD_TYPE is optional. For multi-config
>> generators like Visual Studio
>>   this option is ignored
>> +
>> +== Running the tests ==
>> +
>> +Once we've built in "contrib/buildsystems/out" the tests can be run at
>> +the top-level (note: not the generated "contrib/buildsystems/out/t/"
>> +drectory). If no top-level build is found (as created with the
>> +Makefile) the t/test-lib.sh will discover the git in
>> +"contrib/buildsystems/out" on e.g.:
>> +
>> +	(cd t && ./t0001-init.sh)
>> +	setup: had no ../git, but found & used cmake built git in ../contrib/buildsystems/out/git
>> +	[...]
>> +
>> +The tests can also be run with ctest, e.g. after building with "cmake"
>> +and "make" or "msbuild" run, from the top-level e.g.:
>> +
>> +	ctest --test-dir contrib/buildsystems/out --jobs="$(nproc)"--output-on-failure
>
> CmakeLists.txt claims we only require v3.14 which does not appear to
> support --test-dir (see 
> https://cmake.org/cmake/help/v3.14/manual/ctest.1.html)

Thanks, I missed that, updated docs etc. as appropriate in the incoming
re-roll.

>> +Options can be passed by setting GIT_TEST_OPTIONS before invoking
>> +cmake. E.g. on a Linux system with systemd the tests can be sped up by
>> +using a ramdisk for the scratch files:
>
> Doesn't the systemd wiki warn against using /run for things like this
> as to avoid running out of space. I thought our usual recommendation
> was to use --root=/dev/shm

Used /dev/shm in the updated docs.

>> +	GIT_TEST_OPTS="--root=/run/user/$(id -u)/ctest" cmake -S contrib/buildsystems -B contrib/buildsystems/out
>> +	[...]
>> +	-- Using user-selected test options: --root=/run/user/1001/ctest
>> +
>> +Then running the tests with "ctest" (here with --jobs="$(nproc)"):
>
> I think it would be helpful to show setting --jobs at configure time
> as it makes running the tests simpler.

mm, you mean turn it into a set(...) variable? Sounds useful, or ideally
grabbing some pre-set cmake idea of the parallelism.

But for now I'd like to just leave it as "maybe try running this",
rather than integrate it into the whole cmake/ctest chain.

>> +	ctest --jobs=$(nproc) --test-dir contrib/buildsystems/out
>>   ]]
>>   cmake_minimum_required(VERSION 3.14)
>>   @@ -1110,10 +1139,23 @@ endif()
>>     file(GLOB test_scipts "${CMAKE_SOURCE_DIR}/t/t[0-9]*.sh")
>>   +string(COMPARE NOTEQUAL "$ENV{GIT_TEST_OPTS}" ""
>> HAVE_USER_GIT_TEST_OPTS)
>> +if(HAVE_USER_GIT_TEST_OPTS)
>
> if (DEFINED ENV{GIT_TEST_OPTS}) ?

Thanks! That's much better.

>> +	set(GIT_TEST_OPTS "$ENV{GIT_TEST_OPTS}")
>> +	message(STATUS "Using user-selected test options: ${GIT_TEST_OPTS}")
>> +elseif(WIN32)
>> +	set(GIT_TEST_OPTS "--no-bin-wrappers --no-chain-lint -vx")
>> +	message(STATUS "Using Windowns-specific default test options: ${GIT_TEST_OPTS}")
>> +else()
>> +	set(GIT_TEST_OPTS "")
>
> I'd like to see us setting -vx here so users get debugging logs

I think it might make sense to do that by default, but let's consider
that separately, and if we're going to do that we should set that
default for the t/Makefile (or t/test-lib.h), not just cmake.

The reason it has these settings now is because of Windows-specific
trade-offs in the already landed topic.

But since the topic of this series is to port it to run nicely on *nix I
don't see why we'd have it as a goal to have "cmake" / "ctest" behave
differently than "make" or "make test".

Except of course cases where some inherent differente between the
toolchains suggests that we should.

But in this case I don't see why that's the case, if I run a full
"ctest" run and have failures, I might wish I've got logs, but the same
goes for:

     make test GIT_TEST_OPTS="--verbose-log -vx"

>> +	message(STATUS "No custom test options selected, set e.g. GIT_TEST_OPTS=\"-vixd\"")
>> +endif()
>> +separate_arguments(GIT_TEST_OPTS)
>
> What rules does this use for separating arguments?

Bad ones :( I've updated the commit message accordingly, but kept this,
I couldn't find some non-crappy way to e.g. handle spaces in parameters
on the cmake version we require.