Skip to contentSkip to conversation

Make CMake work out of the box

PATCH Superseded Matthew Rogers via GitGitGadget 8 participants 31 messages Jun 4, 2021 – Jun 22, 2021

lore
Share & tools
lore
#0 lore raw

cover Make CMake work out of the box

This pull request comes from our discussion here[1], and I think these patches provide a good compromise around the concerns discussed there

1:
https://lore.kernel.org/git/CAOjrSZusMSvs7AS-ZDsV8aQUgsF2ZA754vSDjgFKMRgi_oZAWw@mail.gmail.com/

CCing the people involved in the original discussion.

Matthew Rogers (3):
cmake: add knob to disable vcpkg
cmake: create compile_commands.json by default
cmake: add warning for ignored MSGFMT_EXE

 contrib/buildsystems/CMakeLists.txt | 38 ++++++++++++++++++++++-------
 1 file changed, 29 insertions(+), 9 deletions(-)

base-commit: c09b6306c6ca275ed9d0348a8c8014b2ff723cfb
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-970%2FROGERSM94%2Ffix-cmake-v1
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-970/ROGERSM94/fix-cmake-v1
Pull-Request: https://github.com/gitgitgadget/git/pull/970

Signature · 1 line
gitgitgadget
#1 lore raw

PATCH 1/3 cmake: add knob to disable vcpkg

From: Matthew Rogers <[email protected]>

When building on windows users have the option to use vcpkg to provide the dependencies needed to compile. Previously, this was used only when using the Visual Studio generator which was not ideal because:

- Not all users who want to use vcpkg use the Visual Studio generators.

- Some versions of Visual Studio 2019 moved away from using the VS 2019 by default, making it impossible for Visual Studio to configure the project in the likely event that it couldn't find the dependencies.

- Inexperienced users of CMake are very likely to get tripped up by the errors caused by a lack of vcpkg, making the above bullet point both annoying and hard to debug.

As such, lets make using vcpkg the default on windows. Users who want to avoid using vcpkg can disable it by passing -DNO_VCPKG=TRUE.

---
 contrib/buildsystems/CMakeLists.txt | 20 ++++++++++++++++----
 1 file changed, 16 insertions(+), 4 deletions(-)
contrib/buildsystems/CMakeLists.txt +16 -4
index a87841340e6a..41320150bf66 100644
@@ -43,14 +43,24 @@ NOTE: By default CMake uses Makefile as the build tool on Linux and Visual Studi
 to use another tool say `ninja` add this to the command line when configuring.
 `-G Ninja`
 
+NOTE: 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_minimum_required(VERSION 3.14)
 
 #set the source directory to root of git
 set(CMAKE_SOURCE_DIR ${CMAKE_CURRENT_LIST_DIR}/../..)
-if(WIN32)
+
+if (WIN32 AND NOT NO_VCPKG)
+	set(USING_VCPKG TRUE)
+else()
+	set(USING_VCPKG FALSE)
+endif()
+
+if(USING_VCPKG)
 	set(VCPKG_DIR "${CMAKE_SOURCE_DIR}/compat/vcbuild/vcpkg")
-	if(MSVC AND NOT EXISTS ${VCPKG_DIR})
+	if(NOT EXISTS ${VCPKG_DIR})
 		message("Initializing vcpkg and building the Git's dependencies (this will take a while...)")
 		execute_process(COMMAND ${CMAKE_SOURCE_DIR}/compat/vcbuild/vcpkg_install.bat)
 	endif()
@@ -178,7 +188,9 @@ endif()
 
 find_program(MSGFMT_EXE msgfmt)
 if(NOT MSGFMT_EXE)
-	set(MSGFMT_EXE ${CMAKE_SOURCE_DIR}/compat/vcbuild/vcpkg/downloads/tools/msys2/msys64/usr/bin/msgfmt.exe)
+	if (USING_VCPKG)
+		set(MSGFMT_EXE ${CMAKE_SOURCE_DIR}/compat/vcbuild/vcpkg/downloads/tools/msys2/msys64/usr/bin/msgfmt.exe)
+	endif()
 	if(NOT EXISTS ${MSGFMT_EXE})
 		message(WARNING "Text Translations won't be built")
 		unset(MSGFMT_EXE)
