Re: [PATCH v2 2/5] Makefile: Correct a missing FORCE on the binman rule
Hi Pali, On Fri, 21 Oct 2022 at 14:22, Pali Rohár wrote: > > On Friday 21 October 2022 14:17:11 Simon Glass wrote: > > Hi Pali, > > > > On Thu, 20 Oct 2022 at 13:54, Pali Rohár wrote: > > > > > > On Wednesday 19 October 2022 19:55:43 Simon Glass wrote: > > > > Hi Pali, > > > > > > > > On Sun, 16 Oct 2022 at 04:16, Pali Rohár wrote: > > > > > > > > > > On Tuesday 11 October 2022 23:35:22 Masahiro Yamada wrote: > > > > > > On Tue, Oct 11, 2022 at 11:16 PM Simon Glass > > > > > > wrote: > > > > > > > > > > > > > > This is required for if_changed to work correctly. Add it. > > > > > > > > > > > > > > Signed-off-by: Simon Glass > > > > > > > Reviewed-by: Pali Rohár > > > > > > > --- > > > > > > > > > > > > > > (no changes since v1) > > > > > > > > > > > > > > Makefile | 2 +- > > > > > > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > > > > > > > > > > > diff --git a/Makefile b/Makefile > > > > > > > index 3866cc62f9a..d28e8b4e316 100644 > > > > > > > --- a/Makefile > > > > > > > +++ b/Makefile > > > > > > > @@ -,7 +,7 @@ endef > > > > > > > PHONY += inputs > > > > > > > inputs: $(INPUTS-y) > > > > > > > > > > > > > > -all: .binman_stamp inputs > > > > > > > +all: .binman_stamp inputs FORCE > > > > > > > ifeq ($(CONFIG_BINMAN),y) > > > > > > > $(call if_changed,binman) > > > > > > > > > > > > > > > > > > > > > > > > 'all' is usually used as a phony target. > > > > > > > > > > > > > > > > > > I think this went wrong. > > > > > > > > > > > > if_changed should never be used for a phony target. > > > > > > In other words, if_changed should produce a build artifact. > > > > > > > > > > > > > > > > > > > > > > > > FORCE is never used for a phony target because > > > > > > it is added to 'PHONY'. > > > > > > > > > > > > PHONY += all > > > > > > > > > > > > > > > > > > > > > > I agree, this is really written in the wrong way. > > > > > > > > > > Target "all:" should be really phony target and should only depends on > > > > > other target, it should not have any body / commands. > > > > > > > > OK, will need to figure out where to put the binman thing then. > > > > > > > > > > > > > > And binman call should be moved to different non-phony target with the > > > > > correct output file name. > > > > > > > > Binman can generate all sorts of files. I will see if I can use > > > > .binman-stamp since that is always generated. > > > > > > > > Regards, > > > > Simon > > > > > > Would not something like this work? > > > > > > BINMAN_INPUTS := # fill this > > > BINMAN_OUTPUTS := # fill this > > > > > > INPUTS-$(CONFIG_BINMAN) += .binman_stamp > > > > > > .binman_stamp: $(BINMAN_INPUTS) FORCE > > > $(call if_changed,binman) > > > > > > $(BINMAN_OUTPUTS): .binman_stamp > > > > > > all: inputs > > > > Sadly we don't know the outputs without asking binman. > > So what about extending binman via some cmdline arg to just prints outputs? Yes I've thought about that but not done it. Things to do: - put intermediate files in a separate output subdir from the final output - have a way to determine the output filenames without actually building the output > > > I'll send out another rev of this patch by the end of the week. Regards, Simon
Re: [PATCH v2 2/5] Makefile: Correct a missing FORCE on the binman rule
On Friday 21 October 2022 14:17:11 Simon Glass wrote: > Hi Pali, > > On Thu, 20 Oct 2022 at 13:54, Pali Rohár wrote: > > > > On Wednesday 19 October 2022 19:55:43 Simon Glass wrote: > > > Hi Pali, > > > > > > On Sun, 16 Oct 2022 at 04:16, Pali Rohár wrote: > > > > > > > > On Tuesday 11 October 2022 23:35:22 Masahiro Yamada wrote: > > > > > On Tue, Oct 11, 2022 at 11:16 PM Simon Glass > > > > > wrote: > > > > > > > > > > > > This is required for if_changed to work correctly. Add it. > > > > > > > > > > > > Signed-off-by: Simon Glass > > > > > > Reviewed-by: Pali Rohár > > > > > > --- > > > > > > > > > > > > (no changes since v1) > > > > > > > > > > > > Makefile | 2 +- > > > > > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > > > > > > > > > diff --git a/Makefile b/Makefile > > > > > > index 3866cc62f9a..d28e8b4e316 100644 > > > > > > --- a/Makefile > > > > > > +++ b/Makefile > > > > > > @@ -,7 +,7 @@ endef > > > > > > PHONY += inputs > > > > > > inputs: $(INPUTS-y) > > > > > > > > > > > > -all: .binman_stamp inputs > > > > > > +all: .binman_stamp inputs FORCE > > > > > > ifeq ($(CONFIG_BINMAN),y) > > > > > > $(call if_changed,binman) > > > > > > > > > > > > > > > > > > > > 'all' is usually used as a phony target. > > > > > > > > > > > > > > > I think this went wrong. > > > > > > > > > > if_changed should never be used for a phony target. > > > > > In other words, if_changed should produce a build artifact. > > > > > > > > > > > > > > > > > > > > FORCE is never used for a phony target because > > > > > it is added to 'PHONY'. > > > > > > > > > > PHONY += all > > > > > > > > > > > > > > > > > > I agree, this is really written in the wrong way. > > > > > > > > Target "all:" should be really phony target and should only depends on > > > > other target, it should not have any body / commands. > > > > > > OK, will need to figure out where to put the binman thing then. > > > > > > > > > > > And binman call should be moved to different non-phony target with the > > > > correct output file name. > > > > > > Binman can generate all sorts of files. I will see if I can use > > > .binman-stamp since that is always generated. > > > > > > Regards, > > > Simon > > > > Would not something like this work? > > > > BINMAN_INPUTS := # fill this > > BINMAN_OUTPUTS := # fill this > > > > INPUTS-$(CONFIG_BINMAN) += .binman_stamp > > > > .binman_stamp: $(BINMAN_INPUTS) FORCE > > $(call if_changed,binman) > > > > $(BINMAN_OUTPUTS): .binman_stamp > > > > all: inputs > > Sadly we don't know the outputs without asking binman. So what about extending binman via some cmdline arg to just prints outputs? > I'll send out another rev of this patch by the end of the week. > > Regards, > Simon
Re: [PATCH v2 2/5] Makefile: Correct a missing FORCE on the binman rule
Hi Pali, On Thu, 20 Oct 2022 at 13:54, Pali Rohár wrote: > > On Wednesday 19 October 2022 19:55:43 Simon Glass wrote: > > Hi Pali, > > > > On Sun, 16 Oct 2022 at 04:16, Pali Rohár wrote: > > > > > > On Tuesday 11 October 2022 23:35:22 Masahiro Yamada wrote: > > > > On Tue, Oct 11, 2022 at 11:16 PM Simon Glass wrote: > > > > > > > > > > This is required for if_changed to work correctly. Add it. > > > > > > > > > > Signed-off-by: Simon Glass > > > > > Reviewed-by: Pali Rohár > > > > > --- > > > > > > > > > > (no changes since v1) > > > > > > > > > > Makefile | 2 +- > > > > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > > > > > > > diff --git a/Makefile b/Makefile > > > > > index 3866cc62f9a..d28e8b4e316 100644 > > > > > --- a/Makefile > > > > > +++ b/Makefile > > > > > @@ -,7 +,7 @@ endef > > > > > PHONY += inputs > > > > > inputs: $(INPUTS-y) > > > > > > > > > > -all: .binman_stamp inputs > > > > > +all: .binman_stamp inputs FORCE > > > > > ifeq ($(CONFIG_BINMAN),y) > > > > > $(call if_changed,binman) > > > > > > > > > > > > > > > > 'all' is usually used as a phony target. > > > > > > > > > > > > I think this went wrong. > > > > > > > > if_changed should never be used for a phony target. > > > > In other words, if_changed should produce a build artifact. > > > > > > > > > > > > > > > > FORCE is never used for a phony target because > > > > it is added to 'PHONY'. > > > > > > > > PHONY += all > > > > > > > > > > > > > > I agree, this is really written in the wrong way. > > > > > > Target "all:" should be really phony target and should only depends on > > > other target, it should not have any body / commands. > > > > OK, will need to figure out where to put the binman thing then. > > > > > > > > And binman call should be moved to different non-phony target with the > > > correct output file name. > > > > Binman can generate all sorts of files. I will see if I can use > > .binman-stamp since that is always generated. > > > > Regards, > > Simon > > Would not something like this work? > > BINMAN_INPUTS := # fill this > BINMAN_OUTPUTS := # fill this > > INPUTS-$(CONFIG_BINMAN) += .binman_stamp > > .binman_stamp: $(BINMAN_INPUTS) FORCE > $(call if_changed,binman) > > $(BINMAN_OUTPUTS): .binman_stamp > > all: inputs Sadly we don't know the outputs without asking binman. I'll send out another rev of this patch by the end of the week. Regards, Simon
Re: [PATCH v2 2/5] Makefile: Correct a missing FORCE on the binman rule
On Wednesday 19 October 2022 19:55:43 Simon Glass wrote: > Hi Pali, > > On Sun, 16 Oct 2022 at 04:16, Pali Rohár wrote: > > > > On Tuesday 11 October 2022 23:35:22 Masahiro Yamada wrote: > > > On Tue, Oct 11, 2022 at 11:16 PM Simon Glass wrote: > > > > > > > > This is required for if_changed to work correctly. Add it. > > > > > > > > Signed-off-by: Simon Glass > > > > Reviewed-by: Pali Rohár > > > > --- > > > > > > > > (no changes since v1) > > > > > > > > Makefile | 2 +- > > > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > > > > > diff --git a/Makefile b/Makefile > > > > index 3866cc62f9a..d28e8b4e316 100644 > > > > --- a/Makefile > > > > +++ b/Makefile > > > > @@ -,7 +,7 @@ endef > > > > PHONY += inputs > > > > inputs: $(INPUTS-y) > > > > > > > > -all: .binman_stamp inputs > > > > +all: .binman_stamp inputs FORCE > > > > ifeq ($(CONFIG_BINMAN),y) > > > > $(call if_changed,binman) > > > > > > > > > > > > 'all' is usually used as a phony target. > > > > > > > > > I think this went wrong. > > > > > > if_changed should never be used for a phony target. > > > In other words, if_changed should produce a build artifact. > > > > > > > > > > > > FORCE is never used for a phony target because > > > it is added to 'PHONY'. > > > > > > PHONY += all > > > > > > > > > > I agree, this is really written in the wrong way. > > > > Target "all:" should be really phony target and should only depends on > > other target, it should not have any body / commands. > > OK, will need to figure out where to put the binman thing then. > > > > > And binman call should be moved to different non-phony target with the > > correct output file name. > > Binman can generate all sorts of files. I will see if I can use > .binman-stamp since that is always generated. > > Regards, > Simon Would not something like this work? BINMAN_INPUTS := # fill this BINMAN_OUTPUTS := # fill this INPUTS-$(CONFIG_BINMAN) += .binman_stamp .binman_stamp: $(BINMAN_INPUTS) FORCE $(call if_changed,binman) $(BINMAN_OUTPUTS): .binman_stamp all: inputs
Re: [PATCH v2 2/5] Makefile: Correct a missing FORCE on the binman rule
Hi Pali, On Sun, 16 Oct 2022 at 04:16, Pali Rohár wrote: > > On Tuesday 11 October 2022 23:35:22 Masahiro Yamada wrote: > > On Tue, Oct 11, 2022 at 11:16 PM Simon Glass wrote: > > > > > > This is required for if_changed to work correctly. Add it. > > > > > > Signed-off-by: Simon Glass > > > Reviewed-by: Pali Rohár > > > --- > > > > > > (no changes since v1) > > > > > > Makefile | 2 +- > > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > > > diff --git a/Makefile b/Makefile > > > index 3866cc62f9a..d28e8b4e316 100644 > > > --- a/Makefile > > > +++ b/Makefile > > > @@ -,7 +,7 @@ endef > > > PHONY += inputs > > > inputs: $(INPUTS-y) > > > > > > -all: .binman_stamp inputs > > > +all: .binman_stamp inputs FORCE > > > ifeq ($(CONFIG_BINMAN),y) > > > $(call if_changed,binman) > > > > > > > > 'all' is usually used as a phony target. > > > > > > I think this went wrong. > > > > if_changed should never be used for a phony target. > > In other words, if_changed should produce a build artifact. > > > > > > > > FORCE is never used for a phony target because > > it is added to 'PHONY'. > > > > PHONY += all > > > > > > I agree, this is really written in the wrong way. > > Target "all:" should be really phony target and should only depends on > other target, it should not have any body / commands. OK, will need to figure out where to put the binman thing then. > > And binman call should be moved to different non-phony target with the > correct output file name. Binman can generate all sorts of files. I will see if I can use .binman-stamp since that is always generated. Regards, Simon
Re: [PATCH v2 2/5] Makefile: Correct a missing FORCE on the binman rule
On Tuesday 11 October 2022 23:35:22 Masahiro Yamada wrote: > On Tue, Oct 11, 2022 at 11:16 PM Simon Glass wrote: > > > > This is required for if_changed to work correctly. Add it. > > > > Signed-off-by: Simon Glass > > Reviewed-by: Pali Rohár > > --- > > > > (no changes since v1) > > > > Makefile | 2 +- > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > diff --git a/Makefile b/Makefile > > index 3866cc62f9a..d28e8b4e316 100644 > > --- a/Makefile > > +++ b/Makefile > > @@ -,7 +,7 @@ endef > > PHONY += inputs > > inputs: $(INPUTS-y) > > > > -all: .binman_stamp inputs > > +all: .binman_stamp inputs FORCE > > ifeq ($(CONFIG_BINMAN),y) > > $(call if_changed,binman) > > > > 'all' is usually used as a phony target. > > > I think this went wrong. > > if_changed should never be used for a phony target. > In other words, if_changed should produce a build artifact. > > > > FORCE is never used for a phony target because > it is added to 'PHONY'. > > PHONY += all > > I agree, this is really written in the wrong way. Target "all:" should be really phony target and should only depends on other target, it should not have any body / commands. And binman call should be moved to different non-phony target with the correct output file name. > > > > > endif > > -- > > 2.38.0.rc1.362.ged0d419d3c-goog > >
Re: [PATCH v2 2/5] Makefile: Correct a missing FORCE on the binman rule
On Tue, Oct 11, 2022 at 11:16 PM Simon Glass wrote: > > This is required for if_changed to work correctly. Add it. > > Signed-off-by: Simon Glass > Reviewed-by: Pali Rohár > --- > > (no changes since v1) > > Makefile | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/Makefile b/Makefile > index 3866cc62f9a..d28e8b4e316 100644 > --- a/Makefile > +++ b/Makefile > @@ -,7 +,7 @@ endef > PHONY += inputs > inputs: $(INPUTS-y) > > -all: .binman_stamp inputs > +all: .binman_stamp inputs FORCE > ifeq ($(CONFIG_BINMAN),y) > $(call if_changed,binman) 'all' is usually used as a phony target. I think this went wrong. if_changed should never be used for a phony target. In other words, if_changed should produce a build artifact. FORCE is never used for a phony target because it is added to 'PHONY'. PHONY += all > endif > -- > 2.38.0.rc1.362.ged0d419d3c-goog >
[PATCH v2 2/5] Makefile: Correct a missing FORCE on the binman rule
This is required for if_changed to work correctly. Add it. Signed-off-by: Simon Glass Reviewed-by: Pali Rohár --- (no changes since v1) Makefile | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/Makefile b/Makefile index 3866cc62f9a..d28e8b4e316 100644 --- a/Makefile +++ b/Makefile @@ -,7 +,7 @@ endef PHONY += inputs inputs: $(INPUTS-y) -all: .binman_stamp inputs +all: .binman_stamp inputs FORCE ifeq ($(CONFIG_BINMAN),y) $(call if_changed,binman) endif -- 2.38.0.rc1.362.ged0d419d3c-goog
