Re: [PATCH v3 1/8] Introduce CMake support for configuring Git

[Junio C Hamano <gitster@pobox.com>]

"Sibi Siddharthan via GitGitGadget" <gitgitgadget@gmail.com> writes:

> From: Sibi Siddharthan <sibisiddharthan.github@gmail.com>
>
> At the moment, the recommended way to configure Git's builds is to
> ...
> Note: this patch asks for the minimum version v3.14 of CMake (which is
> not all that old as of time of writing) because that is the first
> version to offer a platform-independent way to generate hardlinks as
> part of the build. This is needed to generate all those hardlinks for
> the built-in commands of Git.

All of the above reads reasonable.

> Changes
> The CMake script parses the Makefile for:
> LIB_OBJS
> BUILTIN_OBJS
> XDIFF_OBJS
> VCSSVN_OBJS
> TEST_BUILTINS_OBJS
>
> By doing this we avoid duplication of text between the Makefile and
> the CMake script.
>
> The CMake script has been relocated to contrib/buildsystems.
>
> The CMake script uses GIT-VERSION-GEN to determine the version of Git
> being built.

Everything after the "Changes" line does not belong to the commit
log, as it is no use for those who read "git log" output and
encounter the "first" attempt to add CMake support there.  There is
no need to tell them that you did things differently from this
version in the past, as they simply do not know what you did in the
previous iterations, nor particularly care about them.

These *do* help reviewers who saw previous iterations, and the space
after the three-dash line below is the right/recommended place for
them.

The above applies to other patches in this series.

> Signed-off-by: Sibi Siddharthan <sibisiddharthan.github@gmail.com>
> ---
>  contrib/buildsystems/CMakeLists.txt | 575 ++++++++++++++++++++++++++++
>  1 file changed, 575 insertions(+)
>  create mode 100644 contrib/buildsystems/CMakeLists.txt
>
> diff --git a/contrib/buildsystems/CMakeLists.txt b/contrib/buildsystems/CMakeLists.txt
> new file mode 100644
> index 00000000000..8e2b27f44a6
> --- /dev/null
> +++ b/contrib/buildsystems/CMakeLists.txt
> @@ -0,0 +1,575 @@
> +#
> +#	Copyright (c) 2020 Sibi Siddharthan
> +#
> +
> +#[[
> +
> +Instructions to run CMake:
> +
> +cmake `relative-path-to-CMakeLists.txt` -DCMAKE_BUILD_TYPE=Release

The readers can infer from `relative-path-to-CMakeLists` that they
can run this command from anywhere, e.g.

	$ cd $HOME
	$ tar xf /var/tmp/git-2.7.0.tar
	$ cd /tmp
	$ cmake $HOME/git/contrib/buildsystems/CMakeLists.txt

but when given the freedom/flexibility to use it from "anywhere",
they are faced by an extra choice to make.  It may be helpful to
readers to elaborate a bit more to help them decide where in the
directory hierarchy they would want to run this command.  In the
above example sequence, I chose /tmp, but if I used /var/tmp as the
starting place instead, what becomes different?  The answer might
be "resulting 'git' binary is stored in the directory you run the
'cmake' command in", and by spelling it out, it helps readers to
make an informed decision.

> +Possible build configurations(-DCMAKE_BUILD_TYPE) with corresponding
> +compiler flags
> +Debug : -g
> +Release: -O3
> +RelWithDebInfo : -O2 -g
> +MinSizeRel : -Os
> +empty(default) :
> +
> +NOTE: -DCMAKE_BUILD_TYPE is optional. For multi-config generators like Visual Studio
> +this option is ignored

Quite helpful.

> +This process generates a Makefile(Linux) , Visual Studio solution(Windows) by default.
> +Run `make` to build Git on Linux.
> +Open git.sln on Windows and build Git.
> +
> +NOTE: By default CMake uses Makefile as the build tool on Linux and Visual Studio in Windows,

The above makes it sound as if Linux and VS are the only two systems
we care about, but is it really Linux, or UNIX-flavoured systems in
general?  In other words, are we excluding friends on BSD and macOS
with the above?

The above is not a complaint about "exclusion", but is a complaint
about unclarity.

> +to use another tool say `ninja` add this to the command line when configuring.
> +`-G Ninja`
> +
> +]]
> +cmake_minimum_required(VERSION 3.14)
> ...
> +check_c_source_compiles("

The "source" given to check_c_source_{compiles,runs} may be allowed
to be anything, but I'd prefer to see it follow some consistent
style, preferrably our CodingGuidelines (except for git specific
bits like "do not include standard system file but instead just use
git-compat-util.h", which of course should not apply).  This is a
comment not only about the two instances below I use as examples,
but all C-source snippets in this patch.

> +#include <alloca.h>
> +int
> +main ()
> +{

	int main(void)
	{

is how we start this function.

> +char *p = (char *) alloca (2 * sizeof (int));

And the decl of function local variable here would be indented by a
HT like the remainder of the function.  No SP between function name
and the parentheses around its arguments.  NP SP after sizeof,
either.

> +	if (p) return 0;
> +	return 0;
> +}"
> +HAVE_ALLOCA_H)
> ...
> +check_c_source_runs("
> +#include<stdio.h>
> +#include<stdarg.h>
> +#include<string.h>
> +#include<stdlib.h>
> +int test_vsnprintf(char *str, size_t maxsize, const char *format, ...)
> +{
> +	int ret;
> +	va_list ap;
> +
> +	va_start(ap, format);
> +	ret = vsnprintf(str, maxsize, format, ap);
> +	va_end(ap);
> +	return ret;
> +}
> +
> +int
> +main ()
> +{

Likewise.

> +	char buf[6];
> +	if (test_vsnprintf(buf, 3, \"%s\", \"12345\") != 5
> +		|| strcmp(buf, \"12\")) return 1;
> +	if (snprintf(buf, 3, \"%s\", \"12345\") != 5
> +		|| strcmp(buf, \"12\")) return 1;
> +
> +	return 0;
> +}"

Thanks.