@@ -982,7 +994,7 @@ file(APPEND ${CMAKE_BINARY_DIR}/GIT-BUILD-OPTIONS "NO_GETTEXT='${NO_GETTEXT}'\n"
 file(APPEND ${CMAKE_BINARY_DIR}/GIT-BUILD-OPTIONS "RUNTIME_PREFIX='${RUNTIME_PREFIX}'\n")
 file(APPEND ${CMAKE_BINARY_DIR}/GIT-BUILD-OPTIONS "NO_PYTHON='${NO_PYTHON}'\n")
 file(APPEND ${CMAKE_BINARY_DIR}/GIT-BUILD-OPTIONS "SUPPORTS_SIMPLE_IPC='${SUPPORTS_SIMPLE_IPC}'\n")
-if(WIN32)
+if(USING_VCPKG)
 	file(APPEND ${CMAKE_BINARY_DIR}/GIT-BUILD-OPTIONS "PATH=\"$PATH:$TEST_DIRECTORY/../compat/vcbuild/vcpkg/installed/x64-windows/bin\"\n")
 endif()
 
Signature · 1 line
gitgitgadget
#2 lore raw

Re: [PATCH 1/3] cmake: add knob to disable vcpkg

Quoted 11 lines · Matthew Rogers via GitGitGadget

When building on windows users have the option to use vcpkg to provide the dependencies needed to compile. Previously, this was used only when using the Visual Studio generator which was not ideal because:

- Not all users who want to use vcpkg use the Visual Studio generators.

- Some versions of Visual Studio 2019 moved away from using the VS 2019 by default, making it impossible for Visual Studio to configure the project in the likely event that it couldn't find the dependencies.

Is there something missing between "using the" and "VS 2019"? I'm having a hard time trying to understand what this bullet point is saying due to this apparent gap.

Quoted 6 lines

- Inexperienced users of CMake are very likely to get tripped up by the errors caused by a lack of vcpkg, making the above bullet point both annoying and hard to debug.

As such, lets make using vcpkg the default on windows. Users who want to avoid using vcpkg can disable it by passing -DNO_VCPKG=TRUE.

s/lets/let's/

Quoted 1 line
#3 lore raw

Re: [PATCH 1/3] cmake: add knob to disable vcpkg

Quoted 10 lines · Eric Sunshine
Quoted 7 lines · Matthew Rogers via GitGitGadget

When building on windows users have the option to use vcpkg to provide the dependencies needed to compile. Previously, this was used only when using the Visual Studio generator which was not ideal because:

- Not all users who want to use vcpkg use the Visual Studio generators.

Quoted 9 lines
Quoted 4 lines

- Some versions of Visual Studio 2019 moved away from using the VS 2019 by default, making it impossible for Visual Studio to configure the project in the likely event that it couldn't find the dependencies.

Is there something missing between "using the" and "VS 2019"? I'm having a hard time trying to understand what this bullet point is saying due to this apparent gap.

Yeah, this should really read
- Some versions of Visual Studio 2019 moved away from using the VS 2019 _Generator_ by default, making it impossible for Visual Studio to configure the project in the likely event that it couldn't find the dependencies.

Quoted 10 lines
Quoted 6 lines

- Inexperienced users of CMake are very likely to get tripped up by the errors caused by a lack of vcpkg, making the above bullet point both annoying and hard to debug.

As such, lets make using vcpkg the default on windows. Users who want to avoid using vcpkg can disable it by passing -DNO_VCPKG=TRUE.

s/lets/let's/

Quoted 1 line
Signature · 1 line
Matthew Rogers
#4 lore raw

Re: [PATCH 1/3] cmake: add knob to disable vcpkg

In reply to Matthew Rogers via GitGitGadget

contrib/buildsystems/CMakeLists.txt NOTE: By default CMake uses Makefile as the build tool on Linux and Visual Studi · line 59

Quoted 7 lines · Matthew Rogers via GitGitGadget
contrib/buildsystems/CMakeLists.txt +6 -1
-if(WIN32)
+
+if (WIN32 AND NOT NO_VCPKG)
+       set(USING_VCPKG TRUE)
+else()
+       set(USING_VCPKG FALSE)
+endif()

I think it would be better if we could have an option for this knob. Maybe like this

option(NO_VCPKG "Don't use vcpkg for obtaining dependencies. Only applicable to Windows platforms" OFF)

I would prefer to use USE_VCPKG.

Thank You,
Sibi Siddharthan

#5 lore raw

Re: [PATCH 1/3] cmake: add knob to disable vcpkg

Quoted 15 lines · Sibi Siddharthan

contrib/buildsystems/CMakeLists.txt NOTE: By default CMake uses Makefile as the build tool on Linux and Visual Studi · line 59

Quoted 7 lines · Matthew Rogers via GitGitGadget
contrib/buildsystems/CMakeLists.txt +6 -1
-if(WIN32)
+
+if (WIN32 AND NOT NO_VCPKG)
+       set(USING_VCPKG TRUE)
+else()
+       set(USING_VCPKG FALSE)
+endif()

I think it would be better if we could have an option for this knob. Maybe like this

Quoted 2 lines

option(NO_VCPKG "Don't use vcpkg for obtaining dependencies. Only applicable to Windows platforms" OFF)

Option would definitely be the better tool to use here, I just didn't think about
it when originally writing it, so I'll send a reroll with that and the spelling corrections suggested by Eric Sunshine. I assume you'd prefer something with a final form more like:

option(USE_VCPKG "Whether or not to use vcpkg for obtaining dependencies. Only applicable to Windows platforms" ON)

Quoted 5 lines

I would prefer to use USE_VCPKG.

Thank You,
Sibi Siddharthan

Signature · 1 line
Matthew Rogers
#6 lore raw

Re: [PATCH 1/3] cmake: add knob to disable vcpkg

Quoted 21 lines · Matt Rogers
Quoted 15 lines · Sibi Siddharthan

contrib/buildsystems/CMakeLists.txt NOTE: By default CMake uses Makefile as the build tool on Linux and Visual Studi · line 59

Quoted 7 lines · Matthew Rogers via GitGitGadget
contrib/buildsystems/CMakeLists.txt +6 -1
-if(WIN32)
+
+if (WIN32 AND NOT NO_VCPKG)
+       set(USING_VCPKG TRUE)
+else()
+       set(USING_VCPKG FALSE)
+endif()

I think it would be better if we could have an option for this knob. Maybe like this

Quoted 2 lines

option(NO_VCPKG "Don't use vcpkg for obtaining dependencies. Only applicable to Windows platforms" OFF)

Quoted 9 lines

Option would definitely be the better tool to use here, I just didn't think about
it when originally writing it, so I'll send a reroll with that and the spelling corrections suggested by Eric Sunshine. I assume you'd prefer something with a final form more like:

option(USE_VCPKG "Whether or not to use vcpkg for obtaining dependencies. Only applicable to Windows platforms" ON)

Yes, this would be better.

Thank You,
Sibi Siddharthan

#7 lore raw

PATCH 2/3 cmake: create compile_commands.json by default

In reply to Matthew Rogers via GitGitGadget

From: Matthew Rogers <[email protected]>

Some users have expressed interest in a more "batteries included" way of building via CMake[1], and a big part of that is providing easier access to tooling external tools.

A straightforward way to accomplish this is to make it as simple as possible is to enable the generation of the compile_commands.json file, which is supported by many tools such as: clang-tidy, clang-format, sourcetrail, etc.

This does come with a small run-time overhead during the configuration step (~6 seconds on my machine):

    Time to configure with CMAKE_EXPORT_COMPILE_COMMANDS=TRUE

    real    1m9.840s
    user    0m0.031s
    sys     0m0.031s

    Time to configure with CMAKE_EXPORT_COMPILE_COMMANDS=FALSE

    real    1m3.195s
    user    0m0.015s
    sys     0m0.015s

This seems like a small enough price to pay to make the project more accessible to newer users. Additionally there are other large projects like llvm [2] which has had this enabled by default for >6 years at the time of this writing, and no real negative consequences that I can find with my search-skills.

NOTE: That the comppile_commands.json is currenntly produced only when using the Ninja and Makefile generators. See The CMake documentation[3] for more info.

1: https://lore.kernel.org/git/CAOjrSZusMSvs7AS-ZDsV8aQUgsF2ZA754vSDjgFKMRgi_oZAWw@mail.gmail.com/
2: https://github.com/llvm/llvm-project/commit/2c5712051b31b316a9fc972f692579bd8efa6e67
3: https://cmake.org/cmake/help/latest/variable/CMAKE_EXPORT_COMPILE_COMMANDS.html

---
 contrib/buildsystems/CMakeLists.txt | 4 ++++
 1 file changed, 4 insertions(+)
contrib/buildsystems/CMakeLists.txt +4 -0
index 41320150bf66..99150c8f5853 100644
@@ -58,6 +58,10 @@ else()
 	set(USING_VCPKG FALSE)
 endif()
 
+if (NOT DEFINED CMAKE_EXPORT_COMPILE_COMMANDS)
+	SET(CMAKE_EXPORT_COMPILE_COMMANDS TRUE)
+endif()
+
 if(USING_VCPKG)
 	set(VCPKG_DIR "${CMAKE_SOURCE_DIR}/compat/vcbuild/vcpkg")
 	if(NOT EXISTS ${VCPKG_DIR})
Signature · 1 line
gitgitgadget
#8 lore raw

Re: [PATCH 2/3] cmake: create compile_commands.json by default

Quoted 18 lines · Matthew Rogers via GitGitGadget

Some users have expressed interest in a more "batteries included" way of building via CMake[1], and a big part of that is providing easier access to tooling external tools.

A straightforward way to accomplish this is to make it as simple as possible is to enable the generation of the compile_commands.json file, which is supported by many tools such as: clang-tidy, clang-format, sourcetrail, etc.

This does come with a small run-time overhead during the configuration step (~6 seconds on my machine):

    [...]

This seems like a small enough price to pay to make the project more accessible to newer users. Additionally there are other large projects like llvm [2] which has had this enabled by default for >6 years at the time of this writing, and no real negative consequences that I can find with my search-skills.

Quoted 3 lines

NOTE: That the comppile_commands.json is currenntly produced only when using the Ninja and Makefile generators. See The CMake documentation[3] for more info.

s/comppile/compile/
s/currenntly/currently/

Quoted 1 line
#9 lore raw

Re: [PATCH 2/3] cmake: create compile_commands.json by default

In reply to Matthew Rogers via GitGitGadget
Quoted 21 lines · Matthew Rogers via GitGitGadget

A straightforward way to accomplish this is to make it as simple as possible is to enable the generation of the compile_commands.json file, which is supported by many tools such as: clang-tidy, clang-format, sourcetrail, etc.

This does come with a small run-time overhead during the configuration step (~6 seconds on my machine):

    Time to configure with CMAKE_EXPORT_COMPILE_COMMANDS=TRUE

    real    1m9.840s
    user    0m0.031s
    sys     0m0.031s

    Time to configure with CMAKE_EXPORT_COMPILE_COMMANDS=FALSE

    real    1m3.195s
    user    0m0.015s
    sys     0m0.015s
Quoted 6 lines

This seems like a small enough price to pay to make the project more accessible to newer users. Additionally there are other large projects like llvm [2] which has had this enabled by default for >6 years at the time of this writing, and no real negative consequences that I can find with my search-skills.

The overhead is actually much smaller than that. In my system it is less than 150ms.
The first configure takes this long because we generate command-list.h and config-list.h.
This process is really slow under Windows.

Thank You,
Sibi Siddharthan

#10 lore raw

Re: [PATCH 2/3] cmake: create compile_commands.json by default

Quoted 31 lines · Sibi Siddharthan
Quoted 21 lines · Matthew Rogers via GitGitGadget

A straightforward way to accomplish this is to make it as simple as possible is to enable the generation of the compile_commands.json file, which is supported by many tools such as: clang-tidy, clang-format, sourcetrail, etc.

This does come with a small run-time overhead during the configuration step (~6 seconds on my machine):

    Time to configure with CMAKE_EXPORT_COMPILE_COMMANDS=TRUE

    real    1m9.840s
    user    0m0.031s
    sys     0m0.031s

    Time to configure with CMAKE_EXPORT_COMPILE_COMMANDS=FALSE

    real    1m3.195s
    user    0m0.015s
    sys     0m0.015s
Quoted 6 lines

This seems like a small enough price to pay to make the project more accessible to newer users. Additionally there are other large projects like llvm [2] which has had this enabled by default for >6 years at the time of this writing, and no real negative consequences that I can find with my search-skills.

Quoted 2 lines

The overhead is actually much smaller than that. In my system it is less than 150ms.

Is that 150 ms for the whole process or just the difference between the two options? I'm running this on windows via the git bash provided by the git sdk.

Quoted 4 lines

The first configure takes this long because we generate command-list.h and config-list.h.
This process is really slow under Windows.

I used two different build directories for both my invocations specifically to avoid having to account for cache variables and other side effects from earlier configurations. The variation could also be from network latency since in this test I was downloading vcpkg, etc.

Quoted 2 lines

Thank You,
Sibi Siddharthan

Signature · 1 line
Matthew Rogers
#11 lore raw

Re: [PATCH 2/3] cmake: create compile_commands.json by default

Quoted 34 lines · Matt Rogers
Quoted 31 lines · Sibi Siddharthan
Quoted 27 lines · Matthew Rogers via GitGitGadget

A straightforward way to accomplish this is to make it as simple as possible is to enable the generation of the compile_commands.json file, which is supported by many tools such as: clang-tidy, clang-format, sourcetrail, etc.

This does come with a small run-time overhead during the configuration step (~6 seconds on my machine):

    Time to configure with CMAKE_EXPORT_COMPILE_COMMANDS=TRUE

    real    1m9.840s
    user    0m0.031s
    sys     0m0.031s

    Time to configure with CMAKE_EXPORT_COMPILE_COMMANDS=FALSE

    real    1m3.195s
    user    0m0.015s
    sys     0m0.015s

This seems like a small enough price to pay to make the project more accessible to newer users. Additionally there are other large projects like llvm [2] which has had this enabled by default for >6 years at the time of this writing, and no real negative consequences that I can find with my search-skills.

Quoted 6 lines
Quoted 2 lines

The overhead is actually much smaller than that. In my system it is less than 150ms.

Is that 150 ms for the whole process or just the difference between the two options? I'm running this on windows via the git bash provided by the git sdk.

The difference between the two. Without exporting compile_commands.json it takes around 650ms, with it around 750ms.
NOTE: This is for subsequent CMake runs. (Excludes the initial run)

Thank You,
Sibi Siddharthan

#12 lore raw

PATCH 3/3 cmake: add warning for ignored MSGFMT_EXE

In reply to Matthew Rogers via GitGitGadget

From: Matthew Rogers <[email protected]>

It does not make sense to attempt to set MSGFMT_EXE when NO_GETTEXT is configured, as such add a check for NO_GETTEXT before attempting to set it.

---
 contrib/buildsystems/CMakeLists.txt | 20 ++++++++++++--------
 1 file changed, 12 insertions(+), 8 deletions(-)
contrib/buildsystems/CMakeLists.txt +12 -8
index 99150c8f5853..ea43a4f9cc9f 100644
@@ -190,14 +190,18 @@ if(WIN32 AND NOT MSVC)#not required for visual studio builds
 	endif()
 endif()
 
-find_program(MSGFMT_EXE msgfmt)
-if(NOT MSGFMT_EXE)
-	if (USING_VCPKG)
-		set(MSGFMT_EXE ${CMAKE_SOURCE_DIR}/compat/vcbuild/vcpkg/downloads/tools/msys2/msys64/usr/bin/msgfmt.exe)
-	endif()
-	if(NOT EXISTS ${MSGFMT_EXE})
-		message(WARNING "Text Translations won't be built")
-		unset(MSGFMT_EXE)
+if(NO_GETTEXT)
+	message(STATUS "msgfmt not used under NO_GETTEXT")
+else()
+	find_program(MSGFMT_EXE msgfmt)
+	if(NOT MSGFMT_EXE)
+		if (USING_VCPKG)
+			set(MSGFMT_EXE ${CMAKE_SOURCE_DIR}/compat/vcbuild/vcpkg/downloads/tools/msys2/msys64/usr/bin/msgfmt.exe)
+		endif()
+		if(NOT EXISTS ${MSGFMT_EXE})
+			message(WARNING "Text Translations won't be built")
+			unset(MSGFMT_EXE)
+		endif()
 	endif()
 endif()
 
Signature · 1 line
gitgitgadget
#13 lore raw

Re: [PATCH 3/3] cmake: add warning for ignored MSGFMT_EXE

Quoted 3 lines · Matthew Rogers via GitGitGadget

It does not make sense to attempt to set MSGFMT_EXE when NO_GETTEXT is configured, as such add a check for NO_GETTEXT before attempting to set it.

This would be easier to digest if "as such" is the start of a new
sentence: "As such...". Or "Therefore, add a check...".

Quoted 2 lines

s/suggested-by/Suggested-by:/

Tiny little nits, both. Don't know if it's worth a re-roll, but if you happen to re-roll for some other reason, perhaps these could be tweaked.

#14 lore raw
In reply to Matthew Rogers via GitGitGadget

Hi,

Quoted 7 lines · Matthew Rogers via GitGitGadget

This pull request comes from our discussion here[1], and I think these patches provide a good compromise around the concerns discussed there

1:
https://lore.kernel.org/git/CAOjrSZusMSvs7AS-ZDsV8aQUgsF2ZA754vSDjgFKMRgi_oZAWw@mail.gmail.com/

CCing the people involved in the original discussion.

This focused on improving CMake support, especially on Visual Studio, right?

Then so we have three ways to build Git:
1. plain Makefile
2. ./configure (really just wrapper on top of Makefile)
3. generate build file with CMake

If we want to support all of them, it may makes sense to have CI jobs that perform build with each options above.

Signature · 1 line
An old man doll... just what I always wanted! - Clara
#15 lore raw
Quoted 19 lines · Bagas Sanjaya

Hi,

Quoted 7 lines · Matthew Rogers via GitGitGadget

This pull request comes from our discussion here[1], and I think these patches provide a good compromise around the concerns discussed there

1:
https://lore.kernel.org/git/CAOjrSZusMSvs7AS-ZDsV8aQUgsF2ZA754vSDjgFKMRgi_oZAWw@mail.gmail.com/

CCing the people involved in the original discussion.

This focused on improving CMake support, especially on Visual Studio, right?

Then so we have three ways to build Git:
1. plain Makefile
2. ./configure (really just wrapper on top of Makefile)
3. generate build file with CMake

Quoted 5 lines

If we want to support all of them, it may makes sense to have CI jobs that perform build with each options above.

--
An old man doll... just what I always wanted! - Clara

Here's my understanding of the current pipeline situation:

I know the Visual Studio CMake generator is currently used to build on Windows for gitgitgadget[1].

I'm not sure how worth it it would be to add another pipeline just to test if we correctly set EXPORT_COMPILE_COMMANDS=TRUE on by default correctly.

I think adding support for running cmake builds on linux is a bit of a waste since those platforms should have ready access to make, and that's the build method that gets the official support.

I don't really have much more of a position on this other than "Probably not worth it to add a cmake build on linux"

1: https://github.com/gitgitgadget/git/runs/2748313673

--
Matthew Rogers

#16 lore raw
In reply to Bagas Sanjaya

Hi,

Quoted 11 lines · Bagas Sanjaya
Quoted 7 lines · Matthew Rogers via GitGitGadget

This pull request comes from our discussion here[1], and I think these patches provide a good compromise around the concerns discussed there

1:
https://lore.kernel.org/git/CAOjrSZusMSvs7AS-ZDsV8aQUgsF2ZA754vSDjgFKMRgi_oZAWw@mail.gmail.com/

CCing the people involved in the original discussion.

This focused on improving CMake support, especially on Visual Studio, right?

Quoted 7 lines

Then so we have three ways to build Git:
1. plain Makefile
2. ./configure (really just wrapper on top of Makefile)
3. generate build file with CMake

If we want to support all of them, it may makes sense to have CI jobs that perform build with each options above.

We already exercise the plain Makefile plenty, and the CMake-based build using Windows (in the vs-build job in .github/workflows/main.yml).

I do not see that it is worth spending many electrons exercising the ./configure way, seeing as the preferred way to build Git is by using the Makefile directly.

And our CMake configuration only really works on Windows, the attempts to get it to work on Linux were met with less enthusiasm, seeing as the Makefile approach is the recommended (and supported) one.

tl;dr I don't think we need to augment our CI jobs as suggested.

Ciao,
Dscho

#17 lore raw
Quoted 12 lines · Johannes Schindelin

Hi,

Quoted 8 lines · Bagas Sanjaya
Quoted 7 lines · Matthew Rogers via GitGitGadget

This pull request comes from our discussion here[1], and I think these patches provide a good compromise around the concerns discussed there

1:
https://lore.kernel.org/git/CAOjrSZusMSvs7AS-ZDsV8aQUgsF2ZA754vSDjgFKMRgi_oZAWw@mail.gmail.com/

CCing the people involved in the original discussion.

Matt,
Thanks for picking this up and the approach to working around the updated build approach of recent Visual Studio versions.

It looks good to me, but the CI should also be tweaked (see below) so that it is tested.

Quoted 11 lines
Quoted 9 lines

This focused on improving CMake support, especially on Visual Studio, right?

Then so we have three ways to build Git:
1. plain Makefile
2. ./configure (really just wrapper on top of Makefile)
3. generate build file with CMake

If we want to support all of them, it may makes sense to have CI jobs that perform build with each options above.

We already exercise the plain Makefile plenty, and the CMake-based build using Windows (in the vs-build job in .github/workflows/main.yml).

There is one 'gotcha' in the yml (probably historical) in that it doesn't actually test the approach/changes that Matt addresses regarding my [1].

That is, I'm looking at the 'out of the box' view, while the yml test _preloads_ the vcpkg artefacts.

There is also the (on Windows) issue that the ARM support has recently been developed which also fudges the CmakeLists.txt file but forgot about the assumption in the vcpkg install batch file that the default is the x86 setup.

Quoted 10 lines

I do not see that it is worth spending many electrons exercising the ./configure way, seeing as the preferred way to build Git is by using the Makefile directly.

And our CMake configuration only really works on Windows, the attempts to get it to work on Linux were met with less enthusiasm, seeing as the Makefile approach is the recommended (and supported) one.

tl;dr I don't think we need to augment our CI jobs as suggested.

I'd agree that there's no need to augment the CI job to expressly check the other flags, but the existing test should reflect the intent of the patches (i.e. no preloading of the vcpkg artefacts).

I haven't had much time to catch up on Git, and I'm off-line again from Sat night for another week.

#18 lore raw

Hi Philip,

Quoted 17 lines · Philip Oakley
Quoted 10 lines · Johannes Schindelin
Quoted 7 lines · Bagas Sanjaya

This focused on improving CMake support, especially on Visual Studio, right?

Then so we have three ways to build Git:
1. plain Makefile
2. ./configure (really just wrapper on top of Makefile)
3. generate build file with CMake

Quoted 5 lines
Quoted 2 lines

If we want to support all of them, it may makes sense to have CI jobs that perform build with each options above.

We already exercise the plain Makefile plenty, and the CMake-based build using Windows (in the vs-build job in .github/workflows/main.yml).

Quoted 6 lines

There is one 'gotcha' in the yml (probably historical) in that it doesn't actually test the approach/changes that Matt addresses regarding my [1].

That is, I'm looking at the 'out of the box' view, while the yml test _preloads_ the vcpkg artefacts.

We need to "pre-load" them because building them would add another whopping 20 minutes to each CI run. And I am not talking total time, but wall-clock time.

And we're not in the business of testing vcpkg's build.

So I am really not in favor of even thinking about changing this "pre-loading" strategy.

Ciao,
Dscho

#19 lore raw

Hi Dscho

Quoted 8 lines · Johannes Schindelin
Quoted 8 lines
Quoted 2 lines

We already exercise the plain Makefile plenty, and the CMake-based build using Windows (in the vs-build job in .github/workflows/main.yml).

There is one 'gotcha' in the yml (probably historical) in that it doesn't actually test the approach/changes that Matt addresses regarding my [1].

That is, I'm looking at the 'out of the box' view, while the yml test _preloads_ the vcpkg artefacts.

Quoted 10 lines

We need to "pre-load" them because building them would add another whopping 20 minutes to each CI run. And I am not talking total time, but wall-clock time.

And we're not in the business of testing vcpkg's build.

So I am really not in favor of even thinking about changing this "pre-loading" strategy.

I can see the common sense in that, however I was trying to highlight that the approach in patch series could go stale, as did the previous method. Making the entry ramp to investigating the code for the wide variety windows users should have _some_ testing..

I don't have any good ideas about how to get out of that 20 minute Catch-22 issue at the moment. Maybe it needs an independent, on-demand (i.e. infrequent;-) test.

Maybe there is a way of adding a --CI-test option that at least exercises the logic without needing the vcpkg to be built again (IIRC, and I may well be wrong, we build once, remember the artefacts, and then re-used them, but .. dunno).

Philip

#20 lore raw

Hi Philip,

Quoted 24 lines · Philip Oakley
Quoted 8 lines · Johannes Schindelin
Quoted 8 lines
Quoted 2 lines

We already exercise the plain Makefile plenty, and the CMake-based build using Windows (in the vs-build job in .github/workflows/main.yml).

There is one 'gotcha' in the yml (probably historical) in that it doesn't actually test the approach/changes that Matt addresses regarding my [1].

That is, I'm looking at the 'out of the box' view, while the yml test _preloads_ the vcpkg artefacts.

Quoted 10 lines

We need to "pre-load" them because building them would add another whopping 20 minutes to each CI run. And I am not talking total time, but wall-clock time.

And we're not in the business of testing vcpkg's build.

So I am really not in favor of even thinking about changing this "pre-loading" strategy.

I can see the common sense in that, however I was trying to highlight that the approach in patch series could go stale, as did the previous method. Making the entry ramp to investigating the code for the wide variety windows users should have _some_ testing..

Quoted 8 lines

I don't have any good ideas about how to get out of that 20 minute Catch-22 issue at the moment. Maybe it needs an independent, on-demand (i.e. infrequent;-) test.

Maybe there is a way of adding a --CI-test option that at least exercises the logic without needing the vcpkg to be built again (IIRC, and I may well be wrong, we build once, remember the artefacts, and then re-used them, but .. dunno).

I would strongly discourage tacking this onto the current CI. It is way too rare a use case to merit adding the cost for all developers using the CI runs to verify their work.

All is not lost, though: interested parties (such as yourself!) can easily add their own GitHub workflows in their own repositories and verify that things work.

You could even put the workflow on a timer, and add a matrix job that builds maint, master, next and seen, to verify that things work.

And for extra brownie points, you can monitor the runs and work on fixes whenever you see breakages. That would definitely take a good chunk of the maintenance burden off of the Git maintainers.

Ciao,
Dscho

#21 lore raw

v2 cover Make CMake work out of the box

In reply to Matthew Rogers via GitGitGadget

This pull request comes from our discussion here[1], and I think these patches provide a good compromise around the concerns discussed there

1:
https://lore.kernel.org/git/CAOjrSZusMSvs7AS-ZDsV8aQUgsF2ZA754vSDjgFKMRgi_oZAWw@mail.gmail.com/

CCing the people involved in the original discussion. cc: Philip Oakley [email protected] cc: Sibi Siddharthan [email protected], cc: Johannes Schindelin [email protected], cc: Danh Doan [email protected]

Matthew Rogers (3):
cmake: add knob to disable vcpkg
cmake: create compile_commands.json by default
cmake: add warning for ignored MSGFMT_EXE

 contrib/buildsystems/CMakeLists.txt | 37 ++++++++++++++++++++++-------
 1 file changed, 28 insertions(+), 9 deletions(-)

base-commit: c09b6306c6ca275ed9d0348a8c8014b2ff723cfb
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-970%2FROGERSM94%2Ffix-cmake-v2
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-970/ROGERSM94/fix-cmake-v2
Pull-Request: https://github.com/gitgitgadget/git/pull/970

Range-diff vs v1:

1: 3170f78daa5f ! 1: 485254b49de8 cmake: add knob to disable vcpkg

     @@ Commit message
              generators.
      
            - Some versions of Visual Studio 2019 moved away from using the
     -        VS 2019 by default, making it impossible for Visual Studio to
     -        configure the project in the likely event that it couldn't find the
     -        dependencies.
     +        VS 2019  generator by default, making it impossible for Visual
     +        Studio to configure the project in the likely event that it couldn't
     +        find the dependencies.
      
            - Inexperienced users of CMake are very likely to get tripped up by
              the errors caused by a lack of vcpkg, making the above bullet point
              both annoying and hard to debug.
      
     -    As such, lets make using vcpkg the default on windows.  Users who want
     +    As such, let's make using vcpkg the default on windows.  Users who want
          to avoid using vcpkg can disable it by passing -DNO_VCPKG=TRUE.
      
          Signed-off-by: Matthew Rogers <[email protected]>
     @@ contrib/buildsystems/CMakeLists.txt: NOTE: By default CMake uses Makefile as the
       set(CMAKE_SOURCE_DIR ${CMAKE_CURRENT_LIST_DIR}/../..)
      -if(WIN32)
      +
     -+if (WIN32 AND NOT NO_VCPKG)
     -+	set(USING_VCPKG TRUE)
     -+else()
     -+	set(USING_VCPKG FALSE)
     ++option(USE_VCPKG "Whether or not to use vcpkg for obtaining dependencies.  Only applicable to Windows platforms" ON)
     ++if(NOT WIN32)
     ++	set(USE_VCPKG OFF CACHE BOOL FORCE)
      +endif()
      +
     -+if(USING_VCPKG)
     ++if(USE_VCPKG)
       	set(VCPKG_DIR "${CMAKE_SOURCE_DIR}/compat/vcbuild/vcpkg")
      -	if(MSVC AND NOT EXISTS ${VCPKG_DIR})
      +	if(NOT EXISTS ${VCPKG_DIR})
     @@ contrib/buildsystems/CMakeLists.txt: endif()
       find_program(MSGFMT_EXE msgfmt)
       if(NOT MSGFMT_EXE)
      -	set(MSGFMT_EXE ${CMAKE_SOURCE_DIR}/compat/vcbuild/vcpkg/downloads/tools/msys2/msys64/usr/bin/msgfmt.exe)
     -+	if (USING_VCPKG)
     ++	if (USE_VCPKG)
      +		set(MSGFMT_EXE ${CMAKE_SOURCE_DIR}/compat/vcbuild/vcpkg/downloads/tools/msys2/msys64/usr/bin/msgfmt.exe)
      +	endif()
       	if(NOT EXISTS ${MSGFMT_EXE})
     @@ contrib/buildsystems/CMakeLists.txt: file(APPEND ${CMAKE_BINARY_DIR}/GIT-BUILD-O
       file(APPEND ${CMAKE_BINARY_DIR}/GIT-BUILD-OPTIONS "NO_PYTHON='${NO_PYTHON}'\n")
       file(APPEND ${CMAKE_BINARY_DIR}/GIT-BUILD-OPTIONS "SUPPORTS_SIMPLE_IPC='${SUPPORTS_SIMPLE_IPC}'\n")
      -if(WIN32)
     -+if(USING_VCPKG)
     ++if(USE_VCPKG)
       	file(APPEND ${CMAKE_BINARY_DIR}/GIT-BUILD-OPTIONS "PATH=\"$PATH:$TEST_DIRECTORY/../compat/vcbuild/vcpkg/installed/x64-windows/bin\"\n")
       endif()

2: c3bf266cf03a ! 2: a3b5eef54188 cmake: create compile_commands.json by default
@@ Commit message
time of this writing, and no real negative consequences that I can find with my search-skills.

- NOTE: That the comppile_commands.json is currenntly produced only when
+ NOTE: That the compile_commands.json is currently produced only when
using the Ninja and Makefile generators. See The CMake documentation[3] for more info.

@@ Commit message

## contrib/buildsystems/CMakeLists.txt ##
-@@ contrib/buildsystems/CMakeLists.txt: else()
- set(USING_VCPKG FALSE)
+@@ contrib/buildsystems/CMakeLists.txt: if(NOT WIN32)
+ set(USE_VCPKG OFF CACHE BOOL FORCE)
endif()

-+if (NOT DEFINED CMAKE_EXPORT_COMPILE_COMMANDS)
-+ SET(CMAKE_EXPORT_COMPILE_COMMANDS TRUE)
++if(NOT DEFINED CMAKE_EXPORT_COMPILE_COMMANDS)
++ set(CMAKE_EXPORT_COMPILE_COMMANDS TRUE)
+endif()
+
- if(USING_VCPKG)
+ if(USE_VCPKG)
set(VCPKG_DIR "${CMAKE_SOURCE_DIR}/compat/vcbuild/vcpkg") if(NOT EXISTS ${VCPKG_DIR})
3: 07763a9de723 ! 3: 2110c8ffa423 cmake: add warning for ignored MSGFMT_EXE

     @@ Commit message
          configured, as such add a check for NO_GETTEXT before attempting to set
          it.
      
     -    suggested-by: Johannes Schindelin <[email protected]>
     +    Suggested-by: Johannes Schindelin <[email protected]>
          Signed-off-by: Matthew Rogers <[email protected]>
      
       ## contrib/buildsystems/CMakeLists.txt ##
     @@ contrib/buildsystems/CMakeLists.txt: if(WIN32 AND NOT MSVC)#not required for vis
       
      -find_program(MSGFMT_EXE msgfmt)
      -if(NOT MSGFMT_EXE)
     --	if (USING_VCPKG)
     +-	if (USE_VCPKG)
      -		set(MSGFMT_EXE ${CMAKE_SOURCE_DIR}/compat/vcbuild/vcpkg/downloads/tools/msys2/msys64/usr/bin/msgfmt.exe)
      -	endif()
      -	if(NOT EXISTS ${MSGFMT_EXE})
     @@ contrib/buildsystems/CMakeLists.txt: if(WIN32 AND NOT MSVC)#not required for vis
      +else()
      +	find_program(MSGFMT_EXE msgfmt)
      +	if(NOT MSGFMT_EXE)
     -+		if (USING_VCPKG)
     ++		if(USE_VCPKG)
      +			set(MSGFMT_EXE ${CMAKE_SOURCE_DIR}/compat/vcbuild/vcpkg/downloads/tools/msys2/msys64/usr/bin/msgfmt.exe)
      +		endif()
      +		if(NOT EXISTS ${MSGFMT_EXE})
Signature · 1 line
gitgitgadget
#22 lore raw

PATCH v2 1/3 cmake: add knob to disable vcpkg

From: Matthew Rogers <[email protected]>

When building on windows users have the option to use vcpkg to provide the dependencies needed to compile. Previously, this was used only when using the Visual Studio generator which was not ideal because:

- Not all users who want to use vcpkg use the Visual Studio generators.

- Some versions of Visual Studio 2019 moved away from using the VS 2019 generator by default, making it impossible for Visual Studio to configure the project in the likely event that it couldn't find the dependencies.

- Inexperienced users of CMake are very likely to get tripped up by the errors caused by a lack of vcpkg, making the above bullet point both annoying and hard to debug.

As such, let's make using vcpkg the default on windows. Users who want to avoid using vcpkg can disable it by passing -DNO_VCPKG=TRUE.

---
 contrib/buildsystems/CMakeLists.txt | 19 +++++++++++++++----
 1 file changed, 15 insertions(+), 4 deletions(-)
contrib/buildsystems/CMakeLists.txt +15 -4
index a87841340e6a..be6d9659c387 100644
@@ -43,14 +43,23 @@ NOTE: By default CMake uses Makefile as the build tool on Linux and Visual Studi
 to use another tool say `ninja` add this to the command line when configuring.
 `-G Ninja`
 
+NOTE: 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_minimum_required(VERSION 3.14)
 
 #set the source directory to root of git
 set(CMAKE_SOURCE_DIR ${CMAKE_CURRENT_LIST_DIR}/../..)
-if(WIN32)
+
+option(USE_VCPKG "Whether or not to use vcpkg for obtaining dependencies.  Only applicable to Windows platforms" ON)
+if(NOT WIN32)
+	set(USE_VCPKG OFF CACHE BOOL FORCE)
+endif()
+
+if(USE_VCPKG)
 	set(VCPKG_DIR "${CMAKE_SOURCE_DIR}/compat/vcbuild/vcpkg")
-	if(MSVC AND NOT EXISTS ${VCPKG_DIR})
+	if(NOT EXISTS ${VCPKG_DIR})
 		message("Initializing vcpkg and building the Git's dependencies (this will take a while...)")
 		execute_process(COMMAND ${CMAKE_SOURCE_DIR}/compat/vcbuild/vcpkg_install.bat)
 	endif()
@@ -178,7 +187,9 @@ endif()
 
 find_program(MSGFMT_EXE msgfmt)
 if(NOT MSGFMT_EXE)
-	set(MSGFMT_EXE ${CMAKE_SOURCE_DIR}/compat/vcbuild/vcpkg/downloads/tools/msys2/msys64/usr/bin/msgfmt.exe)
+	if (USE_VCPKG)
+		set(MSGFMT_EXE ${CMAKE_SOURCE_DIR}/compat/vcbuild/vcpkg/downloads/tools/msys2/msys64/usr/bin/msgfmt.exe)
+	endif()
 	if(NOT EXISTS ${MSGFMT_EXE})
 		message(WARNING "Text Translations won't be built")
 		unset(MSGFMT_EXE)
@@ -982,7 +993,7 @@ file(APPEND ${CMAKE_BINARY_DIR}/GIT-BUILD-OPTIONS "NO_GETTEXT='${NO_GETTEXT}'\n"
 file(APPEND ${CMAKE_BINARY_DIR}/GIT-BUILD-OPTIONS "RUNTIME_PREFIX='${RUNTIME_PREFIX}'\n")
 file(APPEND ${CMAKE_BINARY_DIR}/GIT-BUILD-OPTIONS "NO_PYTHON='${NO_PYTHON}'\n")
 file(APPEND ${CMAKE_BINARY_DIR}/GIT-BUILD-OPTIONS "SUPPORTS_SIMPLE_IPC='${SUPPORTS_SIMPLE_IPC}'\n")
-if(WIN32)
+if(USE_VCPKG)
 	file(APPEND ${CMAKE_BINARY_DIR}/GIT-BUILD-OPTIONS "PATH=\"$PATH:$TEST_DIRECTORY/../compat/vcbuild/vcpkg/installed/x64-windows/bin\"\n")
 endif()
 
Signature · 1 line
gitgitgadget
#23 lore raw

PATCH v2 2/3 cmake: create compile_commands.json by default

In reply to Matthew Rogers via GitGitGadget

From: Matthew Rogers <[email protected]>

Some users have expressed interest in a more "batteries included" way of building via CMake[1], and a big part of that is providing easier access to tooling external tools.

A straightforward way to accomplish this is to make it as simple as possible is to enable the generation of the compile_commands.json file, which is supported by many tools such as: clang-tidy, clang-format, sourcetrail, etc.

This does come with a small run-time overhead during the configuration step (~6 seconds on my machine):

    Time to configure with CMAKE_EXPORT_COMPILE_COMMANDS=TRUE

    real    1m9.840s
    user    0m0.031s
    sys     0m0.031s

    Time to configure with CMAKE_EXPORT_COMPILE_COMMANDS=FALSE

    real    1m3.195s
    user    0m0.015s
    sys     0m0.015s

This seems like a small enough price to pay to make the project more accessible to newer users. Additionally there are other large projects like llvm [2] which has had this enabled by default for >6 years at the time of this writing, and no real negative consequences that I can find with my search-skills.

NOTE: That the compile_commands.json is currently produced only when using the Ninja and Makefile generators. See The CMake documentation[3] for more info.

1: https://lore.kernel.org/git/CAOjrSZusMSvs7AS-ZDsV8aQUgsF2ZA754vSDjgFKMRgi_oZAWw@mail.gmail.com/
2: https://github.com/llvm/llvm-project/commit/2c5712051b31b316a9fc972f692579bd8efa6e67
3: https://cmake.org/cmake/help/latest/variable/CMAKE_EXPORT_COMPILE_COMMANDS.html

---
 contrib/buildsystems/CMakeLists.txt | 4 ++++
 1 file changed, 4 insertions(+)
contrib/buildsystems/CMakeLists.txt +4 -0
index be6d9659c387..399a3cd6c071 100644
@@ -57,6 +57,10 @@ if(NOT WIN32)
 	set(USE_VCPKG OFF CACHE BOOL FORCE)
 endif()
 
+if(NOT DEFINED CMAKE_EXPORT_COMPILE_COMMANDS)
+	set(CMAKE_EXPORT_COMPILE_COMMANDS TRUE)
+endif()
+
 if(USE_VCPKG)
 	set(VCPKG_DIR "${CMAKE_SOURCE_DIR}/compat/vcbuild/vcpkg")
 	if(NOT EXISTS ${VCPKG_DIR})
Signature · 1 line
gitgitgadget
#24 lore raw

PATCH v2 3/3 cmake: add warning for ignored MSGFMT_EXE

In reply to Matthew Rogers via GitGitGadget

From: Matthew Rogers <[email protected]>

It does not make sense to attempt to set MSGFMT_EXE when NO_GETTEXT is configured, as such add a check for NO_GETTEXT before attempting to set it.

---
 contrib/buildsystems/CMakeLists.txt | 20 ++++++++++++--------
 1 file changed, 12 insertions(+), 8 deletions(-)
contrib/buildsystems/CMakeLists.txt +12 -8
index 399a3cd6c071..3dc7ffcd98bb 100644
@@ -189,14 +189,18 @@ if(WIN32 AND NOT MSVC)#not required for visual studio builds
 	endif()
 endif()
 
-find_program(MSGFMT_EXE msgfmt)
-if(NOT MSGFMT_EXE)
-	if (USE_VCPKG)
-		set(MSGFMT_EXE ${CMAKE_SOURCE_DIR}/compat/vcbuild/vcpkg/downloads/tools/msys2/msys64/usr/bin/msgfmt.exe)
-	endif()
-	if(NOT EXISTS ${MSGFMT_EXE})
-		message(WARNING "Text Translations won't be built")
-		unset(MSGFMT_EXE)
+if(NO_GETTEXT)
+	message(STATUS "msgfmt not used under NO_GETTEXT")
+else()
+	find_program(MSGFMT_EXE msgfmt)
+	if(NOT MSGFMT_EXE)
+		if(USE_VCPKG)
+			set(MSGFMT_EXE ${CMAKE_SOURCE_DIR}/compat/vcbuild/vcpkg/downloads/tools/msys2/msys64/usr/bin/msgfmt.exe)
+		endif()
+		if(NOT EXISTS ${MSGFMT_EXE})
+			message(WARNING "Text Translations won't be built")
+			unset(MSGFMT_EXE)
+		endif()
 	endif()
 endif()
 
Signature · 1 line
gitgitgadget
#25 lore raw
In reply to Matthew Rogers via GitGitGadget
Quoted 11 lines · Matthew Rogers via GitGitGadget

This pull request comes from our discussion here[1], and I think these patches provide a good compromise around the concerns discussed there

1:
https://lore.kernel.org/git/CAOjrSZusMSvs7AS-ZDsV8aQUgsF2ZA754vSDjgFKMRgi_oZAWw@mail.gmail.com/

CCing the people involved in the original discussion. cc: Philip Oakley [email protected] cc: Sibi Siddharthan [email protected], cc: Johannes Schindelin [email protected], cc: Danh Doan [email protected]

Quoted 4 lines

Matthew Rogers (3):
cmake: add knob to disable vcpkg
cmake: create compile_commands.json by default
cmake: add warning for ignored MSGFMT_EXE

I am neither cmake nor windows person, so I'll queue this as-is and wait for the stakeholders to chime in.

I did wonder if we want this to be applicable to the maintenance track for 2.31, though. There is a textual conflict with the addition of SIMPLE_IPC that happened during 2.32 cycle, which is easily resolvable.

I am tempted to queue a version of these three patches rebased on to 'maint' after making sure that the result of merging that into 'master' is byte-for-byte identical to applying these three patches directly on to 'master'.

The range-diff looks like the attached. Thanks.

1: 546c49cc88 ! 1: 585b7ca371 cmake: add knob to disable vcpkg

    @@ contrib/buildsystems/CMakeLists.txt: endif()
      	if(NOT EXISTS ${MSGFMT_EXE})
      		message(WARNING "Text Translations won't be built")
      		unset(MSGFMT_EXE)
    -@@ contrib/buildsystems/CMakeLists.txt: file(APPEND ${CMAKE_BINARY_DIR}/GIT-BUILD-OPTIONS "X='${EXE_EXTENSION}'\n")
    - file(APPEND ${CMAKE_BINARY_DIR}/GIT-BUILD-OPTIONS "NO_GETTEXT='${NO_GETTEXT}'\n")
    +@@ contrib/buildsystems/CMakeLists.txt: file(APPEND ${CMAKE_BINARY_DIR}/GIT-BUILD-OPTIONS "NO_GETTEXT='${NO_GETTEXT}'\n"
      file(APPEND ${CMAKE_BINARY_DIR}/GIT-BUILD-OPTIONS "RUNTIME_PREFIX='${RUNTIME_PREFIX}'\n")
      file(APPEND ${CMAKE_BINARY_DIR}/GIT-BUILD-OPTIONS "NO_PYTHON='${NO_PYTHON}'\n")
    + file(APPEND ${CMAKE_BINARY_DIR}/GIT-BUILD-OPTIONS "SUPPORTS_SIMPLE_IPC='${SUPPORTS_SIMPLE_IPC}'\n")
     -if(WIN32)
     +if(USE_VCPKG)
      	file(APPEND ${CMAKE_BINARY_DIR}/GIT-BUILD-OPTIONS "PATH=\"$PATH:$TEST_DIRECTORY/../compat/vcbuild/vcpkg/installed/x64-windows/bin\"\n")
2:  efa8681a22 = 2:  1cba2f9bd1 cmake: create compile_commands.json by default
3:  ceeca2bc0d = 3:  7824e74976 cmake: add warning for ignored MSGFMT_EXE
#26 lore raw

Hi Junio,

Quoted 13 lines · Junio C Hamano
Quoted 11 lines · Matthew Rogers via GitGitGadget

This pull request comes from our discussion here[1], and I think these patches provide a good compromise around the concerns discussed there

1:
https://lore.kernel.org/git/CAOjrSZusMSvs7AS-ZDsV8aQUgsF2ZA754vSDjgFKMRgi_oZAWw@mail.gmail.com/

CCing the people involved in the original discussion. cc: Philip Oakley [email protected] cc: Sibi Siddharthan [email protected], cc: Johannes Schindelin [email protected], cc: Danh Doan [email protected]

Quoted 7 lines
Quoted 4 lines

Matthew Rogers (3):
cmake: add knob to disable vcpkg
cmake: create compile_commands.json by default
cmake: add warning for ignored MSGFMT_EXE

I am neither cmake nor windows person, so I'll queue this as-is and wait for the stakeholders to chime in.

As long as the CI builds pass, I am in favor of integrating the patch series.

Quoted 4 lines

I did wonder if we want this to be applicable to the maintenance track for 2.31, though. There is a textual conflict with the addition of SIMPLE_IPC that happened during 2.32 cycle, which is easily resolvable.

If it isn't much work, sure. But I would think that developers who want to build using Visual Studio really should stay on newer branches.

Thanks,
Dscho

Quoted 24 lines

I am tempted to queue a version of these three patches rebased on to 'maint' after making sure that the result of merging that into 'master' is byte-for-byte identical to applying these three patches directly on to 'master'.

The range-diff looks like the attached. Thanks.

1: 546c49cc88 ! 1: 585b7ca371 cmake: add knob to disable vcpkg

    @@ contrib/buildsystems/CMakeLists.txt: endif()
      	if(NOT EXISTS ${MSGFMT_EXE})
      		message(WARNING "Text Translations won't be built")
      		unset(MSGFMT_EXE)
    -@@ contrib/buildsystems/CMakeLists.txt: file(APPEND ${CMAKE_BINARY_DIR}/GIT-BUILD-OPTIONS "X='${EXE_EXTENSION}'\n")
    - file(APPEND ${CMAKE_BINARY_DIR}/GIT-BUILD-OPTIONS "NO_GETTEXT='${NO_GETTEXT}'\n")
    +@@ contrib/buildsystems/CMakeLists.txt: file(APPEND ${CMAKE_BINARY_DIR}/GIT-BUILD-OPTIONS "NO_GETTEXT='${NO_GETTEXT}'\n"
      file(APPEND ${CMAKE_BINARY_DIR}/GIT-BUILD-OPTIONS "RUNTIME_PREFIX='${RUNTIME_PREFIX}'\n")
      file(APPEND ${CMAKE_BINARY_DIR}/GIT-BUILD-OPTIONS "NO_PYTHON='${NO_PYTHON}'\n")
    + file(APPEND ${CMAKE_BINARY_DIR}/GIT-BUILD-OPTIONS "SUPPORTS_SIMPLE_IPC='${SUPPORTS_SIMPLE_IPC}'\n")
     -if(WIN32)
     +if(USE_VCPKG)
      	file(APPEND ${CMAKE_BINARY_DIR}/GIT-BUILD-OPTIONS "PATH=\"$PATH:$TEST_DIRECTORY/../compat/vcbuild/vcpkg/installed/x64-windows/bin\"\n")
2:  efa8681a22 = 2:  1cba2f9bd1 cmake: create compile_commands.json by default
3:  ceeca2bc0d = 3:  7824e74976 cmake: add warning for ignored MSGFMT_EXE
#27 lore raw
Quoted 9 lines · Johannes Schindelin

As long as the CI builds pass, I am in favor of integrating the patch series.

Quoted 4 lines

I did wonder if we want this to be applicable to the maintenance track for 2.31, though. There is a textual conflict with the addition of SIMPLE_IPC that happened during 2.32 cycle, which is easily resolvable.

If it isn't much work, sure. But I would think that developers who want to build using Visual Studio really should stay on newer branches.

Ack. Philip

#28 lore raw
In reply to Junio C Hamano
Quoted 25 lines · Junio C Hamano
Quoted 11 lines · Matthew Rogers via GitGitGadget

This pull request comes from our discussion here[1], and I think these patches provide a good compromise around the concerns discussed there

1:
https://lore.kernel.org/git/CAOjrSZusMSvs7AS-ZDsV8aQUgsF2ZA754vSDjgFKMRgi_oZAWw@mail.gmail.com/

CCing the people involved in the original discussion. cc: Philip Oakley [email protected] cc: Sibi Siddharthan [email protected], cc: Johannes Schindelin [email protected], cc: Danh Doan [email protected]

Quoted 4 lines

Matthew Rogers (3):
cmake: add knob to disable vcpkg
cmake: create compile_commands.json by default
cmake: add warning for ignored MSGFMT_EXE

I am neither cmake nor windows person, so I'll queue this as-is and wait for the stakeholders to chime in.

I did wonder if we want this to be applicable to the maintenance track for 2.31, though. There is a textual conflict with the addition of SIMPLE_IPC that happened during 2.32 cycle, which is easily resolvable.

Quoted 4 lines

I am tempted to queue a version of these three patches rebased on to 'maint' after making sure that the result of merging that into 'master' is byte-for-byte identical to applying these three patches directly on to 'master'.

Sorry for the delay - I've been off-line and I'm only now catching up.

Could we confirm that the CI actually tests the update. IIRC the yml setup preloaded the vcpkg artefacts that this change looks to make work 'out of the box'.

Philip

#29 lore raw
In reply to Matthew Rogers via GitGitGadget

Hi Matt,

Quoted 10 lines · Matthew Rogers via GitGitGadget

This pull request comes from our discussion here[1], and I think these patches provide a good compromise around the concerns discussed there

1:
https://lore.kernel.org/git/CAOjrSZusMSvs7AS-ZDsV8aQUgsF2ZA754vSDjgFKMRgi_oZAWw@mail.gmail.com/

CCing the people involved in the original discussion. cc: Philip Oakley [email protected] cc: Sibi Siddharthan [email protected], cc: Johannes Schindelin [email protected], cc: Danh Doan [email protected]

Just in case that a v3 is needed, I fixed the PR description so that these "Cc:"s are interpreted correctly again by GitGitGadget.

But from a brief glance over v2, all patches look good to me.

Thanks,
Dscho

Quoted 136 lines

Matthew Rogers (3):
cmake: add knob to disable vcpkg
cmake: create compile_commands.json by default
cmake: add warning for ignored MSGFMT_EXE

 contrib/buildsystems/CMakeLists.txt | 37 ++++++++++++++++++++++-------
 1 file changed, 28 insertions(+), 9 deletions(-)

base-commit: c09b6306c6ca275ed9d0348a8c8014b2ff723cfb
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-970%2FROGERSM94%2Ffix-cmake-v2
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-970/ROGERSM94/fix-cmake-v2
Pull-Request: https://github.com/gitgitgadget/git/pull/970

Range-diff vs v1:

1: 3170f78daa5f ! 1: 485254b49de8 cmake: add knob to disable vcpkg

     @@ Commit message
              generators.

            - Some versions of Visual Studio 2019 moved away from using the
     -        VS 2019 by default, making it impossible for Visual Studio to
     -        configure the project in the likely event that it couldn't find the
     -        dependencies.
     +        VS 2019  generator by default, making it impossible for Visual
     +        Studio to configure the project in the likely event that it couldn't
     +        find the dependencies.

            - Inexperienced users of CMake are very likely to get tripped up by
              the errors caused by a lack of vcpkg, making the above bullet point
              both annoying and hard to debug.

     -    As such, lets make using vcpkg the default on windows.  Users who want
     +    As such, let's make using vcpkg the default on windows.  Users who want
          to avoid using vcpkg can disable it by passing -DNO_VCPKG=TRUE.

          Signed-off-by: Matthew Rogers <[email protected]>
     @@ contrib/buildsystems/CMakeLists.txt: NOTE: By default CMake uses Makefile as the
       set(CMAKE_SOURCE_DIR ${CMAKE_CURRENT_LIST_DIR}/../..)
      -if(WIN32)
      +
     -+if (WIN32 AND NOT NO_VCPKG)
     -+	set(USING_VCPKG TRUE)
     -+else()
     -+	set(USING_VCPKG FALSE)
     ++option(USE_VCPKG "Whether or not to use vcpkg for obtaining dependencies.  Only applicable to Windows platforms" ON)
     ++if(NOT WIN32)
     ++	set(USE_VCPKG OFF CACHE BOOL FORCE)
      +endif()
      +
     -+if(USING_VCPKG)
     ++if(USE_VCPKG)
       	set(VCPKG_DIR "${CMAKE_SOURCE_DIR}/compat/vcbuild/vcpkg")
      -	if(MSVC AND NOT EXISTS ${VCPKG_DIR})
      +	if(NOT EXISTS ${VCPKG_DIR})
     @@ contrib/buildsystems/CMakeLists.txt: endif()
       find_program(MSGFMT_EXE msgfmt)
       if(NOT MSGFMT_EXE)
      -	set(MSGFMT_EXE ${CMAKE_SOURCE_DIR}/compat/vcbuild/vcpkg/downloads/tools/msys2/msys64/usr/bin/msgfmt.exe)
     -+	if (USING_VCPKG)
     ++	if (USE_VCPKG)
      +		set(MSGFMT_EXE ${CMAKE_SOURCE_DIR}/compat/vcbuild/vcpkg/downloads/tools/msys2/msys64/usr/bin/msgfmt.exe)
      +	endif()
       	if(NOT EXISTS ${MSGFMT_EXE})
     @@ contrib/buildsystems/CMakeLists.txt: file(APPEND ${CMAKE_BINARY_DIR}/GIT-BUILD-O
       file(APPEND ${CMAKE_BINARY_DIR}/GIT-BUILD-OPTIONS "NO_PYTHON='${NO_PYTHON}'\n")
       file(APPEND ${CMAKE_BINARY_DIR}/GIT-BUILD-OPTIONS "SUPPORTS_SIMPLE_IPC='${SUPPORTS_SIMPLE_IPC}'\n")
      -if(WIN32)
     -+if(USING_VCPKG)
     ++if(USE_VCPKG)
       	file(APPEND ${CMAKE_BINARY_DIR}/GIT-BUILD-OPTIONS "PATH=\"$PATH:$TEST_DIRECTORY/../compat/vcbuild/vcpkg/installed/x64-windows/bin\"\n")
       endif()

2: c3bf266cf03a ! 2: a3b5eef54188 cmake: create compile_commands.json by default
@@ Commit message
time of this writing, and no real negative consequences that I can find with my search-skills.

- NOTE: That the comppile_commands.json is currenntly produced only when
+ NOTE: That the compile_commands.json is currently produced only when
using the Ninja and Makefile generators. See The CMake documentation[3] for more info.

@@ Commit message

## contrib/buildsystems/CMakeLists.txt ##
-@@ contrib/buildsystems/CMakeLists.txt: else()
- set(USING_VCPKG FALSE)
+@@ contrib/buildsystems/CMakeLists.txt: if(NOT WIN32)
+ set(USE_VCPKG OFF CACHE BOOL FORCE)
endif()

-+if (NOT DEFINED CMAKE_EXPORT_COMPILE_COMMANDS)
-+ SET(CMAKE_EXPORT_COMPILE_COMMANDS TRUE)
++if(NOT DEFINED CMAKE_EXPORT_COMPILE_COMMANDS)
++ set(CMAKE_EXPORT_COMPILE_COMMANDS TRUE)
+endif()
+
- if(USING_VCPKG)
+ if(USE_VCPKG)
set(VCPKG_DIR "${CMAKE_SOURCE_DIR}/compat/vcbuild/vcpkg") if(NOT EXISTS ${VCPKG_DIR})
3: 07763a9de723 ! 3: 2110c8ffa423 cmake: add warning for ignored MSGFMT_EXE

     @@ Commit message
          configured, as such add a check for NO_GETTEXT before attempting to set
          it.

     -    suggested-by: Johannes Schindelin <[email protected]>
     +    Suggested-by: Johannes Schindelin <[email protected]>
          Signed-off-by: Matthew Rogers <[email protected]>

       ## contrib/buildsystems/CMakeLists.txt ##
     @@ contrib/buildsystems/CMakeLists.txt: if(WIN32 AND NOT MSVC)#not required for vis

      -find_program(MSGFMT_EXE msgfmt)
      -if(NOT MSGFMT_EXE)
     --	if (USING_VCPKG)
     +-	if (USE_VCPKG)
      -		set(MSGFMT_EXE ${CMAKE_SOURCE_DIR}/compat/vcbuild/vcpkg/downloads/tools/msys2/msys64/usr/bin/msgfmt.exe)
      -	endif()
      -	if(NOT EXISTS ${MSGFMT_EXE})
     @@ contrib/buildsystems/CMakeLists.txt: if(WIN32 AND NOT MSVC)#not required for vis
      +else()
      +	find_program(MSGFMT_EXE msgfmt)
      +	if(NOT MSGFMT_EXE)
     -+		if (USING_VCPKG)
     ++		if(USE_VCPKG)
      +			set(MSGFMT_EXE ${CMAKE_SOURCE_DIR}/compat/vcbuild/vcpkg/downloads/tools/msys2/msys64/usr/bin/msgfmt.exe)
      +		endif()
      +		if(NOT EXISTS ${MSGFMT_EXE})

--
gitgitgadget

#30 lore raw
Quoted 4 lines · Johannes Schindelin

Just in case that a v3 is needed, I fixed the PR description so that these "Cc:"s are interpreted correctly again by GitGitGadget.

But from a brief glance over v2, all patches look good to me.

Let me amend v2 with your Acked-by and merge it down to 'next', then.

Thanks.