[v3,7/8] cmake: support for building git on windows with msvc and clang.
diff mbox series

Message ID 8f36e30cd2218fa56fb0c2b89f6f51726b0eb285.1590759624.git.gitgitgadget@gmail.com
State New
Headers show
Series
  • CMake build system for git
Related show

Commit Message

sunlin via GitGitGadget May 29, 2020, 1:40 p.m. UTC
From: Sibi Siddharthan <sibisiddharthan.github@gmail.com>

This patch adds support for Visual Studio and Clang builds

The minimum required version of CMake is upgraded to 3.15 because
this version offers proper support for Clang builds on Windows.

Libintl is not searched for when building with Visual Studio or Clang
because there is no binary compatible version available yet.

NOTE: In the link options invalidcontinue.obj has to be included.
The reason for this is because by default, Windows calls abort()'s
instead of setting errno=EINVAL when invalid arguments are passed to
standard functions.
This commit explains it in detail:
4b623d80f73528a632576990ca51e34c333d5dd6

On Windows the default generator is Visual Studio,so for Visual Studio
builds do this:

cmake `relative-path-to-srcdir`

NOTE: Visual Studio generator is a multi config generator, which means
that Debug and Release builds can be done on the same build directory.

For Clang builds do this:

On bash
CC=Clang cmake `relative-path-to-srcdir` -G Ninja
		-DCMAKE_BUILD_TYPE=[Debug or Release]

On cmd
set CC=Clang
cmake `relative-path-to-srcdir` -G Ninja
		-DCMAKE_BUILD_TYPE=[Debug or Release]

Signed-off-by: Sibi Siddharthan <sibisiddharthan.github@gmail.com>
---
 contrib/buildsystems/CMakeLists.txt | 55 +++++++++++++++++++++++------
 1 file changed, 45 insertions(+), 10 deletions(-)

Comments

Đoàn Trần Công Danh May 30, 2020, 2:08 p.m. UTC | #1
On 2020-05-29 13:40:23+0000, Sibi Siddharthan via GitGitGadget <gitgitgadget@gmail.com> wrote:
> From: Sibi Siddharthan <sibisiddharthan.github@gmail.com>
> 
> This patch adds support for Visual Studio and Clang builds
> 
> The minimum required version of CMake is upgraded to 3.15 because
> this version offers proper support for Clang builds on Windows.
> 
> Libintl is not searched for when building with Visual Studio or Clang
> because there is no binary compatible version available yet.
> 
> NOTE: In the link options invalidcontinue.obj has to be included.
> The reason for this is because by default, Windows calls abort()'s
> instead of setting errno=EINVAL when invalid arguments are passed to
> standard functions.
> This commit explains it in detail:
> 4b623d80f73528a632576990ca51e34c333d5dd6

I think it's better to say:

	See 4b623d80f7 (MSVC: link in invalidcontinue.obj for better
	POSIX compatibility, 2014-03-29) for detail.


