* [Buildroot] [PATCH 1/1] Makefile: fix rule order for legal-info @ 2023-02-11 18:43 James Hilliard 2023-02-12 9:19 ` Yann E. MORIN 2023-02-14 21:27 ` Arnout Vandecappelle 0 siblings, 2 replies; 6+ messages in thread From: James Hilliard @ 2023-02-11 18:43 UTC (permalink / raw) To: buildroot; +Cc: James Hilliard This command relies on the clean/prepare operations being in a specific order. Currently the execution ordering of the clean/prepare operations may change, for example if --shuffle=reversed is set. To ensure the evaluation order is always correct use double colon rules to make the evaluation order explicit as per make docs: The double-colon rules for a target are executed in the order they appear in the makefile. Signed-off-by: James Hilliard <james.hilliard1@gmail.com> --- Makefile | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/Makefile b/Makefile index 4880b426b5..09575a5e3c 100644 --- a/Makefile +++ b/Makefile @@ -839,7 +839,9 @@ legal-info-prepare: $(LEGAL_INFO_DIR) @cp $(BR2_CONFIG) $(LEGAL_INFO_DIR)/buildroot.config .PHONY: legal-info -legal-info: legal-info-clean legal-info-prepare $(foreach p,$(PACKAGES),$(p)-all-legal-info) \ +legal-info:: legal-info-clean +legal-info:: legal-info-prepare +legal-info:: $(foreach p,$(PACKAGES),$(p)-all-legal-info) \ $(REDIST_SOURCES_DIR_TARGET) $(REDIST_SOURCES_DIR_HOST) @cat support/legal-info/README.header >>$(LEGAL_REPORT) @if [ -r $(LEGAL_WARNINGS) ]; then \ -- 2.34.1 _______________________________________________ buildroot mailing list buildroot@buildroot.org https://lists.buildroot.org/mailman/listinfo/buildroot ^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [Buildroot] [PATCH 1/1] Makefile: fix rule order for legal-info 2023-02-11 18:43 [Buildroot] [PATCH 1/1] Makefile: fix rule order for legal-info James Hilliard @ 2023-02-12 9:19 ` Yann E. MORIN 2023-02-12 10:12 ` James Hilliard 2023-02-14 21:27 ` Arnout Vandecappelle 1 sibling, 1 reply; 6+ messages in thread From: Yann E. MORIN @ 2023-02-12 9:19 UTC (permalink / raw) To: James Hilliard; +Cc: buildroot James, All, On 2023-02-11 11:43 -0700, James Hilliard spake thusly: > This command relies on the clean/prepare operations being in a > specific order. > > Currently the execution ordering of the clean/prepare operations may > change, for example if --shuffle=reversed is set. > > To ensure the evaluation order is always correct use double colon > rules to make the evaluation order explicit as per make docs: > > The double-colon rules for a target are executed in the order they > appear in the makefile. Thanks for this patch! What bothers me a bit about using double-colon, is that the make manual explicitly state that "[d]ouble-colon rules are somewhat obscure and not often very useful; they provide a mechanism for cases in which the method used to update a target differs depending on which prerequisite files caused the update, and such cases are rare." So, I understand that it does fix the issue, but I'm afraid that it is going to be so easy to forget about that issue, and what I get from the make manual is that we should not use them unless as a last resort in very special corner cases, and the case at hand is not identified by the manual, which somewhat implies that double-colon rules were written specifically to handle the case where "the method used to update a target differs depending on which prerequisite files caused the update", not specifically to ensure that the order is guarantedd. It even goes as far as suggesting that this should not really be relied upon: The double-colon rules for a target are executed in the order they appear in the makefile. However, the cases where double-colon rules really make sense are those where the order of executing the recipes would not matter. So, to me, the ordering is only a side-effect of the original goal, which is to have different commands to generate a target, based on what prerequisite caused the the update... I am also afraid that there are so many other places around where we implicitly rely on the order the prerequisites are listed, and that we can't easily spot (that I spotted the legal-info issue and asked you about it, is pure chance, as I was looking at something completely different). However, in the legal-info case, the rule is definitely not parallel-safe, and we do really want it and its dependencies *not* to execute in parallel in fact. Indeed, all PKG-legal-info rules will emit a little blurb that is appended to the manisfest.csv, with commands like: echo 'PKG,VERSION,blablsablab' >> manifest.csv So, if we run that in parallel, this is going to explode [1]. So, I believe in this case, we do not want to use :: rules, but really ensure that legal-info and all PKG-legal-info rules are never run in parallel, *and* that they are executed in the order they are listed in the prerequisites. We probably want to just add in the top-level Makefile: .NOTPARALLEL: legal-info and in package/pkg-generic.mk, something along the lines of: .NOTPARALLEL: $(2)-legal-info #(2)-all-legal-info Thoughts? [0] https://www.gnu.org/software/make/manual/make.html#Double_002dColon [1] with very nice and bright colors, but a bit too loud still Regards, Yann E. MORIN. > Signed-off-by: James Hilliard <james.hilliard1@gmail.com> > --- > Makefile | 4 +++- > 1 file changed, 3 insertions(+), 1 deletion(-) > > diff --git a/Makefile b/Makefile > index 4880b426b5..09575a5e3c 100644 > --- a/Makefile > +++ b/Makefile > @@ -839,7 +839,9 @@ legal-info-prepare: $(LEGAL_INFO_DIR) > @cp $(BR2_CONFIG) $(LEGAL_INFO_DIR)/buildroot.config > > .PHONY: legal-info > -legal-info: legal-info-clean legal-info-prepare $(foreach p,$(PACKAGES),$(p)-all-legal-info) \ > +legal-info:: legal-info-clean > +legal-info:: legal-info-prepare > +legal-info:: $(foreach p,$(PACKAGES),$(p)-all-legal-info) \ > $(REDIST_SOURCES_DIR_TARGET) $(REDIST_SOURCES_DIR_HOST) > @cat support/legal-info/README.header >>$(LEGAL_REPORT) > @if [ -r $(LEGAL_WARNINGS) ]; then \ > -- > 2.34.1 > > _______________________________________________ > buildroot mailing list > buildroot@buildroot.org > https://lists.buildroot.org/mailman/listinfo/buildroot -- .-----------------.--------------------.------------------.--------------------. | Yann E. MORIN | Real-Time Embedded | /"\ ASCII RIBBON | Erics' conspiracy: | | +33 662 376 056 | Software Designer | \ / CAMPAIGN | ___ | | +33 561 099 427 `------------.-------: X AGAINST | \e/ There is no | | http://ymorin.is-a-geek.org/ | _/*\_ | / \ HTML MAIL | v conspiracy. | '------------------------------^-------^------------------^--------------------' _______________________________________________ buildroot mailing list buildroot@buildroot.org https://lists.buildroot.org/mailman/listinfo/buildroot ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [Buildroot] [PATCH 1/1] Makefile: fix rule order for legal-info 2023-02-12 9:19 ` Yann E. MORIN @ 2023-02-12 10:12 ` James Hilliard 2023-02-12 11:06 ` Yann E. MORIN 0 siblings, 1 reply; 6+ messages in thread From: James Hilliard @ 2023-02-12 10:12 UTC (permalink / raw) To: Yann E. MORIN; +Cc: buildroot On Sun, Feb 12, 2023 at 2:19 AM Yann E. MORIN <yann.morin.1998@free.fr> wrote: > > James, All, > > On 2023-02-11 11:43 -0700, James Hilliard spake thusly: > > This command relies on the clean/prepare operations being in a > > specific order. > > > > Currently the execution ordering of the clean/prepare operations may > > change, for example if --shuffle=reversed is set. > > > > To ensure the evaluation order is always correct use double colon > > rules to make the evaluation order explicit as per make docs: > > > > The double-colon rules for a target are executed in the order they > > appear in the makefile. > > Thanks for this patch! > > What bothers me a bit about using double-colon, is that the make manual > explicitly state that "[d]ouble-colon rules are somewhat obscure and not > often very useful; they provide a mechanism for cases in which the > method used to update a target differs depending on which prerequisite > files caused the update, and such cases are rare." > > So, I understand that it does fix the issue, but I'm afraid that it is > going to be so easy to forget about that issue, and what I get from the > make manual is that we should not use them unless as a last resort in > very special corner cases, and the case at hand is not identified by the > manual, which somewhat implies that double-colon rules were written > specifically to handle the case where "the method used to update a > target differs depending on which prerequisite files caused the update", > not specifically to ensure that the order is guarantedd. It even goes as > far as suggesting that this should not really be relied upon: > > The double-colon rules for a target are executed in the order they > appear in the makefile. However, the cases where double-colon rules > really make sense are those where the order of executing the recipes > would not matter. > > So, to me, the ordering is only a side-effect of the original goal, > which is to have different commands to generate a target, based on what > prerequisite caused the the update... > > I am also afraid that there are so many other places around where we > implicitly rely on the order the prerequisites are listed, and that we > can't easily spot (that I spotted the legal-info issue and asked you > about it, is pure chance, as I was looking at something completely > different). Well we should be able to flush them out using make shuffle mode. That's how I caught the other issues at least. > > However, in the legal-info case, the rule is definitely not > parallel-safe, and we do really want it and its dependencies *not* to > execute in parallel in fact. > > Indeed, all PKG-legal-info rules will emit a little blurb that is > appended to the manisfest.csv, with commands like: > echo 'PKG,VERSION,blablsablab' >> manifest.csv > > So, if we run that in parallel, this is going to explode [1]. Hmm, might just need a flock operation when doing the manifest.csv file echo append. We do something like that for downloads already: https://github.com/buildroot/buildroot/blob/2022.11.1/package/pkg-download.mk#L113 > > So, I believe in this case, we do not want to use :: rules, but really > ensure that legal-info and all PKG-legal-info rules are never run in > parallel, *and* that they are executed in the order they are listed in > the prerequisites. Well legal-info can take a while to run so it might make sense to make it work in parallel. > > We probably want to just add in the top-level Makefile: > > .NOTPARALLEL: legal-info > > and in package/pkg-generic.mk, something along the lines of: > > .NOTPARALLEL: $(2)-legal-info #(2)-all-legal-info > > Thoughts? Might work...but it seems like using flock may be simpler and faster. > > [0] https://www.gnu.org/software/make/manual/make.html#Double_002dColon > [1] with very nice and bright colors, but a bit too loud still > > Regards, > Yann E. MORIN. > > > Signed-off-by: James Hilliard <james.hilliard1@gmail.com> > > --- > > Makefile | 4 +++- > > 1 file changed, 3 insertions(+), 1 deletion(-) > > > > diff --git a/Makefile b/Makefile > > index 4880b426b5..09575a5e3c 100644 > > --- a/Makefile > > +++ b/Makefile > > @@ -839,7 +839,9 @@ legal-info-prepare: $(LEGAL_INFO_DIR) > > @cp $(BR2_CONFIG) $(LEGAL_INFO_DIR)/buildroot.config > > > > .PHONY: legal-info > > -legal-info: legal-info-clean legal-info-prepare $(foreach p,$(PACKAGES),$(p)-all-legal-info) \ > > +legal-info:: legal-info-clean > > +legal-info:: legal-info-prepare > > +legal-info:: $(foreach p,$(PACKAGES),$(p)-all-legal-info) \ > > $(REDIST_SOURCES_DIR_TARGET) $(REDIST_SOURCES_DIR_HOST) > > @cat support/legal-info/README.header >>$(LEGAL_REPORT) > > @if [ -r $(LEGAL_WARNINGS) ]; then \ > > -- > > 2.34.1 > > > > _______________________________________________ > > buildroot mailing list > > buildroot@buildroot.org > > https://lists.buildroot.org/mailman/listinfo/buildroot > > -- > .-----------------.--------------------.------------------.--------------------. > | Yann E. MORIN | Real-Time Embedded | /"\ ASCII RIBBON | Erics' conspiracy: | > | +33 662 376 056 | Software Designer | \ / CAMPAIGN | ___ | > | +33 561 099 427 `------------.-------: X AGAINST | \e/ There is no | > | http://ymorin.is-a-geek.org/ | _/*\_ | / \ HTML MAIL | v conspiracy. | > '------------------------------^-------^------------------^--------------------' _______________________________________________ buildroot mailing list buildroot@buildroot.org https://lists.buildroot.org/mailman/listinfo/buildroot ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [Buildroot] [PATCH 1/1] Makefile: fix rule order for legal-info 2023-02-12 10:12 ` James Hilliard @ 2023-02-12 11:06 ` Yann E. MORIN 2023-02-12 11:25 ` James Hilliard 0 siblings, 1 reply; 6+ messages in thread From: Yann E. MORIN @ 2023-02-12 11:06 UTC (permalink / raw) To: James Hilliard; +Cc: buildroot James, All, On 2023-02-12 03:12 -0700, James Hilliard spake thusly: > On Sun, Feb 12, 2023 at 2:19 AM Yann E. MORIN <yann.morin.1998@free.fr> wrote: > > On 2023-02-11 11:43 -0700, James Hilliard spake thusly: > > > This command relies on the clean/prepare operations being in a > > > specific order. [--SNIP--] > > However, in the legal-info case, the rule is definitely not > > parallel-safe, and we do really want it and its dependencies *not* to > > execute in parallel in fact. > > > > Indeed, all PKG-legal-info rules will emit a little blurb that is > > appended to the manisfest.csv, with commands like: > > echo 'PKG,VERSION,blablsablab' >> manifest.csv > > > > So, if we run that in parallel, this is going to explode [1]. > Hmm, might just need a flock operation when doing the manifest.csv > file echo append. Except we probably want, no, sorry: we do want, that the manifest to be reproducible (and alphabeticaly sorted), which would not be guaranteed with an flock. > We do something like that for downloads already: > https://github.com/buildroot/buildroot/blob/2022.11.1/package/pkg-download.mk#L113 Careful there: the flock is to protect against a concurrent Buildroot build, not an internal protection (although it also serves as an internal protection, when two packages have the same dl dir, like mesa3d and mesa3d-headers). > > So, I believe in this case, we do not want to use :: rules, but really > > ensure that legal-info and all PKG-legal-info rules are never run in > > parallel, *and* that they are executed in the order they are listed in > > the prerequisites. > > Well legal-info can take a while to run so it might make sense to make > it work in parallel. I understand that, but I still think legal-info should produce a reproducible output, and that can't be guaranteed if all we change is using an flock around the manifests. Also, legal-info is probably run after a complete build, so the overhead of downloading, extracting, and patching, ahs already been paid during the build. We talked about legal-info during the dev-days, and one idea that floated was that we need to generate an SBOM of the build, and the closest we have now is legal-info. The SBOM should always be generated, and I believe legal-info should awlays be generated, so the overhead point would be moot (but would be replaced by the overhead of running legal-info for every builds...) More needs to be dug on that topic... Regards, Yann E. MORIN. -- .-----------------.--------------------.------------------.--------------------. | Yann E. MORIN | Real-Time Embedded | /"\ ASCII RIBBON | Erics' conspiracy: | | +33 662 376 056 | Software Designer | \ / CAMPAIGN | ___ | | +33 561 099 427 `------------.-------: X AGAINST | \e/ There is no | | http://ymorin.is-a-geek.org/ | _/*\_ | / \ HTML MAIL | v conspiracy. | '------------------------------^-------^------------------^--------------------' _______________________________________________ buildroot mailing list buildroot@buildroot.org https://lists.buildroot.org/mailman/listinfo/buildroot ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [Buildroot] [PATCH 1/1] Makefile: fix rule order for legal-info 2023-02-12 11:06 ` Yann E. MORIN @ 2023-02-12 11:25 ` James Hilliard 0 siblings, 0 replies; 6+ messages in thread From: James Hilliard @ 2023-02-12 11:25 UTC (permalink / raw) To: Yann E. MORIN; +Cc: buildroot On Sun, Feb 12, 2023 at 4:06 AM Yann E. MORIN <yann.morin.1998@free.fr> wrote: > > James, All, > > On 2023-02-12 03:12 -0700, James Hilliard spake thusly: > > On Sun, Feb 12, 2023 at 2:19 AM Yann E. MORIN <yann.morin.1998@free.fr> wrote: > > > On 2023-02-11 11:43 -0700, James Hilliard spake thusly: > > > > This command relies on the clean/prepare operations being in a > > > > specific order. > [--SNIP--] > > > However, in the legal-info case, the rule is definitely not > > > parallel-safe, and we do really want it and its dependencies *not* to > > > execute in parallel in fact. > > > > > > Indeed, all PKG-legal-info rules will emit a little blurb that is > > > appended to the manisfest.csv, with commands like: > > > echo 'PKG,VERSION,blablsablab' >> manifest.csv > > > > > > So, if we run that in parallel, this is going to explode [1]. > > Hmm, might just need a flock operation when doing the manifest.csv > > file echo append. > > Except we probably want, no, sorry: we do want, that the manifest to be > reproducible (and alphabeticaly sorted), which would not be guaranteed > with an flock. Hmm, I thought we were sorting afterwards like this, guess that needs to be checked: https://github.com/buildroot/buildroot/blob/59b2e826f2a6c1874c5d032d3a5852326028bc0e/Makefile#L851 > > > We do something like that for downloads already: > > https://github.com/buildroot/buildroot/blob/2022.11.1/package/pkg-download.mk#L113 > > Careful there: the flock is to protect against a concurrent Buildroot > build, not an internal protection (although it also serves as an > internal protection, when two packages have the same dl dir, like mesa3d > and mesa3d-headers). I assumed it was mostly for packages with the same dl dir, I think it matters more what it can do rather than what it was designed to do ;). > > > > So, I believe in this case, we do not want to use :: rules, but really > > > ensure that legal-info and all PKG-legal-info rules are never run in > > > parallel, *and* that they are executed in the order they are listed in > > > the prerequisites. > > > > Well legal-info can take a while to run so it might make sense to make > > it work in parallel. > > I understand that, but I still think legal-info should produce a > reproducible output, and that can't be guaranteed if all we change is > using an flock around the manifests. It should be reproducible if we run a sort right? > > Also, legal-info is probably run after a complete build, so the overhead > of downloading, extracting, and patching, ahs already been paid during > the build. Doesn't it still take a little while even if that's done? > > We talked about legal-info during the dev-days, and one idea that > floated was that we need to generate an SBOM of the build, and the > closest we have now is legal-info. The SBOM should always be generated, > and I believe legal-info should awlays be generated, so the overhead > point would be moot (but would be replaced by the overhead of running > legal-info for every builds...) More needs to be dug on that topic... Well I think for both cases the faster the generation the better so parallel where possible has advantages there. If something is too slow it's less likely to be used/tested. > > Regards, > Yann E. MORIN. > > -- > .-----------------.--------------------.------------------.--------------------. > | Yann E. MORIN | Real-Time Embedded | /"\ ASCII RIBBON | Erics' conspiracy: | > | +33 662 376 056 | Software Designer | \ / CAMPAIGN | ___ | > | +33 561 099 427 `------------.-------: X AGAINST | \e/ There is no | > | http://ymorin.is-a-geek.org/ | _/*\_ | / \ HTML MAIL | v conspiracy. | > '------------------------------^-------^------------------^--------------------' _______________________________________________ buildroot mailing list buildroot@buildroot.org https://lists.buildroot.org/mailman/listinfo/buildroot ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [Buildroot] [PATCH 1/1] Makefile: fix rule order for legal-info 2023-02-11 18:43 [Buildroot] [PATCH 1/1] Makefile: fix rule order for legal-info James Hilliard 2023-02-12 9:19 ` Yann E. MORIN @ 2023-02-14 21:27 ` Arnout Vandecappelle 1 sibling, 0 replies; 6+ messages in thread From: Arnout Vandecappelle @ 2023-02-14 21:27 UTC (permalink / raw) To: James Hilliard, buildroot On 11/02/2023 19:43, James Hilliard wrote: > This command relies on the clean/prepare operations being in a > specific order. > > Currently the execution ordering of the clean/prepare operations may > change, for example if --shuffle=reversed is set. > > To ensure the evaluation order is always correct use double colon > rules to make the evaluation order explicit as per make docs: > > The double-colon rules for a target are executed in the order they > appear in the makefile. > > Signed-off-by: James Hilliard <james.hilliard1@gmail.com> > --- > Makefile | 4 +++- > 1 file changed, 3 insertions(+), 1 deletion(-) > > diff --git a/Makefile b/Makefile > index 4880b426b5..09575a5e3c 100644 > --- a/Makefile > +++ b/Makefile > @@ -839,7 +839,9 @@ legal-info-prepare: $(LEGAL_INFO_DIR) > @cp $(BR2_CONFIG) $(LEGAL_INFO_DIR)/buildroot.config > > .PHONY: legal-info > -legal-info: legal-info-clean legal-info-prepare $(foreach p,$(PACKAGES),$(p)-all-legal-info) \ > +legal-info:: legal-info-clean > +legal-info:: legal-info-prepare > +legal-info:: $(foreach p,$(PACKAGES),$(p)-all-legal-info) \ > $(REDIST_SOURCES_DIR_TARGET) $(REDIST_SOURCES_DIR_HOST) Like for rebuild, reinstall, reconfigure I think the following should work: legal-info-prepare: | legal-info-clean $(1)-all-legal-info: | legal-info-prepare (the last one would be in pkg-generic instead of here, of course). Although in this case I think a full dependency should be even OK, so without the | Regards, Arnout > @cat support/legal-info/README.header >>$(LEGAL_REPORT) > @if [ -r $(LEGAL_WARNINGS) ]; then \ _______________________________________________ buildroot mailing list buildroot@buildroot.org https://lists.buildroot.org/mailman/listinfo/buildroot ^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2023-02-14 21:27 UTC | newest] Thread overview: 6+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2023-02-11 18:43 [Buildroot] [PATCH 1/1] Makefile: fix rule order for legal-info James Hilliard 2023-02-12 9:19 ` Yann E. MORIN 2023-02-12 10:12 ` James Hilliard 2023-02-12 11:06 ` Yann E. MORIN 2023-02-12 11:25 ` James Hilliard 2023-02-14 21:27 ` Arnout Vandecappelle
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox