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

[Anthony PERARD <anthony.perard@xxxxxxxxxx>]

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)

Thanks,

-- 
Anthony PERARD