On Sat, May 30, 2020 at 7:38 PM Đoàn Trần Công Danh
<congdanhqx@xxxxxxxxx> wrote:
>
> On 2020-05-29 13:40:23+0000, Sibi Siddharthan via GitGitGadget <gitgitgadget@xxxxxxxxx> wrote:
> > From: Sibi Siddharthan <sibisiddharthan.github@xxxxxxxxx>
> >
> > 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@xxxxxxxxx>
> > ---
> > 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
