Re: [PATCH] test-lib.sh: discover "git" in subdirs of "contrib/buildsystems/out"

[Phillip Wood <phillip.wood123@gmail.com>]

On 01/12/2022 16:39, Ævar Arnfjörð Bjarmason wrote:
> Fix a regression in [1] and discover git built by cmake in subdirs of
> "contrib/buildsystems/out", in addition to the "out" directory itself.

How do you propose to find the most recent build? There are different 
directories for different architectures and different directories for 
debug and release builds.

Hard coding the build directory is a bad idea and should be dropped.

Best Wishes

Phillip


> As noted in [2] the default for Visual Studio is to use
> "out/build/<config>", where "<config>" is a default configuration
> name. We might be able to make this deterministic in the future with a
> "CMakePresets.json", but that facility is newer than our oldest
> supported CMake and VS version.
> 
> 1. 16a5421a654 (Makefile + cmake: use environment, not GIT-BUILD-DIR,
>     2022-11-03)
> 2. https://learn.microsoft.com/en-us/cpp/build/cmake-projects-in-visual-studio?view=msvc-170
> 
> Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
> ---
> 
> On Thu, Dec 01 2022, Phillip Wood wrote:
> 
>> On 30/11/2022 10:16, Ævar Arnfjörð Bjarmason wrote:
>>> On Wed, Nov 30 2022, Phillip Wood wrote:
>>>
>>>> Hi Junio
>>> Hi, and thanks for your review of this series.
>>>
>>>> On 29/11/2022 09:40, Junio C Hamano wrote:
>>>>> * ab/cmake-nix-and-ci (2022-11-04) 14 commits
>>>>>      (merged to 'next' on 2022-11-08 at 6ef4e93b36)
>>>>>     + CI: add a "linux-cmake-test" to run cmake & ctest on linux
>>>>>     + cmake: copy over git-p4.py for t983[56] perforce test
>>>>>     + cmake: only look for "sh" in "C:/Program Files" on Windows
>>>>>     + cmake: increase test timeout on Windows only
>>>>>     + cmake: support GIT_TEST_OPTS, abstract away WIN32 defaults
>>>>>     + Makefile + cmake: use environment, not GIT-BUILD-DIR
>>>>>     + test-lib.sh: support a "GIT_TEST_BUILD_DIR"
>>>>>     + cmake: set "USE_LIBPCRE2" in "GIT-BUILD-OPTIONS" for test-lib.sh
>>>>>     + cmake & test-lib.sh: add a $GIT_SOURCE_DIR variable
>>>>>     + cmake: chmod +x the bin-wrappers/* & SCRIPT_{SH,PERL} & git-p4
>>>>>     + cmake: don't copy chainlint.pl to build directory
>>>>>     + cmake: update instructions for portable CMakeLists.txt
>>>>>     + cmake: use "-S" and "-B" to specify source and build directories
>>>>>     + cmake: don't invoke msgfmt with --statistics
>>>>>     Fix assorted issues with CTest on *nix machines.
>>
>> Junio please drop this series when you rebuild next as it breaks
>> manually running individual test scripts when building with Visual
>> Studio.
> 
> I think the issue you've spotted is easily fixed on top. See below.
> 
>>>> If that's all this series did then I think it would be fine. However
>>>> it also makes changes to test-lib.sh to hard code the build directory
>>>> in an attempt to remove GIT-BUILD-DIR. I'm not convinced that is an
>>>> improvement on the status quo.
>>> I think the series as it stands addresses those concerns. In
>>> particular
>>> building outside of contrib/buildsystems/out works, just as before:
>>> 	cmake -S contrib/buildsystems -B /tmp/git-build
>>> -DCMAKE_BUILD_TYPE=Debug &&
>>>           make -C /tmp/git-build &&
>>>           ctest --test-dir /tmp/git-build -R t0001
>>> Per [1] and [2] which added the "ctest" support that's the use-case
>>> for
>>> this part of the build: running the tests with ctest, which works as
>>> before with the default or custom directories.
>>> Perhaps the reason this has been a sticking point for you is that in
>>> summarizing this, Johannes's [3] didn't make that distinction between
>>> running the tests with "ctest" and running them manually by entering the
>>> "t/" directory after the build. I.e.:
>>
>> In other words Johannes thinks both are equally important. The windows
>> build has always supported running the tests manually from /t and he
>> quite reasonable wants that to continue working.
> 
> Yes, that should work. Now, clearly I missed that VS doesn't use "out"
> by default, but a subdirectory, which the below patch fixes.
> 
> But I think we can still draw a distinction between anything under
> "out" and arbitrary user-supplied directories, which can possibly be
> located outside of the source tree.
> 
>>> 	(cd t && ./t0001-init.sh)
>>> It's only that part which acts differently in this series. I.e. if
>>> you
>>> were to build in /tmp/git-build this would no longer find your built
>>> assets:
>>> 	$ ./t0001-init.sh
>>> 	error: GIT-BUILD-OPTIONS missing (has Git been built?).
>>> If you just leave it at the default of "contrib/buildsystems/out"
>>> it'll
>>> work:
>>> 	(cd t && ./t0001-init.sh)
>>> 	ok 1 [...]
>>> I think my [4] convincingly makes the case that nobody will
>>> care. I.e. as the [5] it links to the use-case for running the test
>>> after the build without ctest was ("[...]" insert is mine):
>>> 	[To build and test with VS] open the worktree as a folder, and
>>> 	Visual Studio will find the `CMakeLists.txt` file and
>>> 	automatically generate the project files.
>>> I.e. we want to support the user who builds with that method, and
>>> runs
>>> the tests manually. I think you're worrying about an edge case that
>>> nobody's using in practice.
>>
>> You seem to be assuming that Visual Studio creates its build artifacts
>> in contrib/buildsystems/out based on a gitignore rule. Given the rule
>> ignores _all_ subdirectories below contrib/buildsystems/out that is a
>> big assumption. Despite me repeatedly raising concerns about the hard
>> coded build directory you do not seem to have checked exactly where
>> Visual Studio creates its build artifacts.
> 
> I did check, but I got it wrong from reading the docs & commit message.
> 
> I don't have a local Windows or VS setup. Thanks for testing it.
> 
>> This morning I installed
>> Visual Studio to check this and discovered the build is in a
>> subdirectory below contrib/buildsystems/out so this series will break
>> manual test runs for anyone building git using the recommend method. I
>> find it rather frustrating that you argue below that Windows specific
>> knowledge and testing are not required when you're altering the
>> Windows build.
> 
> I was saying that you could round-trip test the auto-discovery of the
> build directory on *nix.
> 
> Now, clearly I missed the "in a subdir of out" case. But that's
> separate from whether you can test that case cross-platform. With the
> below patch this works:
> 
> 	cmake -S contrib/buildsystems -B contrib/buildsystems/out/a/b/c -DCMAKE_BUILD_TYPE=Debug &&
> 	make -C contrib/buildsystems/out/a/b/c &&
> 	(cd t && ./t0071-sort.sh)
> 
> But did this work for you before, and does it work on "master"? I
> think this never worked out of the box until my series. I.e. if you do
> that on v2.38.0 (before "js/cmake-updates") you'll get:
> 
> 	$ ./t0071-sort.sh
> 	error: GIT-BUILD-OPTIONS missing (has Git been built?).
> 
> The reason is that before "js/cmake-updates" the part of the cmake
> recipe that established the ling between the test-lib.sh and the
> cmake-built tree was contingent on running ctest. E.g.:
> 
> 	ctest --test-dir contrib/buildsystems/out/a/b/c -R t0071
> 
> Once you did that it would patch your t/test-lib.sh:
> 	
> 	diff --git a/t/test-lib.sh b/t/test-lib.sh
> 	index a65df2fd220..70b0a633e4c 100644
> 	--- a/t/test-lib.sh
> 	+++ b/t/test-lib.sh
> 	@@ -44,1 +44,1 @@ fi
> 	-GIT_BUILD_DIR="${TEST_DIRECTORY%/t}"
> 	+GIT_BUILD_DIR="$TEST_DIRECTORY/../contrib/buildsystems/out/a/b/c"
> 
> Likewise, with "js/cmake-updates" it also doesn't work after you
> *build*. I. with it it would create a "GIT-BUILD-DIR" file, but only
> once ctest is run. I.e.:
> 
> 	(cd t && ./t0071-sort.sh);
> 	file GIT-BUILD-DIR;
> 	ctest --test-dir contrib/buildsystems/out/a/b/c -R t0071
> 	file GIT-BUILD-DIR;
> 	(cd t && ./t0071-sort.sh)
> 
> Would emit:
> 
> 	error: GIT-BUILD-OPTIONS missing (has Git been built?).
> 	GIT-BUILD-DIR: cannot open `GIT-BUILD-DIR' (No such file or directory)
> 	<ctest output>
> 	GIT-BUILD-DIR: ASCII text, with no line terminators
> 	<test output>
> 
> It's only with my series that it started working wihout having to run
> "ctest". With the below the test-lib.sh will optimistically find the
> "git" in "contrib/buildsystems/out".
> 
> Does the VS integration run the equivalent of "ctest" by default?
> 
>   t/test-lib.sh | 8 +++++---
>   1 file changed, 5 insertions(+), 3 deletions(-)
> 
> diff --git a/t/test-lib.sh b/t/test-lib.sh
> index ce319c9963e..9c63ee428f2 100644
> --- a/t/test-lib.sh
> +++ b/t/test-lib.sh
> @@ -57,9 +57,11 @@ if test -n "$GIT_TEST_BUILD_DIR"
>   then
>   	GIT_BUILD_DIR="$GIT_TEST_BUILD_DIR"
>   elif ! test -x "$GIT_BUILD_DIR/git" &&
> -     test -x "$GIT_BUILD_DIR/contrib/buildsystems/out/git"
> +     test -d "$GIT_BUILD_DIR/contrib/buildsystems/out"
>   then
> -	GIT_BUILD_DIR="$GIT_SOURCE_DIR/contrib/buildsystems/out"
> +	GIT_BUILD_OPTIONS="$(find "$GIT_BUILD_DIR/contrib/buildsystems/out" \
> +		-type f -name 'GIT-BUILD-OPTIONS')"
> +	GIT_BUILD_DIR="${GIT_BUILD_OPTIONS%/GIT-BUILD-OPTIONS}"
>   	GIT_AUTO_CONTRIB_BUILDSYSTEMS_OUT=t
>   
>   	# On Windows, we must convert Windows paths lest they contain a colon
> @@ -1646,7 +1648,7 @@ remove_trash_directory "$TRASH_DIRECTORY" || {
>   # anything using lib-subtest.sh
>   if test -n "$GIT_AUTO_CONTRIB_BUILDSYSTEMS_OUT" && test -t 1
>   then
> -	say "setup: had no ../git, but found & used cmake built git in ../contrib/buildsystems/out/git"
> +	say "setup: had no ../git, but found & used cmake built git in '$GIT_BUILD_DIR/git'"
>   fi
>   
>   remove_trash=t