[v2,08/11] cmake: added checks for struct stat and libiconv

Message ID	f496cd7d8aa12047db7f6c0212fbcb2497469785.1589302255.git.gitgitgadget@gmail.com (mailing list archive)
State	New, archived
Headers	show Return-Path: <SRS0=6g9E=62=vger.kernel.org=git-owner@kernel.org> Message-Id: <f496cd7d8aa12047db7f6c0212fbcb2497469785.1589302255.git.gitgitgadget@gmail.com> In-Reply-To: <pull.614.v2.git.1589302254.gitgitgadget@gmail.com> References: <pull.614.git.1587700897.gitgitgadget@gmail.com> <pull.614.v2.git.1589302254.gitgitgadget@gmail.com> From: "Sibi Siddharthan via GitGitGadget" <gitgitgadget@gmail.com> Date: Tue, 12 May 2020 16:50:51 +0000 Subject: [PATCH v2 08/11] cmake: added checks for struct stat and libiconv Fcc: Sent Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit MIME-Version: 1.0 To: git@vger.kernel.org Cc: Sibi Siddharthan <sibisiddharthan.github@gmail.com>, Sibi Siddharthan <sibisiddharthan.github@gmail.com> Sender: git-owner@vger.kernel.org Precedence: bulk
Series	CMake build system for git \| expand [v2,00/11] CMake build system for git [v2,01/11] Introduce CMake support for configuring Git on Linux [v2,02/11] cmake: generate the shell/perl/python scripts and templates, translations [v2,03/11] cmake: installation support for git [v2,04/11] cmake: support for testing git with ctest [v2,05/11] cmake: support for testing git when building out of the source tree [v2,06/11] cmake: support for building git on windows with mingw [v2,07/11] cmake: support for building git on windows with msvc and clang. [v2,08/11] cmake: added checks for struct stat and libiconv [v2,09/11] cmake: relocated script file contrib/buildsystems [v2,10/11] cmake: parse the makefile for the sources. [v2,11/11] ci: modification of main.yml to use cmake for vs-build job

Derrick Stolee via GitGitGadget May 12, 2020, 4:50 p.m. UTC

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.

Signed-off-by: Sibi Siddharthan <sibisiddharthan.github@gmail.com>
---
 CMakeLists.txt | 59 ++++++++++++++++++++++++++++++++++++++++++++++++--
 1 file changed, 57 insertions(+), 2 deletions(-)

Junio C Hamano May 12, 2020, 9:16 p.m. UTC | #1

"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)?

Sibi Siddharthan May 13, 2020, 8:05 p.m. UTC | #2

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

Junio C Hamano May 14, 2020, 2 a.m. UTC | #3

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?

Đoàn Trần Công Danh May 14, 2020, 3:31 p.m. UTC | #4

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.

Sibi Siddharthan May 14, 2020, 6:31 p.m. UTC | #5

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

[v2,08/11] cmake: added checks for struct stat and libiconv

Commit Message

Comments

Patch