patch, 9 partscmake: don't copy chainlint.pl to build directory

On Tue, Oct 18 2022, Johannes Schindelin wrote:

[For context: https://lore.kernel.org/git/on2q3qos-sr0n-0p8p-606p-5pq39n46qq4q@tzk.qr/ is upthread of here, but spinning this off into another thread]

You did ask for it :)

To clarify, what I had upthread was a patch on the way towards a solution that didn't edit t/test-lib.sh at all, but that patch itself wasn't sufficient. It's the equivalent of 3/9 here.

This series:

 * Can be grabbed at
   https://github.com/avar/git/tree/avar/cmake-test-path

 * Conflicts with js/cmake-updates[1], although I've seen it go in &
   out of {gitster,git}/seen (currently in gitster/seen, not in
   git.git's "seen".

   But the conflict is easily fixed to keep both, see below.

 * Fixes numerous bugs in the current cmake recipe, running "ctest"
   was a dumpster fire on Linux, after this with 9/9 we can run our
   whole test suite in CI on ubuntu-latest:
   https://github.com/avar/git/actions/runs/3293378319/jobs/5431721245

 * Handles the case where you want to build with cmake and have "(cd t
   && ./t0001-init.sh)" just work, i.e. we spot that we have a built
   "contrib/buildsystems/out" and use that.

 * I pushed out a "js/cmake-update-ci" in my repo, which is a (rebased
   on master) "js/cmake-updates" plus the 9/9 here:
   https://github.com/avar/git/actions/runs/3294352976/jobs/5432095664

   Some of those failures are due to orthagonal fixes here, e.g. you
   clearly haven't been testing cmake+ctest on Linux, or you'd have
   seen the issue 2/9 fixes, a lot of failures are due to a missing
   "chmod +x".

   But others are due to e.g. your [2] missing a lot of cases. 3/9 and
   8/9 cover some of those, i.e. the Windows CI skips a lot of tests
   (e.g. all Perl + Python stuff), no libpcre2 etc. So a lot of
   failures were related to that.

 * I also merged this with yours at
   https://github.com/avar/git/tree/avar/cmake-test-path-plus-js/cmake-updates

   The --remerge-diff of the resulting merge commit is, with
   un-indented inline commentary:

	diff --git a/.gitignore b/.gitignore
	index cb0231fb401..62720c6135d 100644
	--- a/.gitignore
	+++ b/.gitignore
	@@ -1,5 +1,4 @@
	 /fuzz_corpora
	-/GIT-BUILD-DIR
	 /GIT-BUILD-OPTIONS
	 /GIT-CFLAGS
	 /GIT-LDFLAGS
	diff --git a/Makefile b/Makefile
	index 9e3c4588e7d..d93ad956e58 100644
	--- a/Makefile
	+++ b/Makefile
	@@ -3041,7 +3041,6 @@ else
	 	@echo RUNTIME_PREFIX=\'false\' >>$@+
	 endif
	 	@if cmp $@+ $@ >/dev/null 2>&1; then $(RM) $@+; else mv $@+ $@; fi
	-	@if test -f GIT-BUILD-DIR; then rm GIT-BUILD-DIR; fi

   As yuo asked for: We can get what you want without
   GIT-BUILD-DIR. When we run via "ctest" we pass
   GIT_TEST_BUILD_DIR=${CMAKE_BINARY_DIR}, and when we run ad-hoc the
   test-lib.sh checks "contrib/buildsystems/out" and uses it if we
   have a built git there, but not in ../git.

	 ### Detect Python interpreter path changes
	 ifndef NO_PYTHON
	diff --git a/contrib/buildsystems/CMakeLists.txt b/contrib/buildsystems/CMakeLists.txt
	remerge CONFLICT (content): Merge conflict in contrib/buildsystems/CMakeLists.txt
	index bfc4d87026b..fafe6d9cb95 100644
	--- a/contrib/buildsystems/CMakeLists.txt
	+++ b/contrib/buildsystems/CMakeLists.txt
	@@ -1081,24 +1081,6 @@ if(USE_VCPKG)
	 	file(APPEND ${CMAKE_BINARY_DIR}/GIT-BUILD-OPTIONS "PATH=\"$PATH:$TEST_DIRECTORY/../compat/vcbuild/vcpkg/installed/x64-windows/bin\"\n")
	 endif()

	-<<<<<<< b81f18dec61 (CI: add a "linux-cmake-test" to run cmake & ctest on linux)
	-=======
	-#Make the tests work when building out of the source tree
	-get_filename_component(CACHE_PATH ${CMAKE_CURRENT_LIST_DIR}/../../CMakeCache.txt ABSOLUTE)
	-if(NOT ${CMAKE_BINARY_DIR}/CMakeCache.txt STREQUAL ${CACHE_PATH})
	-	#Setting the build directory in test-lib.sh before running tests
	-	file(WRITE ${CMAKE_BINARY_DIR}/CTestCustom.cmake
	-		"file(WRITE ${CMAKE_SOURCE_DIR}/GIT-BUILD-DIR \"${CMAKE_BINARY_DIR}\")")
	-	#misc copies
	-	file(COPY ${CMAKE_SOURCE_DIR}/t/chainlint.pl DESTINATION ${CMAKE_BINARY_DIR}/t/)
	-	file(COPY ${CMAKE_SOURCE_DIR}/po/is.po DESTINATION ${CMAKE_BINARY_DIR}/po/)
	-	file(GLOB mergetools "${CMAKE_SOURCE_DIR}/mergetools/*")
	-	file(COPY ${mergetools} DESTINATION ${CMAKE_BINARY_DIR}/mergetools/)
	-	file(COPY ${CMAKE_SOURCE_DIR}/contrib/completion/git-prompt.sh DESTINATION ${CMAKE_BINARY_DIR}/contrib/completion/)
	-	file(COPY ${CMAKE_SOURCE_DIR}/contrib/completion/git-completion.bash DESTINATION ${CMAKE_BINARY_DIR}/contrib/completion/)
	-endif()
	-
	->>>>>>> 27870f309d0 (cmake: increase time-out for a long-running test)
	 file(GLOB test_scipts "${CMAKE_SOURCE_DIR}/t/t[0-9]*.sh")

   This whole "copy stuff and munge test-lib.sh" part is gone in my
   version. Rather than copy over mergetools/* the relevant tests know
   where to find it (it doesn't need to be built at all).

	 #test
	@@ -1106,11 +1088,7 @@ set(GIT_TEST_OPTS "$ENV{GIT_TEST_OPTS}")
	 separate_arguments(GIT_TEST_OPTS)
	 foreach(tsh ${test_scipts})
	 	add_test(NAME ${tsh}
	-<<<<<<< b81f18dec61 (CI: add a "linux-cmake-test" to run cmake & ctest on linux)
	-		COMMAND env GIT_TEST_BUILD_DIR=${CMAKE_BINARY_DIR} ${SH_EXE} ${tsh} ${GIT_TEST_OPTS}
	-=======
	-		COMMAND ${SH_EXE} ${tsh} --no-bin-wrappers --no-chain-lint -vx
	->>>>>>> 27870f309d0 (cmake: increase time-out for a long-running test)
	+		COMMAND env GIT_TEST_BUILD_DIR=${CMAKE_BINARY_DIR} ${SH_EXE} ${tsh} --no-bin-wrappers --no-chain-lint -vx ${GIT_TEST_OPTS}
	 		WORKING_DIRECTORY ${CMAKE_SOURCE_DIR}/t)
	 endforeach()

   I did not include your "--no-bin-wrappers --no-chain-lint -vx"
   change.

   As 7/9 here shows I pick up GIT_TEST_OPTS from the environment, so
   if you want to set these you can set them in the environment.

   But your 1/5[3] adds these partially for speeding up things on
   Windows, but as this series shows this also runs on
   Linux. Therefore if we still want this I'd think we'd at least want
   to make it specific to that platform (per [4]).

	diff --git a/t/test-lib.sh b/t/test-lib.sh
	index 333079d9127..284b619708a 100644
	--- a/t/test-lib.sh
	+++ b/t/test-lib.sh
	@@ -69,16 +69,6 @@ then
	 	echo "PANIC: Running in a $TEST_DIRECTORY that doesn't end in '/t'?" >&2
	 	exit 1
	 fi
	-if test -f "$GIT_BUILD_DIR/GIT-BUILD-DIR"
	-then
	-	GIT_BUILD_DIR="$(cat "$GIT_BUILD_DIR/GIT-BUILD-DIR")" || exit 1
	-	# On Windows, we must convert Windows paths lest they contain a colon
	-	case "$(uname -s)" in
	-	*MINGW*)
	-		GIT_BUILD_DIR="$(cygpath -au "$GIT_BUILD_DIR")"
	-		;;
	-	esac
	-fi

   The changes I needed to the test-lib.sh are above this context.

	 # Prepend a string to a VAR using an arbitrary ":" delimiter, not
	 # adding the delimiter if VAR or VALUE is empty. I.e. a generalized:

   Elided here is the inclusion of your 5/5[5], which didn't
   conflict. If we'd like to keep it it probably makes sense to make
   it Windows-specific too, I don't see that being needed on Linux.

   Aside from this topic I'd be curious to know if you see the same
   slowdown with my [6], i.e. once git-submodule.sh has gone away. For
   me it's ~25% faster than "master", but I'd expect on a system with
   high fork overhead that would be a lot higher.

1. https://lore.kernel.org/git/pull.1320.v3.git.1666090745.gitgitgadget@gmail.com/
2. https://lore.kernel.org/git/9faca9d5cbe706c00d03ca2a125f2e113294a548.1666090745.git.gitgitgadget@gmail.com/
3. https://lore.kernel.org/git/356b2e9a1007bcd1382f26f333926ff0d5b9abe2.1666090745.git.gitgitgadget@gmail.com/
4. https://lore.kernel.org/git/221018.865yghi6nv.gmgdl@evledraar.gmail.com/
5. https://lore.kernel.org/git/40cf872f48386f8eca0fa814e4cdfb0ded915ed8.1666090745.git.gitgitgadget@gmail.com/
6. https://lore.kernel.org/git/cover-00.10-00000000000-20221017T115544Z-avarab@gmail.com/

Ævar Arnfjörð Bjarmason (9):
  cmake: don't copy chainlint.pl to build directory
  cmake: chmod +x the bin-wrappers/* & SCRIPT_{SH,PERL} & git-p4
  cmake & test-lib.sh: add a $GIT_SOURCE_DIR variable
  cmake: set "USE_LIBPCRE2" in "GIT-BUILD-OPTIONS" for test-lib.sh
  test-lib.sh: support a "GIT_TEST_BUILD_DIR"
  cmake: use GIT_TEST_BUILD_DIR instead of editing hack
  cmake: support using GIT_TEST_OPTS from the environment
  cmake: copy over git-p4.py for t983[56] perforce test
  CI: add a "linux-cmake-test" to run cmake & ctest on linux

 .github/workflows/main.yml          |  3 +++
 ci/run-build-and-tests.sh           | 13 +++++++++--
 contrib/buildsystems/CMakeLists.txt | 33 +++++++++++----------------
 t/README                            |  3 +++
 t/lib-gettext.sh                    |  2 +-
 t/lib-gitweb.sh                     |  2 +-
 t/t7609-mergetool--lib.sh           |  2 +-
 t/t9902-completion.sh               | 14 ++++++------
 t/t9903-bash-prompt.sh              |  2 +-
 t/test-lib.sh                       | 35 +++++++++++++++++++++++++++--
 10 files changed, 74 insertions(+), 35 deletions(-)

When [1] made this copy of "chainlint.sed" the script was invoked in the test-lib.sh as:

	[...] sed -f "$GIT_BUILD_DIR/t/chainlint.sed" [...]

But when [2] replaced "chainlint.sed" with a "chainlint.pl" it also replaced that "$GIT_BUILD_DIR" with "$TEST_DIRECTORY", invoking it as:

	"$PERL_PATH" "$TEST_DIRECTORY/chainlint.pl" "$0"

So this line could have been deleted in [2] but wasn't. Let's do that now.

1. 7f5397a07c6 (cmake: support for testing git when building out of
   the source tree, 2020-06-26)
2. 23a14f30166 (test-lib: replace chainlint.sed with chainlint.pl,
   2022-09-01)

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 contrib/buildsystems/CMakeLists.txt | 1 -
 1 file changed, 1 deletion(-)

The cmake recipe added in [1] did not create the bin-wrappers/* directory, and thus fell back on running the tests with the equivalent of "--no-bin-wrappers".

Thus the. "t/test-lib.sh" would be unable to find "bin-wrappers/git", and we'd fall back on "GIT_EXEC_PATH=$GIT_BUILD_DIR" using the fallback behavior added in [2]:

	$ ./t0001-init.sh
	<GIT_BUILD_DIR>/t/../contrib/buildsystems/out/bin-wrappers/git is not executable; using GIT_EXEC_PATH

Or rather, this is what would have happened on *nix, but until [3] there wasn't any non-Windows support for "cmake". On Windows it didn't matter that the bin-wrappers weren't made executable, since there's no executable bit, instead the emulation layer looks at whether a file has a shebang.

But with [3] we've effectively used the semi-equivalent of "--no-bin-wrappers" unintentionally on *nix, and furthermore because we didn't make these executable

In addition, we'd fail any test that needed to invoke one of our built shell, perl or Python scripts on *nix. E.g. t0012-help.sh would fail on a test that tried to invoke "git web--browse". I.e. the equivalent of this (in the "out" directory) would happen:

	$ ./git --exec-path=$PWD web--browse
	git: 'web--browse' is not a git command. See 'git --help'.

Which we can fix by "chmod +x"-ing the built "git-web--browse":

	$ chmod +x git-web--browse
	$ ./git --exec-path=$PWD web--browse
	usage: git web--browse [--browser=browser|--tool=browser] [--config=conf.var] url/file ...

The same goes for e.g. the "git-p4" tests, which would fail because our built "git-p4" wasn't executable, etc.

This change should ideally use file(CHMOD ...), but that's much newer than our required cmake version[1].

1. a30e4c531d9 (Merge branch 'ss/cmake-build', 2020-08-11)
2. e4597aae659 (run test suite without dashed git-commands in PATH, 2009-12-02)
3. f31b6244950 (Merge branch 'yw/cmake-updates', 2022-06-07)
4. https://cmake.org/cmake/help/latest/command/file.html#chmod

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 contrib/buildsystems/CMakeLists.txt | 6 ++++++
 1 file changed, 6 insertions(+)

Hi Ævar

On 21/10/2022 10:44, Ævar Arnfjörð Bjarmason wrote:

I think it would be nicer to add a write_script function

function(write_script path content)
        file(WRITE ${path} ${content})
        file(CHMOD ${path} FILE_PERMISSIONS OWNER_READ OWNER_WRITE 
OWNER_EXECUTE)
endfunction()

and then do

-       file(WRITE ${CMAKE_BINARY_DIR}/${script} ${content})
+       write_script(${CMAKE_BINARY_DIR}/${script} "${content}")

rather than executing chmod each time

Best Wishes

Phillip

On Fri, Oct 21 2022, Phillip Wood wrote:

Thanks, I'll use that. It looks much nicer.

When we build with libpcre2 which cmake has supported since [1] we need to set "USE_LIBPCRE2='YesPlease'" (or similar) in "GIT-BUILD-OPTIONS".

Without this e.g. t7810-grep.sh will fail, as it has tests that rely on the behavior of !PCRE2. The reason this hasn't been noticed is that the Windows CI doesn't have access to libpcre2.

1. 80431510a2b (cmake: add pcre2 support, 2022-05-24)

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 contrib/buildsystems/CMakeLists.txt | 3 +++
 1 file changed, 3 insertions(+)

Compliment the existing GIT_TEST_INSTALLED variable added in [1] with one that allows for testing a git built in a directory that's not "../".

Instead of "test this installed git" ("GIT_TEST_INSTALLED"), this new "GIT_TEST_BUILD_DIR" is a "test this built directory over there". E.g. this will pass all tests:

	git clone --depth=1 https://github.com/git/git.git git1
	cp -R git1 git2
	make -C git1
	[apply this change to git2]
	(cd git2 && GIT_TEST_BUILD_DIR="$PWD/../git1" make -C t prove)

1. 6720721e152 (test-lib.sh: Allow running the test suite against
   installed git, 2009-03-16)

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 t/README      | 3 +++
 t/test-lib.sh | 4 ++++
 2 files changed, 7 insertions(+)

When cmake builds git in "contrib/buildsystems/out" it will create a "t/" directory there containing only the "t/helper/test-tool", but for running the tests with "cmake" it cd's to the "real" t/ directory, and runs the tests from there.

To get the test-lib.sh to locate "git" and other binaries in "../contrib/buildsystems/out/" rather than "../" we have since [1] been editing the "GIT_BUILD_DIR" assignment in test-lib.sh in-place.

This has meant that when testing cmake we've had to "git reset --hard" before running "make" again.

What this build infrastructure really wanted was some feature like the "GIT_TEST_BUILD_DIR" variable added in the preceding commit, so let's make use of it.

Even though "ctest" will work with this approach, one advantage of the previous arrangement was that we could:

	A. Build with cmake
	B. cd t
	C. Run a test

And have the test itself know to locate and use the cmake binaries, this workflow was documented in [2]. The "t/test-lib".sh modification here is so that we can support this use-case.

As [3] notes "contrib/buildsystems/out" isn't just the directory that happens to be documented in "contrib/buildsystems/CMakeLists.txt", but the one that VS will use when building git.

To make it clear what's happening we emit a "setup: " from the test-lib.sh to note that we used this fallback method:

	$ ./t0001-init.sh
	setup: had no ../git, but found & used cmake built git in ../contrib/buildsystems/out/git
	ok 1 - plain
	[...]

Note: the "On Windows[...]" part of this is lifted from [4].

1. 7f5397a07c6 (cmake: support for testing git when building out of
   the source tree, 2020-06-26)
2. f2f1250c47f (cmake (Windows): recommend using Visual Studio's
   built-in CMake support, 2020-09-30)
3. 3eccc7b99d4 (cmake: ignore files generated by CMake as run in
   Visual Studio, 2020-09-25)
4. https://lore.kernel.org/git/5b0c2a150e9fce1ca0284d65628b42ed5a7aad9a.1666090745.git.gitgitgadget@gmail.com/

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 contrib/buildsystems/CMakeLists.txt | 15 +--------------
 t/test-lib.sh                       | 19 +++++++++++++++++++
 2 files changed, 20 insertions(+), 14 deletions(-)

Hi Ævar

On 21/10/2022 10:44, Ævar Arnfjörð Bjarmason wrote:

Lets squash that commit into this one, so we can see how it is used when it is added.

That may be the directory that VS uses when building git, but it is possible to specify a different build directory when running cmake.

I'm not sure about using env on windows, can we use ${SH_EXE} -c instead to avoid creating an extra process?

	COMMAND ${SH_EXE} -c [[GIT_TEST_BUILD_DIR="$1"; . "$2"]] "${tsh}" 
"${CMAKE_BINARY_DIR}" "${tsh}"

I'm really not keen on hard coding the CMAKE_BINARY_DIR. One of the positive things about dscho's approach is that it does not hard code the build directory. I'm not convinced this approach is an improvement.

Best Wishes

Phillip

On Fri, Oct 21 2022, Phillip Wood wrote:

Heh, I did that to begin with, but found that the commit message & change was too long & trying to explain two different things.

Since GIT_TEST_BUILD_DIR (like GIT_TEST_INSTALLED) is something you can use stand-alone I prefer to keep it this way. The commit message shows how you can use it without anything to do with cmake, and then later (here) we can use it for cmake...

Possible, but important enough to care? It's what it does by default, and we have /contrib/buildsystems/out in the top-level .gitignore.

I could bring it back, basically the GIT-BUILD-DIR again, but is the edge case worth worrying about? This DWIM behavior of "build with cmake, cd and run the the test" is something I think we can safely assume is OK to restrict to the defaults.

Once you're off the defaults and e.g. want a dir in /run/user or whatever it's also easy to set GIT_TEST_BUILD_DIR to that. The "contrib/buildsystem/out" assumption is just so it works by default without tweaking.

In general, if it didn't work a lot of our test suite would fail, so it's definitely supported, and since this is only used to run tests it should be OK with portability.

But I don't have a Windows dev environment other than the CI, are you able to test this & check if it works?

Neat, so that "[[...]]" syntax makes sure we don't have any quoting issues?

See above.

Thanks for the careful review!

On 21/10/2022 15:43, Ævar Arnfjörð Bjarmason wrote:

We're only adding it because we want to use it with cmake though and the only way to see if the previous patch is correct is to start using it.

I keep meaning to set up a git build on windows but I haven't got round to it. Using "sh -c" also gives us more flexibility in later patches.

Yes, it works like lua. https://cmake.org/cmake/help/latest/manual/cmake-language.7.html#bracket-argument

Best Wishes

Phillip

When git is built and tested with cmake & ctest we usually do that outside of the top-level source directory. E.g.

	cmake -S contrib/buildsystems -B contrib/buildsystems/out
	make -C contrib/buildsystems/out
	ctest -j$(nproc) --test-dir contrib/buildsystems/out

Since the test-lib.sh makes assumptions about the TEST_DIRECTORY being the "t" subdirectory of the top-level "$GIT_BUILD_DIR", the cmake recipe has needed to copy various assets to that "contrib/buildsystems/out" directory.

Let's instead teach the test-lib.sh that there's such a thing as the "$GIT_SOURCE_DIR" distinct from the "$GIT_BUILD_DIR".

Just as the "$TEST_DIRECTORY" always points to our actual "t" directory (not the "[...]/out/t" cmake creates), this new "$GIT_SOURCE_DIR" will always be the top-level source directory.

So even though the "GIT_BUILD_DIR=(.*)" line in t/test-lib.sh will still be altered by CMakeLists.txt, that recipe will no longer need to copy over various things from our source directory, as the tests now know where to find those assets.

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 contrib/buildsystems/CMakeLists.txt |  5 -----
 t/lib-gettext.sh                    |  2 +-
 t/lib-gitweb.sh                     |  2 +-
 t/t7609-mergetool--lib.sh           |  2 +-
 t/t9902-completion.sh               | 14 +++++++-------
 t/t9903-bash-prompt.sh              |  2 +-
 t/test-lib.sh                       | 12 ++++++++++--
 7 files changed, 21 insertions(+), 18 deletions(-)

Get "GIT_TEST_OPTS" from the environment, and use it to pass arguments to tests. This allows for passing arguments to tests with e.g.:

	GIT_TEST_OPTS="--verbose --debug" cmake .; ctest -R t0001 --verbose

There's some overlap with this and what was suggested in [1], but as noted there we're not passing "--verbose" and friends unconditionally, so a plain "ctest" invocation without a "cmake" re-build won't pick up the options.

1. https://lore.kernel.org/git/356b2e9a1007bcd1382f26f333926ff0d5b9abe2.1666090745.git.gitgitgadget@gmail.com/

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 contrib/buildsystems/CMakeLists.txt | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

Hi Ævar

On 21/10/2022 10:44, Ævar Arnfjörð Bjarmason wrote:

The aim of dscho's patch was to make debugging information available in the test logs without the user having to do anything, now to get that information every user has to set GIT_TEST_OPTS="--no-bin-wrappers --no-chain-lint -vx" when running cmake.

I think it would be helpful to have some default options set if the user does not pass GIT_TEST_OPTS. Ideally one would be able to do

	GIT_TEST_OPTS=... ctest

and have the tests pick up the options at runtime. Following on from my previous comment, if we used "sh -c" to launch the tests we could have something like

	COMMAND ${SH_EXE} -c [[GIT_TEST_BUILD_DIR="$1"; . "$2" 
${GIT_TEST_OPTS:---no-bin-wrappers --no-chain-lint -vx}]] "${tsh}" 
"${CMAKE_BINARY_DIR}" "${tsh}"

Best Wishes

Phillip

On Fri, Oct 21 2022, Phillip Wood wrote:

That sounds reasonable to me. FWIW I looked into $CTEST_INTERACTIVE_DEBUG_MODE for this purpose, i.e. to stick something like this in test-lib.sh:

	if test -n "$CTEST_INTERACTIVE_DEBUG_MODE"
	then
		verbose=t
		trace=t
	fi

But I was hoping for some way to tell that "ctest" was in "--verbose" mode, but AFAICT there's no way to get at that without something like compat/linux/procinfo.c (basically a fancier way of parsing "ps auxf").

Anyway, as noted in my review of dscho's series I thought this part of it was odd/outdated given that this thing runs on Linux (mostly, but entirely after this series).

I.e. why would we hardcode Windows-specific trade-offs into a portable build-system, and if you do want e.g. "--no-bin-wrappers" why would you want that just when you run "cmake", not "make"? Surely if we're pushing for a new default it should be agnostic to the user's build system.

But in any case I think if we're pushing for new (or cmake-specific) opinionated defaults it makes sense to split those up & justify them separately from bug fixes or workarounds.

E.g. 2/9 in this series makes much of the tests pass on *nix, but so does "--no-bin-wrappers", but just because it happens to bypass the broken-on-master bin-wrappers/* made by cmake.

On 21/10/2022 16:45, Ævar Arnfjörð Bjarmason wrote:

I think it is useful to have -vx set even if we're not passing --verbose to ctest as if a test fails we've got the information to debug it stored in ctest's log without having to re-run the test.

I agree that --no-chain-lint --no-bin-wrappers are there just for windows. We could quite easily add those just for windows builds but the cmake build is basically only there for windows and the linux support is just there to allow testing the cmake build without having to run the ci.

Best Wishes

Phillip

On Tue, Oct 25 2022, Phillip Wood wrote:

Yeah, and to clarify, that's basically what this is, i.e. you don't need to pass --verbose.

Yes, that's sounds useful, but I'm still entirely unclear on why that needs to be build-system specific.

I.e. you'll have the same with "make test" and "--verbose-log", with ctest the equivalent of a hypothetical "--tee-to-al-log" is the default, so we're partway towards a "--verbose-log".

Is it just an omission and we should add it to t/Makefile and/or t/test-lib.sh eventually, or something cmake/ctest-specific in some way I don't get?

That part makes sense.

Since [1] two git-p4 tests have relied on there being a "git-p4.py" in the build directory, but the cmake recipe was not updated to account for this. Let's copy the "git-p4.py" over.

We could also change the test to e.g. grab the built "git-p4" and alter its shebang, which would be friendly to GIT_TEST_INSTALLED, but let's just do the bare minimum here to get cmake+ctest working without altering the test itself.

The reason this hasn't been caught by "vs-build" and "vs-test" is because those tests added in [2] invoke "cmake" with "-DPYTHON_TESTS=OFF", and therefore we'd skip this part of the git-p4 tests before getting past the "do we have python?" check. Even if we got past that the Windows CI wouldn't have a "p4" or "p4d" binary installed, so we'd skip the tests anyway.

In a subsequent commit we'll run "cmake" and "ctest" in CI with "ubuntu-latest", so we'll need this "git-p4.py" file.

1. f7b5ff607fa (git-p4: improve encoding handling to support
   inconsistent encodings, 2022-04-30)
2. 4c2c38e800f (ci: modification of main.yml to use cmake for vs-build
   job, 2020-06-26)

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 contrib/buildsystems/CMakeLists.txt | 1 +
 1 file changed, 1 insertion(+)

Since [1] the "cmake" build method should work properly on Linux, but as seen in preceding commits there were various bugs in it, which are hopefully now all fixed.

To ensure that it doesn't break again let's add a "linux-cmake-ctest" target to build and test "cmake" on Linux, in addition to that we'll also run the tests with "ctest" instead of "make" or "prove", so we can assert that testing with that method works..

This also stress tests running "cmake" (and "ctest") out of a build directory that isn't the top-level. The "vs-build" job uses "cmake" since [2], but clobbers the top-level "Makefile" and builds in the top-level directory.

That was the reason for why we didn't spot that various tests still required missing "mergetools/*" etc, which was fixed by using "$GIT_SOURCE_DIR" in a preceding commit.

Since the "ci/lib.sh" already creates and exports a GIT_TEST_OPTS="..." we'll pick that up in our CI, see the preceding commit. Because we pass the "--verbose-log -x --github-workflow-markup" in the GitHub CI as a result the interaction with "handle_failed_tests" here works correctly. I.e. on failure we'll have saved "t/test-results/*.{exit,out,markup}" files relevant to the failing test(s).

1. f31b6244950 (Merge branch 'yw/cmake-updates', 2022-06-07)
2. 4c2c38e800f (ci: modification of main.yml to use cmake for vs-build
   job, 2020-06-26)

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 .github/workflows/main.yml |  3 +++
 ci/run-build-and-tests.sh  | 13 +++++++++++--
 2 files changed, 14 insertions(+), 2 deletions(-)

Hi Ævar,

On Fri, 21 Oct 2022, Ævar Arnfjörð Bjarmason wrote:

Well, I asked for something elegant, which in my mind is always something succinct. A 9-patch series is not succinct ;-)

Sadly, I am already struggling to whittle down my work queue as it is, and am quite deer-in-the-headlights about having to find the time to review anything longer than a short patch in addition to what is already in the queue :-(

Hopefully next week.

Thank you for working on this, Johannes

On 21/10/2022 10:44, Ævar Arnfjörð Bjarmason wrote:

I think this series contains a couple of interesting ideas but I'm not sure hard coding the build directory or removing the default test options are improvements on dscho's series.

Combining fixes for cmake and linux with changes for running ctest on windows is not particularly helpful and would be better as two separate series. We really need to have a clear policy of what the status of building with cmake on linux is. I think having a usable cmake build on linux could be useful for anyone making changes to CMakeLists.txt as they can do some testing and debugging locally before running our ci to check it works on windows but I'm not sure we need to support it as an alternative to the Makefile.

Best Wishes

Phillip

On Fri, Oct 21 2022, Phillip Wood wrote:

c'mon :) Getting it to a 100%[1] pass rate on *nix from 76%[2] & having "ctest" running in CI now is a bit more than an interesting idea :)

I replied to most of that in [3], but wanted to add that while I think it makes sense to get bugfixes for the system in first before behavior changes (especially Makefile-divergent behavior changes) I'm not going to pick that particular hill to die on.

So if the consensus is that we want it too I'd be happy to bundle it all together in a way that one-to-one maps to the features / behavior changes Johannes submitted (especially as he's short on time[4]).

I started looking at this because I have changes locally that make the Makefile much faster by replacing all that FORCE shell-out stuff with built-in Makefile functions, which really cuts down on the fixed cost of running "make" (really helpful in large interactive rebases).

Sneaking a "while generating this file, let's remove this other file" into the Makefile dependency DAG is pretty much free as long as everything's a FORCE'd shellscript, but that approach would block representing that data in Make's native way of viewing dependencies (which is much faster).

But anyway, if there's an insistance on the GIT-BUILD-DIR approach that can be made to work with that also, in a way that I believe satisfies the stated use-case.

I guess, in summary (G = general, N = *nix, W = Windows) fix:

 1/9: G
 2/9: N
 3/9: G
 5/9: G
 6/9: G
 7/9: G
 8/9: G
 9/9: G

So, aside from the "chmod +x" which we need on *nix it's all general fixes (and 9/9 while adding an Ubuntu job will stress CMake paths used on Windows too).

I wish we didn't have it at all, but I also don't use Windows/VS, or anything that benefits from cmake's IDE integration over plain old make, so it doesn't scratch my itch.

I'm not advocating for removing it, mind you, that's just my knee-jerk reaction.

But I think it's clearly here to stay, so we're going to have to live with it, and seemingly maintain both in perpetuity.

Given that it's staying, and that our answer to "cmake is broken" has by default been "it needs fixing before the topic breaking it is merged" (and not "people who use this cmake contrib thing should fix it eventually") this series makes it *much* easier to deal with it for those of us who use *nix for development.

I had a few local topics that broke cmake in one minor way or another, which I knew about. But before this (and f31b6244950 (Merge branch 'yw/cmake-updates', 2022-06-07) earlier this year) seeing why it was breaking involved long ping-pongs with the GitHub Windows CI.

But once I got this working I could fix several of those issues in a few minutes, because I could run the full cmake/ctest featureset locally.

So I think this makes it much more useful, and as the above "G/N/W" summary shows it's really not a burden to make CMakeLists.txt portable.

1. https://github.com/avar/git/actions/runs/3294344038/jobs/5432094502#step:4:2787
2. https://github.com/avar/git/actions/runs/3294352976/jobs/5432095664#step:4:2785
3. https://lore.kernel.org/git/221021.864jvxfa3c.gmgdl@evledraar.gmail.com/
4. https://lore.kernel.org/git/s42p6rn0-s5oo-2874-636p-2541n475p045@tzk.qr/

On 21/10/2022 16:55, Ævar Arnfjörð Bjarmason wrote:

It's certainly useful, I didn't think the fix which is "make the scripts executable" was particularly interesting. I'm more interested in the ideas around making the tests run without editing them or writing GIT-BUILD-DIR and being able to change the arguments passed to tests when running ctest.

Best Wishes

Phillip

This topig gets our tests from passing ~80% with ctest on *nix to passing 100%. Now on top of next's js/cmake-updates. Comments inline on patches:n

Ævar Arnfjörð Bjarmason (11):
  cmake: don't "mkdir -p" and "cd" in build instructions
  cmake: update instructions for portable CMakeLists.txt

These are new, and document how to use cmake+ctest, and make the instructions more OS-agnostic.

  cmake: don't copy chainlint.pl to build directory

Same, except to rebase on js/cmake-updates.

  cmake: chmod +x the bin-wrappers/* & SCRIPT_{SH,PERL} & git-p4

I tried to use the suggested "write_script" function, but couldn't get past cmake quoting issues. So the commit message is updated, but the change is the same.

  cmake & test-lib.sh: add a $GIT_SOURCE_DIR variable
  cmake: set "USE_LIBPCRE2" in "GIT-BUILD-OPTIONS" for test-lib.sh
  test-lib.sh: support a "GIT_TEST_BUILD_DIR"
  Makefile + cmake: use environment, not GIT-BUILD-DIR

Generalize the whole "test a build dir over there", and have cmake & make not step on each other's toes.

  cmake: support GIT_TEST_OPTS, abstract away WIN32 defaults

The test param defaults in js/cmake-updates are kept on Windows, all platforms benefit from having an easier way to provide custom test options.

  cmake: copy over git-p4.py for t983[56] perforce test

Now at 101% tests passing :)

  CI: add a "linux-cmake-test" to run cmake & ctest on linux

See the CI (and topic itself) at https://github.com/avar/git/actions/runs/3333306724/jobs/5515607569

 .github/workflows/main.yml          |   3 +
 .gitignore                          |   1 -
 Makefile                            |   1 -
 ci/run-build-and-tests.sh           |  13 ++-
 contrib/buildsystems/CMakeLists.txt | 133 +++++++++++++++++++++-------
 t/README                            |   3 +
 t/lib-gettext.sh                    |   2 +-
 t/lib-gitweb.sh                     |   2 +-
 t/t7609-mergetool--lib.sh           |   2 +-
 t/t9902-completion.sh               |  14 +--
 t/t9903-bash-prompt.sh              |   2 +-
 t/test-lib.sh                       |  27 +++++-
 12 files changed, 152 insertions(+), 51 deletions(-)

Range-diff against v1:
 1:  16c99177e6d <  -:  ----------- cmake: don't copy chainlint.pl to build directory
 -:  ----------- >  1:  667a2bd5271 cmake: don't "mkdir -p" and "cd" in build instructions
 -:  ----------- >  2:  9e2470dcb95 cmake: update instructions for portable CMakeLists.txt
 -:  ----------- >  3:  2d7d9742a73 cmake: don't copy chainlint.pl to build directory
 2:  2c1d194e590 !  4:  ea8a3feec81 cmake: chmod +x the bin-wrappers/* & SCRIPT_{SH,PERL} & git-p4
    @@ Metadata
      ## Commit message ##
         cmake: chmod +x the bin-wrappers/* & SCRIPT_{SH,PERL} & git-p4
     
    -    The cmake recipe added in [1] did not create the bin-wrappers/*
    -    directory, and thus fell back on running the tests with the equivalent
    -    of "--no-bin-wrappers".
    +    Since the cmake file was made to run on *nix in [1] running the tests
    +    with "ctest" broken, because we'd attempt to invoke our bin-wrappers/,
    +    but they didn't have the executable bit.
     
    -    Thus the. "t/test-lib.sh" would be unable to find "bin-wrappers/git",
    -    and we'd fall back on "GIT_EXEC_PATH=$GIT_BUILD_DIR" using the
    -    fallback behavior added in [2]:
    +    In the best case, the "t/test-lib.sh" would be unable to find
    +    "bin-wrappers/git", and we'd fall back on
    +    "GIT_EXEC_PATH=$GIT_BUILD_DIR" using the fallback behavior added in
    +    [2]:
     
                 $ ./t0001-init.sh
                 <GIT_BUILD_DIR>/t/../contrib/buildsystems/out/bin-wrappers/git is not executable; using GIT_EXEC_PATH
     
    -    Or rather, this is what would have happened on *nix, but until [3]
    -    there wasn't any non-Windows support for "cmake". On Windows it didn't
    -    matter that the bin-wrappers weren't made executable, since there's no
    -    executable bit, instead the emulation layer looks at whether a file
    -    has a shebang.
    +    This was recently somewhat swept under the rug in [3], as ctest would
    +    run them with "--no-bin-wrappers". But still with [3], running e.g.:
     
    -    But with [3] we've effectively used the semi-equivalent of
    -    "--no-bin-wrappers" unintentionally on *nix, and furthermore because
    -    we didn't make these executable
    +            cmake -S contrib/buildsystems -B contrib/buildsystems/out -DCMAKE_BUILD_TYPE=Debug &&
    +            make -C contrib/buildsystems/out &&
    +            ctest --test-dir contrib/buildsystems/out --jobs="$(nproc)" --output-on-failure
     
    -    In addition, we'd fail any test that needed to invoke one of our built
    -    shell, perl or Python scripts on *nix. E.g. t0012-help.sh would fail
    -    on a test that tried to invoke "git web--browse". I.e. the equivalent
    -    of this (in the "out" directory) would happen:
    +    Fails around 20% of our testts on *nix. So even with [3] we'd fail any
    +    test that needed to invoke one of our built shell, perl or Python
    +    scripts on *nix. E.g. t0012-help.sh would fail on a test that tried to
    +    invoke "git web--browse". The equivalent of this (in the "out"
    +    directory) would happen:
     
                 $ ./git --exec-path=$PWD web--browse
                 git: 'web--browse' is not a git command. See 'git --help'.
    @@ Commit message
                 usage: git web--browse [--browser=browser|--tool=browser] [--config=conf.var] url/file ...
     
         The same goes for e.g. the "git-p4" tests, which would fail because
    -    our built "git-p4" wasn't executable, etc.
    +    our built "git-p4" wasn't executable, etc. There's also a few other
    +    outstanding issues, which will be fixed in subsequent commits.
     
    -    This change should ideally use file(CHMOD ...), but that's much newer
    -    than our required cmake version[1].
    +    This change should ideally use file(CHMOD ...), but the "file(CHMOD"
    +    feature is much newer than our required cmake version[5].
     
    -    1. a30e4c531d9 (Merge branch 'ss/cmake-build', 2020-08-11)
    +    Before this change:
    +
    +            80% tests passed, 196 tests failed out of 977
    +
    +    After:
    +
    +            99% tests passed, 5 tests failed out of 977
    +
    +    The remaining failures will be addressed in subsequent commits.
    +
    +    There was a suggestion of using a function to abstract this away[6],
    +    which sounds good. But after spending too long trying to get all
    +    combinations of "${content}" and ${content} (unqoted) in the function
    +    and its callers working I wasn't able to fix the quoting issues it
    +    introduced.
    +
    +    A lot of this is duplicated already, we can follow-up at some other
    +    time with refactoring, and address any tricky quoting issues in
    +    calling function with these parameters then.
    +
    +    1. f31b6244950 (Merge branch 'yw/cmake-updates', 2022-06-07)
         2. e4597aae659 (run test suite without dashed git-commands in PATH, 2009-12-02)
    -    3. f31b6244950 (Merge branch 'yw/cmake-updates', 2022-06-07)
    -    4. https://cmake.org/cmake/help/latest/command/file.html#chmod
    +    3. 2ea1d8b5563 (cmake: make it easier to diagnose regressions in CTest
    +       runs, 2022-10-18)
    +    4. a30e4c531d9 (Merge branch 'ss/cmake-build', 2020-08-11)
    +    5. https://cmake.org/cmake/help/latest/command/file.html#chmod
    +    6. https://lore.kernel.org/git/0fda0e54-0432-7690-74a7-3d1a59923e0c@dunelm.org.uk/
     
         Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
     
 3:  addaf73992f !  5:  6387682db06 cmake & test-lib.sh: add a $GIT_SOURCE_DIR variable
    @@ Commit message
         recipe has needed to copy various assets to that
         "contrib/buildsystems/out" directory.
     
    -    Let's instead teach the test-lib.sh that there's such a thing as the
    -    "$GIT_SOURCE_DIR" distinct from the "$GIT_BUILD_DIR".
    +    But we've only been doing this for the subsets of tests that run on
    +    Windows, and which have otherwise been covered by that CI target. The
    +    CI target builds (among other things) with "-DPERL_TESTS=OFF
    +    -DPYTHON_TESTS=OFF -DCURL_NO_CURL_CMAKE=ON", see [1]. Furthermore, the
    +    CI isn't testing from the "contrib/buildsystems/out" directory,
    +    instead it clobbers the top-level MAkefile.
    +
    +    There was a recent commit to fix a subset of these issues, see
    +    6a83b5f0810 (cmake: copy the merge tools for testing, 2022-10-18).
    +
    +    Let's stop going for that approach, and instead teach the test-lib.sh
    +    that there's such a thing as the "$GIT_SOURCE_DIR" distinct from the
    +    "$GIT_BUILD_DIR".
     
         Just as the "$TEST_DIRECTORY" always points to our actual "t"
         directory (not the "[...]/out/t" cmake creates), this new
         "$GIT_SOURCE_DIR" will always be the top-level source directory.
     
    -    So even though the "GIT_BUILD_DIR=(.*)" line in t/test-lib.sh will
    -    still be altered by CMakeLists.txt, that recipe will no longer need to
    -    copy over various things from our source directory, as the tests now
    -    know where to find those assets.
    +    With this change we now pass 3/5 of the tests that we still had
    +    failing with the fixes in the preceding commit.
    +
    +    1. 4c2c38e800f (ci: modification of main.yml to use cmake for vs-build
    +       job, 2020-06-26)
     
         Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
     
      ## contrib/buildsystems/CMakeLists.txt ##
     @@ contrib/buildsystems/CMakeLists.txt: if(NOT ${CMAKE_BINARY_DIR}/CMakeCache.txt STREQUAL ${CACHE_PATH})
    - 		"file(STRINGS ${CMAKE_SOURCE_DIR}/t/test-lib.sh content NEWLINE_CONSUME)\n"
    - 		"string(REPLACE \"\${GIT_BUILD_DIR_REPL}\" \"GIT_BUILD_DIR=\\\"$TEST_DIRECTORY/../${BUILD_DIR_RELATIVE}\\\"\" content \"\${content}\")\n"
    - 		"file(WRITE ${CMAKE_SOURCE_DIR}/t/test-lib.sh \${content})")
    + 	#Setting the build directory in test-lib.sh before running tests
    + 	file(WRITE ${CMAKE_BINARY_DIR}/CTestCustom.cmake
    + 		"file(WRITE ${CMAKE_SOURCE_DIR}/GIT-BUILD-DIR \"${CMAKE_BINARY_DIR}\")")
     -	#misc copies
     -	file(COPY ${CMAKE_SOURCE_DIR}/po/is.po DESTINATION ${CMAKE_BINARY_DIR}/po/)
    --	file(COPY ${CMAKE_SOURCE_DIR}/mergetools/tkdiff DESTINATION ${CMAKE_BINARY_DIR}/mergetools/)
    +-	file(GLOB mergetools "${CMAKE_SOURCE_DIR}/mergetools/*")
    +-	file(COPY ${mergetools} DESTINATION ${CMAKE_BINARY_DIR}/mergetools/)
     -	file(COPY ${CMAKE_SOURCE_DIR}/contrib/completion/git-prompt.sh DESTINATION ${CMAKE_BINARY_DIR}/contrib/completion/)
     -	file(COPY ${CMAKE_SOURCE_DIR}/contrib/completion/git-completion.bash DESTINATION ${CMAKE_BINARY_DIR}/contrib/completion/)
      endif()
    @@ t/t9903-bash-prompt.sh: export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME
     
      ## t/test-lib.sh ##
     @@ t/test-lib.sh: then
    - 	# elsewhere
    - 	TEST_OUTPUT_DIRECTORY=$TEST_DIRECTORY
    + 	echo "PANIC: Running in a $TEST_DIRECTORY that doesn't end in '/t'?" >&2
    + 	exit 1
      fi
    --GIT_BUILD_DIR="${TEST_DIRECTORY%/t}"
    -+
    -+# The GIT_{SOURCE,BUILD}_DIR is always the same, except when
    -+# CMakeLists.txt replaces the "GIT_BUILD_DIR" line with the path to
    -+# its build directory.
    -+GIT_SOURCE_DIR="${TEST_DIRECTORY%/t}"
    -+GIT_BUILD_DIR="$GIT_SOURCE_DIR"
     +
    - if test "$TEST_DIRECTORY" = "$GIT_BUILD_DIR"
    ++# For CMake the top-level source directory is different from our build
    ++# directory. With the top-level Makefile they're the same.
    ++GIT_SOURCE_DIR="$GIT_BUILD_DIR"
    + if test -f "$GIT_BUILD_DIR/GIT-BUILD-DIR"
      then
    - 	echo "PANIC: Running in a $TEST_DIRECTORY that doesn't end in '/t'?" >&2
    + 	GIT_BUILD_DIR="$(cat "$GIT_BUILD_DIR/GIT-BUILD-DIR")" || exit 1
     @@ t/test-lib.sh: then
      		make_valgrind_symlink $file
      	done
 4:  52cd674d5b8 !  6:  29a9811857f cmake: set "USE_LIBPCRE2" in "GIT-BUILD-OPTIONS" for test-lib.sh
    @@ Commit message
         on the behavior of !PCRE2. The reason this hasn't been noticed is that
         the Windows CI doesn't have access to libpcre2.
     
    +    With this the remaining two failures we had left after the preceding
    +    step are resolved, but note that that test run didn't include the
    +    git-p4 tests, which a subsequent commit will address).
    +
         1. 80431510a2b (cmake: add pcre2 support, 2022-05-24)
     
         Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
 5:  6199a913c0a !  7:  a9ea3867f5f test-lib.sh: support a "GIT_TEST_BUILD_DIR"
    @@ Commit message
                 [apply this change to git2]
                 (cd git2 && GIT_TEST_BUILD_DIR="$PWD/../git1" make -C t prove)
     
    +    This facility and file-based instructions to have the test suite use
    +    another build directory[2] are mutually exclusive, but in a subsequent
    +    commit we'll make CMake use this instead.
    +
         1. 6720721e152 (test-lib.sh: Allow running the test suite against
            installed git, 2009-03-16)
    +    2. 350a005e366 (cmake: avoid editing t/test-lib.sh, 2022-10-18)
     
         Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
     
    @@ t/README: override the location of the dashed-form subcommands (what
     
      ## t/test-lib.sh ##
     @@ t/test-lib.sh: fi
    - # its build directory.
    - GIT_SOURCE_DIR="${TEST_DIRECTORY%/t}"
    - GIT_BUILD_DIR="$GIT_SOURCE_DIR"
    + # For CMake the top-level source directory is different from our build
    + # directory. With the top-level Makefile they're the same.
    + GIT_SOURCE_DIR="$GIT_BUILD_DIR"
    +-if test -f "$GIT_BUILD_DIR/GIT-BUILD-DIR"
     +if test -n "$GIT_TEST_BUILD_DIR"
     +then
     +	GIT_BUILD_DIR="$GIT_TEST_BUILD_DIR"
    -+fi
    - 
    - if test "$TEST_DIRECTORY" = "$GIT_BUILD_DIR"
    ++elif test -f "$GIT_BUILD_DIR/GIT-BUILD-DIR"
      then
    + 	GIT_BUILD_DIR="$(cat "$GIT_BUILD_DIR/GIT-BUILD-DIR")" || exit 1
    + 	# On Windows, we must convert Windows paths lest they contain a colon
 6:  45f1a4e6f93 <  -:  ----------- cmake: use GIT_TEST_BUILD_DIR instead of editing hack
 7:  fc9f036695f <  -:  ----------- cmake: support using GIT_TEST_OPTS from the environment
 -:  ----------- >  8:  51bb01b99d4 Makefile + cmake: use environment, not GIT-BUILD-DIR
 -:  ----------- >  9:  9f5276d79c9 cmake: support GIT_TEST_OPTS, abstract away WIN32 defaults
 8:  56102c2a4bf = 10:  ef6a304368c cmake: copy over git-p4.py for t983[56] perforce test
 9:  b81f18dec61 = 11:  158a41ca7a4 CI: add a "linux-cmake-test" to run cmake & ctest on linux

Use the "-S" and -B" flags instead of "mkdir -p" and "cd". The "-p" flag to "mkdir" wasn't needed as "contrib/buildsystems" is tracked, and the rest of this is now easier to copy/paste into a shell without having one's directory changed.

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 contrib/buildsystems/CMakeLists.txt | 8 +++-----
 1 file changed, 3 insertions(+), 5 deletions(-)

The instructions for running CMake went back & forth between *nix, Windows and Visual Studio instructions Let's create headings and split the existing instructions up into those new sections.

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 contrib/buildsystems/CMakeLists.txt | 66 +++++++++++++++++++++--------
 1 file changed, 48 insertions(+), 18 deletions(-)

On Wed, Oct 26, 2022 at 11:27 PM Ævar Arnfjörð Bjarmason <avarab@gmail.com> wrote:

First sentence seems to be gobbledygook. I _think_ you wanted to say "Once the recipe is created, something, something, something", though I don't know what "something" was supposed to be.

Too many "Windows".

s/,/;/

When [1] made this copy of "chainlint.sed" the script was invoked in the test-lib.sh as:

	[...] sed -f "$GIT_BUILD_DIR/t/chainlint.sed" [...]

But when [2] replaced "chainlint.sed" with a "chainlint.pl" it also replaced that "$GIT_BUILD_DIR" with "$TEST_DIRECTORY", invoking it as:

	"$PERL_PATH" "$TEST_DIRECTORY/chainlint.pl" "$0"

So this line could have been deleted in [2] but wasn't. Let's do that now.

1. 7f5397a07c6 (cmake: support for testing git when building out of
   the source tree, 2020-06-26)
2. 23a14f30166 (test-lib: replace chainlint.sed with chainlint.pl,
   2022-09-01)

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 contrib/buildsystems/CMakeLists.txt | 1 -
 1 file changed, 1 deletion(-)

When git is built and tested with cmake & ctest we usually do that outside of the top-level source directory. E.g.

	cmake -S contrib/buildsystems -B contrib/buildsystems/out
	make -C contrib/buildsystems/out
	ctest -j$(nproc) --test-dir contrib/buildsystems/out

Since the test-lib.sh makes assumptions about the TEST_DIRECTORY being the "t" subdirectory of the top-level "$GIT_BUILD_DIR", the cmake recipe has needed to copy various assets to that "contrib/buildsystems/out" directory.

But we've only been doing this for the subsets of tests that run on Windows, and which have otherwise been covered by that CI target. The CI target builds (among other things) with "-DPERL_TESTS=OFF -DPYTHON_TESTS=OFF -DCURL_NO_CURL_CMAKE=ON", see [1]. Furthermore, the CI isn't testing from the "contrib/buildsystems/out" directory, instead it clobbers the top-level MAkefile.

There was a recent commit to fix a subset of these issues, see 6a83b5f0810 (cmake: copy the merge tools for testing, 2022-10-18).

Let's stop going for that approach, and instead teach the test-lib.sh that there's such a thing as the "$GIT_SOURCE_DIR" distinct from the "$GIT_BUILD_DIR".

Just as the "$TEST_DIRECTORY" always points to our actual "t" directory (not the "[...]/out/t" cmake creates), this new "$GIT_SOURCE_DIR" will always be the top-level source directory.

With this change we now pass 3/5 of the tests that we still had failing with the fixes in the preceding commit.

1. 4c2c38e800f (ci: modification of main.yml to use cmake for vs-build
   job, 2020-06-26)

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 contrib/buildsystems/CMakeLists.txt |  6 ------
 t/lib-gettext.sh                    |  2 +-
 t/lib-gitweb.sh                     |  2 +-
 t/t7609-mergetool--lib.sh           |  2 +-
 t/t9902-completion.sh               | 14 +++++++-------
 t/t9903-bash-prompt.sh              |  2 +-
 t/test-lib.sh                       |  8 +++++++-
 7 files changed, 18 insertions(+), 18 deletions(-)

Since the cmake file was made to run on *nix in [1] running the tests with "ctest" broken, because we'd attempt to invoke our bin-wrappers/, but they didn't have the executable bit.

In the best case, the "t/test-lib.sh" would be unable to find "bin-wrappers/git", and we'd fall back on "GIT_EXEC_PATH=$GIT_BUILD_DIR" using the fallback behavior added in [2]:

	$ ./t0001-init.sh
	<GIT_BUILD_DIR>/t/../contrib/buildsystems/out/bin-wrappers/git is not executable; using GIT_EXEC_PATH

This was recently somewhat swept under the rug in [3], as ctest would run them with "--no-bin-wrappers". But still with [3], running e.g.:

	cmake -S contrib/buildsystems -B contrib/buildsystems/out -DCMAKE_BUILD_TYPE=Debug &&
	make -C contrib/buildsystems/out &&
	ctest --test-dir contrib/buildsystems/out --jobs="$(nproc)" --output-on-failure

Fails around 20% of our testts on *nix. So even with [3] we'd fail any test that needed to invoke one of our built shell, perl or Python scripts on *nix. E.g. t0012-help.sh would fail on a test that tried to invoke "git web--browse". The equivalent of this (in the "out" directory) would happen:

	$ ./git --exec-path=$PWD web--browse
	git: 'web--browse' is not a git command. See 'git --help'.

Which we can fix by "chmod +x"-ing the built "git-web--browse":

	$ chmod +x git-web--browse
	$ ./git --exec-path=$PWD web--browse
	usage: git web--browse [--browser=browser|--tool=browser] [--config=conf.var] url/file ...

The same goes for e.g. the "git-p4" tests, which would fail because our built "git-p4" wasn't executable, etc. There's also a few other outstanding issues, which will be fixed in subsequent commits.

This change should ideally use file(CHMOD ...), but the "file(CHMOD" feature is much newer than our required cmake version[5].

Before this change:

	80% tests passed, 196 tests failed out of 977

After:

	99% tests passed, 5 tests failed out of 977

The remaining failures will be addressed in subsequent commits.

There was a suggestion of using a function to abstract this away[6], which sounds good. But after spending too long trying to get all combinations of "${content}" and ${content} (unqoted) in the function and its callers working I wasn't able to fix the quoting issues it introduced.

A lot of this is duplicated already, we can follow-up at some other time with refactoring, and address any tricky quoting issues in calling function with these parameters then.

1. f31b6244950 (Merge branch 'yw/cmake-updates', 2022-06-07)
2. e4597aae659 (run test suite without dashed git-commands in PATH, 2009-12-02)
3. 2ea1d8b5563 (cmake: make it easier to diagnose regressions in CTest
   runs, 2022-10-18)
4. a30e4c531d9 (Merge branch 'ss/cmake-build', 2020-08-11)
5. https://cmake.org/cmake/help/latest/command/file.html#chmod
6. https://lore.kernel.org/git/0fda0e54-0432-7690-74a7-3d1a59923e0c@dunelm.org.uk/

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 contrib/buildsystems/CMakeLists.txt | 6 ++++++
 1 file changed, 6 insertions(+)

When we build with libpcre2 which cmake has supported since [1] we need to set "USE_LIBPCRE2='YesPlease'" (or similar) in "GIT-BUILD-OPTIONS".

Without this e.g. t7810-grep.sh will fail, as it has tests that rely on the behavior of !PCRE2. The reason this hasn't been noticed is that the Windows CI doesn't have access to libpcre2.

With this the remaining two failures we had left after the preceding step are resolved, but note that that test run didn't include the git-p4 tests, which a subsequent commit will address).

1. 80431510a2b (cmake: add pcre2 support, 2022-05-24)

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 contrib/buildsystems/CMakeLists.txt | 3 +++
 1 file changed, 3 insertions(+)

Change the method used to have building with "cmake" followed by running the tests manually work without creating inter-dependency issues with the Makefile.

Per ee9e66e4e76 (cmake: avoid editing t/test-lib.sh, 2022-10-18) and the surrounding discussion the goal of that change was to have building with cmake and:

	(cd t && ./t0001-init.sh)

Work, to do so we need some way to make the test-lib.sh aware that we have a build in contrib/buildsystems/out. The downside of doing it via the GIT-BUILD-DIR facility in ee9e66e4e76 is:

- A "remove this file while at it" only fits into the Makefile
  dependency graph if we "FORCE" a shell command to be run, so doing it
  this way precludes speeding up the Makefile by e.g. making use of the
  "$(file)" function (for which I've got follow-up patches).

- If you build with make, then with cmake, and then e.g.:

	make -C t prove

  We'll run the tests against the cmake version of the tests, not the
  "make" built ones. It's important to get any subtle interaction
  between two conflicting build systems right.

Now we'll instead:

- Check if the top-level "git" exists, if not we'll look in
  "contrib/buildsystems/out/git", and when running interactively inform
  the user about the fallback being picked.

	$ ./t0001-init.sh
	setup: had no ../git, but found & used cmake built git in ../contrib/buildsystems/out/git
	ok 1 - plain
	[...]

- This also makes things more obvious on the cmake side, because we'll
  now use an environment variable to select the test environment, which
  ctest it notes in its verbose output, e.g.:

	[...]
	2: Test command: /bin/sh "/home/avar/g/git/contrib/buildsystems/../../t/t0001-init.sh" "--no-bin-wrappers" "--no-chain-lint" "-vx"
	2: Working Directory: /home/avar/g/git/contrib/buildsystems/../../t
	2: Environment variables:
	2:  GIT_TEST_BUILD_DIR=/home/avar/g/git/contrib/buildsystems/out
	[...]

   See [1] for the relevant cmake language references.

- We don't support an arbitrary build directory outside of
  "contrib/buildsystems/out". This is OK because the reason for doing
  this part is to support the workflow documented in [2].

  As [3] notes "contrib/buildsystems/out" isn't just the directory that
  happens to be documented in "contrib/buildsystems/CMakeLists.txt", but
  the one that VS will use when building git.

1. https://cmake.org/cmake/help/latest/command/set_property.html
   https://cmake.org/cmake/help/latest/manual/cmake-properties.7.html#test-properties
   https://cmake.org/cmake/help/latest/manual/cmake-language.7.html#cmake-language-lists
1. f2f1250c47f (cmake (Windows): recommend using Visual Studio's
   built-in CMake support, 2020-09-30)
2. 3eccc7b99d4 (cmake: ignore files generated by CMake as run in
   Visual Studio, 2020-09-25)

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 .gitignore                          |  1 -
 Makefile                            |  1 -
 contrib/buildsystems/CMakeLists.txt | 10 ++--------
 t/test-lib.sh                       | 16 ++++++++++++++--
 4 files changed, 16 insertions(+), 12 deletions(-)

Compliment the existing GIT_TEST_INSTALLED variable added in [1] with one that allows for testing a git built in a directory that's not "../".

Instead of "test this installed git" ("GIT_TEST_INSTALLED"), this new "GIT_TEST_BUILD_DIR" is a "test this built directory over there". E.g. this will pass all tests:

	git clone --depth=1 https://github.com/git/git.git git1
	cp -R git1 git2
	make -C git1
	[apply this change to git2]
	(cd git2 && GIT_TEST_BUILD_DIR="$PWD/../git1" make -C t prove)

This facility and file-based instructions to have the test suite use another build directory[2] are mutually exclusive, but in a subsequent commit we'll make CMake use this instead.

1. 6720721e152 (test-lib.sh: Allow running the test suite against
   installed git, 2009-03-16)
2. 350a005e366 (cmake: avoid editing t/test-lib.sh, 2022-10-18)

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 t/README      | 3 +++
 t/test-lib.sh | 5 ++++-
 2 files changed, 7 insertions(+), 1 deletion(-)

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.

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

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(-)

Since [1] two git-p4 tests have relied on there being a "git-p4.py" in the build directory, but the cmake recipe was not updated to account for this. Let's copy the "git-p4.py" over.

We could also change the test to e.g. grab the built "git-p4" and alter its shebang, which would be friendly to GIT_TEST_INSTALLED, but let's just do the bare minimum here to get cmake+ctest working without altering the test itself.

The reason this hasn't been caught by "vs-build" and "vs-test" is because those tests added in [2] invoke "cmake" with "-DPYTHON_TESTS=OFF", and therefore we'd skip this part of the git-p4 tests before getting past the "do we have python?" check. Even if we got past that the Windows CI wouldn't have a "p4" or "p4d" binary installed, so we'd skip the tests anyway.

In a subsequent commit we'll run "cmake" and "ctest" in CI with "ubuntu-latest", so we'll need this "git-p4.py" file.

1. f7b5ff607fa (git-p4: improve encoding handling to support
   inconsistent encodings, 2022-04-30)
2. 4c2c38e800f (ci: modification of main.yml to use cmake for vs-build
   job, 2020-06-26)

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 contrib/buildsystems/CMakeLists.txt | 1 +
 1 file changed, 1 insertion(+)

Since [1] the "cmake" build method should work properly on Linux, but as seen in preceding commits there were various bugs in it, which are hopefully now all fixed.

To ensure that it doesn't break again let's add a "linux-cmake-ctest" target to build and test "cmake" on Linux, in addition to that we'll also run the tests with "ctest" instead of "make" or "prove", so we can assert that testing with that method works..

This also stress tests running "cmake" (and "ctest") out of a build directory that isn't the top-level. The "vs-build" job uses "cmake" since [2], but clobbers the top-level "Makefile" and builds in the top-level directory.

That was the reason for why we didn't spot that various tests still required missing "mergetools/*" etc, which was fixed by using "$GIT_SOURCE_DIR" in a preceding commit.

Since the "ci/lib.sh" already creates and exports a GIT_TEST_OPTS="..." we'll pick that up in our CI, see the preceding commit. Because we pass the "--verbose-log -x --github-workflow-markup" in the GitHub CI as a result the interaction with "handle_failed_tests" here works correctly. I.e. on failure we'll have saved "t/test-results/*.{exit,out,markup}" files relevant to the failing test(s).

1. f31b6244950 (Merge branch 'yw/cmake-updates', 2022-06-07)
2. 4c2c38e800f (ci: modification of main.yml to use cmake for vs-build
   job, 2020-06-26)

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 .github/workflows/main.yml |  3 +++
 ci/run-build-and-tests.sh  | 13 +++++++++++--
 2 files changed, 14 insertions(+), 2 deletions(-)

This topic gets our tests from passing ~80% with ctest on *nix to passing 100%. Now on top of master, as js/cmake-updates landed.

See passing CI job for "cmake + ctest" on Ubuntu at: https://github.com/avar/git/actions/runs/3370930115/jobs/5593431733

Changes since the v2:

 * Documentation rewording/updates/grammar fixes, thanks to Eric
   Sunshine.

 * A whitespace nit for set_property(), I wasn't using the indenting
   style that's prevalent in the CMakeLists.txt (not our usual "align
   the argument list" tab-indent).

 * Added a Windows ifdef around the recent timeout increase for a
   single submodule test (not an issue on *nix).

Ævar Arnfjörð Bjarmason (12):
  cmake: don't "mkdir -p" and "cd" in build instructions
  cmake: update instructions for portable CMakeLists.txt
  cmake: don't copy chainlint.pl to build directory
  cmake: chmod +x the bin-wrappers/* & SCRIPT_{SH,PERL} & git-p4
  cmake & test-lib.sh: add a $GIT_SOURCE_DIR variable
  cmake: set "USE_LIBPCRE2" in "GIT-BUILD-OPTIONS" for test-lib.sh
  test-lib.sh: support a "GIT_TEST_BUILD_DIR"
  Makefile + cmake: use environment, not GIT-BUILD-DIR
  cmake: support GIT_TEST_OPTS, abstract away WIN32 defaults
  cmake: increase test timeout on Windows only
  cmake: copy over git-p4.py for t983[56] perforce test
  CI: add a "linux-cmake-test" to run cmake & ctest on linux

 .github/workflows/main.yml          |   3 +
 .gitignore                          |   1 -
 Makefile                            |   1 -
 ci/run-build-and-tests.sh           |  13 ++-
 contrib/buildsystems/CMakeLists.txt | 141 +++++++++++++++++++++-------
 t/README                            |   3 +
 t/lib-gettext.sh                    |   2 +-
 t/lib-gitweb.sh                     |   2 +-
 t/t7609-mergetool--lib.sh           |   2 +-
 t/t9902-completion.sh               |  14 +--
 t/t9903-bash-prompt.sh              |   2 +-
 t/test-lib.sh                       |  27 +++++-
 12 files changed, 157 insertions(+), 54 deletions(-)

Range-diff against v2:
 1:  667a2bd5271 =  1:  028fa1436d8 cmake: don't "mkdir -p" and "cd" in build instructions
 2:  9e2470dcb95 !  2:  7a21f4aa24c cmake: update instructions for portable CMakeLists.txt
    @@ contrib/buildsystems/CMakeLists.txt
     +
     +== Building ==
     +
    -+One the recipe is created a build recipe will be created. For Windows
    -+Open contrib/buildsystems/git.sln on Windows and build Git. Or use the
    ++The "cmake" command creates a build file from this recipe. For Windows
    ++Open contrib/buildsystems/git.sln and build Git. Or use the
     +"msbuild" command-line tool (see our own ".github/workflows/main.yml"
     +for a real example):
     +
     +	msbuild git.sln
     +
    -+On all other platforms running "cmake" will generate a Makefile, to
    ++On all other platforms running "cmake" will generate a Makefile; to
     +build with it run:
     +
     +	make -C contrib/buildsystems/out
 3:  2d7d9742a73 =  3:  3bfa873e792 cmake: don't copy chainlint.pl to build directory
 4:  ea8a3feec81 =  4:  ad551f53de9 cmake: chmod +x the bin-wrappers/* & SCRIPT_{SH,PERL} & git-p4
 5:  6387682db06 =  5:  5c7b64286ce cmake & test-lib.sh: add a $GIT_SOURCE_DIR variable
 6:  29a9811857f =  6:  cba90650879 cmake: set "USE_LIBPCRE2" in "GIT-BUILD-OPTIONS" for test-lib.sh
 7:  a9ea3867f5f =  7:  bd7fb1eec24 test-lib.sh: support a "GIT_TEST_BUILD_DIR"
 8:  51bb01b99d4 !  8:  fd3e6be5689 Makefile + cmake: use environment, not GIT-BUILD-DIR
    @@ contrib/buildsystems/CMakeLists.txt: foreach(tsh ${test_scipts})
      	add_test(NAME ${tsh}
      		COMMAND ${SH_EXE} ${tsh} --no-bin-wrappers --no-chain-lint -vx
      		WORKING_DIRECTORY ${CMAKE_SOURCE_DIR}/t)
    -+       set_property(TEST ${tsh} APPEND PROPERTY ENVIRONMENT
    -+		    GIT_TEST_BUILD_DIR=${CMAKE_BINARY_DIR})		
    ++	set_property(TEST ${tsh} APPEND PROPERTY ENVIRONMENT
    ++		GIT_TEST_BUILD_DIR=${CMAKE_BINARY_DIR})
      endforeach()
      
      # This test script takes an extremely long time and is known to time out even
 9:  9f5276d79c9 !  9:  8a660a2baa4 cmake: support GIT_TEST_OPTS, abstract away WIN32 defaults
    @@ contrib/buildsystems/CMakeLists.txt: endif()
     -		COMMAND ${SH_EXE} ${tsh} --no-bin-wrappers --no-chain-lint -vx
     +		COMMAND ${SH_EXE} ${tsh} ${GIT_TEST_OPTS}
      		WORKING_DIRECTORY ${CMAKE_SOURCE_DIR}/t)
    -        set_property(TEST ${tsh} APPEND PROPERTY ENVIRONMENT
    - 		    GIT_TEST_BUILD_DIR=${CMAKE_BINARY_DIR})		
    + 	set_property(TEST ${tsh} APPEND PROPERTY ENVIRONMENT
    + 		GIT_TEST_BUILD_DIR=${CMAKE_BINARY_DIR})
 -:  ----------- > 10:  966fec83b77 cmake: increase test timeout on Windows only
10:  ef6a304368c = 11:  aad17d8f858 cmake: copy over git-p4.py for t983[56] perforce test
11:  158a41ca7a4 = 12:  c27f620dfa3 CI: add a "linux-cmake-test" to run cmake & ctest on linux

Use the "-S" and -B" flags instead of "mkdir -p" and "cd". The "-p" flag to "mkdir" wasn't needed as "contrib/buildsystems" is tracked, and the rest of this is now easier to copy/paste into a shell without having one's directory changed.

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 contrib/buildsystems/CMakeLists.txt | 8 +++-----
 1 file changed, 3 insertions(+), 5 deletions(-)

On 01/11/2022 22:51, Ævar Arnfjörð Bjarmason wrote:

When was "-B" added? This feels like a personal preference rather than an objective improvement. Arguably changing into the build directory is actually better as ctest has to be run from there. It also appears that these instructions have not been tested as the make invocation in wrong.

Best Wishes

Phillip

The instructions for running CMake went back & forth between *nix, Windows and Visual Studio instructions Let's create headings and split the existing instructions up into those new sections.

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 contrib/buildsystems/CMakeLists.txt | 66 +++++++++++++++++++++--------
 1 file changed, 48 insertions(+), 18 deletions(-)

On Tue, Nov 1, 2022 at 6:51 PM Ævar Arnfjörð Bjarmason <avarab@gmail.com> wrote:

I suspect you wanted: s/Windows Open contrib/Windows, open contrib/

On 01/11/2022 22:51, Ævar Arnfjörð Bjarmason wrote:

If that is the primary use-case (which I agree it is) then it perhaps we should but the instructions for that use at the top of the document.

Rather than forcing the reader to jump back and forth, perhaps we could document -DCMAKE_BUILD_TYPE and -GNinja here.

Best Wishes

Phillip

On Thu, Nov 03 2022, Phillip Wood wrote:

Yes, I've tried to re-arrange it like that in the incoming v4.

*nod*, changing...

When [1] made this copy of "chainlint.sed" the script was invoked in the test-lib.sh as:

	[...] sed -f "$GIT_BUILD_DIR/t/chainlint.sed" [...]

But when [2] replaced "chainlint.sed" with a "chainlint.pl" it also replaced that "$GIT_BUILD_DIR" with "$TEST_DIRECTORY", invoking it as:

	"$PERL_PATH" "$TEST_DIRECTORY/chainlint.pl" "$0"

So this line could have been deleted in [2] but wasn't. Let's do that now.

1. 7f5397a07c6 (cmake: support for testing git when building out of
   the source tree, 2020-06-26)
2. 23a14f30166 (test-lib: replace chainlint.sed with chainlint.pl,
   2022-09-01)

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 contrib/buildsystems/CMakeLists.txt | 1 -
 1 file changed, 1 deletion(-)

Since the cmake file was made to run on *nix in [1] running the tests with "ctest" broken, because we'd attempt to invoke our bin-wrappers/, but they didn't have the executable bit.

In the best case, the "t/test-lib.sh" would be unable to find "bin-wrappers/git", and we'd fall back on "GIT_EXEC_PATH=$GIT_BUILD_DIR" using the fallback behavior added in [2]:

	$ ./t0001-init.sh
	<GIT_BUILD_DIR>/t/../contrib/buildsystems/out/bin-wrappers/git is not executable; using GIT_EXEC_PATH

This was recently somewhat swept under the rug in [3], as ctest would run them with "--no-bin-wrappers". But still with [3], running e.g.:

	cmake -S contrib/buildsystems -B contrib/buildsystems/out -DCMAKE_BUILD_TYPE=Debug &&
	make -C contrib/buildsystems/out &&
	ctest --test-dir contrib/buildsystems/out --jobs="$(nproc)" --output-on-failure

Fails around 20% of our testts on *nix. So even with [3] we'd fail any test that needed to invoke one of our built shell, perl or Python scripts on *nix. E.g. t0012-help.sh would fail on a test that tried to invoke "git web--browse". The equivalent of this (in the "out" directory) would happen:

	$ ./git --exec-path=$PWD web--browse
	git: 'web--browse' is not a git command. See 'git --help'.

Which we can fix by "chmod +x"-ing the built "git-web--browse":

	$ chmod +x git-web--browse
	$ ./git --exec-path=$PWD web--browse
	usage: git web--browse [--browser=browser|--tool=browser] [--config=conf.var] url/file ...

The same goes for e.g. the "git-p4" tests, which would fail because our built "git-p4" wasn't executable, etc. There's also a few other outstanding issues, which will be fixed in subsequent commits.

This change should ideally use file(CHMOD ...), but the "file(CHMOD" feature is much newer than our required cmake version[5].

Before this change:

	80% tests passed, 196 tests failed out of 977

After:

	99% tests passed, 5 tests failed out of 977

The remaining failures will be addressed in subsequent commits.

There was a suggestion of using a function to abstract this away[6], which sounds good. But after spending too long trying to get all combinations of "${content}" and ${content} (unqoted) in the function and its callers working I wasn't able to fix the quoting issues it introduced.

A lot of this is duplicated already, we can follow-up at some other time with refactoring, and address any tricky quoting issues in calling function with these parameters then.

1. f31b6244950 (Merge branch 'yw/cmake-updates', 2022-06-07)
2. e4597aae659 (run test suite without dashed git-commands in PATH, 2009-12-02)
3. 2ea1d8b5563 (cmake: make it easier to diagnose regressions in CTest
   runs, 2022-10-18)
4. a30e4c531d9 (Merge branch 'ss/cmake-build', 2020-08-11)
5. https://cmake.org/cmake/help/latest/command/file.html#chmod
6. https://lore.kernel.org/git/0fda0e54-0432-7690-74a7-3d1a59923e0c@dunelm.org.uk/

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 contrib/buildsystems/CMakeLists.txt | 6 ++++++
 1 file changed, 6 insertions(+)

When git is built and tested with cmake & ctest we usually do that outside of the top-level source directory. E.g.

	cmake -S contrib/buildsystems -B contrib/buildsystems/out
	make -C contrib/buildsystems/out
	ctest -j$(nproc) --test-dir contrib/buildsystems/out

Since the test-lib.sh makes assumptions about the TEST_DIRECTORY being the "t" subdirectory of the top-level "$GIT_BUILD_DIR", the cmake recipe has needed to copy various assets to that "contrib/buildsystems/out" directory.

But we've only been doing this for the subsets of tests that run on Windows, and which have otherwise been covered by that CI target. The CI target builds (among other things) with "-DPERL_TESTS=OFF -DPYTHON_TESTS=OFF -DCURL_NO_CURL_CMAKE=ON", see [1]. Furthermore, the CI isn't testing from the "contrib/buildsystems/out" directory, instead it clobbers the top-level MAkefile.

There was a recent commit to fix a subset of these issues, see 6a83b5f0810 (cmake: copy the merge tools for testing, 2022-10-18).

Let's stop going for that approach, and instead teach the test-lib.sh that there's such a thing as the "$GIT_SOURCE_DIR" distinct from the "$GIT_BUILD_DIR".

Just as the "$TEST_DIRECTORY" always points to our actual "t" directory (not the "[...]/out/t" cmake creates), this new "$GIT_SOURCE_DIR" will always be the top-level source directory.

With this change we now pass 3/5 of the tests that we still had failing with the fixes in the preceding commit.

1. 4c2c38e800f (ci: modification of main.yml to use cmake for vs-build
   job, 2020-06-26)

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 contrib/buildsystems/CMakeLists.txt |  6 ------
 t/lib-gettext.sh                    |  2 +-
 t/lib-gitweb.sh                     |  2 +-
 t/t7609-mergetool--lib.sh           |  2 +-
 t/t9902-completion.sh               | 14 +++++++-------
 t/t9903-bash-prompt.sh              |  2 +-
 t/test-lib.sh                       |  8 +++++++-
 7 files changed, 18 insertions(+), 18 deletions(-)

When we build with libpcre2 which cmake has supported since [1] we need to set "USE_LIBPCRE2='YesPlease'" (or similar) in "GIT-BUILD-OPTIONS".

Without this e.g. t7810-grep.sh will fail, as it has tests that rely on the behavior of !PCRE2. The reason this hasn't been noticed is that the Windows CI doesn't have access to libpcre2.

With this the remaining two failures we had left after the preceding step are resolved, but note that that test run didn't include the git-p4 tests, which a subsequent commit will address).

1. 80431510a2b (cmake: add pcre2 support, 2022-05-24)

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 contrib/buildsystems/CMakeLists.txt | 3 +++
 1 file changed, 3 insertions(+)

Compliment the existing GIT_TEST_INSTALLED variable added in [1] with one that allows for testing a git built in a directory that's not "../".

Instead of "test this installed git" ("GIT_TEST_INSTALLED"), this new "GIT_TEST_BUILD_DIR" is a "test this built directory over there". E.g. this will pass all tests:

	git clone --depth=1 https://github.com/git/git.git git1
	cp -R git1 git2
	make -C git1
	[apply this change to git2]
	(cd git2 && GIT_TEST_BUILD_DIR="$PWD/../git1" make -C t prove)

This facility and file-based instructions to have the test suite use another build directory[2] are mutually exclusive, but in a subsequent commit we'll make CMake use this instead.

1. 6720721e152 (test-lib.sh: Allow running the test suite against
   installed git, 2009-03-16)
2. 350a005e366 (cmake: avoid editing t/test-lib.sh, 2022-10-18)

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 t/README      | 3 +++
 t/test-lib.sh | 5 ++++-
 2 files changed, 7 insertions(+), 1 deletion(-)

Change the method used to have building with "cmake" followed by running the tests manually work without creating inter-dependency issues with the Makefile.

Per ee9e66e4e76 (cmake: avoid editing t/test-lib.sh, 2022-10-18) and the surrounding discussion the goal of that change was to have building with cmake and:

	(cd t && ./t0001-init.sh)

Work, to do so we need some way to make the test-lib.sh aware that we have a build in contrib/buildsystems/out. The downside of doing it via the GIT-BUILD-DIR facility in ee9e66e4e76 is:

- A "remove this file while at it" only fits into the Makefile
  dependency graph if we "FORCE" a shell command to be run, so doing it
  this way precludes speeding up the Makefile by e.g. making use of the
  "$(file)" function (for which I've got follow-up patches).

- If you build with make, then with cmake, and then e.g.:

	make -C t prove

  We'll run the tests against the cmake version of the tests, not the
  "make" built ones. It's important to get any subtle interaction
  between two conflicting build systems right.

Now we'll instead:

- Check if the top-level "git" exists, if not we'll look in
  "contrib/buildsystems/out/git", and when running interactively inform
  the user about the fallback being picked.

	$ ./t0001-init.sh
	setup: had no ../git, but found & used cmake built git in ../contrib/buildsystems/out/git
	ok 1 - plain
	[...]

- This also makes things more obvious on the cmake side, because we'll
  now use an environment variable to select the test environment, which
  ctest it notes in its verbose output, e.g.:

	[...]
	2: Test command: /bin/sh "/home/avar/g/git/contrib/buildsystems/../../t/t0001-init.sh" "--no-bin-wrappers" "--no-chain-lint" "-vx"
	2: Working Directory: /home/avar/g/git/contrib/buildsystems/../../t
	2: Environment variables:
	2:  GIT_TEST_BUILD_DIR=/home/avar/g/git/contrib/buildsystems/out
	[...]

   See [1] for the relevant cmake language references.

- We don't support an arbitrary build directory outside of
  "contrib/buildsystems/out". This is OK because the reason for doing
  this part is to support the workflow documented in [2].

  As [3] notes "contrib/buildsystems/out" isn't just the directory that
  happens to be documented in "contrib/buildsystems/CMakeLists.txt", but
  the one that VS will use when building git.

1. https://cmake.org/cmake/help/latest/command/set_property.html
   https://cmake.org/cmake/help/latest/manual/cmake-properties.7.html#test-properties
   https://cmake.org/cmake/help/latest/manual/cmake-language.7.html#cmake-language-lists
1. f2f1250c47f (cmake (Windows): recommend using Visual Studio's
   built-in CMake support, 2020-09-30)
2. 3eccc7b99d4 (cmake: ignore files generated by CMake as run in
   Visual Studio, 2020-09-25)

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 .gitignore                          |  1 -
 Makefile                            |  1 -
 contrib/buildsystems/CMakeLists.txt | 10 ++--------
 t/test-lib.sh                       | 16 ++++++++++++++--
 4 files changed, 16 insertions(+), 12 deletions(-)

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.

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

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(-)

On 01/11/2022 22:51, Ævar Arnfjörð Bjarmason wrote:

Shouldn't there a documentation string for the variable if you want to support cmake-gui?

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.

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)

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

I think it would be helpful to show setting --jobs at configure time as it makes running the tests simpler.

if (DEFINED ENV{GIT_TEST_OPTS}) ?

I'd like to see us setting -vx here so users get debugging logs

What rules does this use for separating arguments?

Best Wishes

Phillip

On Thu, Nov 03 2022, Phillip Wood wrote:

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

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.

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

Used /dev/shm in the updated docs.

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.

Thanks! That's much better.

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"

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.

Since [1] two git-p4 tests have relied on there being a "git-p4.py" in the build directory, but the cmake recipe was not updated to account for this. Let's copy the "git-p4.py" over.

We could also change the test to e.g. grab the built "git-p4" and alter its shebang, which would be friendly to GIT_TEST_INSTALLED, but let's just do the bare minimum here to get cmake+ctest working without altering the test itself.

The reason this hasn't been caught by "vs-build" and "vs-test" is because those tests added in [2] invoke "cmake" with "-DPYTHON_TESTS=OFF", and therefore we'd skip this part of the git-p4 tests before getting past the "do we have python?" check. Even if we got past that the Windows CI wouldn't have a "p4" or "p4d" binary installed, so we'd skip the tests anyway.

In a subsequent commit we'll run "cmake" and "ctest" in CI with "ubuntu-latest", so we'll need this "git-p4.py" file.

1. f7b5ff607fa (git-p4: improve encoding handling to support
   inconsistent encodings, 2022-04-30)
2. 4c2c38e800f (ci: modification of main.yml to use cmake for vs-build
   job, 2020-06-26)

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 contrib/buildsystems/CMakeLists.txt | 1 +
 1 file changed, 1 insertion(+)

Per c858750b41c (cmake: increase time-out for a long-running test, 2022-10-18) the reason to set a custom timeout for t7112-reset-submodule.sh is Windows-specific. Let's only do that on Windows then.

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 contrib/buildsystems/CMakeLists.txt | 8 +++++---
 1 file changed, 5 insertions(+), 3 deletions(-)

Since [1] the "cmake" build method should work properly on Linux, but as seen in preceding commits there were various bugs in it, which are hopefully now all fixed.

To ensure that it doesn't break again let's add a "linux-cmake-ctest" target to build and test "cmake" on Linux, in addition to that we'll also run the tests with "ctest" instead of "make" or "prove", so we can assert that testing with that method works..

This also stress tests running "cmake" (and "ctest") out of a build directory that isn't the top-level. The "vs-build" job uses "cmake" since [2], but clobbers the top-level "Makefile" and builds in the top-level directory.

That was the reason for why we didn't spot that various tests still required missing "mergetools/*" etc, which was fixed by using "$GIT_SOURCE_DIR" in a preceding commit.

Since the "ci/lib.sh" already creates and exports a GIT_TEST_OPTS="..." we'll pick that up in our CI, see the preceding commit. Because we pass the "--verbose-log -x --github-workflow-markup" in the GitHub CI as a result the interaction with "handle_failed_tests" here works correctly. I.e. on failure we'll have saved "t/test-results/*.{exit,out,markup}" files relevant to the failing test(s).

1. f31b6244950 (Merge branch 'yw/cmake-updates', 2022-06-07)
2. 4c2c38e800f (ci: modification of main.yml to use cmake for vs-build
   job, 2020-06-26)

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 .github/workflows/main.yml |  3 +++
 ci/run-build-and-tests.sh  | 13 +++++++++++--
 2 files changed, 14 insertions(+), 2 deletions(-)

This topic gets our tests from passing ~80% with ctest on *nix to passing 100%.

See passing CI job for "cmake + ctest" on Ubuntu at: https://github.com/avar/git/actions/runs/3387253536/jobs/5627772317

See http://lore.kernel.org/git/cover-v3-00.12-00000000000-20221101T225022Z-avarab@gmail.com for v3.

Changes since the v3:

 * A small "while at it" change in 1/14 to make the build output nicer
   (particularly for "ninja, as it's not verbose). It's just the
   "msgfmt without --statistics" change we've got in the Makefile
   already.

 * Discuss more about -S and -B in 2/14. It's supported by our cmake
   version(s0.

 * Re-arranged the docs in 3/14 as Phillip Wood suggested.

 * Used Phillip's write_script(). I tested the emitted files
   carefully, and they're the same. So something went wrong in my
   testing before.

 * Use Phillip's file(CHMOD ...) if we have a new enough cmake
   version, otherwise shell out to "chmod +x".

 * Use /dev/shm instead of /run/user/`id -u` in an exmaple, and other
   small things noted in replies to v3.

 * A new 12/14 to do less Windows-specific stuff on *nix, we were
   looking for "sh" in C:\Program\ Files on *nix...

 * The GIT_TEST_OPTS integration with cmake-gui now works.

 * Various small rewording/typo etc. changes to commit messages, see
   range-diff below.

Ævar Arnfjörð Bjarmason (14):
  cmake: don't invoke msgfmt with --statistics
  cmake: use "-S" and "-B" to specify source and build directories
  cmake: update instructions for portable CMakeLists.txt
  cmake: don't copy chainlint.pl to build directory
  cmake: chmod +x the bin-wrappers/* & SCRIPT_{SH,PERL} & git-p4
  cmake & test-lib.sh: add a $GIT_SOURCE_DIR variable
  cmake: set "USE_LIBPCRE2" in "GIT-BUILD-OPTIONS" for test-lib.sh
  test-lib.sh: support a "GIT_TEST_BUILD_DIR"
  Makefile + cmake: use environment, not GIT-BUILD-DIR
  cmake: support GIT_TEST_OPTS, abstract away WIN32 defaults
  cmake: increase test timeout on Windows only
  cmake: only look for "sh" in "C:/Program Files" on Windows
  cmake: copy over git-p4.py for t983[56] perforce test
  CI: add a "linux-cmake-test" to run cmake & ctest on linux

 .github/workflows/main.yml          |   3 +
 .gitignore                          |   1 -
 Makefile                            |   1 -
 ci/run-build-and-tests.sh           |  13 +-
 contrib/buildsystems/CMakeLists.txt | 183 +++++++++++++++++++++-------
 t/README                            |   3 +
 t/lib-gettext.sh                    |   2 +-
 t/lib-gitweb.sh                     |   2 +-
 t/t7609-mergetool--lib.sh           |   2 +-
 t/t9902-completion.sh               |  14 +--
 t/t9903-bash-prompt.sh              |   2 +-
 t/test-lib.sh                       |  27 +++-
 12 files changed, 188 insertions(+), 65 deletions(-)

Range-diff against v3:
 -:  ----------- >  1:  78dfc2a69b7 cmake: don't invoke msgfmt with --statistics
 1:  028fa1436d8 !  2:  dd934b0597d cmake: don't "mkdir -p" and "cd" in build instructions
    @@ Metadata
     Author: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
     
      ## Commit message ##
    -    cmake: don't "mkdir -p" and "cd" in build instructions
    +    cmake: use "-S" and "-B" to specify source and build directories
     
    -    Use the "-S" and -B" flags instead of "mkdir -p" and "cd". The "-p"
    -    flag to "mkdir" wasn't needed as "contrib/buildsystems" is tracked,
    -    and the rest of this is now easier to copy/paste into a shell without
    -    having one's directory changed.
    +    Rather than the multi-line "mkdir/cd/cmake" recipe provide an
    +    equivalent one-liner using the "-S" and "-B" options, and then suggest building with "make -C <build-dir>".
    +
    +    The rest of these instructions discuss e.g. running tests from our
    +    top-level "t/" directory, so it's more helpful to avoid changing the
    +    user's current directory.
    +
    +    The "-S" and "-B" options were added in cmake v3.13.0, which is older
    +    than the version we have a hard dependency on[1].
    +
    +    As an aside The "-p" flag to "mkdir" in the pre-image wasn't needed,
    +    as "contrib/buildsystems" is tracked
    +
    +    1. 061c2240b1b (Introduce CMake support for configuring Git, 2020-06-12)
     
         Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
     
    @@ contrib/buildsystems/CMakeLists.txt: NOTE: -DCMAKE_BUILD_TYPE is optional. For m
      This process generates a Makefile(Linux/*BSD/MacOS) , Visual Studio solution(Windows) by default.
     -Run `make` to build Git on Linux/*BSD/MacOS.
     -Open git.sln on Windows and build Git.
    -+Run `make -C contrib/buildsystems` to build Git on Linux/*BSD/MacOS.
    ++Run `make -C contrib/buildsystems/out` to build Git on Linux/*BSD/MacOS.
     +Open contrib/buildsystems/git.sln on Windows and build Git.
      
      NOTE: By default CMake uses Makefile as the build tool on Linux and Visual Studio in Windows,
 2:  7a21f4aa24c !  3:  6f0eae7a02a cmake: update instructions for portable CMakeLists.txt
    @@ contrib/buildsystems/CMakeLists.txt
     +alternative.
     +
     +The primary use-case for maintaining this CMake build recipe is to
    -+have nicer IDE integration on Windows. To get Visual Studio-specific
    -+instructions see "== Visual Studio & Windows ==" below.
    ++have nicer IDE integration on Windows.
     +
     +== Creating a build recipe ==
     +
    -+To create the build recipe run:
    ++The "cmake" command creates a build file from this recipe:
     +
     +    cmake -S contrib/buildsystems -B contrib/buildsystems/out -DCMAKE_BUILD_TYPE=Release
     +
    -+For alternative "-DCMAKE_BUILD_TYPE=<type>" flags see instructions
    -+under the "== -DCMAKE_BUILD_TYPE=<type> ==" heading below.
    ++Running this will create files in the contrib/buildsystems/out
    ++directory (our top-level .gitignore file knows to ignore contents of
    ++this directory).
     +
    -+== Building ==
    ++See "cmake options" below for a discussion of
    ++"-DCMAKE_BUILD_TYPE=Release" and other options to "cmake".
     +
    -+The "cmake" command creates a build file from this recipe. For Windows
    -+Open contrib/buildsystems/git.sln and build Git. Or use the
    -+"msbuild" command-line tool (see our own ".github/workflows/main.yml"
    -+for a real example):
    ++== Building with Visual Visual Studio ==
    ++
    ++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,
    +@@ contrib/buildsystems/CMakeLists.txt: Note: Visual Studio also has the option of opening `CMakeLists.txt`
    + directly; Using this option, Visual Studio will not find the source code,
    + though, therefore the `File>Open>Folder...` option is preferred.
    + 
    +-Instructions to run CMake manually:
    ++By default CMake will install vcpkg locally to your source tree on configuration,
    ++to avoid this, add `-DNO_VCPKG=TRUE` to the command line when configuring.
    + 
    +-    cmake -S contrib/buildsystems -B contrib/buildsystems/out -DCMAKE_BUILD_TYPE=Release
    ++== Building on Windows without Visual Studio ==
    + 
    +-This will build the git binaries in contrib/buildsystems/out
    +-directory (our top-level .gitignore file knows to ignore contents of
    +-this directory).
    ++Open contrib/buildsystems/git.sln and build Git. Or use the "msbuild"
    ++command-line tool (see our own ".github/workflows/main.yml" for a real
    ++example):
     +
     +	msbuild git.sln
     +
    ++== Building on *nix ==
    ++
     +On all other platforms running "cmake" will generate a Makefile; to
     +build with it run:
     +
    @@ contrib/buildsystems/CMakeLists.txt
     +
     +It's also possible to use other generators, e.g. Ninja has arguably
     +slightly better output. Add "-G Ninja" to the cmake command above,
    -+then e.g.:
    ++then:
     +
     +	ninja -C contrib/buildsystems/out
     +
    -+== Visual Studio & Windows ==
    ++== cmake options ==
     +
    -+To use this in Visual Studio:
    ++=== -DCMAKE_BUILD_TYPE=<type> ===
      
    - Open the worktree as a folder. Visual Studio 2019 and later will detect
    - the CMake configuration automatically and set everything up for you,
    -@@ contrib/buildsystems/CMakeLists.txt: Note: Visual Studio also has the option of opening `CMakeLists.txt`
    - directly; Using this option, Visual Studio will not find the source code,
    - though, therefore the `File>Open>Folder...` option is preferred.
    - 
    --Instructions to run CMake manually:
    --
    --    cmake -S contrib/buildsystems -B contrib/buildsystems/out -DCMAKE_BUILD_TYPE=Release
    --
    --This will build the git binaries in contrib/buildsystems/out
    -+Following the instructions above will build the git binaries in the contrib/buildsystems/out
    - directory (our top-level .gitignore file knows to ignore contents of
    - this directory).
    - 
    -+By default CMake will install vcpkg locally to your source tree on configuration,
    -+to avoid this, add `-DNO_VCPKG=TRUE` to the command line when configuring.
    -+
    -+== -DCMAKE_BUILD_TYPE=<type> ==
    -+
      Possible build configurations(-DCMAKE_BUILD_TYPE) with corresponding
      compiler flags
    - Debug : -g
     @@ contrib/buildsystems/CMakeLists.txt: empty(default) :
      
      NOTE: -DCMAKE_BUILD_TYPE is optional. For multi-config generators like Visual Studio
      this option is ignored
     -
     -This process generates a Makefile(Linux/*BSD/MacOS) , Visual Studio solution(Windows) by default.
    --Run `make -C contrib/buildsystems` to build Git on Linux/*BSD/MacOS.
    +-Run `make -C contrib/buildsystems/out` to build Git on Linux/*BSD/MacOS.
     -Open contrib/buildsystems/git.sln on Windows and build Git.
     -
     -NOTE: By default CMake uses Makefile as the build tool on Linux and Visual Studio in Windows,
 3:  3bfa873e792 =  4:  0f0eb2a76c7 cmake: don't copy chainlint.pl to build directory
 4:  ad551f53de9 !  5:  eda1c1e95e5 cmake: chmod +x the bin-wrappers/* & SCRIPT_{SH,PERL} & git-p4
    @@ Commit message
                 make -C contrib/buildsystems/out &&
                 ctest --test-dir contrib/buildsystems/out --jobs="$(nproc)" --output-on-failure
     
    -    Fails around 20% of our testts on *nix. So even with [3] we'd fail any
    +    Fails around 20% of our tests on *nix. So even with [3] we'd fail any
         test that needed to invoke one of our built shell, perl or Python
         scripts on *nix. E.g. t0012-help.sh would fail on a test that tried to
         invoke "git web--browse". The equivalent of this (in the "out"
    @@ Commit message
         our built "git-p4" wasn't executable, etc. There's also a few other
         outstanding issues, which will be fixed in subsequent commits.
     
    -    This change should ideally use file(CHMOD ...), but the "file(CHMOD"
    -    feature is much newer than our required cmake version[5].
    +    Ideally we'd use the file(CHMOD ...) form everywhere, but that syntax
    +    was introduced in cmake 3.19[4], whereas we only require 3.14. Let's
    +    provide a fallback behind a version check, so that we'll eventually be
    +    able to delete the "else" part. Both forms result in the same file
    +    modes.
     
         Before this change:
     
    @@ Commit message
     
         The remaining failures will be addressed in subsequent commits.
     
    -    There was a suggestion of using a function to abstract this away[6],
    -    which sounds good. But after spending too long trying to get all
    -    combinations of "${content}" and ${content} (unqoted) in the function
    -    and its callers working I wasn't able to fix the quoting issues it
    -    introduced.
    -
    -    A lot of this is duplicated already, we can follow-up at some other
    -    time with refactoring, and address any tricky quoting issues in
    -    calling function with these parameters then.
    -
         1. f31b6244950 (Merge branch 'yw/cmake-updates', 2022-06-07)
         2. e4597aae659 (run test suite without dashed git-commands in PATH, 2009-12-02)
         3. 2ea1d8b5563 (cmake: make it easier to diagnose regressions in CTest
            runs, 2022-10-18)
    -    4. a30e4c531d9 (Merge branch 'ss/cmake-build', 2020-08-11)
    -    5. https://cmake.org/cmake/help/latest/command/file.html#chmod
    -    6. https://lore.kernel.org/git/0fda0e54-0432-7690-74a7-3d1a59923e0c@dunelm.org.uk/
    +    4. https://cmake.org/cmake/help/latest/command/file.html#chmod
     
         Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
     
    +    write script
    +
      ## contrib/buildsystems/CMakeLists.txt ##
    +@@ contrib/buildsystems/CMakeLists.txt: add_custom_command(OUTPUT ${git_links} ${git_http_links}
    + 		DEPENDS git git-remote-http)
    + add_custom_target(git-links ALL DEPENDS ${git_links} ${git_http_links})
    + 
    ++function(write_script path content)
    ++	file(WRITE ${path} ${content})
    ++
    ++	if("${CMAKE_MAJOR_VERSION}.${CMAKE_MINOR_VERSION}" VERSION_GREATER_EQUAL "3.19")
    ++		file(CHMOD ${path} FILE_PERMISSIONS OWNER_READ OWNER_WRITE OWNER_EXECUTE)
    ++	else()
    ++		execute_process(COMMAND chmod +x ${path}
    ++				RESULT_VARIABLE CHILD_ERROR)
    ++		if(CHILD_ERROR)
    ++			message(FATAL_ERROR "failed to chmod +x '${path}': '${CHILD_ERROR}'")
    ++		endif()
    ++	endif()
    ++endfunction()
    + 
    + #creating required scripts
    + set(SHELL_PATH /bin/sh)
     @@ contrib/buildsystems/CMakeLists.txt: foreach(script ${git_shell_scripts})
    + 	string(REPLACE "# @@BROKEN_PATH_FIX@@" "" content "${content}")
      	string(REPLACE "@@PERL@@" "${PERL_PATH}" content "${content}")
      	string(REPLACE "@@PAGER_ENV@@" "LESS=FRX LV=-c" content "${content}")
    - 	file(WRITE ${CMAKE_BINARY_DIR}/${script} ${content})
    -+	execute_process(COMMAND chmod +x ${CMAKE_BINARY_DIR}/${script})
    +-	file(WRITE ${CMAKE_BINARY_DIR}/${script} ${content})
    ++	write_script(${CMAKE_BINARY_DIR}/${script} "${content}")
      endforeach()
      
      #perl scripts
     @@ contrib/buildsystems/CMakeLists.txt: foreach(script ${git_perl_scripts})
    + 	file(STRINGS ${CMAKE_SOURCE_DIR}/${script}.perl content NEWLINE_CONSUME)
      	string(REPLACE "#!/usr/bin/perl" "#!/usr/bin/perl\n${perl_header}\n" content "${content}")
      	string(REPLACE "@@GIT_VERSION@@" "${PROJECT_VERSION}" content "${content}")
    - 	file(WRITE ${CMAKE_BINARY_DIR}/${script} ${content})
    -+	execute_process(COMMAND chmod +x ${CMAKE_BINARY_DIR}/${script})
    +-	file(WRITE ${CMAKE_BINARY_DIR}/${script} ${content})
    ++	write_script(${CMAKE_BINARY_DIR}/${script} "${content}")
      endforeach()
      
      #python script
      file(STRINGS ${CMAKE_SOURCE_DIR}/git-p4.py content NEWLINE_CONSUME)
      string(REPLACE "#!/usr/bin/env python" "#!/usr/bin/python" content "${content}")
    - file(WRITE ${CMAKE_BINARY_DIR}/git-p4 ${content})
    -+execute_process(COMMAND chmod +x ${CMAKE_BINARY_DIR}/git-p4)
    +-file(WRITE ${CMAKE_BINARY_DIR}/git-p4 ${content})
    ++write_script(${CMAKE_BINARY_DIR}/git-p4 "${content}")
      
      #perl modules
      file(GLOB_RECURSE perl_modules "${CMAKE_SOURCE_DIR}/perl/*.pm")
     @@ contrib/buildsystems/CMakeLists.txt: foreach(script ${wrapper_scripts})
    + 	file(STRINGS ${CMAKE_SOURCE_DIR}/wrap-for-bin.sh content NEWLINE_CONSUME)
      	string(REPLACE "@@BUILD_DIR@@" "${CMAKE_BINARY_DIR}" content "${content}")
      	string(REPLACE "@@PROG@@" "${script}${EXE_EXTENSION}" content "${content}")
    - 	file(WRITE ${CMAKE_BINARY_DIR}/bin-wrappers/${script} ${content})
    -+	execute_process(COMMAND chmod +x ${CMAKE_BINARY_DIR}/bin-wrappers/${script})
    +-	file(WRITE ${CMAKE_BINARY_DIR}/bin-wrappers/${script} ${content})
    ++	write_script(${CMAKE_BINARY_DIR}/bin-wrappers/${script} "${content}")
      endforeach()
      
      foreach(script ${wrapper_test_scripts})
    -@@ contrib/buildsystems/CMakeLists.txt: foreach(script ${wrapper_test_scripts})
    + 	file(STRINGS ${CMAKE_SOURCE_DIR}/wrap-for-bin.sh content NEWLINE_CONSUME)
      	string(REPLACE "@@BUILD_DIR@@" "${CMAKE_BINARY_DIR}" content "${content}")
      	string(REPLACE "@@PROG@@" "t/helper/${script}${EXE_EXTENSION}" content "${content}")
    - 	file(WRITE ${CMAKE_BINARY_DIR}/bin-wrappers/${script} ${content})
    -+	execute_process(COMMAND chmod +x ${CMAKE_BINARY_DIR}/bin-wrappers/${script})
    +-	file(WRITE ${CMAKE_BINARY_DIR}/bin-wrappers/${script} ${content})
    ++	write_script(${CMAKE_BINARY_DIR}/bin-wrappers/${script} "${content}")
      endforeach()
      
      file(STRINGS ${CMAKE_SOURCE_DIR}/wrap-for-bin.sh content NEWLINE_CONSUME)
      string(REPLACE "@@BUILD_DIR@@" "${CMAKE_BINARY_DIR}" content "${content}")
      string(REPLACE "@@PROG@@" "git-cvsserver" content "${content}")
    - file(WRITE ${CMAKE_BINARY_DIR}/bin-wrappers/git-cvsserver ${content})
    -+execute_process(COMMAND chmod +x ${CMAKE_BINARY_DIR}/bin-wrappers/git-cvsserver)
    +-file(WRITE ${CMAKE_BINARY_DIR}/bin-wrappers/git-cvsserver ${content})
    ++write_script(${CMAKE_BINARY_DIR}/bin-wrappers/git-cvsserver "${content}")
      
      #options for configuring test options
      option(PERL_TESTS "Perform tests that use perl" ON)
 5:  5c7b64286ce =  6:  6c254f8cb9f cmake & test-lib.sh: add a $GIT_SOURCE_DIR variable
 6:  cba90650879 =  7:  1685a58e13a cmake: set "USE_LIBPCRE2" in "GIT-BUILD-OPTIONS" for test-lib.sh
 7:  bd7fb1eec24 =  8:  87a2c77ee68 test-lib.sh: support a "GIT_TEST_BUILD_DIR"
 8:  fd3e6be5689 =  9:  6187ded42db Makefile + cmake: use environment, not GIT-BUILD-DIR
 9:  8a660a2baa4 ! 10:  3ea4a61698e cmake: support GIT_TEST_OPTS, abstract away WIN32 defaults
    @@ Commit message
         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.
    +    the set(... CACHE <type> <docstring>) facility. As noted in cmake's
    +    documentation[1] this integrates nicely with e.g. cmake-gui.
     
         On *nix we don't set any custom options. The change in 2ea1d8b5563
         didn't discuss why Windows should have divergent defaults with "cmake"
    @@ Commit message
                 GIT_TEST_OPTS=-i cmake -S contrib/buildsystems -B contrib/buildsystems/out &&
                 ctest --jobs=$(nproc) --test-dir contrib/buildsystems/out -R t0071 --verbose
     
    +    The "separate_arguments()" here will do the right thing for arguments
    +    that don't contain whitespace, so e.g. the path to --root="" can't
    +    have a space in it. There's supposedly a way to work around that with
    +    separate_arguments(), but it requires features newer than our required
    +    cmake version, so let's live with that edge case for now.
    +
         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: empty(default) :
     +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.:
     +
    ++	# "--test-dir" is new in cmake v3.20, so "(cd
    ++	# contrib/buildsystems/out && ctest ...)" on older versions.
     +	ctest --test-dir contrib/buildsystems/out --jobs="$(nproc)"--output-on-failure
     +
     +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:
     +
    -+	GIT_TEST_OPTS="--root=/run/user/$(id -u)/ctest" cmake -S contrib/buildsystems -B contrib/buildsystems/out
    ++	GIT_TEST_OPTS="--root=/dev/shm/$(id -u)/ctest" cmake -S contrib/buildsystems -B contrib/buildsystems/out
     +	[...]
    -+	-- Using user-selected test options: --root=/run/user/1001/ctest
    ++	-- Using user-selected test options: --root=/dev/shm/<uid>/ctest
     +
     +Then running the tests with "ctest" (here with --jobs="$(nproc)"):
     +
    @@ contrib/buildsystems/CMakeLists.txt: 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)
    -+	set(GIT_TEST_OPTS "$ENV{GIT_TEST_OPTS}")
    ++if(DEFINED ENV{GIT_TEST_OPTS})
    ++	set(GIT_TEST_OPTS "$ENV{GIT_TEST_OPTS}"
    ++		CACHE STRING "test options, see t/README")
     +	message(STATUS "Using user-selected test options: ${GIT_TEST_OPTS}")
     +elseif(WIN32)
    -+	set(GIT_TEST_OPTS "--no-bin-wrappers --no-chain-lint -vx")
    ++	set(GIT_TEST_OPTS "--no-bin-wrappers --no-chain-lint -vx"
    ++		CACHE STRING "test options, see t/README")
     +	message(STATUS "Using Windowns-specific default test options: ${GIT_TEST_OPTS}")
     +else()
    -+	set(GIT_TEST_OPTS "")
    ++	set(GIT_TEST_OPTS ""
    ++		CACHE STRING "test options, see t/README")
     +	message(STATUS "No custom test options selected, set e.g. GIT_TEST_OPTS=\"-vixd\"")
     +endif()
     +separate_arguments(GIT_TEST_OPTS)
10:  966fec83b77 = 11:  8ccf5c8c265 cmake: increase test timeout on Windows only
 -:  ----------- > 12:  7b1f10eb4c0 cmake: only look for "sh" in "C:/Program Files" on Windows
11:  aad17d8f858 ! 13:  0699e398e89 cmake: copy over git-p4.py for t983[56] perforce test
    @@ Commit message
         Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
     
      ## contrib/buildsystems/CMakeLists.txt ##
    -@@ contrib/buildsystems/CMakeLists.txt: file(STRINGS ${CMAKE_SOURCE_DIR}/git-p4.py content NEWLINE_CONSUME)
    +@@ contrib/buildsystems/CMakeLists.txt: endforeach()
    + file(STRINGS ${CMAKE_SOURCE_DIR}/git-p4.py content NEWLINE_CONSUME)
      string(REPLACE "#!/usr/bin/env python" "#!/usr/bin/python" content "${content}")
    - file(WRITE ${CMAKE_BINARY_DIR}/git-p4 ${content})
    - execute_process(COMMAND chmod +x ${CMAKE_BINARY_DIR}/git-p4)
    + write_script(${CMAKE_BINARY_DIR}/git-p4 "${content}")
     +file(COPY ${CMAKE_SOURCE_DIR}/git-p4.py DESTINATION ${CMAKE_BINARY_DIR}/)
      
      #perl modules
12:  c27f620dfa3 = 14:  277028678c8 CI: add a "linux-cmake-test" to run cmake & ctest on linux

Rather than the multi-line "mkdir/cd/cmake" recipe provide an equivalent one-liner using the "-S" and "-B" options, and then suggest building with "make -C <build-dir>".

The rest of these instructions discuss e.g. running tests from our top-level "t/" directory, so it's more helpful to avoid changing the user's current directory.

The "-S" and "-B" options were added in cmake v3.13.0, which is older than the version we have a hard dependency on[1].

As an aside The "-p" flag to "mkdir" in the pre-image wasn't needed, as "contrib/buildsystems" is tracked

1. 061c2240b1b (Introduce CMake support for configuring Git, 2020-06-12)

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 contrib/buildsystems/CMakeLists.txt | 8 +++-----
 1 file changed, 3 insertions(+), 5 deletions(-)

Since the cmake file was made to run on *nix in [1] running the tests with "ctest" broken, because we'd attempt to invoke our bin-wrappers/, but they didn't have the executable bit.

In the best case, the "t/test-lib.sh" would be unable to find "bin-wrappers/git", and we'd fall back on "GIT_EXEC_PATH=$GIT_BUILD_DIR" using the fallback behavior added in [2]:

	$ ./t0001-init.sh
	<GIT_BUILD_DIR>/t/../contrib/buildsystems/out/bin-wrappers/git is not executable; using GIT_EXEC_PATH

This was recently somewhat swept under the rug in [3], as ctest would run them with "--no-bin-wrappers". But still with [3], running e.g.:

	cmake -S contrib/buildsystems -B contrib/buildsystems/out -DCMAKE_BUILD_TYPE=Debug &&
	make -C contrib/buildsystems/out &&
	ctest --test-dir contrib/buildsystems/out --jobs="$(nproc)" --output-on-failure

Fails around 20% of our tests on *nix. So even with [3] we'd fail any test that needed to invoke one of our built shell, perl or Python scripts on *nix. E.g. t0012-help.sh would fail on a test that tried to invoke "git web--browse". The equivalent of this (in the "out" directory) would happen:

	$ ./git --exec-path=$PWD web--browse
	git: 'web--browse' is not a git command. See 'git --help'.

Which we can fix by "chmod +x"-ing the built "git-web--browse":

	$ chmod +x git-web--browse
	$ ./git --exec-path=$PWD web--browse
	usage: git web--browse [--browser=browser|--tool=browser] [--config=conf.var] url/file ...

The same goes for e.g. the "git-p4" tests, which would fail because our built "git-p4" wasn't executable, etc. There's also a few other outstanding issues, which will be fixed in subsequent commits.

Ideally we'd use the file(CHMOD ...) form everywhere, but that syntax was introduced in cmake 3.19[4], whereas we only require 3.14. Let's provide a fallback behind a version check, so that we'll eventually be able to delete the "else" part. Both forms result in the same file modes.

Before this change:

	80% tests passed, 196 tests failed out of 977

After:

	99% tests passed, 5 tests failed out of 977

The remaining failures will be addressed in subsequent commits.

1. f31b6244950 (Merge branch 'yw/cmake-updates', 2022-06-07)
2. e4597aae659 (run test suite without dashed git-commands in PATH, 2009-12-02)
3. 2ea1d8b5563 (cmake: make it easier to diagnose regressions in CTest
   runs, 2022-10-18)
4. https://cmake.org/cmake/help/latest/command/file.html#chmod

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>

write script
---
 contrib/buildsystems/CMakeLists.txt | 25 +++++++++++++++++++------
 1 file changed, 19 insertions(+), 6 deletions(-)

When [1] made this copy of "chainlint.sed" the script was invoked in the test-lib.sh as:

	[...] sed -f "$GIT_BUILD_DIR/t/chainlint.sed" [...]

But when [2] replaced "chainlint.sed" with a "chainlint.pl" it also replaced that "$GIT_BUILD_DIR" with "$TEST_DIRECTORY", invoking it as:

	"$PERL_PATH" "$TEST_DIRECTORY/chainlint.pl" "$0"

So this line could have been deleted in [2] but wasn't. Let's do that now.

1. 7f5397a07c6 (cmake: support for testing git when building out of
   the source tree, 2020-06-26)
2. 23a14f30166 (test-lib: replace chainlint.sed with chainlint.pl,
   2022-09-01)

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 contrib/buildsystems/CMakeLists.txt | 1 -
 1 file changed, 1 deletion(-)

In 2f12b31b746 (Makefile: don't invoke msgfmt with --statistics, 2021-12-17) I made the same change to our Makefile, let's follow-up and do the same here.

For "cmake" this is particularly nice with "-G Ninja", as before we'd emit ~40 lines of overflowed progress bar output, but now it's only the one line of "ninja"'s progress bar.

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 contrib/buildsystems/CMakeLists.txt | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

The instructions for running CMake went back & forth between *nix, Windows and Visual Studio instructions Let's create headings and split the existing instructions up into those new sections.

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 contrib/buildsystems/CMakeLists.txt | 68 +++++++++++++++++++++--------
 1 file changed, 50 insertions(+), 18 deletions(-)

When git is built and tested with cmake & ctest we usually do that outside of the top-level source directory. E.g.

	cmake -S contrib/buildsystems -B contrib/buildsystems/out
	make -C contrib/buildsystems/out
	ctest -j$(nproc) --test-dir contrib/buildsystems/out

Since the test-lib.sh makes assumptions about the TEST_DIRECTORY being the "t" subdirectory of the top-level "$GIT_BUILD_DIR", the cmake recipe has needed to copy various assets to that "contrib/buildsystems/out" directory.

But we've only been doing this for the subsets of tests that run on Windows, and which have otherwise been covered by that CI target. The CI target builds (among other things) with "-DPERL_TESTS=OFF -DPYTHON_TESTS=OFF -DCURL_NO_CURL_CMAKE=ON", see [1]. Furthermore, the CI isn't testing from the "contrib/buildsystems/out" directory, instead it clobbers the top-level MAkefile.

There was a recent commit to fix a subset of these issues, see 6a83b5f0810 (cmake: copy the merge tools for testing, 2022-10-18).

Let's stop going for that approach, and instead teach the test-lib.sh that there's such a thing as the "$GIT_SOURCE_DIR" distinct from the "$GIT_BUILD_DIR".

Just as the "$TEST_DIRECTORY" always points to our actual "t" directory (not the "[...]/out/t" cmake creates), this new "$GIT_SOURCE_DIR" will always be the top-level source directory.

With this change we now pass 3/5 of the tests that we still had failing with the fixes in the preceding commit.

1. 4c2c38e800f (ci: modification of main.yml to use cmake for vs-build
   job, 2020-06-26)

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 contrib/buildsystems/CMakeLists.txt |  6 ------
 t/lib-gettext.sh                    |  2 +-
 t/lib-gitweb.sh                     |  2 +-
 t/t7609-mergetool--lib.sh           |  2 +-
 t/t9902-completion.sh               | 14 +++++++-------
 t/t9903-bash-prompt.sh              |  2 +-
 t/test-lib.sh                       |  8 +++++++-
 7 files changed, 18 insertions(+), 18 deletions(-)

When we build with libpcre2 which cmake has supported since [1] we need to set "USE_LIBPCRE2='YesPlease'" (or similar) in "GIT-BUILD-OPTIONS".

Without this e.g. t7810-grep.sh will fail, as it has tests that rely on the behavior of !PCRE2. The reason this hasn't been noticed is that the Windows CI doesn't have access to libpcre2.

With this the remaining two failures we had left after the preceding step are resolved, but note that that test run didn't include the git-p4 tests, which a subsequent commit will address).

1. 80431510a2b (cmake: add pcre2 support, 2022-05-24)

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 contrib/buildsystems/CMakeLists.txt | 3 +++
 1 file changed, 3 insertions(+)

Since [1] two git-p4 tests have relied on there being a "git-p4.py" in the build directory, but the cmake recipe was not updated to account for this. Let's copy the "git-p4.py" over.

We could also change the test to e.g. grab the built "git-p4" and alter its shebang, which would be friendly to GIT_TEST_INSTALLED, but let's just do the bare minimum here to get cmake+ctest working without altering the test itself.

The reason this hasn't been caught by "vs-build" and "vs-test" is because those tests added in [2] invoke "cmake" with "-DPYTHON_TESTS=OFF", and therefore we'd skip this part of the git-p4 tests before getting past the "do we have python?" check. Even if we got past that the Windows CI wouldn't have a "p4" or "p4d" binary installed, so we'd skip the tests anyway.

In a subsequent commit we'll run "cmake" and "ctest" in CI with "ubuntu-latest", so we'll need this "git-p4.py" file.

1. f7b5ff607fa (git-p4: improve encoding handling to support
   inconsistent encodings, 2022-04-30)
2. 4c2c38e800f (ci: modification of main.yml to use cmake for vs-build
   job, 2020-06-26)

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 contrib/buildsystems/CMakeLists.txt | 1 +
 1 file changed, 1 insertion(+)

Change the method used to have building with "cmake" followed by running the tests manually work without creating inter-dependency issues with the Makefile.

Per ee9e66e4e76 (cmake: avoid editing t/test-lib.sh, 2022-10-18) and the surrounding discussion the goal of that change was to have building with cmake and:

	(cd t && ./t0001-init.sh)

Work, to do so we need some way to make the test-lib.sh aware that we have a build in contrib/buildsystems/out. The downside of doing it via the GIT-BUILD-DIR facility in ee9e66e4e76 is:

- A "remove this file while at it" only fits into the Makefile
  dependency graph if we "FORCE" a shell command to be run, so doing it
  this way precludes speeding up the Makefile by e.g. making use of the
  "$(file)" function (for which I've got follow-up patches).

- If you build with make, then with cmake, and then e.g.:

	make -C t prove

  We'll run the tests against the cmake version of the tests, not the
  "make" built ones. It's important to get any subtle interaction
  between two conflicting build systems right.

Now we'll instead:

- Check if the top-level "git" exists, if not we'll look in
  "contrib/buildsystems/out/git", and when running interactively inform
  the user about the fallback being picked.

	$ ./t0001-init.sh
	setup: had no ../git, but found & used cmake built git in ../contrib/buildsystems/out/git
	ok 1 - plain
	[...]

- This also makes things more obvious on the cmake side, because we'll
  now use an environment variable to select the test environment, which
  ctest it notes in its verbose output, e.g.:

	[...]
	2: Test command: /bin/sh "/home/avar/g/git/contrib/buildsystems/../../t/t0001-init.sh" "--no-bin-wrappers" "--no-chain-lint" "-vx"
	2: Working Directory: /home/avar/g/git/contrib/buildsystems/../../t
	2: Environment variables:
	2:  GIT_TEST_BUILD_DIR=/home/avar/g/git/contrib/buildsystems/out
	[...]

   See [1] for the relevant cmake language references.

- We don't support an arbitrary build directory outside of
  "contrib/buildsystems/out". This is OK because the reason for doing
  this part is to support the workflow documented in [2].

  As [3] notes "contrib/buildsystems/out" isn't just the directory that
  happens to be documented in "contrib/buildsystems/CMakeLists.txt", but
  the one that VS will use when building git.

1. https://cmake.org/cmake/help/latest/command/set_property.html
   https://cmake.org/cmake/help/latest/manual/cmake-properties.7.html#test-properties
   https://cmake.org/cmake/help/latest/manual/cmake-language.7.html#cmake-language-lists
1. f2f1250c47f (cmake (Windows): recommend using Visual Studio's
   built-in CMake support, 2020-09-30)
2. 3eccc7b99d4 (cmake: ignore files generated by CMake as run in
   Visual Studio, 2020-09-25)

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 .gitignore                          |  1 -
 Makefile                            |  1 -
 contrib/buildsystems/CMakeLists.txt | 10 ++--------
 t/test-lib.sh                       | 16 ++++++++++++++--
 4 files changed, 16 insertions(+), 12 deletions(-)

Compliment the existing GIT_TEST_INSTALLED variable added in [1] with one that allows for testing a git built in a directory that's not "../".

Instead of "test this installed git" ("GIT_TEST_INSTALLED"), this new "GIT_TEST_BUILD_DIR" is a "test this built directory over there". E.g. this will pass all tests:

	git clone --depth=1 https://github.com/git/git.git git1
	cp -R git1 git2
	make -C git1
	[apply this change to git2]
	(cd git2 && GIT_TEST_BUILD_DIR="$PWD/../git1" make -C t prove)

This facility and file-based instructions to have the test suite use another build directory[2] are mutually exclusive, but in a subsequent commit we'll make CMake use this instead.

1. 6720721e152 (test-lib.sh: Allow running the test suite against
   installed git, 2009-03-16)
2. 350a005e366 (cmake: avoid editing t/test-lib.sh, 2022-10-18)

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 t/README      | 3 +++
 t/test-lib.sh | 5 ++++-
 2 files changed, 7 insertions(+), 1 deletion(-)

Per c858750b41c (cmake: increase time-out for a long-running test, 2022-10-18) the reason to set a custom timeout for t7112-reset-submodule.sh is Windows-specific. Let's only do that on Windows then.

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 contrib/buildsystems/CMakeLists.txt | 8 +++++---
 1 file changed, 5 insertions(+), 3 deletions(-)

Guard the finding of "SH_EXE" in "C:\Program Files" with a check for whether we're on Windows.

This Windows-specific code was first added in [1], and later expanded on [2], but since some of that was added this build recipe has been made portable outside of Windows.

1. 72b6eeb81b1 (cmake: do find Git for Windows' shell interpreter,
   2020-09-28)
2. 476e54b1c60 (cmake: support local installations of git,
   2022-07-27).

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 contrib/buildsystems/CMakeLists.txt | 15 +++++++++++----
 1 file changed, 11 insertions(+), 4 deletions(-)

Since [1] the "cmake" build method should work properly on Linux, but as seen in preceding commits there were various bugs in it, which are hopefully now all fixed.

To ensure that it doesn't break again let's add a "linux-cmake-ctest" target to build and test "cmake" on Linux, in addition to that we'll also run the tests with "ctest" instead of "make" or "prove", so we can assert that testing with that method works..

This also stress tests running "cmake" (and "ctest") out of a build directory that isn't the top-level. The "vs-build" job uses "cmake" since [2], but clobbers the top-level "Makefile" and builds in the top-level directory.

That was the reason for why we didn't spot that various tests still required missing "mergetools/*" etc, which was fixed by using "$GIT_SOURCE_DIR" in a preceding commit.

Since the "ci/lib.sh" already creates and exports a GIT_TEST_OPTS="..." we'll pick that up in our CI, see the preceding commit. Because we pass the "--verbose-log -x --github-workflow-markup" in the GitHub CI as a result the interaction with "handle_failed_tests" here works correctly. I.e. on failure we'll have saved "t/test-results/*.{exit,out,markup}" files relevant to the failing test(s).

1. f31b6244950 (Merge branch 'yw/cmake-updates', 2022-06-07)
2. 4c2c38e800f (ci: modification of main.yml to use cmake for vs-build
   job, 2020-06-26)

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 .github/workflows/main.yml |  3 +++
 ci/run-build-and-tests.sh  | 13 +++++++++++--
 2 files changed, 14 insertions(+), 2 deletions(-)

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(... CACHE <type> <docstring>) facility. As noted in cmake's documentation[1] this integrates nicely with e.g. cmake-gui.

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

The "separate_arguments()" here will do the right thing for arguments that don't contain whitespace, so e.g. the path to --root="" can't have a space in it. There's supposedly a way to work around that with separate_arguments(), but it requires features newer than our required cmake version, so let's live with that edge case for now.

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 | 50 +++++++++++++++++++++++++++--
 1 file changed, 48 insertions(+), 2 deletions(-)

On Thu, Nov 03, 2022 at 05:37:50PM +0100, Ævar Arnfjörð Bjarmason wrote:

Thanks, will queue.

Thanks, Taylor

On 03/11/2022 16:37, Ævar Arnfjörð Bjarmason wrote:

Thanks, looking the range-diff there seems to be an overlong line in the commit message.

I'm not really seeing that. I suggested putting the windows build instructions at the top and inlining the -DCMAKE_BUILD_TYPE discussion but looking at the range diff there are still forward references in the text.

I'm glad that works for you now

Kudos for the version test

The range-diff for patch 5 shows a reference to "ctest --test-dir" which is not supported by cmake 3.14 but I see that in patch 10 you provide alternative instructions for older versions. I would be simpler just to provide a single recipe that works everywhere. I think you only need "ctest --test-dir" because you changed the build instructions to use "cmake -B"

Best Wishes

Phillip

This topic gets our tests from passing ~80% with ctest on *nix to passing 100%.

See passing CI job for "cmake + ctest" on Ubuntu at: https://github.com/avar/git/actions/runs/3598398023

See https://lore.kernel.org/git/cover-v4-00.14-00000000000-20221103T160255Z-avarab@gmail.com/ for the v4.

The v4 of this is in "next", but expected to be ejected & re-queued per [1].

Changes since v4.

* Rebased on master. Junio: I think the merge you had for main.yml was
  a mismerge, i.e. the new job here wanted "ubuntu-latest". Per
  0178420b9ca (github-actions: run gcc-8 on ubuntu-20.04 image,
  2022-11-25) only the gcc-8 jobs want to pin the version.

  In practice that merge resolution was harmless, as the job ran just
  fine on ubuntu-20.04.

* Per the discussion starting at [2] this v5 doesn't remove the
  GIT-BUILD-DIR file, but if we have it and build directory built
  with "make" we'll prefer the "make" one.

  This supports the use-case of running the tests discovering the
  "cmake" build directory using the same mechanism as before, while
  fixing the edge cases where the two will trip over one
  another. E.g. a "make git" after a "cmake" still running the tests
  with the "cmake".

* There are other behavior differences to fix what I consider bugs in
  the existing behavior, but that new behavior is guarded by a
  "GIT_CTEST_SETS_BUILD_DIR" variable. On Windows it's "ON" by default
  (the old behavior), elsewhere it's "OFF".

  As the test suite wasn't close to running succesfully before this
  the non-Windows users should benefit from the new behavior, but it
  is different. So Windows users get old semantics, unless they'd like
  to opt-in.

  The resulting behavior differences are the following (table taken
  from 10/15):

	|---+----+-----|
	|   | ON | OFF |
	|---+----+-----|
	| A | N  | Y   |
	| B | N  | Y   |
	| C | Y  | Y   |
	| D | Y  | N   |
	| E | N  | Y   |
	|---+----+-----|
	A. Can manually run tests before ctest?
	B. Manually run tests point to latest cmake build?...
	C. Manually run tests point to latest "ctest"'d build?
        D. "git" picked at ctest start time?
        E. "git" picked at test start time?

1. https://lore.kernel.org/git/xmqq5yeuspam.fsf@gitster.g/
2. https://lore.kernel.org/git/544fff8a-7d56-57a1-00a3-d1a9302e227c@dunelm.org.uk/

Branch & passing CI at https://github.com/avar/git/tree/avar/cmake-test-path-5

Ævar Arnfjörð Bjarmason (15):
  cmake: don't invoke msgfmt with --statistics
  cmake: use "-S" and "-B" to specify source and build directories
  cmake: update instructions for portable CMakeLists.txt
  cmake: don't copy chainlint.pl to build directory
  cmake: chmod +x the bin-wrappers/* & SCRIPT_{SH,PERL} & git-p4
  cmake & test-lib.sh: add a $GIT_SOURCE_DIR variable
  cmake: set "USE_LIBPCRE2" in "GIT-BUILD-OPTIONS" for test-lib.sh
  Makefile + test-lib.sh: don't prefer cmake-built to make-built git
  test-lib.sh: support a "GIT_TEST_BUILD_DIR"
  cmake: optionally be able to run tests before "ctest"
  cmake: support GIT_TEST_OPTS, abstract away WIN32 defaults
  cmake: increase test timeout on Windows only
  cmake: only look for "sh" in "C:/Program Files" on Windows
  cmake: copy over git-p4.py for t983[56] perforce test
  CI: add a "linux-cmake-test" to run cmake & ctest on linux

 .github/workflows/main.yml          |   3 +
 Makefile                            |   1 -
 ci/run-build-and-tests.sh           |  13 +-
 contrib/buildsystems/CMakeLists.txt | 211 ++++++++++++++++++++++------
 t/README                            |   3 +
 t/lib-gettext.sh                    |   2 +-
 t/lib-gitweb.sh                     |   2 +-
 t/t7609-mergetool--lib.sh           |   2 +-
 t/t9902-completion.sh               |  14 +-
 t/t9903-bash-prompt.sh              |   2 +-
 t/test-lib.sh                       |  19 ++-
 11 files changed, 211 insertions(+), 61 deletions(-)

Range-diff against v4:
 1:  78dfc2a69b7 =  1:  7d83ff44c61 cmake: don't invoke msgfmt with --statistics
 2:  dd934b0597d =  2:  314c5e40cff cmake: use "-S" and "-B" to specify source and build directories
 3:  6f0eae7a02a =  3:  5c4ba2425b6 cmake: update instructions for portable CMakeLists.txt
 4:  0f0eb2a76c7 =  4:  e86245ffbab cmake: don't copy chainlint.pl to build directory
 5:  eda1c1e95e5 =  5:  435452314fa cmake: chmod +x the bin-wrappers/* & SCRIPT_{SH,PERL} & git-p4
 6:  6c254f8cb9f =  6:  7ee341245a8 cmake & test-lib.sh: add a $GIT_SOURCE_DIR variable
 7:  1685a58e13a =  7:  49645cb1643 cmake: set "USE_LIBPCRE2" in "GIT-BUILD-OPTIONS" for test-lib.sh
 -:  ----------- >  8:  9e10cd5bd8a Makefile + test-lib.sh: don't prefer cmake-built to make-built git
 8:  87a2c77ee68 !  9:  df5aea35bbd test-lib.sh: support a "GIT_TEST_BUILD_DIR"
    @@ Commit message
                 (cd git2 && GIT_TEST_BUILD_DIR="$PWD/../git1" make -C t prove)
     
         This facility and file-based instructions to have the test suite use
    -    another build directory[2] are mutually exclusive, but in a subsequent
    -    commit we'll make CMake use this instead.
    +    another build directory[2] are mutually exclusive. In a subsequent
    +    commit we'll make CMake and CTest optionally prefer this method.
     
         1. 6720721e152 (test-lib.sh: Allow running the test suite against
            installed git, 2009-03-16)
    @@ t/README: override the location of the dashed-form subcommands (what
      --------------
     
      ## t/test-lib.sh ##
    -@@ t/test-lib.sh: fi
    - # For CMake the top-level source directory is different from our build
    - # directory. With the top-level Makefile they're the same.
    - GIT_SOURCE_DIR="$GIT_BUILD_DIR"
    --if test -f "$GIT_BUILD_DIR/GIT-BUILD-DIR"
    -+if test -n "$GIT_TEST_BUILD_DIR"
    -+then
    -+	GIT_BUILD_DIR="$GIT_TEST_BUILD_DIR"
    -+elif test -f "$GIT_BUILD_DIR/GIT-BUILD-DIR"
    +@@ t/test-lib.sh: if test ! -x "$GIT_BUILD_DIR/git" &&
    + 	test -f "$GIT_BUILD_DIR/GIT-BUILD-DIR"
      then
      	GIT_BUILD_DIR="$(cat "$GIT_BUILD_DIR/GIT-BUILD-DIR")" || exit 1
    ++elif test -n "$GIT_TEST_BUILD_DIR"
    ++then
    ++	GIT_BUILD_DIR="$GIT_TEST_BUILD_DIR"
    ++fi
    ++
    ++if test "$GIT_SOURCE_DIR" != "$GIT_BUILD_DIR"
    ++then
      	# On Windows, we must convert Windows paths lest they contain a colon
    + 	case "$(uname -s)" in
    + 	*MINGW*)
 9:  6187ded42db ! 10:  529e18f7f72 Makefile + cmake: use environment, not GIT-BUILD-DIR
    @@ Metadata
     Author: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
     
      ## Commit message ##
    -    Makefile + cmake: use environment, not GIT-BUILD-DIR
    +    cmake: optionally be able to run tests before "ctest"
     
    -    Change the method used to have building with "cmake" followed by
    -    running the tests manually work without creating inter-dependency issues with the Makefile.
    +    Since [1] the "cmake" build has had support for running the tests
    +    manually from the "t/" directory once we've built with "cmake", but
    +    only after a one-off run of "ctest".
     
    -    Per ee9e66e4e76 (cmake: avoid editing t/test-lib.sh, 2022-10-18) and
    -    the surrounding discussion the goal of that change was to have
    -    building with cmake and:
    +    I.e. we create the build recipe via "cmake" and build with
    +    e.g. "make", but after doing so would get:
     
    -            (cd t && ./t0001-init.sh)
    +            $ (cd t && ./t0071-sort.sh)
    +            error: GIT-BUILD-OPTIONS missing (has Git been built?).
     
    -    Work, to do so we need some way to make the test-lib.sh aware that we
    -    have a build in contrib/buildsystems/out. The downside of doing it via
    -    the GIT-BUILD-DIR facility in ee9e66e4e76 is:
    +    The recently added amendment to this of using a "GIT-BUILD-DIR"[2]
    +    didn't change these semantics. After we did a one-off run of "ctest"
    +    we'd be able to run the tests without "ctest":
     
    -    - A "remove this file while at it" only fits into the Makefile
    -      dependency graph if we "FORCE" a shell command to be run, so doing it
    -      this way precludes speeding up the Makefile by e.g. making use of the
    -      "$(file)" function (for which I've got follow-up patches).
    -
    -    - If you build with make, then with cmake, and then e.g.:
    -
    -            make -C t prove
    -
    -      We'll run the tests against the cmake version of the tests, not the
    -      "make" built ones. It's important to get any subtle interaction
    -      between two conflicting build systems right.
    -
    -    Now we'll instead:
    -
    -    - Check if the top-level "git" exists, if not we'll look in
    -      "contrib/buildsystems/out/git", and when running interactively inform
    -      the user about the fallback being picked.
    -
    -            $ ./t0001-init.sh
    -            setup: had no ../git, but found & used cmake built git in ../contrib/buildsystems/out/git
    -            ok 1 - plain
    +            $ ctest --test-dir contrib/buildsystems/out -R t0071
                 [...]
    -
    -    - This also makes things more obvious on the cmake side, because we'll
    -      now use an environment variable to select the test environment, which
    -      ctest it notes in its verbose output, e.g.:
    -
    -            [...]
    -            2: Test command: /bin/sh "/home/avar/g/git/contrib/buildsystems/../../t/t0001-init.sh" "--no-bin-wrappers" "--no-chain-lint" "-vx"
    -            2: Working Directory: /home/avar/g/git/contrib/buildsystems/../../t
    -            2: Environment variables:
    -            2:  GIT_TEST_BUILD_DIR=/home/avar/g/git/contrib/buildsystems/out
    +            100% tests passed, 0 tests failed out of 1
                 [...]
    -
    -       See [1] for the relevant cmake language references.
    -
    -    - We don't support an arbitrary build directory outside of
    -      "contrib/buildsystems/out". This is OK because the reason for doing
    -      this part is to support the workflow documented in [2].
    -
    -      As [3] notes "contrib/buildsystems/out" isn't just the directory that
    -      happens to be documented in "contrib/buildsystems/CMakeLists.txt", but
    -      the one that VS will use when building git.
    -
    -    1. https://cmake.org/cmake/help/latest/command/set_property.html
    -       https://cmake.org/cmake/help/latest/manual/cmake-properties.7.html#test-properties
    -       https://cmake.org/cmake/help/latest/manual/cmake-language.7.html#cmake-language-lists
    -    1. f2f1250c47f (cmake (Windows): recommend using Visual Studio's
    -       built-in CMake support, 2020-09-30)
    -    2. 3eccc7b99d4 (cmake: ignore files generated by CMake as run in
    -       Visual Studio, 2020-09-25)
    +            $ (cd t && ./t0071-sort.sh)
    +            ok 1 - DEFINE_LIST_SORT_DEBUG
    +            # passed all 1 test(s)
    +            1..1
    +
    +    This change optionally closes that gap, and allows for more sensible
    +    behavior. Due to concerns about existing MS Visual Studio users
    +    relying on some of these edge cases this is being made optional, with
    +    the default of the new "GIT_CTEST_SETS_BUILD_DIR" being "ON" on
    +    Windows (i.e. the old behavior), and "OFF" elsewhere.
    +
    +    The resulting behavior differences are the following:
    +
    +            |---+----+-----|
    +            |   | ON | OFF |
    +            |---+----+-----|
    +            | A | N  | Y   |
    +            | B | N  | Y   |
    +            | C | Y  | Y   |
    +            | D | Y  | N   |
    +            | E | N  | Y   |
    +            |---+----+-----|
    +            A. Can manually run tests before ctest?
    +            B. Manually run tests point to latest cmake build?...
    +            C. Manually run tests point to latest "ctest"'d build?
    +            D. "git" picked at ctest start time?
    +            E. "git" picked at test start time?
    +
    +    On "D" and "E": Because GIT_CTEST_SETS_BUILD_DIR=ON relies on the
    +    global "GIT-BUILD-DIR" when using "ctest" it cannot be used to run
    +    concurrent tests for two different builds from the same source
    +    directory.
    +
    +    But as noted in [3] existing users using Visual Studio may be relying
    +    on some of these edge cases, so let's make this optional on Windows,
    +    but switch the default to this new discover method on other platforms.
    +
    +    1. 7f5397a07c6 (cmake: support for testing git when building out of
    +       the source tree, 2020-06-26)
    +    2. ee9e66e4e76 (cmake: avoid editing t/test-lib.sh, 2022-10-18)
    +    3. https://lore.kernel.org/git/663b93ef-0c89-a5f6-1069-b4be97915d20@dunelm.org.uk/
     
         Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
     
    - ## .gitignore ##
    -@@
    - /fuzz_corpora
    --/GIT-BUILD-DIR
    - /GIT-BUILD-OPTIONS
    - /GIT-CFLAGS
    - /GIT-LDFLAGS
    -
    - ## Makefile ##
    -@@ Makefile: else
    - 	@echo RUNTIME_PREFIX=\'false\' >>$@+
    - endif
    - 	@if cmp $@+ $@ >/dev/null 2>&1; then $(RM) $@+; else mv $@+ $@; fi
    --	@if test -f GIT-BUILD-DIR; then rm GIT-BUILD-DIR; fi
    - 
    - ### Detect Python interpreter path changes
    - ifndef NO_PYTHON
    -
      ## contrib/buildsystems/CMakeLists.txt ##
     @@ contrib/buildsystems/CMakeLists.txt: if(USE_VCPKG)
      	file(APPEND ${CMAKE_BINARY_DIR}/GIT-BUILD-OPTIONS "PATH=\"$PATH:$TEST_DIRECTORY/../compat/vcbuild/vcpkg/installed/x64-windows/bin\"\n")
    @@ contrib/buildsystems/CMakeLists.txt: if(USE_VCPKG)
     -	#Setting the build directory in test-lib.sh before running tests
     -	file(WRITE ${CMAKE_BINARY_DIR}/CTestCustom.cmake
     -		"file(WRITE ${CMAKE_SOURCE_DIR}/GIT-BUILD-DIR \"${CMAKE_BINARY_DIR}\")")
    --endif()
    --
    - file(GLOB test_scipts "${CMAKE_SOURCE_DIR}/t/t[0-9]*.sh")
    ++set(GIT_CTEST_SETS_BUILD_DIR_DOC "find cmake build dir via ctest-set GIT-BUILD-DIR?")
    ++if(DEFINED ENV{GIT_CTEST_SETS_BUILD_DIR})
    ++	set(GIT_CTEST_SETS_BUILD_DIR "$ENV{GIT_CTEST_SETS_BUILD_DIR}"
    ++		CACHE BOOL GIT_CTEST_SETS_BUILD_DIR_DOC)
    ++elseif(WIN32)
    ++	set(GIT_CTEST_SETS_BUILD_DIR "ON" CACHE BOOL GIT_CTEST_SETS_BUILD_DIR_DOC)
    ++else()
    ++	set(GIT_CTEST_SETS_BUILD_DIR "OFF" CACHE BOOL GIT_CTEST_SETS_BUILD_DIR_DOC)
    ++endif()
    ++if(GIT_CTEST_SETS_BUILD_DIR)
    ++	message(STATUS "Enabling manual runs of t/* tests once ctest writes CMAKE-BUILD-DIR")
    ++else()
    ++	message(STATUS "Enabling manual runs of t/* tests via cmake build dir discovery")
    ++endif()
    ++
    ++# When using GIT_CTEST_SETS_BUILD_DIR, running the tests from the "t/"
    ++# directory will only work once "ctest" has been run, as we write to
    ++# "GIT-BUILD-DIR" from the optional "ctest" invocation following the
    ++# "cmake"
    ++if(GIT_CTEST_SETS_BUILD_DIR)
    ++	get_filename_component(CACHE_PATH ${CMAKE_CURRENT_LIST_DIR}/../../CMakeCache.txt ABSOLUTE)
    ++	if(NOT ${CMAKE_BINARY_DIR}/CMakeCache.txt STREQUAL ${CACHE_PATH})
    ++		file(WRITE ${CMAKE_BINARY_DIR}/CTestCustom.cmake
    ++			"file(WRITE ${CMAKE_SOURCE_DIR}/GIT-BUILD-DIR \"${CMAKE_BINARY_DIR}\")")
    ++	endif()
    ++else()
    ++	file(WRITE "${CMAKE_SOURCE_DIR}/GIT-BUILD-DIR" "${CMAKE_BINARY_DIR}")
    + endif()
      
    - #test
    + file(GLOB test_scipts "${CMAKE_SOURCE_DIR}/t/t[0-9]*.sh")
     @@ contrib/buildsystems/CMakeLists.txt: foreach(tsh ${test_scipts})
      	add_test(NAME ${tsh}
      		COMMAND ${SH_EXE} ${tsh} --no-bin-wrappers --no-chain-lint -vx
      		WORKING_DIRECTORY ${CMAKE_SOURCE_DIR}/t)
    ++if(NOT GIT_CTEST_SETS_BUILD_DIR)
     +	set_property(TEST ${tsh} APPEND PROPERTY ENVIRONMENT
    ++		GIT_TEST_PREFER_BUILD_DIR_ENV="Yes, ignore GIT-BUILD-DIR"
     +		GIT_TEST_BUILD_DIR=${CMAKE_BINARY_DIR})
    ++endif()
      endforeach()
      
      # This test script takes an extremely long time and is known to time out even
    @@ t/test-lib.sh: fi
      # For CMake the top-level source directory is different from our build
      # directory. With the top-level Makefile they're the same.
      GIT_SOURCE_DIR="$GIT_BUILD_DIR"
    -+
    -+GIT_AUTO_CONTRIB_BUILDSYSTEMS_OUT=
    - if test -n "$GIT_TEST_BUILD_DIR"
    - then
    - 	GIT_BUILD_DIR="$GIT_TEST_BUILD_DIR"
    --elif test -f "$GIT_BUILD_DIR/GIT-BUILD-DIR"
    -+elif ! test -x "$GIT_BUILD_DIR/git" &&
    -+     test -x "$GIT_BUILD_DIR/contrib/buildsystems/out/git"
    - then
    --	GIT_BUILD_DIR="$(cat "$GIT_BUILD_DIR/GIT-BUILD-DIR")" || exit 1
    -+	GIT_BUILD_DIR="$GIT_SOURCE_DIR/contrib/buildsystems/out"
    -+	GIT_AUTO_CONTRIB_BUILDSYSTEMS_OUT=t
    -+
    - 	# On Windows, we must convert Windows paths lest they contain a colon
    - 	case "$(uname -s)" in
    - 	*MINGW*)
    -@@ t/test-lib.sh: remove_trash_directory "$TRASH_DIRECTORY" || {
    - 	BAIL_OUT 'cannot prepare test area'
    - }
    - 
    -+# Emitting this now because earlier we didn't have "say", but not in
    -+# 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"
    -+fi
    -+
    - remove_trash=t
    - if test -z "$TEST_NO_CREATE_REPO"
    +-if test ! -x "$GIT_BUILD_DIR/git" &&
    ++if test -z "$GIT_TEST_PREFER_BUILD_DIR_ENV" &&
    ++	test ! -x "$GIT_BUILD_DIR/git" &&
    + 	test -f "$GIT_BUILD_DIR/GIT-BUILD-DIR"
      then
    + 	GIT_BUILD_DIR="$(cat "$GIT_BUILD_DIR/GIT-BUILD-DIR")" || exit 1
10:  3ea4a61698e ! 11:  a5dd4fa1d00 cmake: support GIT_TEST_OPTS, abstract away WIN32 defaults
    @@ contrib/buildsystems/CMakeLists.txt: endif()
     -		COMMAND ${SH_EXE} ${tsh} --no-bin-wrappers --no-chain-lint -vx
     +		COMMAND ${SH_EXE} ${tsh} ${GIT_TEST_OPTS}
      		WORKING_DIRECTORY ${CMAKE_SOURCE_DIR}/t)
    + if(NOT GIT_CTEST_SETS_BUILD_DIR)
      	set_property(TEST ${tsh} APPEND PROPERTY ENVIRONMENT
    - 		GIT_TEST_BUILD_DIR=${CMAKE_BINARY_DIR})
11:  8ccf5c8c265 ! 12:  c9d550b8558 cmake: increase test timeout on Windows only
    @@ Commit message
         Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
     
      ## contrib/buildsystems/CMakeLists.txt ##
    -@@ contrib/buildsystems/CMakeLists.txt: foreach(tsh ${test_scipts})
    - 		GIT_TEST_BUILD_DIR=${CMAKE_BINARY_DIR})
    +@@ contrib/buildsystems/CMakeLists.txt: if(NOT GIT_CTEST_SETS_BUILD_DIR)
    + endif()
      endforeach()
      
     -# This test script takes an extremely long time and is known to time out even
12:  7b1f10eb4c0 = 13:  7a5dd684d40 cmake: only look for "sh" in "C:/Program Files" on Windows
13:  0699e398e89 = 14:  c9c895bb794 cmake: copy over git-p4.py for t983[56] perforce test
14:  277028678c8 ! 15:  a6f343b6221 CI: add a "linux-cmake-test" to run cmake & ctest on linux
    @@ Commit message
     
      ## .github/workflows/main.yml ##
     @@ .github/workflows/main.yml: jobs:
    -             os: ubuntu
    +             cc: gcc
                  cc_package: gcc-8
    -             pool: ubuntu-latest
    +             pool: ubuntu-20.04
     +          - jobname: linux-cmake-ctest
     +            cc: gcc
     +            pool: ubuntu-latest

In 2f12b31b746 (Makefile: don't invoke msgfmt with --statistics, 2021-12-17) I made the same change to our Makefile, let's follow-up and do the same here.

For "cmake" this is particularly nice with "-G Ninja", as before we'd emit ~40 lines of overflowed progress bar output, but now it's only the one line of "ninja"'s progress bar.

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 contrib/buildsystems/CMakeLists.txt | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

The instructions for running CMake went back & forth between *nix, Windows and Visual Studio instructions Let's create headings and split the existing instructions up into those new sections.

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 contrib/buildsystems/CMakeLists.txt | 68 +++++++++++++++++++++--------
 1 file changed, 50 insertions(+), 18 deletions(-)

Rather than the multi-line "mkdir/cd/cmake" recipe provide an equivalent one-liner using the "-S" and "-B" options, and then suggest building with "make -C <build-dir>".

The rest of these instructions discuss e.g. running tests from our top-level "t/" directory, so it's more helpful to avoid changing the user's current directory.

The "-S" and "-B" options were added in cmake v3.13.0, which is older than the version we have a hard dependency on[1].

As an aside The "-p" flag to "mkdir" in the pre-image wasn't needed, as "contrib/buildsystems" is tracked

1. 061c2240b1b (Introduce CMake support for configuring Git, 2020-06-12)

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 contrib/buildsystems/CMakeLists.txt | 8 +++-----
 1 file changed, 3 insertions(+), 5 deletions(-)

On Fri, Dec 2, 2022 at 6:28 AM Ævar Arnfjörð Bjarmason <avarab@gmail.com> wrote:

s/aside The/aside, the/

When [1] made this copy of "chainlint.sed" the script was invoked in the test-lib.sh as:

	[...] sed -f "$GIT_BUILD_DIR/t/chainlint.sed" [...]

But when [2] replaced "chainlint.sed" with a "chainlint.pl" it also replaced that "$GIT_BUILD_DIR" with "$TEST_DIRECTORY", invoking it as:

	"$PERL_PATH" "$TEST_DIRECTORY/chainlint.pl" "$0"

So this line could have been deleted in [2] but wasn't. Let's do that now.

1. 7f5397a07c6 (cmake: support for testing git when building out of
   the source tree, 2020-06-26)
2. 23a14f30166 (test-lib: replace chainlint.sed with chainlint.pl,
   2022-09-01)

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 contrib/buildsystems/CMakeLists.txt | 1 -
 1 file changed, 1 deletion(-)

Since the cmake file was made to run on *nix in [1] running the tests with "ctest" broken, because we'd attempt to invoke our bin-wrappers/, but they didn't have the executable bit.

In the best case, the "t/test-lib.sh" would be unable to find "bin-wrappers/git", and we'd fall back on "GIT_EXEC_PATH=$GIT_BUILD_DIR" using the fallback behavior added in [2]:

	$ ./t0001-init.sh
	<GIT_BUILD_DIR>/t/../contrib/buildsystems/out/bin-wrappers/git is not executable; using GIT_EXEC_PATH

This was recently somewhat swept under the rug in [3], as ctest would run them with "--no-bin-wrappers". But still with [3], running e.g.:

	cmake -S contrib/buildsystems -B contrib/buildsystems/out -DCMAKE_BUILD_TYPE=Debug &&
	make -C contrib/buildsystems/out &&
	ctest --test-dir contrib/buildsystems/out --jobs="$(nproc)" --output-on-failure

Fails around 20% of our tests on *nix. So even with [3] we'd fail any test that needed to invoke one of our built shell, perl or Python scripts on *nix. E.g. t0012-help.sh would fail on a test that tried to invoke "git web--browse". The equivalent of this (in the "out" directory) would happen:

	$ ./git --exec-path=$PWD web--browse
	git: 'web--browse' is not a git command. See 'git --help'.

Which we can fix by "chmod +x"-ing the built "git-web--browse":

	$ chmod +x git-web--browse
	$ ./git --exec-path=$PWD web--browse
	usage: git web--browse [--browser=browser|--tool=browser] [--config=conf.var] url/file ...

The same goes for e.g. the "git-p4" tests, which would fail because our built "git-p4" wasn't executable, etc. There's also a few other outstanding issues, which will be fixed in subsequent commits.

Ideally we'd use the file(CHMOD ...) form everywhere, but that syntax was introduced in cmake 3.19[4], whereas we only require 3.14. Let's provide a fallback behind a version check, so that we'll eventually be able to delete the "else" part. Both forms result in the same file modes.

Before this change:

	80% tests passed, 196 tests failed out of 977

After:

	99% tests passed, 5 tests failed out of 977

The remaining failures will be addressed in subsequent commits.

1. f31b6244950 (Merge branch 'yw/cmake-updates', 2022-06-07)
2. e4597aae659 (run test suite without dashed git-commands in PATH, 2009-12-02)
3. 2ea1d8b5563 (cmake: make it easier to diagnose regressions in CTest
   runs, 2022-10-18)
4. https://cmake.org/cmake/help/latest/command/file.html#chmod

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>

write script
---
 contrib/buildsystems/CMakeLists.txt | 25 +++++++++++++++++++------
 1 file changed, 19 insertions(+), 6 deletions(-)

When we build with libpcre2 which cmake has supported since [1] we need to set "USE_LIBPCRE2='YesPlease'" (or similar) in "GIT-BUILD-OPTIONS".

Without this e.g. t7810-grep.sh will fail, as it has tests that rely on the behavior of !PCRE2. The reason this hasn't been noticed is that the Windows CI doesn't have access to libpcre2.

With this the remaining two failures we had left after the preceding step are resolved, but note that that test run didn't include the git-p4 tests, which a subsequent commit will address).

1. 80431510a2b (cmake: add pcre2 support, 2022-05-24)

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 contrib/buildsystems/CMakeLists.txt | 3 +++
 1 file changed, 3 insertions(+)

When git is built and tested with cmake & ctest we usually do that outside of the top-level source directory. E.g.

	cmake -S contrib/buildsystems -B contrib/buildsystems/out
	make -C contrib/buildsystems/out
	ctest -j$(nproc) --test-dir contrib/buildsystems/out

Since the test-lib.sh makes assumptions about the TEST_DIRECTORY being the "t" subdirectory of the top-level "$GIT_BUILD_DIR", the cmake recipe has needed to copy various assets to that "contrib/buildsystems/out" directory.

But we've only been doing this for the subsets of tests that run on Windows, and which have otherwise been covered by that CI target. The CI target builds (among other things) with "-DPERL_TESTS=OFF -DPYTHON_TESTS=OFF -DCURL_NO_CURL_CMAKE=ON", see [1]. Furthermore, the CI isn't testing from the "contrib/buildsystems/out" directory, instead it clobbers the top-level MAkefile.

There was a recent commit to fix a subset of these issues, see 6a83b5f0810 (cmake: copy the merge tools for testing, 2022-10-18).

Let's stop going for that approach, and instead teach the test-lib.sh that there's such a thing as the "$GIT_SOURCE_DIR" distinct from the "$GIT_BUILD_DIR".

Just as the "$TEST_DIRECTORY" always points to our actual "t" directory (not the "[...]/out/t" cmake creates), this new "$GIT_SOURCE_DIR" will always be the top-level source directory.

With this change we now pass 3/5 of the tests that we still had failing with the fixes in the preceding commit.

1. 4c2c38e800f (ci: modification of main.yml to use cmake for vs-build
   job, 2020-06-26)

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 contrib/buildsystems/CMakeLists.txt |  6 ------
 t/lib-gettext.sh                    |  2 +-
 t/lib-gitweb.sh                     |  2 +-
 t/t7609-mergetool--lib.sh           |  2 +-
 t/t9902-completion.sh               | 14 +++++++-------
 t/t9903-bash-prompt.sh              |  2 +-
 t/test-lib.sh                       |  8 +++++++-
 7 files changed, 18 insertions(+), 18 deletions(-)

Per ee9e66e4e76 (cmake: avoid editing t/test-lib.sh, 2022-10-18) and the surrounding discussion the goal of that change was to have building with cmake and:

	(cd t && ./t0001-init.sh)

Work, to do so we need some way to make the test-lib.sh aware that we have a build in contrib/buildsystems/out. The downside of doing it via the GIT-BUILD-DIR facility in ee9e66e4e76 is:

- A "remove this file while at it" only fits into the Makefile
  dependency graph if we "FORCE" a shell command to be run, so doing it
  this way precludes speeding up the Makefile by e.g. making use of the
  "$(file)" function (for which I've got follow-up patches).

- If you build with make, then with cmake, and then e.g.:

	make -C t prove

  We'll run the tests against the cmake version of the tests, not the
  "make" built ones. It's important to get any subtle interaction
  between two conflicting build systems right.

Now we'll instead check if the top-level "git" exists, and only if it doesn't look for the one built by cmake. This ensures that we'll pick it up as required when the user is using CMake, without being subject to the odd interaction of flip-flopping between the two environments.

It also means that we can remove the rule to remove the "GIT-BUILD-DIR", if we get to creating "GIT-BUILD-OPTIONS" in the top-level git. That's because now when building with both "make" and "cmake" we will always prefer the former for running the tests, so there's no need to remove the "GIT-BUILD-DIR" file to resolve the conflicts between the two.

In practice users are building with either "make" or "cmake", and not mixing the two, but in a subsequent change we'll provide an out for that "make"-preferring selection behavior, by having "ctest" runs always prefer the "cmake" built one.

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 Makefile      | 1 -
 t/test-lib.sh | 3 ++-
 2 files changed, 2 insertions(+), 2 deletions(-)

Compliment the existing GIT_TEST_INSTALLED variable added in [1] with one that allows for testing a git built in a directory that's not "../".

Instead of "test this installed git" ("GIT_TEST_INSTALLED"), this new "GIT_TEST_BUILD_DIR" is a "test this built directory over there". E.g. this will pass all tests:

	git clone --depth=1 https://github.com/git/git.git git1
	cp -R git1 git2
	make -C git1
	[apply this change to git2]
	(cd git2 && GIT_TEST_BUILD_DIR="$PWD/../git1" make -C t prove)

This facility and file-based instructions to have the test suite use another build directory[2] are mutually exclusive. In a subsequent commit we'll make CMake and CTest optionally prefer this method.

1. 6720721e152 (test-lib.sh: Allow running the test suite against
   installed git, 2009-03-16)
2. 350a005e366 (cmake: avoid editing t/test-lib.sh, 2022-10-18)

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 t/README      | 3 +++
 t/test-lib.sh | 7 +++++++
 2 files changed, 10 insertions(+)

On Fri, Dec 2, 2022 at 6:28 AM Ævar Arnfjörð Bjarmason <avarab@gmail.com> wrote:

s/Compliment/Complement/

Since [1] the "cmake" build has had support for running the tests manually from the "t/" directory once we've built with "cmake", but only after a one-off run of "ctest".

I.e. we create the build recipe via "cmake" and build with e.g. "make", but after doing so would get:

	$ (cd t && ./t0071-sort.sh)
	error: GIT-BUILD-OPTIONS missing (has Git been built?).

The recently added amendment to this of using a "GIT-BUILD-DIR"[2] didn't change these semantics. After we did a one-off run of "ctest" we'd be able to run the tests without "ctest":

	$ ctest --test-dir contrib/buildsystems/out -R t0071
	[...]
	100% tests passed, 0 tests failed out of 1
	[...]
	$ (cd t && ./t0071-sort.sh)
	ok 1 - DEFINE_LIST_SORT_DEBUG
	# passed all 1 test(s)
	1..1

This change optionally closes that gap, and allows for more sensible behavior. Due to concerns about existing MS Visual Studio users relying on some of these edge cases this is being made optional, with the default of the new "GIT_CTEST_SETS_BUILD_DIR" being "ON" on Windows (i.e. the old behavior), and "OFF" elsewhere.

The resulting behavior differences are the following:

	|---+----+-----|
	|   | ON | OFF |
	|---+----+-----|
	| A | N  | Y   |
	| B | N  | Y   |
	| C | Y  | Y   |
	| D | Y  | N   |
	| E | N  | Y   |
	|---+----+-----|
	A. Can manually run tests before ctest?
	B. Manually run tests point to latest cmake build?...
	C. Manually run tests point to latest "ctest"'d build?
        D. "git" picked at ctest start time?
        E. "git" picked at test start time?

On "D" and "E": Because GIT_CTEST_SETS_BUILD_DIR=ON relies on the global "GIT-BUILD-DIR" when using "ctest" it cannot be used to run concurrent tests for two different builds from the same source directory.

But as noted in [3] existing users using Visual Studio may be relying on some of these edge cases, so let's make this optional on Windows, but switch the default to this new discover method on other platforms.

1. 7f5397a07c6 (cmake: support for testing git when building out of
   the source tree, 2020-06-26)
2. ee9e66e4e76 (cmake: avoid editing t/test-lib.sh, 2022-10-18)
3. https://lore.kernel.org/git/663b93ef-0c89-a5f6-1069-b4be97915d20@dunelm.org.uk/

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 contrib/buildsystems/CMakeLists.txt | 38 ++++++++++++++++++++++++-----
 t/test-lib.sh                       |  3 ++-
 2 files changed, 34 insertions(+), 7 deletions(-)

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(... CACHE <type> <docstring>) facility. As noted in cmake's documentation[1] this integrates nicely with e.g. cmake-gui.

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

The "separate_arguments()" here will do the right thing for arguments that don't contain whitespace, so e.g. the path to --root="" can't have a space in it. There's supposedly a way to work around that with separate_arguments(), but it requires features newer than our required cmake version, so let's live with that edge case for now.

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 | 50 +++++++++++++++++++++++++++--
 1 file changed, 48 insertions(+), 2 deletions(-)

On Fri, Dec 2, 2022 at 6:28 AM Ævar Arnfjörð Bjarmason <avarab@gmail.com> wrote:

s/Windowns/Windows/

On Sat, Dec 03 2022, Eric Sunshine wrote:

Thanks, the rest of this is just typos, but maybe this one's a Freudian slip :)

On Sat, Dec 3, 2022 at 8:11 AM Ævar Arnfjörð Bjarmason <avarab@gmail.com> wrote:

I had the same thought and nearly said so.

Per c858750b41c (cmake: increase time-out for a long-running test, 2022-10-18) the reason to set a custom timeout for t7112-reset-submodule.sh is Windows-specific. Let's only do that on Windows then.

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 contrib/buildsystems/CMakeLists.txt | 8 +++++---
 1 file changed, 5 insertions(+), 3 deletions(-)

Guard the finding of "SH_EXE" in "C:\Program Files" with a check for whether we're on Windows.

This Windows-specific code was first added in [1], and later expanded on [2], but since some of that was added this build recipe has been made portable outside of Windows.

1. 72b6eeb81b1 (cmake: do find Git for Windows' shell interpreter,
   2020-09-28)
2. 476e54b1c60 (cmake: support local installations of git,
   2022-07-27).

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 contrib/buildsystems/CMakeLists.txt | 15 +++++++++++----
 1 file changed, 11 insertions(+), 4 deletions(-)

Since [1] two git-p4 tests have relied on there being a "git-p4.py" in the build directory, but the cmake recipe was not updated to account for this. Let's copy the "git-p4.py" over.

We could also change the test to e.g. grab the built "git-p4" and alter its shebang, which would be friendly to GIT_TEST_INSTALLED, but let's just do the bare minimum here to get cmake+ctest working without altering the test itself.

The reason this hasn't been caught by "vs-build" and "vs-test" is because those tests added in [2] invoke "cmake" with "-DPYTHON_TESTS=OFF", and therefore we'd skip this part of the git-p4 tests before getting past the "do we have python?" check. Even if we got past that the Windows CI wouldn't have a "p4" or "p4d" binary installed, so we'd skip the tests anyway.

In a subsequent commit we'll run "cmake" and "ctest" in CI with "ubuntu-latest", so we'll need this "git-p4.py" file.

1. f7b5ff607fa (git-p4: improve encoding handling to support
   inconsistent encodings, 2022-04-30)
2. 4c2c38e800f (ci: modification of main.yml to use cmake for vs-build
   job, 2020-06-26)

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 contrib/buildsystems/CMakeLists.txt | 1 +
 1 file changed, 1 insertion(+)

Since [1] the "cmake" build method should work properly on Linux, but as seen in preceding commits there were various bugs in it, which are hopefully now all fixed.

To ensure that it doesn't break again let's add a "linux-cmake-ctest" target to build and test "cmake" on Linux, in addition to that we'll also run the tests with "ctest" instead of "make" or "prove", so we can assert that testing with that method works..

This also stress tests running "cmake" (and "ctest") out of a build directory that isn't the top-level. The "vs-build" job uses "cmake" since [2], but clobbers the top-level "Makefile" and builds in the top-level directory.

That was the reason for why we didn't spot that various tests still required missing "mergetools/*" etc, which was fixed by using "$GIT_SOURCE_DIR" in a preceding commit.

Since the "ci/lib.sh" already creates and exports a GIT_TEST_OPTS="..." we'll pick that up in our CI, see the preceding commit. Because we pass the "--verbose-log -x --github-workflow-markup" in the GitHub CI as a result the interaction with "handle_failed_tests" here works correctly. I.e. on failure we'll have saved "t/test-results/*.{exit,out,markup}" files relevant to the failing test(s).

1. f31b6244950 (Merge branch 'yw/cmake-updates', 2022-06-07)
2. 4c2c38e800f (ci: modification of main.yml to use cmake for vs-build
   job, 2020-06-26)

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 .github/workflows/main.yml |  3 +++
 ci/run-build-and-tests.sh  | 13 +++++++++++--
 2 files changed, 14 insertions(+), 2 deletions(-)

On Fri, Dec 2, 2022 at 6:28 AM Ævar Arnfjörð Bjarmason <avarab@gmail.com> wrote:

s/\.\.$/./

Probably want: s/CI,/CI;/

This topic gets our tests from passing ~80% with ctest on *nix to passing 100%.

See passing CI job for "cmake + ctest" on Ubuntu at: https://github.com/avar/git/actions/runs/3625189647

See https://lore.kernel.org/git/cover-v5-00.15-00000000000-20221202T110947Z-avarab@gmail.com/ for the v5.

Changes since v5:

* Typo/grammar etc. fixes noted by Eric.
* Phillip reported the "chmod" fallback no working on Windows. I
  couldn't reproduce that, but running it was always redundant there
  we'll now skip doing that on Windows.
* Adjust 15/15 to fold into an existing "case" statement, rather than
  adding a new "if" statement.

For this v6 I experimented with changing the "win+VS build|test" job build in "contrib/buildsystems/out" rather than at the top-level[1]. I left those changes out here, but doing so makes that a lot simpler, as it no longer needs to rely on the Makefile to tell it what cmake might have built.

But even without including that here, those changes should give more confidence in these changes, i.e. they show that our CI's "msbuild" doesn't require the "cmake" to build things at the top-level.

1. https://github.com/avar/git/tree/avar/cmake-test-path-no-make-for-windows-ci

Ævar Arnfjörð Bjarmason (15):
  cmake: don't invoke msgfmt with --statistics
  cmake: use "-S" and "-B" to specify source and build directories
  cmake: update instructions for portable CMakeLists.txt
  cmake: don't copy chainlint.pl to build directory
  cmake: chmod +x the bin-wrappers/* & SCRIPT_{SH,PERL} & git-p4
  cmake & test-lib.sh: add a $GIT_SOURCE_DIR variable
  cmake: set "USE_LIBPCRE2" in "GIT-BUILD-OPTIONS" for test-lib.sh
  Makefile + test-lib.sh: don't prefer cmake-built to make-built git
  test-lib.sh: support a "GIT_TEST_BUILD_DIR"
  cmake: optionally be able to run tests before "ctest"
  cmake: support GIT_TEST_OPTS, abstract away WIN32 defaults
  cmake: increase test timeout on Windows only
  cmake: only look for "sh" in "C:/Program Files" on Windows
  cmake: copy over git-p4.py for t983[56] perforce test
  CI: add a "linux-cmake-test" to run cmake & ctest on linux

 .github/workflows/main.yml          |   3 +
 Makefile                            |   1 -
 ci/run-build-and-tests.sh           |  11 +-
 contrib/buildsystems/CMakeLists.txt | 213 ++++++++++++++++++++++------
 t/README                            |   3 +
 t/lib-gettext.sh                    |   2 +-
 t/lib-gitweb.sh                     |   2 +-
 t/t7609-mergetool--lib.sh           |   2 +-
 t/t9902-completion.sh               |  14 +-
 t/t9903-bash-prompt.sh              |   2 +-
 t/test-lib.sh                       |  19 ++-
 11 files changed, 211 insertions(+), 61 deletions(-)

Range-diff against v5:
 1:  7d83ff44c61 !  1:  fc190b379cd cmake: don't invoke msgfmt with --statistics
    @@ Metadata
      ## Commit message ##
         cmake: don't invoke msgfmt with --statistics
     
    -    In 2f12b31b746 (Makefile: don't invoke msgfmt with --statistics,
    -    2021-12-17) I made the same change to our Makefile, let's follow-up
    -    and do the same here.
    +    In [1] I made the same change to our Makefile, let's follow-up and do
    +    the same here.
     
         For "cmake" this is particularly nice with "-G Ninja", as before we'd
         emit ~40 lines of overflowed progress bar output, but now it's only
         the one line of "ninja"'s progress bar.
     
    +    1. 2f12b31b746 (Makefile: don't invoke msgfmt with --statistics,
    +       2021-12-17)
    +
         Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
     
      ## contrib/buildsystems/CMakeLists.txt ##
 2:  314c5e40cff !  2:  1a11aa233a3 cmake: use "-S" and "-B" to specify source and build directories
    @@ Commit message
         The "-S" and "-B" options were added in cmake v3.13.0, which is older
         than the version we have a hard dependency on[1].
     
    -    As an aside The "-p" flag to "mkdir" in the pre-image wasn't needed,
    +    As an aside, the "-p" flag to "mkdir" in the pre-image wasn't needed,
         as "contrib/buildsystems" is tracked
     
         1. 061c2240b1b (Introduce CMake support for configuring Git, 2020-06-12)
 3:  5c4ba2425b6 =  3:  b9ddb5db1d3 cmake: update instructions for portable CMakeLists.txt
 4:  e86245ffbab =  4:  7b7850c00ee cmake: don't copy chainlint.pl to build directory
 5:  435452314fa !  5:  82ecb797915 cmake: chmod +x the bin-wrappers/* & SCRIPT_{SH,PERL} & git-p4
    @@ Commit message
     
         The remaining failures will be addressed in subsequent commits.
     
    +    As this isn't needed on Windows let's skip this there. There's also an
    +    unconfirmed (it works in CI) report[5] that invoking the "chmod"
    +    command fails in some scenarios.
    +
         1. f31b6244950 (Merge branch 'yw/cmake-updates', 2022-06-07)
         2. e4597aae659 (run test suite without dashed git-commands in PATH, 2009-12-02)
         3. 2ea1d8b5563 (cmake: make it easier to diagnose regressions in CTest
            runs, 2022-10-18)
         4. https://cmake.org/cmake/help/latest/command/file.html#chmod
    +    5. https://lore.kernel.org/git/87f22a55-ee84-2f76-7b9b-924a97f44f89@dunelm.org.uk/
     
         Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
     
    @@ contrib/buildsystems/CMakeLists.txt: add_custom_command(OUTPUT ${git_links} ${gi
     +function(write_script path content)
     +	file(WRITE ${path} ${content})
     +
    -+	if("${CMAKE_MAJOR_VERSION}.${CMAKE_MINOR_VERSION}" VERSION_GREATER_EQUAL "3.19")
    ++	if(WIN32)
    ++		message(TRACE "skipping chmod +x '${path}' on Windows")
    ++	elseif("${CMAKE_MAJOR_VERSION}.${CMAKE_MINOR_VERSION}" VERSION_GREATER_EQUAL "3.19")
     +		file(CHMOD ${path} FILE_PERMISSIONS OWNER_READ OWNER_WRITE OWNER_EXECUTE)
     +	else()
     +		execute_process(COMMAND chmod +x ${path}
 6:  7ee341245a8 =  6:  1f326944a07 cmake & test-lib.sh: add a $GIT_SOURCE_DIR variable
 7:  49645cb1643 =  7:  973c8038f54 cmake: set "USE_LIBPCRE2" in "GIT-BUILD-OPTIONS" for test-lib.sh
 8:  9e10cd5bd8a =  8:  b8448c7a8a6 Makefile + test-lib.sh: don't prefer cmake-built to make-built git
 9:  df5aea35bbd !  9:  5135e40969e test-lib.sh: support a "GIT_TEST_BUILD_DIR"
    @@ Metadata
      ## Commit message ##
         test-lib.sh: support a "GIT_TEST_BUILD_DIR"
     
    -    Compliment the existing GIT_TEST_INSTALLED variable added in [1] with
    +    Complement the existing GIT_TEST_INSTALLED variable added in [1] with
         one that allows for testing a git built in a directory that's not
         "../".
     
10:  529e18f7f72 ! 10:  65204463730 cmake: optionally be able to run tests before "ctest"
    @@ contrib/buildsystems/CMakeLists.txt: if(USE_VCPKG)
     +	set(GIT_CTEST_SETS_BUILD_DIR "OFF" CACHE BOOL GIT_CTEST_SETS_BUILD_DIR_DOC)
     +endif()
     +if(GIT_CTEST_SETS_BUILD_DIR)
    -+	message(STATUS "Enabling manual runs of t/* tests once ctest writes CMAKE-BUILD-DIR")
    ++	message(STATUS "Have GIT_CTEST_SETS_BUILD_DIR=ON. Need a 'ctest' run to write GIT-BUILD-DIR")
     +else()
    -+	message(STATUS "Enabling manual runs of t/* tests via cmake build dir discovery")
    ++	message(STATUS "Have GIT_CTEST_SETS_BUILD_DIR=OFF. Wrote out a GIT-BUILD-DIR, no 'ctest' required")
     +endif()
     +
     +# When using GIT_CTEST_SETS_BUILD_DIR, running the tests from the "t/"
11:  a5dd4fa1d00 ! 11:  e25992b16f1 cmake: support GIT_TEST_OPTS, abstract away WIN32 defaults
    @@ contrib/buildsystems/CMakeLists.txt: endif()
     +elseif(WIN32)
     +	set(GIT_TEST_OPTS "--no-bin-wrappers --no-chain-lint -vx"
     +		CACHE STRING "test options, see t/README")
    -+	message(STATUS "Using Windowns-specific default test options: ${GIT_TEST_OPTS}")
    ++	message(STATUS "Using Windows-specific default test options: ${GIT_TEST_OPTS}")
     +else()
     +	set(GIT_TEST_OPTS ""
     +		CACHE STRING "test options, see t/README")
12:  c9d550b8558 = 12:  4905ce5321d cmake: increase test timeout on Windows only
13:  7a5dd684d40 = 13:  6c6b530965d cmake: only look for "sh" in "C:/Program Files" on Windows
14:  c9c895bb794 = 14:  563f1b9b045 cmake: copy over git-p4.py for t983[56] perforce test
15:  a6f343b6221 ! 15:  917a884eb65 CI: add a "linux-cmake-test" to run cmake & ctest on linux
    @@ Commit message
         To ensure that it doesn't break again let's add a "linux-cmake-ctest"
         target to build and test "cmake" on Linux, in addition to that we'll
         also run the tests with "ctest" instead of "make" or "prove", so we
    -    can assert that testing with that method works..
    +    can assert that testing with that method works.
     
         This also stress tests running "cmake" (and "ctest") out of a build
         directory that isn't the top-level. The "vs-build" job uses "cmake"
    @@ Commit message
         "$GIT_SOURCE_DIR" in a preceding commit.
     
         Since the "ci/lib.sh" already creates and exports a
    -    GIT_TEST_OPTS="..." we'll pick that up in our CI, see the preceding
    +    GIT_TEST_OPTS="..." we'll pick that up in our CI; see the preceding
         commit. Because we pass the "--verbose-log -x
         --github-workflow-markup" in the GitHub CI as a result the interaction
         with "handle_failed_tests" here works correctly. I.e. on failure we'll
    @@ .github/workflows/main.yml: jobs:
                  pool: macos-latest
     
      ## ci/run-build-and-tests.sh ##
    -@@ ci/run-build-and-tests.sh: pedantic)
    - 	;;
    - esac
    +@@ ci/run-build-and-tests.sh: esac
    + 
    + run_tests=t
      
    --group Build make
     +mc=
    -+if test "$jobname" = "linux-cmake-ctest"
    -+then
    + case "$jobname" in
    + linux-gcc)
    + 	export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME=main
    +@@ ci/run-build-and-tests.sh: pedantic)
    + 	export DEVOPTS=pedantic
    + 	run_tests=
    + 	;;
    ++linux-cmake-ctest)
     +	cb=contrib/buildsystems
     +	group CMake cmake -S "$cb" -B "$cb/out"
     +	mc="-C $cb/out"
    -+fi
    -+
    ++	;;
    + esac
    + 
    +-group Build make
     +group Build make $mc
     +
      if test -n "$run_tests"

In [1] I made the same change to our Makefile, let's follow-up and do the same here.

For "cmake" this is particularly nice with "-G Ninja", as before we'd emit ~40 lines of overflowed progress bar output, but now it's only the one line of "ninja"'s progress bar.

1. 2f12b31b746 (Makefile: don't invoke msgfmt with --statistics,
   2021-12-17)

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 contrib/buildsystems/CMakeLists.txt | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

Rather than the multi-line "mkdir/cd/cmake" recipe provide an equivalent one-liner using the "-S" and "-B" options, and then suggest building with "make -C <build-dir>".

The rest of these instructions discuss e.g. running tests from our top-level "t/" directory, so it's more helpful to avoid changing the user's current directory.

The "-S" and "-B" options were added in cmake v3.13.0, which is older than the version we have a hard dependency on[1].

As an aside, the "-p" flag to "mkdir" in the pre-image wasn't needed, as "contrib/buildsystems" is tracked

1. 061c2240b1b (Introduce CMake support for configuring Git, 2020-06-12)

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 contrib/buildsystems/CMakeLists.txt | 8 +++-----
 1 file changed, 3 insertions(+), 5 deletions(-)

When [1] made this copy of "chainlint.sed" the script was invoked in the test-lib.sh as:

	[...] sed -f "$GIT_BUILD_DIR/t/chainlint.sed" [...]

But when [2] replaced "chainlint.sed" with a "chainlint.pl" it also replaced that "$GIT_BUILD_DIR" with "$TEST_DIRECTORY", invoking it as:

	"$PERL_PATH" "$TEST_DIRECTORY/chainlint.pl" "$0"

So this line could have been deleted in [2] but wasn't. Let's do that now.

1. 7f5397a07c6 (cmake: support for testing git when building out of
   the source tree, 2020-06-26)
2. 23a14f30166 (test-lib: replace chainlint.sed with chainlint.pl,
   2022-09-01)

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 contrib/buildsystems/CMakeLists.txt | 1 -
 1 file changed, 1 deletion(-)

The instructions for running CMake went back & forth between *nix, Windows and Visual Studio instructions Let's create headings and split the existing instructions up into those new sections.

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 contrib/buildsystems/CMakeLists.txt | 68 +++++++++++++++++++++--------
 1 file changed, 50 insertions(+), 18 deletions(-)

Since the cmake file was made to run on *nix in [1] running the tests with "ctest" broken, because we'd attempt to invoke our bin-wrappers/, but they didn't have the executable bit.

In the best case, the "t/test-lib.sh" would be unable to find "bin-wrappers/git", and we'd fall back on "GIT_EXEC_PATH=$GIT_BUILD_DIR" using the fallback behavior added in [2]:

	$ ./t0001-init.sh
	<GIT_BUILD_DIR>/t/../contrib/buildsystems/out/bin-wrappers/git is not executable; using GIT_EXEC_PATH

This was recently somewhat swept under the rug in [3], as ctest would run them with "--no-bin-wrappers". But still with [3], running e.g.:

	cmake -S contrib/buildsystems -B contrib/buildsystems/out -DCMAKE_BUILD_TYPE=Debug &&
	make -C contrib/buildsystems/out &&
	ctest --test-dir contrib/buildsystems/out --jobs="$(nproc)" --output-on-failure

Fails around 20% of our tests on *nix. So even with [3] we'd fail any test that needed to invoke one of our built shell, perl or Python scripts on *nix. E.g. t0012-help.sh would fail on a test that tried to invoke "git web--browse". The equivalent of this (in the "out" directory) would happen:

	$ ./git --exec-path=$PWD web--browse
	git: 'web--browse' is not a git command. See 'git --help'.

Which we can fix by "chmod +x"-ing the built "git-web--browse":

	$ chmod +x git-web--browse
	$ ./git --exec-path=$PWD web--browse
	usage: git web--browse [--browser=browser|--tool=browser] [--config=conf.var] url/file ...

The same goes for e.g. the "git-p4" tests, which would fail because our built "git-p4" wasn't executable, etc. There's also a few other outstanding issues, which will be fixed in subsequent commits.

Ideally we'd use the file(CHMOD ...) form everywhere, but that syntax was introduced in cmake 3.19[4], whereas we only require 3.14. Let's provide a fallback behind a version check, so that we'll eventually be able to delete the "else" part. Both forms result in the same file modes.

Before this change:

	80% tests passed, 196 tests failed out of 977

After:

	99% tests passed, 5 tests failed out of 977

The remaining failures will be addressed in subsequent commits.

As this isn't needed on Windows let's skip this there. There's also an unconfirmed (it works in CI) report[5] that invoking the "chmod" command fails in some scenarios.

1. f31b6244950 (Merge branch 'yw/cmake-updates', 2022-06-07)
2. e4597aae659 (run test suite without dashed git-commands in PATH, 2009-12-02)
3. 2ea1d8b5563 (cmake: make it easier to diagnose regressions in CTest
   runs, 2022-10-18)
4. https://cmake.org/cmake/help/latest/command/file.html#chmod
5. https://lore.kernel.org/git/87f22a55-ee84-2f76-7b9b-924a97f44f89@dunelm.org.uk/

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>

write script
---
 contrib/buildsystems/CMakeLists.txt | 27 +++++++++++++++++++++------
 1 file changed, 21 insertions(+), 6 deletions(-)

When git is built and tested with cmake & ctest we usually do that outside of the top-level source directory. E.g.

	cmake -S contrib/buildsystems -B contrib/buildsystems/out
	make -C contrib/buildsystems/out
	ctest -j$(nproc) --test-dir contrib/buildsystems/out

Since the test-lib.sh makes assumptions about the TEST_DIRECTORY being the "t" subdirectory of the top-level "$GIT_BUILD_DIR", the cmake recipe has needed to copy various assets to that "contrib/buildsystems/out" directory.

But we've only been doing this for the subsets of tests that run on Windows, and which have otherwise been covered by that CI target. The CI target builds (among other things) with "-DPERL_TESTS=OFF -DPYTHON_TESTS=OFF -DCURL_NO_CURL_CMAKE=ON", see [1]. Furthermore, the CI isn't testing from the "contrib/buildsystems/out" directory, instead it clobbers the top-level MAkefile.

There was a recent commit to fix a subset of these issues, see 6a83b5f0810 (cmake: copy the merge tools for testing, 2022-10-18).

Let's stop going for that approach, and instead teach the test-lib.sh that there's such a thing as the "$GIT_SOURCE_DIR" distinct from the "$GIT_BUILD_DIR".

Just as the "$TEST_DIRECTORY" always points to our actual "t" directory (not the "[...]/out/t" cmake creates), this new "$GIT_SOURCE_DIR" will always be the top-level source directory.

With this change we now pass 3/5 of the tests that we still had failing with the fixes in the preceding commit.

1. 4c2c38e800f (ci: modification of main.yml to use cmake for vs-build
   job, 2020-06-26)

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 contrib/buildsystems/CMakeLists.txt |  6 ------
 t/lib-gettext.sh                    |  2 +-
 t/lib-gitweb.sh                     |  2 +-
 t/t7609-mergetool--lib.sh           |  2 +-
 t/t9902-completion.sh               | 14 +++++++-------
 t/t9903-bash-prompt.sh              |  2 +-
 t/test-lib.sh                       |  8 +++++++-
 7 files changed, 18 insertions(+), 18 deletions(-)

When we build with libpcre2 which cmake has supported since [1] we need to set "USE_LIBPCRE2='YesPlease'" (or similar) in "GIT-BUILD-OPTIONS".

Without this e.g. t7810-grep.sh will fail, as it has tests that rely on the behavior of !PCRE2. The reason this hasn't been noticed is that the Windows CI doesn't have access to libpcre2.

With this the remaining two failures we had left after the preceding step are resolved, but note that that test run didn't include the git-p4 tests, which a subsequent commit will address).

1. 80431510a2b (cmake: add pcre2 support, 2022-05-24)

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 contrib/buildsystems/CMakeLists.txt | 3 +++
 1 file changed, 3 insertions(+)

Per ee9e66e4e76 (cmake: avoid editing t/test-lib.sh, 2022-10-18) and the surrounding discussion the goal of that change was to have building with cmake and:

	(cd t && ./t0001-init.sh)

Work, to do so we need some way to make the test-lib.sh aware that we have a build in contrib/buildsystems/out. The downside of doing it via the GIT-BUILD-DIR facility in ee9e66e4e76 is:

- A "remove this file while at it" only fits into the Makefile
  dependency graph if we "FORCE" a shell command to be run, so doing it
  this way precludes speeding up the Makefile by e.g. making use of the
  "$(file)" function (for which I've got follow-up patches).

- If you build with make, then with cmake, and then e.g.:

	make -C t prove

  We'll run the tests against the cmake version of the tests, not the
  "make" built ones. It's important to get any subtle interaction
  between two conflicting build systems right.

Now we'll instead check if the top-level "git" exists, and only if it doesn't look for the one built by cmake. This ensures that we'll pick it up as required when the user is using CMake, without being subject to the odd interaction of flip-flopping between the two environments.

It also means that we can remove the rule to remove the "GIT-BUILD-DIR", if we get to creating "GIT-BUILD-OPTIONS" in the top-level git. That's because now when building with both "make" and "cmake" we will always prefer the former for running the tests, so there's no need to remove the "GIT-BUILD-DIR" file to resolve the conflicts between the two.

In practice users are building with either "make" or "cmake", and not mixing the two, but in a subsequent change we'll provide an out for that "make"-preferring selection behavior, by having "ctest" runs always prefer the "cmake" built one.

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 Makefile      | 1 -
 t/test-lib.sh | 3 ++-
 2 files changed, 2 insertions(+), 2 deletions(-)

Complement the existing GIT_TEST_INSTALLED variable added in [1] with one that allows for testing a git built in a directory that's not "../".

Instead of "test this installed git" ("GIT_TEST_INSTALLED"), this new "GIT_TEST_BUILD_DIR" is a "test this built directory over there". E.g. this will pass all tests:

	git clone --depth=1 https://github.com/git/git.git git1
	cp -R git1 git2
	make -C git1
	[apply this change to git2]
	(cd git2 && GIT_TEST_BUILD_DIR="$PWD/../git1" make -C t prove)

This facility and file-based instructions to have the test suite use another build directory[2] are mutually exclusive. In a subsequent commit we'll make CMake and CTest optionally prefer this method.

1. 6720721e152 (test-lib.sh: Allow running the test suite against
   installed git, 2009-03-16)
2. 350a005e366 (cmake: avoid editing t/test-lib.sh, 2022-10-18)

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 t/README      | 3 +++
 t/test-lib.sh | 7 +++++++
 2 files changed, 10 insertions(+)

Since [1] the "cmake" build has had support for running the tests manually from the "t/" directory once we've built with "cmake", but only after a one-off run of "ctest".

I.e. we create the build recipe via "cmake" and build with e.g. "make", but after doing so would get:

	$ (cd t && ./t0071-sort.sh)
	error: GIT-BUILD-OPTIONS missing (has Git been built?).

The recently added amendment to this of using a "GIT-BUILD-DIR"[2] didn't change these semantics. After we did a one-off run of "ctest" we'd be able to run the tests without "ctest":

	$ ctest --test-dir contrib/buildsystems/out -R t0071
	[...]
	100% tests passed, 0 tests failed out of 1
	[...]
	$ (cd t && ./t0071-sort.sh)
	ok 1 - DEFINE_LIST_SORT_DEBUG
	# passed all 1 test(s)
	1..1

This change optionally closes that gap, and allows for more sensible behavior. Due to concerns about existing MS Visual Studio users relying on some of these edge cases this is being made optional, with the default of the new "GIT_CTEST_SETS_BUILD_DIR" being "ON" on Windows (i.e. the old behavior), and "OFF" elsewhere.

The resulting behavior differences are the following:

	|---+----+-----|
	|   | ON | OFF |
	|---+----+-----|
	| A | N  | Y   |
	| B | N  | Y   |
	| C | Y  | Y   |
	| D | Y  | N   |
	| E | N  | Y   |
	|---+----+-----|
	A. Can manually run tests before ctest?
	B. Manually run tests point to latest cmake build?...
	C. Manually run tests point to latest "ctest"'d build?
        D. "git" picked at ctest start time?
        E. "git" picked at test start time?

On "D" and "E": Because GIT_CTEST_SETS_BUILD_DIR=ON relies on the global "GIT-BUILD-DIR" when using "ctest" it cannot be used to run concurrent tests for two different builds from the same source directory.

But as noted in [3] existing users using Visual Studio may be relying on some of these edge cases, so let's make this optional on Windows, but switch the default to this new discover method on other platforms.

1. 7f5397a07c6 (cmake: support for testing git when building out of
   the source tree, 2020-06-26)
2. ee9e66e4e76 (cmake: avoid editing t/test-lib.sh, 2022-10-18)
3. https://lore.kernel.org/git/663b93ef-0c89-a5f6-1069-b4be97915d20@dunelm.org.uk/

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 contrib/buildsystems/CMakeLists.txt | 38 ++++++++++++++++++++++++-----
 t/test-lib.sh                       |  3 ++-
 2 files changed, 34 insertions(+), 7 deletions(-)

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(... CACHE <type> <docstring>) facility. As noted in cmake's documentation[1] this integrates nicely with e.g. cmake-gui.

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

The "separate_arguments()" here will do the right thing for arguments that don't contain whitespace, so e.g. the path to --root="" can't have a space in it. There's supposedly a way to work around that with separate_arguments(), but it requires features newer than our required cmake version, so let's live with that edge case for now.

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 | 50 +++++++++++++++++++++++++++--
 1 file changed, 48 insertions(+), 2 deletions(-)

Per c858750b41c (cmake: increase time-out for a long-running test, 2022-10-18) the reason to set a custom timeout for t7112-reset-submodule.sh is Windows-specific. Let's only do that on Windows then.

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 contrib/buildsystems/CMakeLists.txt | 8 +++++---
 1 file changed, 5 insertions(+), 3 deletions(-)

Since [1] two git-p4 tests have relied on there being a "git-p4.py" in the build directory, but the cmake recipe was not updated to account for this. Let's copy the "git-p4.py" over.

We could also change the test to e.g. grab the built "git-p4" and alter its shebang, which would be friendly to GIT_TEST_INSTALLED, but let's just do the bare minimum here to get cmake+ctest working without altering the test itself.

The reason this hasn't been caught by "vs-build" and "vs-test" is because those tests added in [2] invoke "cmake" with "-DPYTHON_TESTS=OFF", and therefore we'd skip this part of the git-p4 tests before getting past the "do we have python?" check. Even if we got past that the Windows CI wouldn't have a "p4" or "p4d" binary installed, so we'd skip the tests anyway.

In a subsequent commit we'll run "cmake" and "ctest" in CI with "ubuntu-latest", so we'll need this "git-p4.py" file.

1. f7b5ff607fa (git-p4: improve encoding handling to support
   inconsistent encodings, 2022-04-30)
2. 4c2c38e800f (ci: modification of main.yml to use cmake for vs-build
   job, 2020-06-26)

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 contrib/buildsystems/CMakeLists.txt | 1 +
 1 file changed, 1 insertion(+)

Guard the finding of "SH_EXE" in "C:\Program Files" with a check for whether we're on Windows.

This Windows-specific code was first added in [1], and later expanded on [2], but since some of that was added this build recipe has been made portable outside of Windows.

1. 72b6eeb81b1 (cmake: do find Git for Windows' shell interpreter,
   2020-09-28)
2. 476e54b1c60 (cmake: support local installations of git,
   2022-07-27).

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 contrib/buildsystems/CMakeLists.txt | 15 +++++++++++----
 1 file changed, 11 insertions(+), 4 deletions(-)

Since [1] the "cmake" build method should work properly on Linux, but as seen in preceding commits there were various bugs in it, which are hopefully now all fixed.

To ensure that it doesn't break again let's add a "linux-cmake-ctest" target to build and test "cmake" on Linux, in addition to that we'll also run the tests with "ctest" instead of "make" or "prove", so we can assert that testing with that method works.

This also stress tests running "cmake" (and "ctest") out of a build directory that isn't the top-level. The "vs-build" job uses "cmake" since [2], but clobbers the top-level "Makefile" and builds in the top-level directory.

That was the reason for why we didn't spot that various tests still required missing "mergetools/*" etc, which was fixed by using "$GIT_SOURCE_DIR" in a preceding commit.

Since the "ci/lib.sh" already creates and exports a GIT_TEST_OPTS="..." we'll pick that up in our CI; see the preceding commit. Because we pass the "--verbose-log -x --github-workflow-markup" in the GitHub CI as a result the interaction with "handle_failed_tests" here works correctly. I.e. on failure we'll have saved "t/test-results/*.{exit,out,markup}" files relevant to the failing test(s).

1. f31b6244950 (Merge branch 'yw/cmake-updates', 2022-06-07)
2. 4c2c38e800f (ci: modification of main.yml to use cmake for vs-build
   job, 2020-06-26)

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 .github/workflows/main.yml |  3 +++
 ci/run-build-and-tests.sh  | 11 +++++++++--
 2 files changed, 12 insertions(+), 2 deletions(-)

Hi Ævar

On 06/12/2022 02:08, Ævar Arnfjörð Bjarmason wrote:

I suspect the issue is that "chmod" is not in %path% (i.e. $PATH in posix shell speak) when CMake is run from visual studio (the mingw stuff is only available when running "sh" by default to prevent name clashes with the windows find command etc.) but is added to $PATH in the CI after installing the git-for-windows sdk.

Long term I think that is a good direction for the CI job. The CI job is not a good simulation of running CMake from Visual Studio. That does not normally matter but has been a problem for this series. Ideally it would run the test with CTest as well, I'm not sure how much work that would be.

As far as I can see this changes test-lib.sh to prefer the make-built git if it exists even after running cmake and ctest. Ideally we'd test the most recent build.

Best Wishes

Phillip

On Wed, Dec 07 2022, Phillip Wood wrote:

Maybe, presumably the "sh" then sets the $PATH, because we do run shellscripts at the top-level that expect to find the binutils, e.g. "sed" and the like, which presumably are in the same location as "chmod".

But in any case this v6 avoids the issue entirely.

Yes, I just didn't want to make this series even larger, but I can submit that after.

In principle trivial, I did try, but it seems the Windows image doesn't have "ctest", but does have "cmake" (unlike the Ubuntu image).

(Or come to think of it, maybe it does and I tried "ctest" and not "ctest.exe". I can't remember)

Other than that it should just work, and the "linux-cmake-ctest" CI job added by this series already does a run with "ctest".

Ideally, but as summarized in the v5 CL this already isn't the case on "master". I.e. you can e.g. build with cmake, then "make git", then your tests will use the cmake (older) git, not your newly built one with "make".

I think the only sane state of affairs here is what this series changes it to.

If we have a top-level "git" we should always prefer it, as "make" is our primary build system, and it would be impractical to plug all the gaps and intercept the "latest git" in all places.

But more importantly, isn't this concern entirely academic?

We can imagine scenarios where you build with both, but the VS users that are the primary audience of the cmake recipe aren't going to also build with "make", they're just building with "cmake" and "msbuild".

So I don't think this will impact anyone in practice.

Even you have both a "make" and "cmake" build in play the "make test" and "ctest" will always select the "make" and "cmake" built one, respectively.

The only case where we need to pick one is if you run a test manually by cd-ing to "t" and invoking a shellscript.

You also brought up a concern about multiple builds in contrib/buildsystems/ in a previous iteration. With the way this series leaves it that'll actually work in some sensible way. I.e. your "ctest" run for a "prod" build run won't switch to a new git that you built midway through the run, as it would on "master".

On 27/10/2022 04:26, Ævar Arnfjörð Bjarmason wrote:

That's a shame - what was the issue? My suggestion was cut and pasted from https://github.com/phillipwood/git/commit/0bb195b00c3a5ff27ec66a5966d0a9d91a187738 I don't remember having any problems when I wrote it.

Best Wishes

Phillip