* [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 an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.