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

[Sibi Siddharthan <sibisiddharthan.github@gmail.com>]

On Sat, May 30, 2020 at 12:57 AM Junio C Hamano <gitster@pobox.com> wrote:
>
> "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.

Do you mean this line '---' below?
If so how do I place the changelog below them?
I just do `/submit` at gigitgadet/git.

>
> > 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.
>

Okay

> > +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.
>

Will specify for other systems as well.

> > +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;
> > +}"
>

Will format the checks.

Thank You,
Sibi Siddharthan

> Thanks.