From: Matthew Rogers via GitGitGadget <hidden> Date: 2021-06-04 17:43:41
From: Matthew Rogers <redacted>
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.
Signed-off-by: Matthew Rogers <redacted>
---
contrib/buildsystems/CMakeLists.txt | 20 ++++++++++++++++----
1 file changed, 16 insertions(+), 4 deletions(-)
@@ -43,14 +43,24 @@ NOTE: By default CMake uses Makefile as the build tool on Linux and Visual Studitouseanothertoolsay`ninja`addthistothecommandlinewhenconfiguring.`-GNinja`+NOTE:BydefaultCMakewillinstallvcpkglocallytoyoursourcetreeonconfiguration,+toavoidthis,add`-DNO_VCPKG=TRUE`tothecommandlinewhenconfiguring.+]]cmake_minimum_required(VERSION3.14)#set the source directory to root of gitset(CMAKE_SOURCE_DIR${CMAKE_CURRENT_LIST_DIR}/../..)-if(WIN32)++if(WIN32ANDNOTNO_VCPKG)+set(USING_VCPKGTRUE)+else()+set(USING_VCPKGFALSE)+endif()++if(USING_VCPKG)set(VCPKG_DIR"${CMAKE_SOURCE_DIR}/compat/vcbuild/vcpkg")-if(MSVCANDNOTEXISTS${VCPKG_DIR})+if(NOTEXISTS${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_EXEmsgfmt)if(NOTMSGFMT_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(NOTEXISTS${MSGFMT_EXE})message(WARNING"Text Translations won't be built")unset(MSGFMT_EXE)
From: Matthew Rogers via GitGitGadget <hidden> Date: 2021-06-04 17:44:30
From: Matthew Rogers <redacted>
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.
suggested-by: Johannes Schindelin <redacted>
Signed-off-by: Matthew Rogers <redacted>
---
contrib/buildsystems/CMakeLists.txt | 20 ++++++++++++--------
1 file changed, 12 insertions(+), 8 deletions(-)
@@ -190,14 +190,18 @@ if(WIN32 AND NOT MSVC)#not required for visual studio buildsendif()endif()-find_program(MSGFMT_EXEmsgfmt)-if(NOTMSGFMT_EXE)-if(USING_VCPKG)-set(MSGFMT_EXE${CMAKE_SOURCE_DIR}/compat/vcbuild/vcpkg/downloads/tools/msys2/msys64/usr/bin/msgfmt.exe)-endif()-if(NOTEXISTS${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_EXEmsgfmt)+if(NOTMSGFMT_EXE)+if(USING_VCPKG)+set(MSGFMT_EXE${CMAKE_SOURCE_DIR}/compat/vcbuild/vcpkg/downloads/tools/msys2/msys64/usr/bin/msgfmt.exe)+endif()+if(NOTEXISTS${MSGFMT_EXE})+message(WARNING"Text Translations won't be built")+unset(MSGFMT_EXE)+endif()endif()endif()
From: Matthew Rogers via GitGitGadget <hidden> Date: 2021-06-04 17:44:44
From: Matthew Rogers <redacted>
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
Signed-off-by: Matthew Rogers <redacted>
---
contrib/buildsystems/CMakeLists.txt | 4 ++++
1 file changed, 4 insertions(+)
From: Eric Sunshine <hidden> Date: 2021-06-04 18:03:29
On Fri, Jun 4, 2021 at 1:44 PM Matthew Rogers via GitGitGadget
[off-list ref] wrote:
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.
- 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.
From: Eric Sunshine <hidden> Date: 2021-06-04 18:05:59
On Fri, Jun 4, 2021 at 1:44 PM Matthew Rogers via GitGitGadget
[off-list ref] wrote:
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.
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.
From: Eric Sunshine <hidden> Date: 2021-06-04 18:10:21
On Fri, Jun 4, 2021 at 1:44 PM Matthew Rogers via GitGitGadget
[off-list ref] wrote:
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...".
suggested-by: Johannes Schindelin <redacted>
Signed-off-by: Matthew Rogers <redacted>
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.
From: Matt Rogers <hidden> Date: 2021-06-04 18:34:40
On Fri, Jun 4, 2021 at 2:03 PM Eric Sunshine [off-list ref] wrote:
On Fri, Jun 4, 2021 at 1:44 PM Matthew Rogers via GitGitGadget
[off-list ref] wrote:
quoted
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.
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
- 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.
From: Sibi Siddharthan <hidden> Date: 2021-06-04 20:56:49
On Fri, Jun 4, 2021 at 11:13 PM Matthew Rogers via GitGitGadget
[off-list ref] wrote:
-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
From: Sibi Siddharthan <hidden> Date: 2021-06-04 21:09:25
On Fri, Jun 4, 2021 at 11:13 PM Matthew Rogers via GitGitGadget
[off-list ref] wrote:
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.
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
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.
--
An old man doll... just what I always wanted! - Clara
From: Matt Rogers <hidden> Date: 2021-06-05 22:32:05
On Fri, Jun 4, 2021 at 4:55 PM Sibi Siddharthan
[off-list ref] wrote:
On Fri, Jun 4, 2021 at 11:13 PM Matthew Rogers via GitGitGadget
[off-list ref] wrote:
quoted
-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)
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)
I would prefer to use `USE_VCPKG`.
Thank You,
Sibi Siddharthan
From: Matt Rogers <hidden> Date: 2021-06-05 22:36:17
On Fri, Jun 4, 2021 at 5:09 PM Sibi Siddharthan
[off-list ref] wrote:
On Fri, Jun 4, 2021 at 11:13 PM Matthew Rogers via GitGitGadget
[off-list ref] wrote:
quoted
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.
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 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.
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.
--
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
From: Sibi Siddharthan <hidden> Date: 2021-06-06 04:33:26
On Sun, Jun 6, 2021 at 4:01 AM Matt Rogers [off-list ref] wrote:
On Fri, Jun 4, 2021 at 4:55 PM Sibi Siddharthan
[off-list ref] wrote:
quoted
On Fri, Jun 4, 2021 at 11:13 PM Matthew Rogers via GitGitGadget
[off-list ref] wrote:
quoted
-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)
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
From: Sibi Siddharthan <hidden> Date: 2021-06-06 04:40:06
On Sun, Jun 6, 2021 at 4:06 AM Matt Rogers [off-list ref] wrote:
On Fri, Jun 4, 2021 at 5:09 PM Sibi Siddharthan
[off-list ref] wrote:
quoted
On Fri, Jun 4, 2021 at 11:13 PM Matthew Rogers via GitGitGadget
[off-list ref] wrote:
quoted
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.
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
From: Matthew Rogers via GitGitGadget <hidden> Date: 2021-06-06 12:03:02
From: Matthew Rogers <redacted>
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.
Signed-off-by: Matthew Rogers <redacted>
---
contrib/buildsystems/CMakeLists.txt | 19 +++++++++++++++----
1 file changed, 15 insertions(+), 4 deletions(-)
@@ -43,14 +43,23 @@ NOTE: By default CMake uses Makefile as the build tool on Linux and Visual Studitouseanothertoolsay`ninja`addthistothecommandlinewhenconfiguring.`-GNinja`+NOTE:BydefaultCMakewillinstallvcpkglocallytoyoursourcetreeonconfiguration,+toavoidthis,add`-DNO_VCPKG=TRUE`tothecommandlinewhenconfiguring.+]]cmake_minimum_required(VERSION3.14)#set the source directory to root of gitset(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(NOTWIN32)+set(USE_VCPKGOFFCACHEBOOLFORCE)+endif()++if(USE_VCPKG)set(VCPKG_DIR"${CMAKE_SOURCE_DIR}/compat/vcbuild/vcpkg")-if(MSVCANDNOTEXISTS${VCPKG_DIR})+if(NOTEXISTS${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_EXEmsgfmt)if(NOTMSGFMT_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(NOTEXISTS${MSGFMT_EXE})message(WARNING"Text Translations won't be built")unset(MSGFMT_EXE)
From: Matthew Rogers via GitGitGadget <hidden> Date: 2021-06-06 12:03:17
From: Matthew Rogers <redacted>
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
Signed-off-by: Matthew Rogers <redacted>
---
contrib/buildsystems/CMakeLists.txt | 4 ++++
1 file changed, 4 insertions(+)
From: Matthew Rogers via GitGitGadget <hidden> Date: 2021-06-06 12:04:13
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
philipoakley@iee.email cc: Sibi Siddharthan
sibisiddharthan.github@gmail.com, cc: Johannes Schindelin
johannes.schindelin@gmx.de, cc: Danh Doan congdanhqx@gmail.com
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 [off-list ref]
@@ 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
Signed-off-by: Matthew Rogers [off-list ref]
## 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 [off-list ref]
+ Suggested-by: Johannes Schindelin [off-list ref]
Signed-off-by: Matthew Rogers [off-list ref]
## 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
From: Matthew Rogers via GitGitGadget <hidden> Date: 2021-06-06 12:04:14
From: Matthew Rogers <redacted>
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.
Suggested-by: Johannes Schindelin <redacted>
Signed-off-by: Matthew Rogers <redacted>
---
contrib/buildsystems/CMakeLists.txt | 20 ++++++++++++--------
1 file changed, 12 insertions(+), 8 deletions(-)
@@ -189,14 +189,18 @@ if(WIN32 AND NOT MSVC)#not required for visual studio buildsendif()endif()-find_program(MSGFMT_EXEmsgfmt)-if(NOTMSGFMT_EXE)-if(USE_VCPKG)-set(MSGFMT_EXE${CMAKE_SOURCE_DIR}/compat/vcbuild/vcpkg/downloads/tools/msys2/msys64/usr/bin/msgfmt.exe)-endif()-if(NOTEXISTS${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_EXEmsgfmt)+if(NOTMSGFMT_EXE)+if(USE_VCPKG)+set(MSGFMT_EXE${CMAKE_SOURCE_DIR}/compat/vcbuild/vcpkg/downloads/tools/msys2/msys64/usr/bin/msgfmt.exe)+endif()+if(NOTEXISTS${MSGFMT_EXE})+message(WARNING"Text Translations won't be built")+unset(MSGFMT_EXE)+endif()endif()endif()
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`).
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
From: Johannes Schindelin <hidden> Date: 2021-06-10 09:47:39
Hi Matt,
On Sun, 6 Jun 2021, Matthew Rogers via GitGitGadget wrote:
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
philipoakley@iee.email cc: Sibi Siddharthan
sibisiddharthan.github@gmail.com, cc: Johannes Schindelin
johannes.schindelin@gmx.de, cc: Danh Doan congdanhqx@gmail.com
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
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 [off-list ref]
@@ 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
Signed-off-by: Matthew Rogers [off-list ref]
## 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 [off-list ref]
+ Suggested-by: Johannes Schindelin [off-list ref]
Signed-off-by: Matthew Rogers [off-list ref]
## 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
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
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.
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.
From: Johannes Schindelin <hidden> Date: 2021-06-18 13:42:56
Hi Philip,
On Fri, 18 Jun 2021, Philip Oakley wrote:
On 10/06/2021 10:43, Johannes Schindelin wrote:
quoted
On Sat, 5 Jun 2021, Bagas Sanjaya wrote:
quoted
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.
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
From: Philip Oakley <hidden> Date: 2021-06-18 14:03:14
Hi Dscho
On 18/06/2021 14:42, Johannes Schindelin wrote:
quoted
quoted
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.
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
From: Johannes Schindelin <hidden> Date: 2021-06-22 22:32:43
Hi Philip,
On Fri, 18 Jun 2021, Philip Oakley wrote:
On 18/06/2021 14:42, Johannes Schindelin wrote:
quoted
quoted
quoted
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.
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).
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