| Message ID | 3ccf0255-8a15-effc-ce6b-eabb61625f90@ramsayjones.plus.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Using sparse in a CI job | expand |
Ramsay Jones <ramsay@ramsayjones.plus.com> writes: > In order to enable greater user customisation of the SPARSE_FLAGS > variable, we introduce a new SP_EXTRA_FLAGS variable to use for > target specific settings. Without using the new variable, setting > the SPARSE_FLAGS on the 'make' command-line would also override the > value set by the target-specific rules in the Makefile (effectively > making them useless). In addition, we initialise the SPARSE_FLAGS > to the default (empty) value using a conditional assignment (?=). > This allows the SPARSE_FLAGS to be set from the environment as > well as from the command-line. Thanks for a detailed and clear explanation here and in the cover letter. I agree with the motivation and most of the things I see in this patch, but one thing that stands out at me is if we still want to += append to SP_EXTRA_FLAGS in target specific way. Before this patch, because SPARSE_FLAGS was a dual use variable, it needed += appending to it in these two places, but that rationale is gone with this patch. Also, don't we want to clear SP_EXTRA_FLAGS at the beginning? The reason I raise these is because I do not quite see a clear answer to "I want to set SP_EXTRA_FLAGS and not SPARSE_FLAGS, because ...". > Signed-off-by: Ramsay Jones <ramsay@ramsayjones.plus.com> > --- > Makefile | 10 +++++----- > 1 file changed, 5 insertions(+), 5 deletions(-) > > diff --git a/Makefile b/Makefile > index 6e8d017e8e..dc02825c88 100644 > --- a/Makefile > +++ b/Makefile > @@ -574,7 +574,7 @@ SPATCH = spatch > > export TCL_PATH TCLTK_PATH > > -SPARSE_FLAGS = > +SPARSE_FLAGS ?= > SPATCH_FLAGS = --all-includes --patch . > > > @@ -2369,10 +2369,10 @@ gettext.sp gettext.s gettext.o: GIT-PREFIX > gettext.sp gettext.s gettext.o: EXTRA_CPPFLAGS = \ > -DGIT_LOCALE_PATH='"$(localedir_relative_SQ)"' > > -http-push.sp http.sp http-walker.sp remote-curl.sp imap-send.sp: SPARSE_FLAGS += \ > +http-push.sp http.sp http-walker.sp remote-curl.sp imap-send.sp: SP_EXTRA_FLAGS += \ > -DCURL_DISABLE_TYPECHECK > > -pack-revindex.sp: SPARSE_FLAGS += -Wno-memcpy-max-count > +pack-revindex.sp: SP_EXTRA_FLAGS += -Wno-memcpy-max-count > > ifdef NO_EXPAT > http-walker.sp http-walker.s http-walker.o: EXTRA_CPPFLAGS = -DNO_EXPAT > @@ -2386,7 +2386,7 @@ endif > ifdef USE_NED_ALLOCATOR > compat/nedmalloc/nedmalloc.sp compat/nedmalloc/nedmalloc.o: EXTRA_CPPFLAGS = \ > -DNDEBUG -DREPLACE_SYSTEM_ALLOCATOR > -compat/nedmalloc/nedmalloc.sp: SPARSE_FLAGS += -Wno-non-pointer-null > +compat/nedmalloc/nedmalloc.sp: SP_EXTRA_FLAGS += -Wno-non-pointer-null > endif > > git-%$X: %.o GIT-LDFLAGS $(GITLIBS) > @@ -2710,7 +2710,7 @@ SP_OBJ = $(patsubst %.o,%.sp,$(C_OBJ)) > > $(SP_OBJ): %.sp: %.c GIT-CFLAGS FORCE > $(QUIET_SP)cgcc -no-compile $(ALL_CFLAGS) $(EXTRA_CPPFLAGS) \ > - $(SPARSE_FLAGS) $< > + $(SPARSE_FLAGS) $(SP_EXTRA_FLAGS) $< > > .PHONY: sparse $(SP_OBJ) > sparse: $(SP_OBJ)
On Fri, Feb 01, 2019 at 01:46:13PM -0800, Junio C Hamano wrote: > Ramsay Jones <ramsay@ramsayjones.plus.com> writes: > > > In order to enable greater user customisation of the SPARSE_FLAGS > > variable, we introduce a new SP_EXTRA_FLAGS variable to use for > > target specific settings. Without using the new variable, setting > > the SPARSE_FLAGS on the 'make' command-line would also override the > > value set by the target-specific rules in the Makefile (effectively > > making them useless). In addition, we initialise the SPARSE_FLAGS > > to the default (empty) value using a conditional assignment (?=). > > This allows the SPARSE_FLAGS to be set from the environment as > > well as from the command-line. > > Thanks for a detailed and clear explanation here and in the cover > letter. I agree with the motivation and most of the things I see in > this patch, but one thing that stands out at me is if we still want > to += append to SP_EXTRA_FLAGS in target specific way. Before this > patch, because SPARSE_FLAGS was a dual use variable, it needed += > appending to it in these two places, but that rationale is gone with > this patch. > > Also, don't we want to clear SP_EXTRA_FLAGS at the beginning? > > The reason I raise these is because I do not quite see a clear > answer to "I want to set SP_EXTRA_FLAGS and not SPARSE_FLAGS, > because ...". I think the intent here is to *only* use SP_SPARSE_FLAGS as the internal-only variable and to use SPARSE_FLAGS *only* as the additional user-controlable flags. If it is indeed the case, then I think it looks good but maybe it would be better to use another variable's name to make this more explicit or add a small comment. -- Luc
On 01/02/2019 21:46, Junio C Hamano wrote: > Ramsay Jones <ramsay@ramsayjones.plus.com> writes: > >> In order to enable greater user customisation of the SPARSE_FLAGS >> variable, we introduce a new SP_EXTRA_FLAGS variable to use for >> target specific settings. Without using the new variable, setting >> the SPARSE_FLAGS on the 'make' command-line would also override the >> value set by the target-specific rules in the Makefile (effectively >> making them useless). In addition, we initialise the SPARSE_FLAGS >> to the default (empty) value using a conditional assignment (?=). >> This allows the SPARSE_FLAGS to be set from the environment as >> well as from the command-line. > > Thanks for a detailed and clear explanation here and in the cover > letter. I agree with the motivation and most of the things I see in > this patch, but one thing that stands out at me is if we still want > to += append to SP_EXTRA_FLAGS in target specific way. Before this > patch, because SPARSE_FLAGS was a dual use variable, it needed += > appending to it in these two places, but that rationale is gone with > this patch. As Luc surmised, in his reply, my intention was that SP_EXTRA_FLAGS should be used for any 'internal' settings (not just the target specific settings), whereas SPARSE_FLAGS would now be used _only_ for user customisation. The commit message doesn't make that clear, (and the patch text adds to the confusion, since only target specific settings are changed) so I need to reword that somehow. Also, ... > Also, don't we want to clear SP_EXTRA_FLAGS at the beginning? ... (Ahem) I just simply forgot to initialise the new variable! :( (Yes, it actually doesn't matter, but it gives a wrong impression). ;-) BTW, the first name I chose was SP_FLAGS, but while editing the second hunk I decided that wasn't a good name. On several other projects I have seen exactly this 'split' happen, where the user facing variable was called <something>_FLAGS and the 'internal' variable was then called <something>_EXTRA_FLAGS, so I decided to go with that instead. (Yes, I abbreviated SPARSE). However, I have to say that I have also seen (less often) the exact opposite: "... if some idiot user wants to add extra flags ...". :-D So, yes SP_EXTRA_FLAGS could be used for other 'internal' uses; for example, look back to commit 6bc8606be3 ("config.mak.uname: remove SPARSE_FLAGS setting for cygwin", 2018-02-12), which removed: 'SPARSE_FLAGS = -isystem /usr/include/w32api -Wno-one-bit-signed-bitfield' from config.mak.uname. As you can see, although gcc could find the win32 header files, sparse needed a little help. Also, the win32 system header files had an instance of a 'one-bit signed bitfield', which caused sparse to spew many many many errors. If I needed to do something like that again, then I would use SP_EXTRA_FLAGS instead. [Looking back now, I am a little shocked that it seems to have taken me nearly 5 years to submit that patch! :-P ] I could give quite a few examples, but ... Oh wait! ... Hmm, it seems that I need to add a new patch to remove line 558 of config.mak.uname. This line has a setting for SPARSE_FLAGS in the MINGW section of that file. Back in around 2011, having ported sparse to MinGW (the original msysgit, not MSYS2), I naturally had the same issue with the Win32 header files. Since I didn't upstream my sparse patches, I don't think anyone can be running sparse on MinGW these days. Anyway, its late, so I will look at redoing the patches soon. ATB, Ramsay Jones
Ramsay Jones <ramsay@ramsayjones.plus.com> writes: >> Thanks for a detailed and clear explanation here and in the cover >> letter. I agree with the motivation and most of the things I see in >> this patch, but one thing that stands out at me is if we still want >> to += append to SP_EXTRA_FLAGS in target specific way. Before this >> patch, because SPARSE_FLAGS was a dual use variable, it needed += >> appending to it in these two places, but that rationale is gone with >> this patch. > > As Luc surmised, in his reply, my intention was that SP_EXTRA_FLAGS > should be used for any 'internal' settings (not just the target > specific settings), whereas SPARSE_FLAGS would now be used _only_ for > user customisation. OK, if that is the case, then not using "+= append" on SP_EXTRA_FLAGS and initializing it without allowing environment would be two improvements that can make the intention more clear, I think. > Anyway, its late, so I will look at redoing the patches soon. Thanks.
On 04/02/2019 18:12, Junio C Hamano wrote: > Ramsay Jones <ramsay@ramsayjones.plus.com> writes: > >>> Thanks for a detailed and clear explanation here and in the cover >>> letter. I agree with the motivation and most of the things I see in >>> this patch, but one thing that stands out at me is if we still want >>> to += append to SP_EXTRA_FLAGS in target specific way. Before this >>> patch, because SPARSE_FLAGS was a dual use variable, it needed += >>> appending to it in these two places, but that rationale is gone with >>> this patch. >> >> As Luc surmised, in his reply, my intention was that SP_EXTRA_FLAGS >> should be used for any 'internal' settings (not just the target >> specific settings), whereas SPARSE_FLAGS would now be used _only_ for >> user customisation. > > OK, if that is the case, then not using "+= append" on SP_EXTRA_FLAGS Err, no, that clearly wouldn't be an improvement! As I said above, this is not just for target specific settings. Am I missing something? ATB, Ramsay Jones
Ramsay Jones <ramsay@ramsayjones.plus.com> writes: > On 04/02/2019 18:12, Junio C Hamano wrote: >> Ramsay Jones <ramsay@ramsayjones.plus.com> writes: >> >>>> Thanks for a detailed and clear explanation here and in the cover >>>> letter. I agree with the motivation and most of the things I see in >>>> this patch, but one thing that stands out at me is if we still want >>>> to += append to SP_EXTRA_FLAGS in target specific way. Before this >>>> patch, because SPARSE_FLAGS was a dual use variable, it needed += >>>> appending to it in these two places, but that rationale is gone with >>>> this patch. >>> >>> As Luc surmised, in his reply, my intention was that SP_EXTRA_FLAGS >>> should be used for any 'internal' settings (not just the target >>> specific settings), whereas SPARSE_FLAGS would now be used _only_ for >>> user customisation. >> >> OK, if that is the case, then not using "+= append" on SP_EXTRA_FLAGS > > Err, no, that clearly wouldn't be an improvement! As I said above, > this is not just for target specific settings. Ah, do you mean that there may be globally applicable internal setting? I would have expected that such an option would be done directly on the command line, e.g. $(SP_OBJ): %.sp: %.c GIT-CFLAGS FORCE $(QUIET_SP)cgcc -no-compile $(ALL_CFLAGS) $(EXTRA_CPPFLAGS) \ $(SPARSE_FLAGS) $(SP_EXTRA_FLAGS) \ -Wsparse-settings-for-everybody $< But it is fine either way, as long as the purpose of the macro is documented clearly enough ;-) Thanks.
On 04/02/2019 20:15, Junio C Hamano wrote: > Ramsay Jones <ramsay@ramsayjones.plus.com> writes: > >> On 04/02/2019 18:12, Junio C Hamano wrote: >>> Ramsay Jones <ramsay@ramsayjones.plus.com> writes: >>> >>>>> Thanks for a detailed and clear explanation here and in the cover >>>>> letter. I agree with the motivation and most of the things I see in >>>>> this patch, but one thing that stands out at me is if we still want >>>>> to += append to SP_EXTRA_FLAGS in target specific way. Before this >>>>> patch, because SPARSE_FLAGS was a dual use variable, it needed += >>>>> appending to it in these two places, but that rationale is gone with >>>>> this patch. >>>> >>>> As Luc surmised, in his reply, my intention was that SP_EXTRA_FLAGS >>>> should be used for any 'internal' settings (not just the target >>>> specific settings), whereas SPARSE_FLAGS would now be used _only_ for >>>> user customisation. >>> >>> OK, if that is the case, then not using "+= append" on SP_EXTRA_FLAGS >> >> Err, no, that clearly wouldn't be an improvement! As I said above, >> this is not just for target specific settings. > > Ah, do you mean that there may be globally applicable internal > setting? I would have expected that such an option would be done > directly on the command line, e.g. > > $(SP_OBJ): %.sp: %.c GIT-CFLAGS FORCE > $(QUIET_SP)cgcc -no-compile $(ALL_CFLAGS) $(EXTRA_CPPFLAGS) \ > $(SPARSE_FLAGS) $(SP_EXTRA_FLAGS) \ > -Wsparse-settings-for-everybody $< global, possibly, but more likely platform variations - as I tried (but obviously failed) to indicate with the cygwin and MinGW examples in my previous email. ATB, Ramsay Jones
diff --git a/Makefile b/Makefile index 6e8d017e8e..dc02825c88 100644 --- a/Makefile +++ b/Makefile @@ -574,7 +574,7 @@ SPATCH = spatch export TCL_PATH TCLTK_PATH -SPARSE_FLAGS = +SPARSE_FLAGS ?= SPATCH_FLAGS = --all-includes --patch . @@ -2369,10 +2369,10 @@ gettext.sp gettext.s gettext.o: GIT-PREFIX gettext.sp gettext.s gettext.o: EXTRA_CPPFLAGS = \ -DGIT_LOCALE_PATH='"$(localedir_relative_SQ)"' -http-push.sp http.sp http-walker.sp remote-curl.sp imap-send.sp: SPARSE_FLAGS += \ +http-push.sp http.sp http-walker.sp remote-curl.sp imap-send.sp: SP_EXTRA_FLAGS += \ -DCURL_DISABLE_TYPECHECK -pack-revindex.sp: SPARSE_FLAGS += -Wno-memcpy-max-count +pack-revindex.sp: SP_EXTRA_FLAGS += -Wno-memcpy-max-count ifdef NO_EXPAT http-walker.sp http-walker.s http-walker.o: EXTRA_CPPFLAGS = -DNO_EXPAT @@ -2386,7 +2386,7 @@ endif ifdef USE_NED_ALLOCATOR compat/nedmalloc/nedmalloc.sp compat/nedmalloc/nedmalloc.o: EXTRA_CPPFLAGS = \ -DNDEBUG -DREPLACE_SYSTEM_ALLOCATOR -compat/nedmalloc/nedmalloc.sp: SPARSE_FLAGS += -Wno-non-pointer-null +compat/nedmalloc/nedmalloc.sp: SP_EXTRA_FLAGS += -Wno-non-pointer-null endif git-%$X: %.o GIT-LDFLAGS $(GITLIBS) @@ -2710,7 +2710,7 @@ SP_OBJ = $(patsubst %.o,%.sp,$(C_OBJ)) $(SP_OBJ): %.sp: %.c GIT-CFLAGS FORCE $(QUIET_SP)cgcc -no-compile $(ALL_CFLAGS) $(EXTRA_CPPFLAGS) \ - $(SPARSE_FLAGS) $< + $(SPARSE_FLAGS) $(SP_EXTRA_FLAGS) $< .PHONY: sparse $(SP_OBJ) sparse: $(SP_OBJ)
In order to enable greater user customisation of the SPARSE_FLAGS variable, we introduce a new SP_EXTRA_FLAGS variable to use for target specific settings. Without using the new variable, setting the SPARSE_FLAGS on the 'make' command-line would also override the value set by the target-specific rules in the Makefile (effectively making them useless). In addition, we initialise the SPARSE_FLAGS to the default (empty) value using a conditional assignment (?=). This allows the SPARSE_FLAGS to be set from the environment as well as from the command-line. Signed-off-by: Ramsay Jones <ramsay@ramsayjones.plus.com> --- Makefile | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-)