From: Philippe Blain <levraiphilippeblain@gmail.com>
To: Junio C Hamano <gitster@pobox.com>
Cc: Philippe Blain via GitGitGadget <gitgitgadget@gmail.com>,
Git mailing list <git@vger.kernel.org>,
"brian m. carlson" <sandals@crustytoothpaste.net>,
Jeff King <peff@peff.net>
Subject: Re: [PATCH v2] Makefile: add support for generating JSON compilation database
Date: Thu, 3 Sep 2020 17:17:23 -0400 [thread overview]
Message-ID: <41D647C5-BC9E-431F-BEF9-C0040F4E0C94@gmail.com> (raw)
In-Reply-To: <xmqq1rjkccw8.fsf@gitster.c.googlers.com>
Hi Junio,
> 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.
>> +# 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.
>> +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.
>> @@ -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.
>> @@ -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.
next prev parent reply other threads:[~2020-09-03 21:17 UTC|newest]
Thread overview: 15+ messages / expand[flat|nested] mbox.gz Atom feed top
2020-08-30 19:28 [PATCH] Makefile: add support for generating JSON compilation database Philippe Blain via GitGitGadget
2020-08-30 22:10 ` brian m. carlson
2020-08-31 2:37 ` Philippe Blain
2020-08-31 4:24 ` Junio C Hamano
2020-09-01 7:38 ` Jeff King
2020-09-01 13:18 ` Philippe Blain
2020-09-02 1:33 ` brian m. carlson
2020-09-02 8:04 ` Jeff King
2020-08-30 22:17 ` Philippe Blain
2020-09-01 23:09 ` [PATCH v2] " Philippe Blain via GitGitGadget
2020-09-02 17:21 ` Junio C Hamano
2020-09-03 21:17 ` Philippe Blain [this message]
2020-09-03 21:31 ` Junio C Hamano
2020-09-03 22:04 ` Philippe Blain
2020-09-03 22:13 ` [PATCH v3] " Philippe Blain via GitGitGadget
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
List information: http://vger.kernel.org/majordomo-info.html
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=41D647C5-BC9E-431F-BEF9-C0040F4E0C94@gmail.com \
--to=levraiphilippeblain@gmail.com \
--cc=git@vger.kernel.org \
--cc=gitgitgadget@gmail.com \
--cc=gitster@pobox.com \
--cc=peff@peff.net \
--cc=sandals@crustytoothpaste.net \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
Code repositories for project(s) associated with this public inbox
https://80x24.org/mirrors/git.git
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).