Re: ab/cmake-nix-and-ci (was Re: What's cooking in git.git (Nov 2022, #07; Tue, 29))

> 
> On Wed, Nov 30 2022, Phillip Wood wrote:
> 
>> Hi Junio
> 
> Hi, and thanks for your review of this series.
> 
>> On 29/11/2022 09:40, Junio C Hamano wrote:
>>> * ab/cmake-nix-and-ci (2022-11-04) 14 commits
>>>     (merged to 'next' on 2022-11-08 at 6ef4e93b36)
>>>    + CI: add a "linux-cmake-test" to run cmake & ctest on linux
>>>    + cmake: copy over git-p4.py for t983[56] perforce test
>>>    + cmake: only look for "sh" in "C:/Program Files" on Windows
>>>    + cmake: increase test timeout on Windows only
>>>    + cmake: support GIT_TEST_OPTS, abstract away WIN32 defaults
>>>    + Makefile + cmake: use environment, not GIT-BUILD-DIR
>>>    + test-lib.sh: support a "GIT_TEST_BUILD_DIR"
>>>    + cmake: set "USE_LIBPCRE2" in "GIT-BUILD-OPTIONS" for test-lib.sh
>>>    + cmake & test-lib.sh: add a $GIT_SOURCE_DIR variable
>>>    + cmake: chmod +x the bin-wrappers/* & SCRIPT_{SH,PERL} & git-p4
>>>    + cmake: don't copy chainlint.pl to build directory
>>>    + cmake: update instructions for portable CMakeLists.txt
>>>    + cmake: use "-S" and "-B" to specify source and build directories
>>>    + cmake: don't invoke msgfmt with --statistics
>>>    Fix assorted issues with CTest on *nix machines.

>> If that's all this series did then I think it would be fine. However
>> it also makes changes to test-lib.sh to hard code the build directory
>> in an attempt to remove GIT-BUILD-DIR. I'm not convinced that is an
>> improvement on the status quo.
> 
> I think the series as it stands addresses those concerns. In particular
> building outside of contrib/buildsystems/out works, just as before:
> 
> 	cmake -S contrib/buildsystems -B /tmp/git-build -DCMAKE_BUILD_TYPE=Debug &&
>          make -C /tmp/git-build &&
>          ctest --test-dir /tmp/git-build -R t0001
> 
> Per [1] and [2] which added the "ctest" support that's the use-case for
> this part of the build: running the tests with ctest, which works as
> before with the default or custom directories.
> 
> Perhaps the reason this has been a sticking point for you is that in
> summarizing this, Johannes's [3] didn't make that distinction between
> running the tests with "ctest" and running them manually by entering the
> "t/" directory after the build. I.e.:

> 	(cd t && ./t0001-init.sh)
> 
> It's only that part which acts differently in this series. I.e. if you
> were to build in /tmp/git-build this would no longer find your built
> assets:
> 
> 	$ ./t0001-init.sh
> 	error: GIT-BUILD-OPTIONS missing (has Git been built?).
> 
> If you just leave it at the default of "contrib/buildsystems/out" it'll
> work:
> 
> 	(cd t && ./t0001-init.sh)
> 	ok 1 [...]
> 
> I think my [4] convincingly makes the case that nobody will
> care. I.e. as the [5] it links to the use-case for running the test
> after the build without ctest was ("[...]" insert is mine):
> 
> 	[To build and test with VS] open the worktree as a folder, and
> 	Visual Studio will find the `CMakeLists.txt` file and
> 	automatically generate the project files.
> 
> I.e. we want to support the user who builds with that method, and runs
> the tests manually. I think you're worrying about an edge case that
> nobody's using in practice.

>> As I mentioned previously [1] I think
>> the non-*nix related patches could do with a review from the windows
>> folks before this hits master.
> 
> I'd welcome another review of it, but at this point it's not for lack of
> waiting for interest from the CC'd Windows people.
> 
> Per the above I don't think any special Windows knowledge is really
> needed, just a reading of the above history & use-cases.
> 
> All of which I've been careful not to break, and which you can now
> simply test on *nix with this series, so no Windows-specific testing is
> needed anymore for this concern you're raising.
> 
> *If* we have someone that's been using this on Windows and e.g. building
> this in /tmp/git-build (or whatever the Windows equivalent) with a
> custom recipe all they'll need to have it work as before is:
> 
> 	GIT_TEST_BUILD_DIR=/tmp/git-build ./t0001-init.sh
> 
> I think nobody's straying off the golden path to do that, but if they
> are doing so and building in some custom directory they're already
> tweaking, just setting an environment variable doesn't seem like a big
> imposition.
> 
> The flip-side of that trade-off is (on both Windows and *nix) that the
> existing way to support the use-case has unintended side-effects, which
> the series improves:
> 
> * When we pick up the not-a-Makefile tree implicitly like this we'll
>    now emit a message telling you what git we're implicitly using.
> 
> * We no longer have edge cases where you can e.g. build with make, then
>    cmake, then run some "make" target that won't go through the path to
>    remove GIT-BUILD-DIR (e.g. changing "git.c" and "make git").
> 
>    Then when you run the tests you'll end up with a different git running
>    it than what you'd expect, i.e. the old stale cmake one.
> 
> Even that hypothetical user who's going to need to set
> "GIT_TEST_BUILD_DIR" would benefit from that, as they'd no longer
> accidentally flip-flop between the two if they ran "make" and wiped away
> the rather fragile link between the source directory & the linked-to
> cmake build directory.
> 
>> [1]
>> https://lore.kernel.org/git/64b91b29-bbcd-e946-1f20-c0a5be63d9b7@dunelm.org.uk/
>>
>>>    Will cook in 'next'.
>>>    source: <cover-v4-00.14-00000000000-20221103T160255Z-avarab@gmail.com>
> 
> 1. c4b2f41b5f5 (cmake: support for testing git with ctest, 2020-06-26)
> 2. 7f5397a07c6 (cmake: support for testing git when building out of the
>     source tree, 2020-06-26)
> 3. ee9e66e4e76 (cmake: avoid editing t/test-lib.sh, 2022-10-18)
> 4. 16a5421a654 (Makefile + cmake: use environment, not GIT-BUILD-DIR,
>     2022-11-03)
> 5. 3eccc7b99d4 (cmake: ignore files generated by CMake as run in Visual
>     Studio, 2020-09-25)