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

From: Philippe Blain <levraiphilippeblain@gmail.com>
Date: Sep 3, 2020, 21:17 UTC
Message-ID: <41D647C5-BC9E-431F-BEF9-C0040F4E0C94@gmail.com>
In-Reply-To: <xmqq1rjkccw8.fsf@gitster.c.googlers.com>

Hi Junio,

Show 22 quoted lines

> Le 2 sept. 2020 à 13:21, Junio C Hamano <gitster@pobox.com> a écrit :
> 
> "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 ;-)

This addition to the .gitignore is for the individual JSON files (one per source file), that are placed in the $(compdb_dir). I think naming "rebase.o.json" the JSON file that describes how to compile "rebase.c" into "rebase.o" makes sense. I don't know what is the convention for other projects.

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

The name `compile_commands.json` for the database itself is standard. The name of the directory where the '*.o.json' files are placed is a name I chose, and I don't feel strongly about it. I thought it made sense to name it like that, then its purpose is clear. We could make it a hidden directory if we don't want to add a new folder to the root of the repo when using this feature.

Show 12 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?

We should. I'll tweak (and simplify) that for v3.

Show 17 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?

Yes, it is because the `$(dir $@)` Makefile function will return `./` for source files 
at the base of the repo, so the JSON files get named eg. `.-rebase.o.json` and then they are 
hidden. So it's just to make them non-hidden, so as not to confuse someone that would
count the number of source files and compare with the number of (non-hidden)
 '*.o.json' files in $(comdb_dir) and get a different number.

Show 12 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 ']'.

Yes, and the comma after the last entry removed for JSON compliance.

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

OK I'll change that.

> Thanks.

Thank you for your comments!

Philippe.