(I haven't read the referenced commit, FWIW)

> On Windows the default generator is Visual Studio,so for Visual Studio
> builds do this:
> 
> cmake `relative-path-to-srcdir`
> 
> NOTE: Visual Studio generator is a multi config generator, which means
> that Debug and Release builds can be done on the same build directory.
> 
> For Clang builds do this:
> 
> On bash
> CC=Clang cmake `relative-path-to-srcdir` -G Ninja

I think you meant CC=clang, note the lowercase c,
that will cmake this note applicable for case-sensitive filesystem.

... reading this patch ...

So, this is applicable for Windows only, it's fine as is, then.
It's still better to lowercase it, though.

> 		-DCMAKE_BUILD_TYPE=[Debug or Release]
> 
> On cmd
> set CC=Clang
> cmake `relative-path-to-srcdir` -G Ninja
> 		-DCMAKE_BUILD_TYPE=[Debug or Release]
> 
> Signed-off-by: Sibi Siddharthan <sibisiddharthan.github@gmail.com>
> ---
>  contrib/buildsystems/CMakeLists.txt | 55 +++++++++++++++++++++++------
>  1 file changed, 45 insertions(+), 10 deletions(-)
> 
> diff --git a/contrib/buildsystems/CMakeLists.txt b/contrib/buildsystems/CMakeLists.txt
> index 46197d0b806..0a3f711db88 100644
> --- a/contrib/buildsystems/CMakeLists.txt
> +++ b/contrib/buildsystems/CMakeLists.txt
> @@ -98,8 +98,11 @@ find_package(ZLIB REQUIRED)
>  find_package(CURL)
>  find_package(EXPAT)
>  find_package(Iconv)
> -find_package(Intl)
>  
> +#Don't use libintl on Windows Visual Studio and Clang builds
> +if(NOT (WIN32 AND (CMAKE_C_COMPILER_ID STREQUAL "MSVC" OR CMAKE_C_COMPILER_ID STREQUAL "Clang")))

Personally, I find this is a bit ugly.
Does it work to move the if(WIN32) down there, here.
Then make sub-condition for MSVC and Clang?

> +	find_package(Intl)
> +endif()
>  
>  if(NOT Intl_FOUND)
>  	add_compile_definitions(NO_GETTEXT)
> @@ -123,7 +126,7 @@ if(Intl_FOUND)
>  endif()
>  
>  
> -if(WIN32)
> +if(WIN32 AND NOT MSVC)#not required for visual studio builds

for the down there, I meant here.
Using "NOT MSVC" here and `CMAKE_C_COMPILER_ID STREQUAL "MSVC"` above
may puzzle people interest in this patch.

>  	find_program(WINDRES_EXE windres)
>  	if(NOT WINDRES_EXE)
>  		message(FATAL_ERROR "Install windres on Windows for resource files")
> @@ -135,6 +138,13 @@ if(NOT MSGFMT_EXE)
>  	message(WARNING "Text Translations won't be build")
>  endif()
>  
> +#Force all visual studio outputs to CMAKE_BINARY_DIR
> +if(CMAKE_C_COMPILER_ID STREQUAL "MSVC")
> +	set(CMAKE_RUNTIME_OUTPUT_DIRECTORY_DEBUG ${CMAKE_BINARY_DIR})
> +	set(CMAKE_RUNTIME_OUTPUT_DIRECTORY_RELEASE ${CMAKE_BINARY_DIR})
> +	add_compile_options(/MP)
> +endif()
> +
>  #default behaviour
>  include_directories(${CMAKE_SOURCE_DIR})
>  add_compile_definitions(GIT_HOST_CPU="${CMAKE_SYSTEM_PROCESSOR}")
> @@ -172,6 +182,10 @@ endif()
>  
>  #Platform Specific
>  if(CMAKE_SYSTEM_NAME STREQUAL "Windows")

I missed this line from previous patch
(and my comment in previous round, which suggested remove ${})
Does `if(WIN32)` and `if(CMAKE_SYSTEM_NAME STREQUAL "Windows")` have
different meanings?
I don't know much about CMake on Windows to really sure about this.
Sibi Siddharthan May 30, 2020, 7:08 p.m. UTC | #2
On Sat, May 30, 2020 at 7:38 PM Đoàn Trần Công Danh
<congdanhqx@gmail.com> wrote:
>
> On 2020-05-29 13:40:23+0000, Sibi Siddharthan via GitGitGadget <gitgitgadget@gmail.com> wrote:
> > From: Sibi Siddharthan <sibisiddharthan.github@gmail.com>
> >
> > This patch adds support for Visual Studio and Clang builds
> >
> > The minimum required version of CMake is upgraded to 3.15 because
> > this version offers proper support for Clang builds on Windows.
> >
> > Libintl is not searched for when building with Visual Studio or Clang
> > because there is no binary compatible version available yet.
> >
> > NOTE: In the link options invalidcontinue.obj has to be included.
> > The reason for this is because by default, Windows calls abort()'s
> > instead of setting errno=EINVAL when invalid arguments are passed to
> > standard functions.
> > This commit explains it in detail:
> > 4b623d80f73528a632576990ca51e34c333d5dd6
>
> I think it's better to say:
>
>         See 4b623d80f7 (MSVC: link in invalidcontinue.obj for better
>         POSIX compatibility, 2014-03-29) for detail.
>
>
> (I haven't read the referenced commit, FWIW)
>
> > On Windows the default generator is Visual Studio,so for Visual Studio
> > builds do this:
> >
> > cmake `relative-path-to-srcdir`
> >
> > NOTE: Visual Studio generator is a multi config generator, which means
> > that Debug and Release builds can be done on the same build directory.
> >
> > For Clang builds do this:
> >
> > On bash
> > CC=Clang cmake `relative-path-to-srcdir` -G Ninja
>
> I think you meant CC=clang, note the lowercase c,
> that will cmake this note applicable for case-sensitive filesystem.
>
> ... reading this patch ...
>
> So, this is applicable for Windows only, it's fine as is, then.
> It's still better to lowercase it, though.

will change it,(typo)

>
> >               -DCMAKE_BUILD_TYPE=[Debug or Release]
> >
> > On cmd
> > set CC=Clang
> > cmake `relative-path-to-srcdir` -G Ninja
> >               -DCMAKE_BUILD_TYPE=[Debug or Release]
> >
> > Signed-off-by: Sibi Siddharthan <sibisiddharthan.github@gmail.com>
> > ---
> >  contrib/buildsystems/CMakeLists.txt | 55 +++++++++++++++++++++++------
> >  1 file changed, 45 insertions(+), 10 deletions(-)
> >
> > diff --git a/contrib/buildsystems/CMakeLists.txt b/contrib/buildsystems/CMakeLists.txt
> > index 46197d0b806..0a3f711db88 100644
> > --- a/contrib/buildsystems/CMakeLists.txt
> > +++ b/contrib/buildsystems/CMakeLists.txt
> > @@ -98,8 +98,11 @@ find_package(ZLIB REQUIRED)
> >  find_package(CURL)
> >  find_package(EXPAT)
> >  find_package(Iconv)
> > -find_package(Intl)
> >
> > +#Don't use libintl on Windows Visual Studio and Clang builds
> > +if(NOT (WIN32 AND (CMAKE_C_COMPILER_ID STREQUAL "MSVC" OR CMAKE_C_COMPILER_ID STREQUAL "Clang")))
>
> Personally, I find this is a bit ugly.
> Does it work to move the if(WIN32) down there, here.
> Then make sub-condition for MSVC and Clang?
>
> > +     find_package(Intl)
> > +endif()
> >
> >  if(NOT Intl_FOUND)
> >       add_compile_definitions(NO_GETTEXT)
> > @@ -123,7 +126,7 @@ if(Intl_FOUND)
> >  endif()
> >
> >
> > -if(WIN32)
> > +if(WIN32 AND NOT MSVC)#not required for visual studio builds
>

Can do that.

> for the down there, I meant here.
> Using "NOT MSVC" here and `CMAKE_C_COMPILER_ID STREQUAL "MSVC"` above
> may puzzle people interest in this patch.
>
> >       find_program(WINDRES_EXE windres)
> >       if(NOT WINDRES_EXE)
> >               message(FATAL_ERROR "Install windres on Windows for resource files")
> > @@ -135,6 +138,13 @@ if(NOT MSGFMT_EXE)
> >       message(WARNING "Text Translations won't be build")
> >  endif()
> >
> > +#Force all visual studio outputs to CMAKE_BINARY_DIR
> > +if(CMAKE_C_COMPILER_ID STREQUAL "MSVC")
> > +     set(CMAKE_RUNTIME_OUTPUT_DIRECTORY_DEBUG ${CMAKE_BINARY_DIR})
> > +     set(CMAKE_RUNTIME_OUTPUT_DIRECTORY_RELEASE ${CMAKE_BINARY_DIR})
> > +     add_compile_options(/MP)
> > +endif()
> > +
> >  #default behaviour
> >  include_directories(${CMAKE_SOURCE_DIR})
> >  add_compile_definitions(GIT_HOST_CPU="${CMAKE_SYSTEM_PROCESSOR}")
> > @@ -172,6 +182,10 @@ endif()
> >
> >  #Platform Specific
> >  if(CMAKE_SYSTEM_NAME STREQUAL "Windows")
>
> I missed this line from previous patch
> (and my comment in previous round, which suggested remove ${})
> Does `if(WIN32)` and `if(CMAKE_SYSTEM_NAME STREQUAL "Windows")` have
> different meanings?
> I don't know much about CMake on Windows to really sure about this.
>

They are the same.
The reason for using CMAKE_SYSTEM_NAME instead of (WIN32,UNIX,APPLE...) is that
if you specify `if(UNIX)` it means both Linux and Darwin, whereas if you specify
if(CMAKE_SYSTEM_NAME STREQUAL "Linux") it means Linux only.

Thank You,
Sibi Siddharthan

> --
> Danh

Patch
diff mbox series

diff --git a/contrib/buildsystems/CMakeLists.txt b/contrib/buildsystems/CMakeLists.txt
index 46197d0b806..0a3f711db88 100644
--- a/contrib/buildsystems/CMakeLists.txt
+++ b/contrib/buildsystems/CMakeLists.txt
@@ -98,8 +98,11 @@  find_package(ZLIB REQUIRED)
 find_package(CURL)
 find_package(EXPAT)
 find_package(Iconv)
-find_package(Intl)
 
+#Don't use libintl on Windows Visual Studio and Clang builds
+if(NOT (WIN32 AND (CMAKE_C_COMPILER_ID STREQUAL "MSVC" OR CMAKE_C_COMPILER_ID STREQUAL "Clang")))
+	find_package(Intl)
+endif()
 
 if(NOT Intl_FOUND)
 	add_compile_definitions(NO_GETTEXT)
@@ -123,7 +126,7 @@  if(Intl_FOUND)
 endif()
 
 
-if(WIN32)
+if(WIN32 AND NOT MSVC)#not required for visual studio builds
 	find_program(WINDRES_EXE windres)
 	if(NOT WINDRES_EXE)
 		message(FATAL_ERROR "Install windres on Windows for resource files")
@@ -135,6 +138,13 @@  if(NOT MSGFMT_EXE)
 	message(WARNING "Text Translations won't be build")
 endif()
 
+#Force all visual studio outputs to CMAKE_BINARY_DIR
+if(CMAKE_C_COMPILER_ID STREQUAL "MSVC")
+	set(CMAKE_RUNTIME_OUTPUT_DIRECTORY_DEBUG ${CMAKE_BINARY_DIR})
+	set(CMAKE_RUNTIME_OUTPUT_DIRECTORY_RELEASE ${CMAKE_BINARY_DIR})
+	add_compile_options(/MP)
+endif()
+
 #default behaviour
 include_directories(${CMAKE_SOURCE_DIR})
 add_compile_definitions(GIT_HOST_CPU="${CMAKE_SYSTEM_PROCESSOR}")
@@ -172,6 +182,10 @@  endif()
 
 #Platform Specific
 if(CMAKE_SYSTEM_NAME STREQUAL "Windows")
+	if(CMAKE_C_COMPILER_ID STREQUAL "MSVC" OR CMAKE_C_COMPILER_ID STREQUAL "Clang")
+		include_directories(${CMAKE_SOURCE_DIR}/compat/vcbuild/include)
+		add_compile_definitions(_CRT_SECURE_NO_WARNINGS _CRT_NONSTDC_NO_DEPRECATE)
+	endif()
 	include_directories(${CMAKE_SOURCE_DIR}/compat/win32)
 	add_compile_definitions(HAVE_ALLOCA_H NO_POSIX_GOODIES NATIVE_CRLF NO_UNIX_SOCKETS WIN32
 				_CONSOLE DETECT_MSYS_TTY STRIP_EXTENSION=".exe"  NO_SYMLINK_HEAD UNRELIABLE_FSTAT
@@ -551,14 +565,22 @@  parse_makefile_for_sources(libvcs-svn_SOURCES "VCSSVN_OBJS")
 list(TRANSFORM libvcs-svn_SOURCES PREPEND "${CMAKE_SOURCE_DIR}/")
 add_library(vcs-svn STATIC ${libvcs-svn_SOURCES})
 
-#add git.rc for gcc
 if(WIN32)
-	add_custom_command(OUTPUT ${CMAKE_BINARY_DIR}/git.res
-			COMMAND ${WINDRES_EXE} -O coff -DMAJOR=${PROJECT_VERSION_MAJOR} -DMINOR=${PROJECT_VERSION_MINOR}
-				-DMICRO=${PROJECT_VERSION_PATCH} -DPATCHLEVEL=0 -DGIT_VERSION="\\\"${PROJECT_VERSION}.GIT\\\""
-				-i ${CMAKE_SOURCE_DIR}/git.rc -o ${CMAKE_BINARY_DIR}/git.res
-			WORKING_DIRECTORY ${CMAKE_SOURCE_DIR}
-			VERBATIM)
+	if(NOT MSVC)#use windres when compiling with gcc and clang
+		add_custom_command(OUTPUT ${CMAKE_BINARY_DIR}/git.res
+				COMMAND ${WINDRES_EXE} -O coff -DMAJOR=${PROJECT_VERSION_MAJOR} -DMINOR=${PROJECT_VERSION_MINOR}
+					-DMICRO=${PROJECT_VERSION_PATCH} -DPATCHLEVEL=0 -DGIT_VERSION="\\\"${PROJECT_VERSION}.GIT\\\""
+					-i ${CMAKE_SOURCE_DIR}/git.rc -o ${CMAKE_BINARY_DIR}/git.res
+				WORKING_DIRECTORY ${CMAKE_SOURCE_DIR}
+				VERBATIM)
+	else()#MSVC use rc
+		add_custom_command(OUTPUT ${CMAKE_BINARY_DIR}/git.res
+				COMMAND ${CMAKE_RC_COMPILER} /d MAJOR=${PROJECT_VERSION_MAJOR} /d MINOR=${PROJECT_VERSION_MINOR}
+					/d MICRO=${PROJECT_VERSION_PATCH} /d PATCHLEVEL=0 /d GIT_VERSION="${PROJECT_VERSION}.GIT"
+					/fo ${CMAKE_BINARY_DIR}/git.res ${CMAKE_SOURCE_DIR}/git.rc
+				WORKING_DIRECTORY ${CMAKE_SOURCE_DIR}
+				VERBATIM)
+	endif()
 	add_custom_target(git-rc DEPENDS ${CMAKE_BINARY_DIR}/git.res)
 endif()
 
@@ -575,7 +597,13 @@  endif()
 if(WIN32)
 	target_link_libraries(common-main ws2_32 ntdll ${CMAKE_BINARY_DIR}/git.res)
 	add_dependencies(common-main git-rc)
-	target_link_options(common-main PUBLIC -municode -Wl,--nxcompat -Wl,--dynamicbase -Wl,--pic-executable,-e,mainCRTStartup)
+	if(CMAKE_C_COMPILER_ID STREQUAL "GNU")
+		target_link_options(common-main PUBLIC -municode -Wl,--nxcompat -Wl,--dynamicbase -Wl,--pic-executable,-e,mainCRTStartup)
+	elseif(CMAKE_C_COMPILER_ID STREQUAL "Clang")
+		target_link_options(common-main PUBLIC -municode -Wl,-nxcompat -Wl,-dynamicbase -Wl,-entry:wmainCRTStartup -Wl,invalidcontinue.obj)
+	elseif(CMAKE_C_COMPILER_ID STREQUAL "MSVC")
+		target_link_options(common-main PUBLIC /IGNORE:4217 /IGNORE:4049 /NOLOGO /ENTRY:wmainCRTStartup /SUBSYSTEM:CONSOLE invalidcontinue.obj)
+	endif()
 elseif(UNIX)
 	target_link_libraries(common-main pthread rt)
 endif()
@@ -831,6 +859,13 @@  target_link_libraries(test-tool common-main)
 set_target_properties(test-fake-ssh test-line-buffer test-svn-fe test-tool
 			PROPERTIES RUNTIME_OUTPUT_DIRECTORY ${CMAKE_BINARY_DIR}/t/helper)
 
+if(MSVC)
+	set_target_properties(test-fake-ssh test-line-buffer test-svn-fe test-tool
+				PROPERTIES RUNTIME_OUTPUT_DIRECTORY_DEBUG ${CMAKE_BINARY_DIR}/t/helper)
+	set_target_properties(test-fake-ssh test-line-buffer test-svn-fe test-tool
+				PROPERTIES RUNTIME_OUTPUT_DIRECTORY_RELEASE ${CMAKE_BINARY_DIR}/t/helper)
+endif()
+
 #wrapper scripts
 set(wrapper_scripts
 	git git-upload-pack git-receive-pack git-upload-archive git-shell git-remote-ext)