| Message ID | 8f36e30cd2218fa56fb0c2b89f6f51726b0eb285.1590759624.git.gitgitgadget@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | CMake build system for git | expand |
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.
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
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)