Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Make Git for Windows start builds in modern Visual Studio #3306

Merged
merged 4 commits into from
Oct 12, 2021
Merged

Make Git for Windows start builds in modern Visual Studio #3306

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
03b742e
CMake: default Visual Studio generator has changed
PhilipOakley Jul 1, 2021
204e0d5
.gitignore: add Visual Studio CMakeSetting.json file
PhilipOakley Apr 24, 2021
2c669d0
CMakeLists: add default "x64-windows" arch for Visual Studio
PhilipOakley Apr 22, 2021
0e1dc0b
CMake: show Win32 and Generator_platform build-option values
PhilipOakley May 10, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions .gitignore
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -243,3 +243,4 @@ Release/
/git.VC.db
*.dSYM
/contrib/buildsystems/out
CMakeSettings.json
PhilipOakley marked this conversation as resolved.
Show resolved Hide resolved
30 changes: 24 additions & 6 deletions contrib/buildsystems/CMakeLists.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,11 @@ Note: Visual Studio also has the option of opening `CMakeLists.txt`
directly; Using this option, Visual Studio will not find the source code,
though, therefore the `File>Open>Folder...` option is preferred.

Visual Studio does not produce a .sln solution file nor the .vcxproj files
that may be required by VS extension tools.

To generate the .sln/.vcxproj files run CMake manually, as described below.

Instructions to run CMake manually:

mkdir -p contrib/buildsystems/out
Expand All @@ -22,7 +27,7 @@ Instructions to run CMake manually:

This will build the git binaries in contrib/buildsystems/out
directory (our top-level .gitignore file knows to ignore contents of
this directory).
this directory). The project .sln and .vcxproj files are also generated.

Possible build configurations(-DCMAKE_BUILD_TYPE) with corresponding
compiler flags
Expand All @@ -35,17 +40,16 @@ empty(default) :
NOTE: -DCMAKE_BUILD_TYPE is optional. For multi-config generators like Visual Studio
this option is ignored

This process generates a Makefile(Linux/*BSD/MacOS) , Visual Studio solution(Windows) by default.
This process generates a Makefile(Linux/*BSD/MacOS), Visual Studio solution(Windows) by default.
Run `make` to build Git on Linux/*BSD/MacOS.
Open git.sln on Windows and build Git.

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`

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.

The Visual Studio default generator changed in v16.6 from its Visual Studio
implemenation to `Ninja` This required changes to many CMake scripts.

]]
cmake_minimum_required(VERSION 3.14)

Expand All @@ -59,14 +63,28 @@ endif()

if(NOT DEFINED CMAKE_EXPORT_COMPILE_COMMANDS)
set(CMAKE_EXPORT_COMPILE_COMMANDS TRUE)
message("settting CMAKE_EXPORT_COMPILE_COMMANDS: ${CMAKE_EXPORT_COMPILE_COMMANDS}")
endif()

if(USE_VCPKG)
set(VCPKG_DIR "${CMAKE_SOURCE_DIR}/compat/vcbuild/vcpkg")
message("WIN32: ${WIN32}") # show its underlying text values
message("VCPKG_DIR: ${VCPKG_DIR}")
message("VCPKG_ARCH: ${VCPKG_ARCH}") # maybe unset
message("MSVC: ${MSVC}")
message("CMAKE_GENERATOR: ${CMAKE_GENERATOR}")
message("CMAKE_CXX_COMPILER_ID: ${CMAKE_CXX_COMPILER_ID}")
message("CMAKE_GENERATOR_PLATFORM: ${CMAKE_GENERATOR_PLATFORM}")
message("CMAKE_EXPORT_COMPILE_COMMANDS: ${CMAKE_EXPORT_COMPILE_COMMANDS}")
message("ENV(CMAKE_EXPORT_COMPILE_COMMANDS): $ENV{CMAKE_EXPORT_COMPILE_COMMANDS}")
PhilipOakley marked this conversation as resolved.
Show resolved Hide resolved
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 ${VCPKG_ARCH})
endif()
if(NOT EXISTS ${VCPKG_ARCH})
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I lost track. Does this still need to be resolved, or has it been addressed already?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess this concern has been addressed as part of a reply to a different comment.

message("VCPKG_ARCH: unset, using 'x64-windows'")
set(VCPKG_ARCH "x64-windows") # default from vcpkg_install.bat
PhilipOakley marked this conversation as resolved.
Show resolved Hide resolved
endif()
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Don't you want to print out VCPKG_ARCH in case it was already specified?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Don't you want to print out VCPKG_ARCH in case it was already specified?

Good idea. Will add.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Still a good idea?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess this concern has been addressed as part of a reply to a different comment.

list(APPEND CMAKE_PREFIX_PATH "${VCPKG_DIR}/installed/${VCPKG_ARCH}")

# In the vcpkg edition, we need this to be able to link to libcurl
Expand Down