Re: [PATCH 1/1] Makefile: improve SPARSE_FLAGS customisation

From: Ramsay Jones <ramsay@ramsayjones.plus.com>
Date: Feb 3, 2019, 01:25 UTC
Message-ID: <4a8f6b3f-3844-b4e8-2ed2-4078b592dd4b@ramsayjones.plus.com>
In-Reply-To: <xmqqmunfxjve.fsf@gitster-ct.c.googlers.com>

On 01/02/2019 21:46, Junio C Hamano wrote:

Show 19 quoted lines

> 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