| Message ID | 20210701141011.785641-18-anthony.perard@citrix.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | xen: Build system improvements | expand |
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). Jan
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. 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) Still want to move cmd_binfile to Rules.mk? But I can at least spell "tools/binfile" instead of using $<. Thanks,
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)? Jan
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,
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
diff --git a/xen/common/Makefile b/xen/common/Makefile index 54de70d42278..93df3178b71f 100644 --- 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 + +config_data.S: $(BASEDIR)/tools/binfile FORCE + $(call if_changed,binfile) +targets += config_data.S clean:: rm -f config_data.S config.gz 2>/dev/null diff --git a/xen/xsm/flask/Makefile b/xen/xsm/flask/Makefile index 637159ad8276..0ad15cb16606 100644 --- a/xen/xsm/flask/Makefile +++ b/xen/xsm/flask/Makefile @@ -35,8 +35,12 @@ $(subst include/,%/,$(AV_H_FILES)): $(AV_H_DEPEND) $(mkaccess) FORCE obj-bin-$(CONFIG_XSM_FLASK_POLICY) += flask-policy.o flask-policy.o: policy.bin -flask-policy.S: $(BASEDIR)/tools/binfile - $(SHELL) $(BASEDIR)/tools/binfile -i $@ policy.bin xsm_flask_init_policy +quiet_cmd_binfile = BINFILE $@ +cmd_binfile = $(SHELL) $< -i $@ policy.bin xsm_flask_init_policy + +flask-policy.S: $(BASEDIR)/tools/binfile FORCE + $(call if_changed,binfile) +targets += flask-policy.S FLASK_BUILD_DIR := $(CURDIR) POLICY_SRC := $(FLASK_BUILD_DIR)/xenpolicy-$(XEN_FULLVERSION)
This will allow to detect command line changes and allow to regenerate the file in that case. Signed-off-by: Anthony PERARD <anthony.perard@citrix.com> --- xen/common/Makefile | 8 ++++++-- xen/xsm/flask/Makefile | 8 ++++++-- 2 files changed, 12 insertions(+), 4 deletions(-)