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

[Philippe Blain <levraiphilippeblain@gmail.com>]

> Le 3 sept. 2020 à 17:31, Junio C Hamano <gitster@pobox.com> a écrit :
> 
> Philippe Blain <levraiphilippeblain@gmail.com> writes:
> 
>> 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.
> 
> I agree rebase.o.$somesuffix does make sense, but I do not know
> 'json' is a great value for $somesuffix.  I wouldn't be surprised if
> 'cdb' or some other silly abbreviation for "compilation database" is
> how other people use this feature.
> 
> Those watching from the sidelines.  Does anybody know if there is an
> established convention used by other projects?  If we hear nothing
> by early next week, let's declare 'json' is good enough and move on.
> 
>> 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.
> 
> I think both of these are sensible.  Again if we hear nothing about
> common practice, let's move on with these constants as-is.

OK. 

> 
>>>> +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.
> 
> I think
> 
> - GENERATE_COMPILATION_DATABASE is set to 'no': don't even probe
> 
> - GENERATE_COMPILATION_DATABASE is set to 'yes': probe and turn it
>   to 'no' if unavailable.
> 
> - GENERATE_COMPILATION_DATABASE is set to anything else: either
>   error out, or turn it into 'no' (I have no preference between
>   them).
> 
> would cover all the cases.

I agree. I'll do that.

> 
>>>> +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.
> 
> Hmph.  Would $(subst /,-,$@) instead of "only substitute leading
> directory part, and concatenate the basename part unmolested" work
> better then?  After all, by definition the basename part would not
> have a slash in it, so substituting all '/' to '-' in the whole
> pathname should do the same thing and we won't have to worry about
> the spurious './', no?

This indeed works, and reads better. Thanks!