Message ID | f496cd7d8aa12047db7f6c0212fbcb2497469785.1589302255.git.gitgitgadget@gmail.com (mailing list archive) |
---|---|
State | New, archived |
Headers | show |
Series | CMake build system for git | expand |
"Sibi Siddharthan via GitGitGadget" <gitgitgadget@gmail.com> writes: > From: Sibi Siddharthan <sibisiddharthan.github@gmail.com> > Subject: Re: [PATCH v2 08/11] cmake: added checks for struct stat and libiconv s/added/add/; give a command to the codebase to "be like so", or a command to whoever is typing changes to the editor to "make this happen". > The CMake script now checks whether st_blocks is a member of struct stat > and set the compile definition NO_ST_BLOCKS_IN_STRUCT_STAT accordingly. Teach the CMake script to check ... > diff --git a/CMakeLists.txt b/CMakeLists.txt > index 4353080b708..975791c8b89 100644 > --- a/CMakeLists.txt > +++ b/CMakeLists.txt > @@ -22,6 +22,7 @@ project(git > include(CheckTypeSize) > include(CheckCSourceRuns) > include(CheckCSourceCompiles) > +include(CheckCSourceRuns) > ... > +#check for st_blocks in struct stat > +check_struct_has_member("struct stat" st_blocks "sys/stat.h" STRUCT_STAT_HAS_ST_BLOCKS) > +if(NOT STRUCT_STAT_HAS_ST_BLOCKS) > + add_compile_definitions(NO_ST_BLOCKS_IN_STRUCT_STAT) > +endif() All of these compatibility stuff makes sense, but how do we decide where to draw the line between the checks we saw added in [01/11] and the checks added here? It feels pretty arbitrary to me and if that is the case, perhaps we want to move all the "add checks" to a commit separate from [01/11] (whose primary purpose is to add the basic rules without these build tweaks in the file at the final place)?
On Wed, May 13, 2020 at 2:46 AM Junio C Hamano <gitster@pobox.com> wrote: > > "Sibi Siddharthan via GitGitGadget" <gitgitgadget@gmail.com> writes: > > > From: Sibi Siddharthan <sibisiddharthan.github@gmail.com> > > Subject: Re: [PATCH v2 08/11] cmake: added checks for struct stat and libiconv > > s/added/add/; give a command to the codebase to "be like so", or a > command to whoever is typing changes to the editor to "make this > happen". > > > The CMake script now checks whether st_blocks is a member of struct stat > > and set the compile definition NO_ST_BLOCKS_IN_STRUCT_STAT accordingly. > > Teach the CMake script to check ... > > > diff --git a/CMakeLists.txt b/CMakeLists.txt > > index 4353080b708..975791c8b89 100644 > > --- a/CMakeLists.txt > > +++ b/CMakeLists.txt > > @@ -22,6 +22,7 @@ project(git > > include(CheckTypeSize) > > include(CheckCSourceRuns) > > include(CheckCSourceCompiles) > > +include(CheckCSourceRuns) > > ... > > +#check for st_blocks in struct stat > > +check_struct_has_member("struct stat" st_blocks "sys/stat.h" STRUCT_STAT_HAS_ST_BLOCKS) > > +if(NOT STRUCT_STAT_HAS_ST_BLOCKS) > > + add_compile_definitions(NO_ST_BLOCKS_IN_STRUCT_STAT) > > +endif() > > All of these compatibility stuff makes sense, but how do we decide > where to draw the line between the checks we saw added in [01/11] > and the checks added here? It feels pretty arbitrary to me and if > that is the case, perhaps we want to move all the "add checks" to a > commit separate from [01/11] (whose primary purpose is to add the > basic rules without these build tweaks in the file at the final > place)? > The checks are added on a "demand" based the target platform. In the future, if apple support is needed, we need to add ST_TIMESPEC checks. Thank You, Sibi Siddharthan
Sibi Siddharthan <sibisiddharthan.github@gmail.com> writes: >> All of these compatibility stuff makes sense, but how do we decide >> where to draw the line between the checks we saw added in [01/11] >> and the checks added here? It feels pretty arbitrary to me and if >> that is the case, perhaps we want to move all the "add checks" to a >> commit separate from [01/11] (whose primary purpose is to add the >> basic rules without these build tweaks in the file at the final >> place)? >> > > The checks are added on a "demand" based the target platform. > In the future, if apple support is needed, we need to add ST_TIMESPEC checks. Well, let's ask a related question then. Given that the primary reason why the project may be interested in adding cmake-based build system is because of its support for Windows build, how much smaller would this 11-patch series can become if we discard support for any platforms other than Windows? Let's say we add support for other platforms only after the series proves capable to build on/for Windows by going through the usual "queued in 'pu', merged to 'next' and then down to 'master'" route and appears in a tagged release. Would it reduce the time we need to spend before seeing a cmake-based build for Windows by doing so?
On 2020-05-12 16:50:51+0000, Sibi Siddharthan via GitGitGadget <gitgitgadget@gmail.com> wrote: > From: Sibi Siddharthan <sibisiddharthan.github@gmail.com> > > The CMake script now checks whether st_blocks is a member of struct stat > and set the compile definition NO_ST_BLOCKS_IN_STRUCT_STAT accordingly. > > The check for whether ICONV_OMITS_BOM is also added as requested by Danh. Please don't write my name in the commit message like this. This maybe rephased to: While we're as it, add the check for ICONV_OMITS_BOM. > Signed-off-by: Sibi Siddharthan <sibisiddharthan.github@gmail.com> > --- > CMakeLists.txt | 59 ++++++++++++++++++++++++++++++++++++++++++++++++-- > 1 file changed, 57 insertions(+), 2 deletions(-) > > diff --git a/CMakeLists.txt b/CMakeLists.txt > index 4353080b708..975791c8b89 100644 > --- a/CMakeLists.txt > +++ b/CMakeLists.txt > @@ -22,6 +22,7 @@ project(git > include(CheckTypeSize) > include(CheckCSourceRuns) > include(CheckCSourceCompiles) > +include(CheckCSourceRuns) > include(CheckIncludeFile) > include(CheckFunctionExists) > include(CheckSymbolExists) > @@ -128,7 +129,7 @@ if(${CMAKE_SYSTEM_NAME} STREQUAL "Windows") > include_directories(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 > - NOGDI OBJECT_CREATION_MODE=1 __USE_MINGW_ANSI_STDIO=0 NO_ST_BLOCKS_IN_STRUCT_STAT > + NOGDI OBJECT_CREATION_MODE=1 __USE_MINGW_ANSI_STDIO=0 > USE_NED_ALLOCATOR OVERRIDE_STRDUP MMAP_PREVENTS_DELETE USE_WIN32_MMAP > UNICODE _UNICODE HAVE_WPGMPTR ENSURE_MSYSTEM_IS_SET) > list(APPEND compat_SOURCES compat/mingw.c compat/winansi.c compat/win32/path-utils.c > @@ -280,6 +281,11 @@ if(HAVE_CLOCK_MONOTONIC) > add_compile_definitions(HAVE_CLOCK_MONOTONIC) > endif() > > +#check for st_blocks in struct stat > +check_struct_has_member("struct stat" st_blocks "sys/stat.h" STRUCT_STAT_HAS_ST_BLOCKS) > +if(NOT STRUCT_STAT_HAS_ST_BLOCKS) > + add_compile_definitions(NO_ST_BLOCKS_IN_STRUCT_STAT) > +endif() > > #compile checks > check_c_source_runs(" > @@ -344,7 +350,6 @@ if(NOT HAVE_REGEX) > add_compile_definitions(NO_REGEX NO_MBSUPPORT GAWK) > endif() > > - > check_c_source_compiles(" > #include <stddef.h> > #include <sys/types.h> > @@ -368,6 +373,56 @@ if(HAVE_BSD_SYSCTL) > add_compile_definitions(HAVE_BSD_SYSCTL) > endif() > > +set(CMAKE_REQUIRED_LIBRARIES ${Iconv_LIBRARIES}) > +set(CMAKE_REQUIRED_INCLUDES ${Iconv_INCLUDE_DIRS}) > + > +check_c_source_compiles(" > +#include <iconv.h> > + > +extern size_t iconv(iconv_t cd, > + char **inbuf, size_t *inbytesleft, > + char **outbuf, size_t *outbytesleft); > + > +int main(){return 0;}" > +HAVE_NEW_ICONV) > +if(HAVE_NEW_ICONV) > + set(HAVE_OLD_ICONV 0) > +else() > + set(HAVE_OLD_ICONV 1) > +endif() > + > +check_c_source_runs(" > +#include <iconv.h> > +#if ${HAVE_OLD_ICONV} > +typedef const char *iconv_ibp; > +#else > +typedef char *iconv_ibp; > +#endif > + > +int main() > +{ > + int v; > + iconv_t conv; > + char in[] = \"a\"; iconv_ibp pin = in; > + char out[20] = \"\"; char *pout = out; > + size_t isz = sizeof in; > + size_t osz = sizeof out; > + > + conv = iconv_open(\"UTF-16\", \"UTF-8\"); > + iconv(conv, &pin, &isz, &pout, &osz); > + iconv_close(conv); > + v = (unsigned char)(out[0]) + (unsigned char)(out[1]); > + return v != 0xfe + 0xff; > +}" I think the closing double-quote should be placed in a newline, in order to make sure the source file ended with newline, old C standard requires final newline.
On Thu, May 14, 2020 at 9:01 PM Đoàn Trần Công Danh <congdanhqx@gmail.com> wrote: > > On 2020-05-12 16:50:51+0000, Sibi Siddharthan via GitGitGadget <gitgitgadget@gmail.com> wrote: > > From: Sibi Siddharthan <sibisiddharthan.github@gmail.com> > > > > The CMake script now checks whether st_blocks is a member of struct stat > > and set the compile definition NO_ST_BLOCKS_IN_STRUCT_STAT accordingly. > > > > The check for whether ICONV_OMITS_BOM is also added as requested by Danh. > > Please don't write my name in the commit message like this. > This maybe rephased to: > > While we're as it, add the check for ICONV_OMITS_BOM. Sure, will remove your name from the message. I added it because gitgitgadget PR bot suggested it as a good practice. > > > Signed-off-by: Sibi Siddharthan <sibisiddharthan.github@gmail.com> > > --- > > CMakeLists.txt | 59 ++++++++++++++++++++++++++++++++++++++++++++++++-- > > 1 file changed, 57 insertions(+), 2 deletions(-) > > > > diff --git a/CMakeLists.txt b/CMakeLists.txt > > index 4353080b708..975791c8b89 100644 > > --- a/CMakeLists.txt > > +++ b/CMakeLists.txt > > @@ -22,6 +22,7 @@ project(git > > include(CheckTypeSize) > > include(CheckCSourceRuns) > > include(CheckCSourceCompiles) > > +include(CheckCSourceRuns) > > include(CheckIncludeFile) > > include(CheckFunctionExists) > > include(CheckSymbolExists) > > @@ -128,7 +129,7 @@ if(${CMAKE_SYSTEM_NAME} STREQUAL "Windows") > > include_directories(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 > > - NOGDI OBJECT_CREATION_MODE=1 __USE_MINGW_ANSI_STDIO=0 NO_ST_BLOCKS_IN_STRUCT_STAT > > + NOGDI OBJECT_CREATION_MODE=1 __USE_MINGW_ANSI_STDIO=0 > > USE_NED_ALLOCATOR OVERRIDE_STRDUP MMAP_PREVENTS_DELETE USE_WIN32_MMAP > > UNICODE _UNICODE HAVE_WPGMPTR ENSURE_MSYSTEM_IS_SET) > > list(APPEND compat_SOURCES compat/mingw.c compat/winansi.c compat/win32/path-utils.c > > @@ -280,6 +281,11 @@ if(HAVE_CLOCK_MONOTONIC) > > add_compile_definitions(HAVE_CLOCK_MONOTONIC) > > endif() > > > > +#check for st_blocks in struct stat > > +check_struct_has_member("struct stat" st_blocks "sys/stat.h" STRUCT_STAT_HAS_ST_BLOCKS) > > +if(NOT STRUCT_STAT_HAS_ST_BLOCKS) > > + add_compile_definitions(NO_ST_BLOCKS_IN_STRUCT_STAT) > > +endif() > > > > #compile checks > > check_c_source_runs(" > > @@ -344,7 +350,6 @@ if(NOT HAVE_REGEX) > > add_compile_definitions(NO_REGEX NO_MBSUPPORT GAWK) > > endif() > > > > - > > check_c_source_compiles(" > > #include <stddef.h> > > #include <sys/types.h> > > @@ -368,6 +373,56 @@ if(HAVE_BSD_SYSCTL) > > add_compile_definitions(HAVE_BSD_SYSCTL) > > endif() > > > > +set(CMAKE_REQUIRED_LIBRARIES ${Iconv_LIBRARIES}) > > +set(CMAKE_REQUIRED_INCLUDES ${Iconv_INCLUDE_DIRS}) > > + > > +check_c_source_compiles(" > > +#include <iconv.h> > > + > > +extern size_t iconv(iconv_t cd, > > + char **inbuf, size_t *inbytesleft, > > + char **outbuf, size_t *outbytesleft); > > + > > +int main(){return 0;}" > > +HAVE_NEW_ICONV) > > +if(HAVE_NEW_ICONV) > > + set(HAVE_OLD_ICONV 0) > > +else() > > + set(HAVE_OLD_ICONV 1) > > +endif() > > + > > +check_c_source_runs(" > > +#include <iconv.h> > > +#if ${HAVE_OLD_ICONV} > > +typedef const char *iconv_ibp; > > +#else > > +typedef char *iconv_ibp; > > +#endif > > + > > +int main() > > +{ > > + int v; > > + iconv_t conv; > > + char in[] = \"a\"; iconv_ibp pin = in; > > + char out[20] = \"\"; char *pout = out; > > + size_t isz = sizeof in; > > + size_t osz = sizeof out; > > + > > + conv = iconv_open(\"UTF-16\", \"UTF-8\"); > > + iconv(conv, &pin, &isz, &pout, &osz); > > + iconv_close(conv); > > + v = (unsigned char)(out[0]) + (unsigned char)(out[1]); > > + return v != 0xfe + 0xff; > > +}" > > I think the closing double-quote should be placed in a newline, > in order to make sure the source file ended with newline, > old C standard requires final newline. > Okay Thank You, Sibi Siddharthan > > -- > Danh
diff --git a/CMakeLists.txt b/CMakeLists.txt index 4353080b708..975791c8b89 100644 --- a/CMakeLists.txt +++ b/CMakeLists.txt @@ -22,6 +22,7 @@ project(git include(CheckTypeSize) include(CheckCSourceRuns) include(CheckCSourceCompiles) +include(CheckCSourceRuns) include(CheckIncludeFile) include(CheckFunctionExists) include(CheckSymbolExists) @@ -128,7 +129,7 @@ if(${CMAKE_SYSTEM_NAME} STREQUAL "Windows") include_directories(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 - NOGDI OBJECT_CREATION_MODE=1 __USE_MINGW_ANSI_STDIO=0 NO_ST_BLOCKS_IN_STRUCT_STAT + NOGDI OBJECT_CREATION_MODE=1 __USE_MINGW_ANSI_STDIO=0 USE_NED_ALLOCATOR OVERRIDE_STRDUP MMAP_PREVENTS_DELETE USE_WIN32_MMAP UNICODE _UNICODE HAVE_WPGMPTR ENSURE_MSYSTEM_IS_SET) list(APPEND compat_SOURCES compat/mingw.c compat/winansi.c compat/win32/path-utils.c @@ -280,6 +281,11 @@ if(HAVE_CLOCK_MONOTONIC) add_compile_definitions(HAVE_CLOCK_MONOTONIC) endif() +#check for st_blocks in struct stat +check_struct_has_member("struct stat" st_blocks "sys/stat.h" STRUCT_STAT_HAS_ST_BLOCKS) +if(NOT STRUCT_STAT_HAS_ST_BLOCKS) + add_compile_definitions(NO_ST_BLOCKS_IN_STRUCT_STAT) +endif() #compile checks check_c_source_runs(" @@ -344,7 +350,6 @@ if(NOT HAVE_REGEX) add_compile_definitions(NO_REGEX NO_MBSUPPORT GAWK) endif() - check_c_source_compiles(" #include <stddef.h> #include <sys/types.h> @@ -368,6 +373,56 @@ if(HAVE_BSD_SYSCTL) add_compile_definitions(HAVE_BSD_SYSCTL) endif() +set(CMAKE_REQUIRED_LIBRARIES ${Iconv_LIBRARIES}) +set(CMAKE_REQUIRED_INCLUDES ${Iconv_INCLUDE_DIRS}) + +check_c_source_compiles(" +#include <iconv.h> + +extern size_t iconv(iconv_t cd, + char **inbuf, size_t *inbytesleft, + char **outbuf, size_t *outbytesleft); + +int main(){return 0;}" +HAVE_NEW_ICONV) +if(HAVE_NEW_ICONV) + set(HAVE_OLD_ICONV 0) +else() + set(HAVE_OLD_ICONV 1) +endif() + +check_c_source_runs(" +#include <iconv.h> +#if ${HAVE_OLD_ICONV} +typedef const char *iconv_ibp; +#else +typedef char *iconv_ibp; +#endif + +int main() +{ + int v; + iconv_t conv; + char in[] = \"a\"; iconv_ibp pin = in; + char out[20] = \"\"; char *pout = out; + size_t isz = sizeof in; + size_t osz = sizeof out; + + conv = iconv_open(\"UTF-16\", \"UTF-8\"); + iconv(conv, &pin, &isz, &pout, &osz); + iconv_close(conv); + v = (unsigned char)(out[0]) + (unsigned char)(out[1]); + return v != 0xfe + 0xff; +}" +ICONV_DOESNOT_OMIT_BOM) +if(NOT ICONV_DOESNOT_OMIT_BOM) + add_compile_definitions(ICONV_OMITS_BOM) +endif() + +unset(CMAKE_REQUIRED_LIBRARIES) +unset(CMAKE_REQUIRED_INCLUDES) + + #programs set(PROGRAMS_BUILT git git-bugreport git-credential-store git-daemon git-fast-import git-http-backend git-sh-i18n--envsubst