Re: [PATCH v2] Makefile: add support for generating JSON compilation database
- From
Junio C Hamano <gitster@pobox.com>
- Date
- Sep 2, 2020, 17:21 UTC
- Message-ID
- <xmqq1rjkccw8.fsf@gitster.c.googlers.com>
- In-Reply-To
- <pull.714.v2.git.1599001759548.gitgitgadget@gmail.com>
"Philippe Blain via GitGitGadget" <gitgitgadget@gmail.com> writes:
Show 9 quoted lines
> 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 ;-)
Show 5 quoted lines
> +# 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.
Show 7 quoted lines
> +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.
Show 12 quoted lines
> @@ -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.
Show 9 quoted lines
> @@ -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.