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

Hi Junio, 

Le 2 sept. 2020 à 13:21, Junio C Hamano [off-list ref] a écrit :

"Philippe Blain via GitGitGadget" [off-list ref] writes:

quoted

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.

quoted

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

quoted

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

quoted

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

quoted

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