Re: [XEN PATCH v6 17/31] build: convert binfile use to if_changed

From: Jan Beulich <jbeulich@suse.com>
To: Anthony PERARD <anthony.perard@citrix.com>
Cc: Andrew Cooper <andrew.cooper3@citrix.com>,
	George Dunlap <george.dunlap@citrix.com>,
	Ian Jackson <iwj@xenproject.org>, Julien Grall <julien@xen.org>,
	Stefano Stabellini <sstabellini@kernel.org>, Wei Liu <wl@xen.org>,
	Daniel De Graaf <dgdegra@tycho.nsa.gov>,
	"Daniel P. Smith" <dpsmith@apertussolutions.com>,
	xen-devel@lists.xenproject.org
Subject: Re: [XEN PATCH v6 17/31] build: convert binfile use to if_changed
Date: Tue, 13 Jul 2021 17:33:53 +0200	[thread overview]
Message-ID: <85755375-8674-04be-bf2b-31b96763e202@suse.com> (raw)
In-Reply-To: <YO2qEaFOMVM3xvkO@perard>

On 13.07.2021 16:58, Anthony PERARD wrote:
> On Tue, Jul 13, 2021 at 09:51:45AM +0200, Jan Beulich wrote:
>> On 12.07.2021 18:32, Anthony PERARD wrote:
>>> On Wed, Jul 07, 2021 at 05:48:57PM +0200, Jan Beulich wrote:
>>>> On 01.07.2021 16:09, Anthony PERARD wrote:
>>>>> --- a/xen/common/Makefile
>>>>> +++ b/xen/common/Makefile
>>>>> @@ -80,8 +80,12 @@ config.gz: $(CONF_FILE)
>>>>>  
>>>>>  config_data.o: config.gz
>>>>>  
>>>>> -config_data.S: $(BASEDIR)/tools/binfile
>>>>> -	$(SHELL) $(BASEDIR)/tools/binfile $@ config.gz xen_config_data
>>>>> +quiet_cmd_binfile = BINFILE $@
>>>>> +cmd_binfile = $(SHELL) $< $@ config.gz xen_config_data
>>>>
>>>> This is an abuse of $< which I consider overly confusing:
>>>> $(BASEDIR)/tools/binfile is not the input file to the rule. Instead
>>>> the script generates an assembly file "out of thin air", with not
>>>> input files at all. The rule and ...
>>>>
>>>>> +config_data.S: $(BASEDIR)/tools/binfile FORCE
>>>>
>>>> ... dependency shouldn't give a different impression. What would
>>>> be nice (without having checked how difficult this might be) would
>>>> be if quiet_cmd_binfile and cmd_binfile could move to xen/Rules.mk
>>>> and merely be used from here (and the other location, where the
>>>> same concern obviously applies).
>>>
>>> I've though of having cmd_binfile in Rules.mk, but it's made more
>>> complicated by having a "-i" flag used in flask/.
>>>
>>> So one things I've writen was:
>>>
>>> config_data.S: $(BASEDIR)/tools/binfile FORCE
>>>        $(call if_changed,binfile,,config.gz xen_config_data)
>>> flask-policy.S: $(BASEDIR)/tools/binfile FORCE
>>>        $(call if_changed,binfile,-i,policy.bin xsm_flask_init_policy)
>>>
>>> with:
>>> cmd_binfile = $(SHELL) $(BASEDIR)/tools/binfile $(2) $@ $(3)
>>>
>>> I thought this would be confusing, so I avoid it.
>>
>> Indeed that's why I did write "without having checked how difficult
>> this might be", because I definitely didn't want to suggest such
>> anomalies to get introduced. It's unhelpful that options have to
>> come first.
>>
>>> But maybe with the lists of flags at the end, it would be better:
>>>    $(call if_changed,binfile,policy.bin xsm_flask_init_policy,-i)
>>
>> Yes, this is a little better imo, but still not pretty.
>>
>>> Still want to move cmd_binfile to Rules.mk?
>>
>> I'd still like it to be moved, but without resulting in a rule
>> that's not consistent with others. Maybe we should have a
>> BINFILE_FLAGS variable (paralleling e.g. CFLAGS)?
> 
> Sound good.
> 
>     cmd_binfile = $(SHELL) $(BASEDIR)/tools/binfile $(BINFILE_FLAGS) $@ $(2)
> 
>     flask-policy.S: BINFILE_FLAGS := -i
>     flask-policy.S: $(BASEDIR)/tools/binfile FORCE
>            $(call if_changed,binfile,policy.bin xsm_flask_init_policy)
> 
> Would the above be OK?
> Otherwise, maybe you'll prefer the following:
> 
>     flask-policy.S: BINFILE_FLAGS = -i $@ policy.bin xsm_flask_init_policy
>     flask-policy.S: $(BASEDIR)/tools/binfile FORCE
>            $(call if_changed,binfile)

I think I like the former better than the latter, but I could live
with either.

Jan