Re: [PATCH v2] Makefile: add support for generating JSON compilation database

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

"Philippe Blain via GitGitGadget" <gitgitgadget@gmail.com> writes:

> diff --git a/.gitignore b/.gitignore
> index ee509a2ad2..f4c51300e0 100644
> --- a/.gitignore
> +++ b/.gitignore
> @@ -197,6 +197,7 @@
>  /git.spec
>  *.exe
>  *.[aos]
> +*.o.json

Does this naming/suffix follow the common practice followed among
those projects that use the -MJ option?  Even though I may have
heard of it, I am not familiar with the use of the feature at all,
and to such a person, naming a file after what format it is written
in (i.e. 'json') feels a lot less useful than what it contains
(i.e. compilation database entries).  

It's like naming our source files with .txt suffix ;-)

> +# Define GENERATE_COMPILATION_DATABASE to generate JSON compilation database
> +# entries during compilation if your compiler supports it, using the `-MJ` flag.
> +# The JSON entries will be placed in the `compile_commands/` directory,
> +# and the JSON compilation database 'compile_commands.json' will be created 
> +# at the root of the repository. 

Likewise for the name of the directory and the resulting single
database file.  I just want to make sure that we are following the
common convention, so people familiar with the use of the feature
would feel at home, so a simple answer "yes" would suffice.

> +ifdef GENERATE_COMPILATION_DATABASE
> +compdb_check = $(shell $(CC) $(ALL_CFLAGS) \
> +	-c -MJ /dev/null \
> +	-x c /dev/null -o /dev/null 2>&1; \
> +	echo $$?)
> +ifeq ($(compdb_check),0)
> +override GENERATE_COMPILATION_DATABASE = yes

This feels strange.  If the end user said to GENERATE and we find we
are capable, we still override to 'yes'?  What if the end user set
'no' to the GENERATE_COMPILATION_DATABASE macro?  Shouldn't we be
honoring that wish?

> +else
> +override GENERATE_COMPILATION_DATABASE = no
> +$(warning GENERATE_COMPILATION_DATABASE is set, but your compiler does not \
> +support generating compilation database entries)

This side is perfectly understandable and I think it is a valid use
of override.  But I do not understand the other side.

> @@ -2381,16 +2401,30 @@ missing_dep_dirs =
>  dep_args =
>  endif
>  
> +compdb_dir = compile_commands/
> +
> +ifeq ($(GENERATE_COMPILATION_DATABASE),yes)
> +missing_compdb_dir = $(compdb_dir)
> +$(missing_compdb_dir):
> +	@mkdir -p $@
> +
> +compdb_file = $(compdb_dir)$(subst .-,,$(subst /,-,$(dir $@)))$(notdir $@).json

This detail does not matter as long as the end result ensures unique
output for all source files, but I am having trouble guessing what
the outermost subst, which removes ".-" sequence, is trying to make
prettier.  Care to explain?

> +compdb_args = -MJ $(compdb_file)
> +else
> +missing_compdb_dir =
> +compdb_args =

These are unfortunate.  I briefly wondered if we can make GIT-CFLAGS
depend on these two variables so that ASM_OBJ and C_OBJ do not have
to depend on them, but the actual rules need to be updated anyway,
so it cannot be helped.

I do wonder if we can unify dep_args and compdb_args into a single
extra_args (and similarly dep_dirs and compdb_dir to extra_dirs) so
that future similar options can all piggyback on it, but this can do
for now.

> @@ -2413,6 +2447,14 @@ else
>  $(OBJECTS): $(LIB_H) $(GENERATED_H)
>  endif
>  
> +ifeq ($(GENERATE_COMPILATION_DATABASE),yes)
> +all:: compile_commands.json
> +compile_commands.json:
> +	@$(RM) $@
> +	$(QUIET_GEN)sed -e '1s/^/[/' -e '$$s/,$$/]/' $(compdb_dir)*.o.json > $@+

OK.  The entire thing is concatenated and enclosed by a single pair
of '[' and ']'.

If we are planning to allow developers customize compdb_dir,
requiring trailing slash in the value of $(compdb_dir) is not very
friendly.


Thanks.