Make Git for Windows start builds in modern Visual Studio #3306
Conversation
There was a problem hiding this comment.
From the description, I expected that a ton of variables were missing defaults, but the vast majority of the patch is not about that. The only missing default, according to the diff, seems to have been VCPKG_ARCH.
I do like the direction this is going, though.
contrib/buildsystems/CMakeLists.txt
Outdated
| Visual Studio does not produce a .sln solution file nor the .vcproj files | ||
| that may be required by VS extension tools. | ||
|
|
||
| To generate the .sln/.vcproj files run Cmake manually, as below. |
There was a problem hiding this comment.
| To generate the .sln/.vcproj files run Cmake manually, as below. | |
| To generate the .sln/.vcproj files run CMake manually, as described below. |
There was a problem hiding this comment.
To generate the .sln/.vcproj files run CMake manually, as described below.
OK.
It's not clear to me if this will be necessary as VS Ninja will run CMakeLists.txt anyway (lots of round incircles interactions, still need to setup the doubly clean testing)
contrib/buildsystems/CMakeLists.txt
Outdated
| Run `make` to build Git on Linux/*BSD/MacOS. | ||
| Open git.sln on Windows and build Git. | ||
|
|
||
| NOTE: By default CMake uses Makefile as the build tool on Linux and Visual Studio in Windows, | ||
| to use another tool say `ninja` add this to the command line when configuring. | ||
| `-G Ninja` | ||
|
|
||
| The Visual Studio default generator changed in v16.6 from its visual studio |
There was a problem hiding this comment.
| The Visual Studio default generator changed in v16.6 from its visual studio | |
| The Visual Studio default generator changed in v16.6 from its Visual Studio |
There was a problem hiding this comment.
The Visual Studio default generator changed in v16.6 from its Visual Studio
OK
contrib/buildsystems/CMakeLists.txt
Outdated
| ]] | ||
| cmake_minimum_required(VERSION 3.14) | ||
|
|
||
| #set the source directory to root of git | ||
| set(CMAKE_SOURCE_DIR ${CMAKE_CURRENT_LIST_DIR}/../..) | ||
| if(WIN32) | ||
| set(VCPKG_DIR "${CMAKE_SOURCE_DIR}/compat/vcbuild/vcpkg") | ||
| message("WIN32: ${WIN32}") |
There was a problem hiding this comment.
Why print out WIN32 when it is a Boolean and we're inside the conditional block where we know that it is true?
There was a problem hiding this comment.
Why print out
WIN32when it is a Boolean and we're inside the conditional block where we know that it istrue?
Thanks for the prompt comments.
While we may assume it's evaluated to true, it's not obvious what it's actual text value is (or has become in future cases).
I found that much of the CMake documentation left a lot to be desired about those sorts of things (confusions between existence, particular text values etc. Previously it was part of a logic term (AND MSVC iirc).
| if(NOT EXISTS ${VCPKG_ARCH}) | ||
| message("VCPKG_ARCH: unset, using 'x64-windows'") | ||
| set(VCPKG_ARCH "x64-windows") # default from vcpkg_install.bat | ||
| endif() |
There was a problem hiding this comment.
Don't you want to print out VCPKG_ARCH in case it was already specified?
There was a problem hiding this comment.
Don't you want to print out
VCPKG_ARCHin case it was already specified?
Good idea. Will add.
There was a problem hiding this comment.
I guess this concern has been addressed as part of a reply to a different comment.
| if(MSVC AND 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 ${VCPKG_ARCH}) | ||
| endif() | ||
| if(NOT EXISTS ${VCPKG_ARCH}) |
There was a problem hiding this comment.
It seems a bit weird to do this after we print out all the other build configuration information and run the vcpkg_install script using this option anyways, is there a reason it is printed out here rather than with the others?
There was a problem hiding this comment.
a bit weird to do this after we print out all the other build configuration information and run the vcpkg_install script using this option anyways, is there a reason
The other printouts are as a reflection of the implicit Build Options. Dscho has asked that I include the VCPKG_ARCH in the list of those options.
The reason I put the message here is because the default arch is set within vcpkg_install script and I was keeping that dependency logic, so if a user ran vcpkg_install first, manually, without parameters, then we'd have the same end result.
Dscho made a similar comment wrt detecting the Visual Studio default, and the same logic should apply that we should only have one source of truth.
Hmm. Just rechecking. The on-line gui suggests you only highlighted one line, but maybe not. I'll need to check that the rebase onto upstream hasn't created some out of sequence interaction - I see an AND MSVC above which I wasn't expecting.
will re-check
There was a problem hiding this comment.
Yep. Looks like I goofed up the rebase / conflict.
I'd also thought that the MSVC dependency had gone, because Visual Studio is now using Ninja, but maybe my memory is getting confused.
There was a problem hiding this comment.
Yeah, so I had a patch make it into seen that does do some work in regards to that dependency here. Where we generate the compile_commands.json file by default. So many tools can operate off of CMake builds with little work.
More relevant here we add the USE_VCPKG option, here so maybe it's better to base this off of that work as well?
There was a problem hiding this comment.
Yeah, so I had a patch make it into seen that does do some work in regards to that dependency here. Where we generate the compile_commands.json file by default. So many tools can operate off of CMake builds with little work.
More relevant here we add the USE_VCPKG option, here so maybe it's better to base this off of that work as well?
Definitely. I had it in my mind that that work was already in GfW, and so would have been picked up when I rebased my series onto GfW (my upstream)...
I did a quick tweak of script to assume MSVC was true (by deleting it!), and VS has loaded vcpkg - Yay.
I'm preping to be away for a four-day weekend. Hopefully have something done on Wedn next.
There was a problem hiding this comment.
I lost track. Does this still need to be resolved, or has it been addressed already?
There was a problem hiding this comment.
I guess this concern has been addressed as part of a reply to a different comment.
|
here's the screenshot of my Visual Studio deciding that the CMakeLists script and it disagree.. (I had included the agreed minor corrections)
Annoying. and Confused. |
|
Ok so the Output window also gives |
|
@dscho @ROGERSM94 @dennisameling I'm progressing the fixup for the out of the box building of Git for Windows in Visual Studio, but I've hit a CI failure that looks to be a set of intertwined dependencies that I haven't got to the bottom of and I looking for a bit of help. The Visual Studio build does complete, though shows the information message mentioned later. In part it looks as if some vcpkg dependencies have gone missing (around the effects of I've picked up Matt's CMake improvements that are in However it has CI build failures around picking up the In my local testing I'm seeing that i.e. https://github.com/git/git/blame/master/contrib/buildsystems/CMakeLists.txt#L181-L185 isn't finding I'm not sure why How would the msys2 tools normally be added to that vcpkg subdir normally (locally and the CI)? Should the CI system see that same, or different, issues? Maybe the Git, and Git-for-Windows views of the CI are different. Thoughts? |
[…]
That sounds a lot like the issue Junio had with v1 of gitgitgadget#878 Does your |
IIUC the I'll look at the linked Junio comment as well. |
|
I looked at gitgitgadget#878 and it does look to be the same CI-breakage issue. Still not sure of the history of the sometimes included |
|
@PhilipOakley if you don't want to base your commits on Git for Windows |
|
@dscho hi,
my series is on the GfW
Hmm, that suggestion made me do a double-think to realise that I'd been working on the expectation that I'd need the VS compiled codebase to be based on the Git-for-Windows version, to be a 'real', installable version. If it's just to get a clean compile, both locally (via the CMakeLists.txt) and in the CI (when pushed to I can try your suggestion and see how it goes for the local test aspects. Your series should implicitly include Matt's fixes as that's on next already. |
|
@gifhuppp Thanks for the support. It's not quite ready yet as there are a few different patch sets that need to come together to ensure that both the Git (upstream) and Git-for-Windows CI (continuous integration) tests pass and that it works well on my (and hopefully everyone else's) machines 😉 Longer term I've still got a desire to look at the '4Gb' file size limit on GfW, but that is a big change (hundreds of Int -> Size_t; a bit ugly!) |
I showed you how to break it into smaller chunks. I'm still convinced that would be a better way to go. |
Hmm. You suggested it should be broken into many smaller chunks, but I didn't see how. That's the gap. The test of rebasing my fixups (for this issue ) on your js/ci-windows-update branch has worked locally, but I've jet to push it up to GitHub (as a fresh branch) to see if the CI works OK (should do but, need to check ..) I've got household tasks at the moment. |
|
I pushed my local changes to https://github.com/PhilipOakley/git/tree/vs-sln-git with (passing) results at https://github.com/PhilipOakley/git/actions/runs/1034204853. I had had it in my head that the |
|
Just rechecked the build for the Not sure if there is a cache issue. @ROGERSM94, any ideas? |
|
|
|
Thanks Matt, The I did see in other web searches that suggest MSVC doesn't provide that https://stackoverflow.com/a/67384920/717355 While others suggest that there is a cache issue making it look like it needs two passes https://gitlab.kitware.com/cmake/cmake/-/issues/16588 (linked from another SO answer https://stackoverflow.com/a/66560758/717355 to same question). I'll maybe try and see what happens if I add the extra Cache setting... ... goes and looks Hmm, my CMakeLists.txt file doesn't even appear to have that setting. Maybe I've got the wrong mental model of dscho's upstream branch (in Loks like I've some extra digging to do. |
|
ci-
Sigh. Yes, mental model was wrong. I had it in my head that, ultimately, dscho's https://github.com/gitster/git/commits/js/ci-windows-update would be on top of Matt's Hey-ho. |
OK, so been round the houses a couple of times. I had perceived that everything in I think I have a better view of the mental model (sic), and I've shifted to the regularly rebuilt The This means that all the different 'dependent' series needed to ensure that Visual Studio will build this mega series (no, it's a micro series, but needed a lot of prereqs ;-) straight out of the box, and will pass the CI (i.e. no sparse warnings or any of the other niff-naff and trivia from elsewhere ;-) I have included all the changes requested, except that the Testing is still manual. I haven't looked into which CI system would provide the Windows & Visual Studio environments that can be driven by a scheduled event (e.g. weekly), nor how to code it... Question for @dscho , given that the 'series is on top of |
This comment was marked as spam.
This comment was marked as spam.
|
The best you could do is by starting with a merge of the respective gitster/git branch you rely on (as identified by the merge commit in upstream's |
It does properly combine the two sets of problematic commits, it is the most easy to access.
This is essentially the same as However, in the meanwhile, the |
It is a moving target. Every time either Git for Windows' or Git's main branch advances,
With the notable difference that it isn't force-pushed all the time, and the other notable difference that it only contains what you actually need. The |
That hasn't occurred, at all. In fact it's the opposite. It ( Anyway. It (my little 4 patch series) is now on top of I've separately to liaise with @ROGERSM94 about his |
Make Git for Windows start builds in modern Visual Studio
Make Git for Windows start builds in modern Visual Studio
Make Git for Windows start builds in modern Visual Studio
Make Git for Windows start builds in modern Visual Studio
Make Git for Windows start builds in modern Visual Studio
Make Git for Windows start builds in modern Visual Studio
Make Git for Windows start builds in modern Visual Studio
Make Git for Windows start builds in modern Visual Studio
Make Git for Windows start builds in modern Visual Studio
Make Git for Windows start builds in modern Visual Studio
Make Git for Windows start builds in modern Visual Studio
Make Git for Windows start builds in modern Visual Studio
Make Git for Windows start builds in modern Visual Studio
Make Git for Windows start builds in modern Visual Studio
Make Git for Windows start builds in modern Visual Studio
Make Git for Windows start builds in modern Visual Studio
Make Git for Windows start builds in modern Visual Studio
Make Git for Windows start builds in modern Visual Studio
Make Git for Windows start builds in modern Visual Studio
Make Git for Windows start builds in modern Visual Studio
Make Git for Windows start builds in modern Visual Studio
Make Git for Windows start builds in modern Visual Studio
Make Git for Windows start builds in modern Visual Studio
Make Git for Windows start builds in modern Visual Studio
Make Git for Windows start builds in modern Visual Studio
Make Git for Windows start builds in modern Visual Studio
Make Git for Windows start builds in modern Visual Studio
Make Git for Windows start builds in modern Visual Studio
Make Git for Windows start builds in modern Visual Studio
Make Git for Windows start builds in modern Visual Studio
The goal of opening and building Git for Windows (GfW) in Visual Studio, without any special configuration, has been a moving target.
Historically Git provided it's own hand rolled generator for VS project .sln (etc.) files in contrib/buildsystems/Generators/* and compat/vcbuild/*.
Git and GfW has now added a CMakeLists.txt script for the use of Cmake as the buildsystem.
Visual Studio has also updated it's default generator (now Ninja).
Builds on ARM are now also possible of GfW, through the addition of a VCPKG_ARCH parameter.
Unfortunately, this left behind the default build as expecting parameters that aren't provided.
This series tries to tidy up the default scenario and the description in the CMakeLists.txt header comment.
Testing of the update requires that the install be 'clean' for both Visual Studio and the worktree.
[updated 28Aug2021]