Re: [PATCH v3 7/8] cmake: support for building git on windows with msvc and clang.

["Đoàn Trần Công Danh" <congdanhqx@gmail.com>]

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.

-- 
Danh