| Message ID | pull.714.v2.git.1599001759548.gitgitgadget@gmail.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [v2] Makefile: add support for generating JSON compilation database | expand |
"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 ;-) > +# 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. > +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. > @@ -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. > @@ -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.
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.
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. >>> +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. >>> +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?
> 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!
diff --git a/.gitignore b/.gitignore index ee509a2ad2..f4c51300e0 100644 --- a/.gitignore +++ b/.gitignore @@ -197,6 +197,7 @@ /git.spec *.exe *.[aos] +*.o.json *.py[co] .depend/ *.gcda @@ -218,6 +219,7 @@ /tags /TAGS /cscope* +/compile_commands.json *.hcc *.obj *.lib diff --git a/Makefile b/Makefile index 65f8cfb236..51cd0f302b 100644 --- a/Makefile +++ b/Makefile @@ -462,6 +462,12 @@ all:: # the global variable _wpgmptr containing the absolute path of the current # executable (this is the case on Windows). # +# 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. +# # Define DEVELOPER to enable more compiler warnings. Compiler version # and family are auto detected, but could be overridden by defining # COMPILER_FEATURES (see config.mak.dev). You can still set @@ -1258,6 +1264,20 @@ $(error please set COMPUTE_HEADER_DEPENDENCIES to yes, no, or auto \ endif endif +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 +else +override GENERATE_COMPILATION_DATABASE = no +$(warning GENERATE_COMPILATION_DATABASE is set, but your compiler does not \ +support generating compilation database entries) +endif +endif + ifdef SANE_TOOL_PATH SANE_TOOL_PATH_SQ = $(subst ','\'',$(SANE_TOOL_PATH)) BROKEN_PATH_FIX = 's|^\# @@BROKEN_PATH_FIX@@$$|git_broken_path_fix "$(SANE_TOOL_PATH_SQ)"|' @@ -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 +compdb_args = -MJ $(compdb_file) +else +missing_compdb_dir = +compdb_args = +endif + ASM_SRC := $(wildcard $(OBJECTS:o=S)) ASM_OBJ := $(ASM_SRC:S=o) C_OBJ := $(filter-out $(ASM_OBJ),$(OBJECTS)) .SUFFIXES: -$(C_OBJ): %.o: %.c GIT-CFLAGS $(missing_dep_dirs) - $(QUIET_CC)$(CC) -o $*.o -c $(dep_args) $(ALL_CFLAGS) $(EXTRA_CPPFLAGS) $< -$(ASM_OBJ): %.o: %.S GIT-CFLAGS $(missing_dep_dirs) - $(QUIET_CC)$(CC) -o $*.o -c $(dep_args) $(ALL_CFLAGS) $(EXTRA_CPPFLAGS) $< +$(C_OBJ): %.o: %.c GIT-CFLAGS $(missing_dep_dirs) $(missing_compdb_dir) + $(QUIET_CC)$(CC) -o $*.o -c $(dep_args) $(compdb_args) $(ALL_CFLAGS) $(EXTRA_CPPFLAGS) $< +$(ASM_OBJ): %.o: %.S GIT-CFLAGS $(missing_dep_dirs) $(missing_compdb_dir) + $(QUIET_CC)$(CC) -o $*.o -c $(dep_args) $(compdb_args) $(ALL_CFLAGS) $(EXTRA_CPPFLAGS) $< %.s: %.c GIT-CFLAGS FORCE $(QUIET_CC)$(CC) -o $@ -S $(ALL_CFLAGS) $(EXTRA_CPPFLAGS) $< @@ -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 > $@+ + @if test -s $@+; then mv $@+ $@; else $(RM) $@+; fi +endif + exec-cmd.sp exec-cmd.s exec-cmd.o: GIT-PREFIX exec-cmd.sp exec-cmd.s exec-cmd.o: EXTRA_CPPFLAGS = \ '-DGIT_EXEC_PATH="$(gitexecdir_SQ)"' \ @@ -3117,7 +3159,7 @@ clean: profile-clean coverage-clean cocciclean $(RM) $(TEST_PROGRAMS) $(RM) $(FUZZ_PROGRAMS) $(RM) $(HCC) - $(RM) -r bin-wrappers $(dep_dirs) + $(RM) -r bin-wrappers $(dep_dirs) $(compdb_dir) compile_commands.json $(RM) -r po/build/ $(RM) *.pyc *.pyo */*.pyc */*.pyo $(GENERATED_H) $(ETAGS_TARGET) tags cscope* $(RM) -r $(GIT_TARNAME) .doc-tmp-dir