[Buildroot] [PATCH 1/1] package/pkg-generic.mk: fix rule order for re{install, build, configure}

[Buildroot] [PATCH 1/1] package/pkg-generic.mk: fix rule order for re{install, build, configure}

[James Hilliard]

These command rely on the clean operations being first so that the
stamp files being deleted will rebuild the targets.

The execution ordering of the clean and rebuild 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>
---
 package/pkg-generic.mk | 9 ++++++---
 1 file changed, 6 insertions(+), 3 deletions(-)

diff --git a/package/pkg-generic.mk b/package/pkg-generic.mk
index f24e03a325..6cb461af90 100644
--- a/package/pkg-generic.mk
+++ b/package/pkg-generic.mk
@@ -1057,17 +1057,20 @@ endif
 			rm -f $$($(2)_TARGET_INSTALL_IMAGES)
 			rm -f $$($(2)_TARGET_INSTALL_HOST)
 
-$(1)-reinstall:		$(1)-clean-for-reinstall $(1)
+$(1)-reinstall::		$(1)-clean-for-reinstall
+$(1)-reinstall::		$(1)
 
 $(1)-clean-for-rebuild: $(1)-clean-for-reinstall
 			rm -f $$($(2)_TARGET_BUILD)
 
-$(1)-rebuild:		$(1)-clean-for-rebuild $(1)
+$(1)-rebuild::		$(1)-clean-for-rebuild
+$(1)-rebuild::		$(1)
 
 $(1)-clean-for-reconfigure: $(1)-clean-for-rebuild
 			rm -f $$($(2)_TARGET_CONFIGURE)
 
-$(1)-reconfigure:	$(1)-clean-for-reconfigure $(1)
+$(1)-reconfigure::	$(1)-clean-for-reconfigure
+$(1)-reconfigure::	$(1)
 
 # define the PKG variable for all targets, containing the
 # uppercase package variable prefix
-- 
2.34.1

_______________________________________________
buildroot mailing list
buildroot@buildroot.org
https://lists.buildroot.org/mailman/listinfo/buildroot

Re: [Buildroot] [PATCH 1/1] package/pkg-generic.mk: fix rule order for re{install, build, configure}

[Charles Hardin]

[-- Attachment #1.1: Type: text/plain, Size: 2172 bytes --]

On Mon, Oct 17, 2022 at 8:46 PM James Hilliard <james.hilliard1@gmail.com>
wrote:

> These command rely on the clean operations being first so that the
> stamp files being deleted will rebuild the targets.
>
> The execution ordering of the clean and rebuild 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>
>

Tested-by: Charles Hardin <ckhardin@gmail.com>

Set LINUX_OVERRIDE_SRCDIR in local.mk
touch arm64/kernel/setup.c
make linux-rebuild
See Building steps with CC on the changed file setup.c


> ---
>  package/pkg-generic.mk | 9 ++++++---
>  1 file changed, 6 insertions(+), 3 deletions(-)
>
> diff --git a/package/pkg-generic.mk b/package/pkg-generic.mk
> index f24e03a325..6cb461af90 100644
> --- a/package/pkg-generic.mk
> +++ b/package/pkg-generic.mk
> @@ -1057,17 +1057,20 @@ endif
>                         rm -f $$($(2)_TARGET_INSTALL_IMAGES)
>                         rm -f $$($(2)_TARGET_INSTALL_HOST)
>
> -$(1)-reinstall:                $(1)-clean-for-reinstall $(1)
> +$(1)-reinstall::               $(1)-clean-for-reinstall
> +$(1)-reinstall::               $(1)
>
>  $(1)-clean-for-rebuild: $(1)-clean-for-reinstall
>                         rm -f $$($(2)_TARGET_BUILD)
>
> -$(1)-rebuild:          $(1)-clean-for-rebuild $(1)
> +$(1)-rebuild::         $(1)-clean-for-rebuild
> +$(1)-rebuild::         $(1)
>
>  $(1)-clean-for-reconfigure: $(1)-clean-for-rebuild
>                         rm -f $$($(2)_TARGET_CONFIGURE)
>
> -$(1)-reconfigure:      $(1)-clean-for-reconfigure $(1)
> +$(1)-reconfigure::     $(1)-clean-for-reconfigure
> +$(1)-reconfigure::     $(1)
>
>  # define the PKG variable for all targets, containing the
>  # uppercase package variable prefix
> --
> 2.34.1
>
> _______________________________________________
> buildroot mailing list
> buildroot@buildroot.org
> https://lists.buildroot.org/mailman/listinfo/buildroot
>

[-- Attachment #1.2: Type: text/html, Size: 3627 bytes --]

[-- Attachment #2: Type: text/plain, Size: 150 bytes --]

_______________________________________________
buildroot mailing list
buildroot@buildroot.org
https://lists.buildroot.org/mailman/listinfo/buildroot

Re: [Buildroot] [PATCH 1/1] package/pkg-generic.mk: fix rule order for re{install, build, configure}

[Yann E. MORIN]

James, All,

Thanks again for working on such grunt work, it's much appreaciated!

On 2022-10-17 21:46 -0600, James Hilliard spake thusly:
> These command rely on the clean operations being first so that the
> stamp files being deleted will rebuild the targets.
> 
> The execution ordering of the clean and rebuild 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.

I am not convinced that we should switch over to using double-colons.

As I reviewed your other related patch about legal-info [0], the make
manual [1] suggests that double-colons are rally intended for cases
where "the method used to update a target differs depending on which
prerequisite files caused the update" and "double-colon rules really
make sense are those where the order of executing the recipes would not
matter".

So, we'd use a mechanism where order should not matter, to solve an
issue with ordering. That'd be weird...

Yes, the manual states that "double-colon rules for a target are
executed in the order they appear in the makefile", but that really
looks like a side-effect of another goal.

Also, we'd need to carefully review all our Makefile to ensure that
there are no other cases wehre ordering of prerequisites matter. We'd
also have to be careful every time we introduce a new rule and remember
to check whether it depends on the ordering prereauisites or not, adn
worse yet, also whether we can afford the rule to be parallel-safe or
not (see the explanation for that in the legal-info review).

Having colons and double-colons is going to be quite a headache... We
already have issues with remembering that we need to use $$ to expand
variables in some places, and the rules there are relatively clean and
explicit, but for the :: case, I'm afraid that's going to be a bit more
complex.

We currently have a single location where we use double-colon:
    support/kconfig/Makefile.br

which is added by our patch against kconfig:
    support/kconfig/patches/10-br-build-system.patch

which dates back to 2010:
    7c524dd0b683 Clean up our patches against kconfig

which got in three in 2007:
    a665ed34960b - pull kconfig from linux-2.6.21.5

So, I'd like that we get another way to fix that, if possible...

Again, thanks a lot for spending efforts in that area, this is really
great!

[0] https://lore.kernel.org/buildroot/20230212091914.GJ2796@scaer/
[1] https://www.gnu.org/software/make/manual/make.html#Double_002dColon

Regards,
Yann E. MORIN.

> Signed-off-by: James Hilliard <james.hilliard1@gmail.com>
> ---
>  package/pkg-generic.mk | 9 ++++++---
>  1 file changed, 6 insertions(+), 3 deletions(-)
> 
> diff --git a/package/pkg-generic.mk b/package/pkg-generic.mk
> index f24e03a325..6cb461af90 100644
> --- a/package/pkg-generic.mk
> +++ b/package/pkg-generic.mk
> @@ -1057,17 +1057,20 @@ endif
>  			rm -f $$($(2)_TARGET_INSTALL_IMAGES)
>  			rm -f $$($(2)_TARGET_INSTALL_HOST)
>  
> -$(1)-reinstall:		$(1)-clean-for-reinstall $(1)
> +$(1)-reinstall::		$(1)-clean-for-reinstall
> +$(1)-reinstall::		$(1)
>  
>  $(1)-clean-for-rebuild: $(1)-clean-for-reinstall
>  			rm -f $$($(2)_TARGET_BUILD)
>  
> -$(1)-rebuild:		$(1)-clean-for-rebuild $(1)
> +$(1)-rebuild::		$(1)-clean-for-rebuild
> +$(1)-rebuild::		$(1)
>  
>  $(1)-clean-for-reconfigure: $(1)-clean-for-rebuild
>  			rm -f $$($(2)_TARGET_CONFIGURE)
>  
> -$(1)-reconfigure:	$(1)-clean-for-reconfigure $(1)
> +$(1)-reconfigure::	$(1)-clean-for-reconfigure
> +$(1)-reconfigure::	$(1)
>  
>  # define the PKG variable for all targets, containing the
>  # uppercase package variable prefix
> -- 
> 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

Re: [Buildroot] [PATCH 1/1] package/pkg-generic.mk: fix rule order for re{install, build, configure}

[James Hilliard]

On Sun, Feb 12, 2023 at 2:42 AM Yann E. MORIN <yann.morin.1998@free.fr> wrote:
>
> James, All,
>
> Thanks again for working on such grunt work, it's much appreaciated!
>
> On 2022-10-17 21:46 -0600, James Hilliard spake thusly:
> > These command rely on the clean operations being first so that the
> > stamp files being deleted will rebuild the targets.
> >
> > The execution ordering of the clean and rebuild 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.
>
> I am not convinced that we should switch over to using double-colons.
>
> As I reviewed your other related patch about legal-info [0], the make
> manual [1] suggests that double-colons are rally intended for cases
> where "the method used to update a target differs depending on which
> prerequisite files caused the update" and "double-colon rules really
> make sense are those where the order of executing the recipes would not
> matter".

This appears to be the only documented method of enforcing execution
order without disabling parallel builds...that I've come across at least.

>
> So, we'd use a mechanism where order should not matter, to solve an
> issue with ordering. That'd be weird...
>
> Yes, the manual states that "double-colon rules for a target are
> executed in the order they appear in the makefile", but that really
> looks like a side-effect of another goal.

Yeah, I agree it's not really the intended use case for double-colon rules,
however the ordering behavior is documented so I would not expect it to
break.

>
> Also, we'd need to carefully review all our Makefile to ensure that
> there are no other cases wehre ordering of prerequisites matter. We'd
> also have to be careful every time we introduce a new rule and remember
> to check whether it depends on the ordering prereauisites or not, adn
> worse yet, also whether we can afford the rule to be parallel-safe or
> not (see the explanation for that in the legal-info review).

So unlike the legal-info issue using .NOTPARALLEL would not work
here as these make targets do need to work with parallel builds.

>
> Having colons and double-colons is going to be quite a headache... We
> already have issues with remembering that we need to use $$ to expand
> variables in some places, and the rules there are relatively clean and
> explicit, but for the :: case, I'm afraid that's going to be a bit more
> complex.

I don't think we have all that many of these issues, but I could be wrong.

The clean-for-re{install, build, configure} make targets are a bit hacky
to begin with so I don't think make was intended to handle those cases
in the first place. We're already in unintended feature use case territory
here so this doesn't seem to make things much worse.

>
> We currently have a single location where we use double-colon:
>     support/kconfig/Makefile.br
>
> which is added by our patch against kconfig:
>     support/kconfig/patches/10-br-build-system.patch
>
> which dates back to 2010:
>     7c524dd0b683 Clean up our patches against kconfig
>
> which got in three in 2007:
>     a665ed34960b - pull kconfig from linux-2.6.21.5
>
> So, I'd like that we get another way to fix that, if possible...

I have unfortunately not found any other method to fix the execution
ordering...

>
> Again, thanks a lot for spending efforts in that area, this is really
> great!
>
> [0] https://lore.kernel.org/buildroot/20230212091914.GJ2796@scaer/
> [1] https://www.gnu.org/software/make/manual/make.html#Double_002dColon
>
> Regards,
> Yann E. MORIN.
>
> > Signed-off-by: James Hilliard <james.hilliard1@gmail.com>
> > ---
> >  package/pkg-generic.mk | 9 ++++++---
> >  1 file changed, 6 insertions(+), 3 deletions(-)
> >
> > diff --git a/package/pkg-generic.mk b/package/pkg-generic.mk
> > index f24e03a325..6cb461af90 100644
> > --- a/package/pkg-generic.mk
> > +++ b/package/pkg-generic.mk
> > @@ -1057,17 +1057,20 @@ endif
> >                       rm -f $$($(2)_TARGET_INSTALL_IMAGES)
> >                       rm -f $$($(2)_TARGET_INSTALL_HOST)
> >
> > -$(1)-reinstall:              $(1)-clean-for-reinstall $(1)
> > +$(1)-reinstall::             $(1)-clean-for-reinstall
> > +$(1)-reinstall::             $(1)
> >
> >  $(1)-clean-for-rebuild: $(1)-clean-for-reinstall
> >                       rm -f $$($(2)_TARGET_BUILD)
> >
> > -$(1)-rebuild:                $(1)-clean-for-rebuild $(1)
> > +$(1)-rebuild::               $(1)-clean-for-rebuild
> > +$(1)-rebuild::               $(1)
> >
> >  $(1)-clean-for-reconfigure: $(1)-clean-for-rebuild
> >                       rm -f $$($(2)_TARGET_CONFIGURE)
> >
> > -$(1)-reconfigure:    $(1)-clean-for-reconfigure $(1)
> > +$(1)-reconfigure::   $(1)-clean-for-reconfigure
> > +$(1)-reconfigure::   $(1)
> >
> >  # define the PKG variable for all targets, containing the
> >  # uppercase package variable prefix
> > --
> > 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

Re: [Buildroot] [PATCH 1/1] package/pkg-generic.mk: fix rule order for re{install, build, configure}

[Yann E. MORIN]

James, All,

On 2023-02-12 03:02 -0700, James Hilliard spake thusly:
> On Sun, Feb 12, 2023 at 2:42 AM Yann E. MORIN <yann.morin.1998@free.fr> wrote:
> > On 2022-10-17 21:46 -0600, James Hilliard spake thusly:
> > > 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.
[--SNIP--]
> > So, I'd like that we get another way to fix that, if possible...
> I have unfortunately not found any other method to fix the execution
> ordering...

What about something along the lines of (untestd!):

    $(1)-rebuild: $(1): $(1)-clean-for-rebuild
    $(1)-rebuild: $(1)

We already use such a construct in package/pkg-kconfig.mk, but only
once, so this is not as if it were widespread either...

Still, I think it is more easy to read...

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

Re: [Buildroot] [PATCH 1/1] package/pkg-generic.mk: fix rule order for re{install, build, configure}

[James Hilliard]

On Sun, Feb 12, 2023 at 3:11 AM Yann E. MORIN <yann.morin.1998@free.fr> wrote:
>
> James, All,
>
> On 2023-02-12 03:02 -0700, James Hilliard spake thusly:
> > On Sun, Feb 12, 2023 at 2:42 AM Yann E. MORIN <yann.morin.1998@free.fr> wrote:
> > > On 2022-10-17 21:46 -0600, James Hilliard spake thusly:
> > > > 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.
> [--SNIP--]
> > > So, I'd like that we get another way to fix that, if possible...
> > I have unfortunately not found any other method to fix the execution
> > ordering...
>
> What about something along the lines of (untestd!):
>
>     $(1)-rebuild: $(1): $(1)-clean-for-rebuild
>     $(1)-rebuild: $(1)

That would appear to be invalid syntax.

>
> We already use such a construct in package/pkg-kconfig.mk, but only
> once, so this is not as if it were widespread either...

I don't see anything like that there, just this which isn't doing anything
unusual with multiple prerequisites that require a specific execution
order like the re{install,build,configure} rules in pkg-generic.mk:

# Force olddefconfig again on -reconfigure
$(1)-clean-for-reconfigure: $(1)-clean-kconfig-for-reconfigure

$(1)-clean-kconfig-for-reconfigure:
    rm -f $$($(2)_DIR)/$$($(2)_KCONFIG_STAMP_DOTCONFIG)

>
> Still, I think it is more easy to read...
>
> 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

Re: [Buildroot] [PATCH 1/1] package/pkg-generic.mk: fix rule order for re{install, build, configure}

[Yann E. MORIN]

James, All,

On 2023-02-12 03:22 -0700, James Hilliard spake thusly:
> On Sun, Feb 12, 2023 at 3:11 AM Yann E. MORIN <yann.morin.1998@free.fr> wrote:
> >
> > James, All,
> >
> > On 2023-02-12 03:02 -0700, James Hilliard spake thusly:
> > > On Sun, Feb 12, 2023 at 2:42 AM Yann E. MORIN <yann.morin.1998@free.fr> wrote:
> > > > On 2022-10-17 21:46 -0600, James Hilliard spake thusly:
> > > > > 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.
> > [--SNIP--]
> > > > So, I'd like that we get another way to fix that, if possible...
> > > I have unfortunately not found any other method to fix the execution
> > > ordering...
> >
> > What about something along the lines of (untestd!):
> >
> >     $(1)-rebuild: $(1): $(1)-clean-for-rebuild
> >     $(1)-rebuild: $(1)
> 
> That would appear to be invalid syntax.
> 
> >
> > We already use such a construct in package/pkg-kconfig.mk, but only
> > once, so this is not as if it were widespread either...
> 
> I don't see anything like that there,

    240:  $$(addprefix $(1)-,$$($(2)_KCONFIG_EDITORS)): $(1)-%: $$($(2)_DIR)/.kconfig_editor_%

So, this looks like it is named "target pattern", and it works with a
pattern (eh!).

    $ cat Makefile
     1  all: a
     2
     3  a:
     4          @echo a
     5
     6  a-clean:
     7          @sleep 1
     8          @echo a-clean
     9
    10  a-rebuild: %-rebuild: %-clean
    11  a-rebuild: a

However, that does not work with parallel builds:

    $ make
    a

    $ make a
    a

    $ make a-rebuild
    a-clean
    a

    $ make -j a-rebuild
    a
    a-clean

However, you stated in a previous reply:

    So unlike the legal-info issue using .NOTPARALLEL would not work
    here as these make targets do need to work with parallel builds.

I don't think that's true. Indeed, foo-rebuild is usually done after a
ful build, when iterating over the foo package, iether to test a new
source tree (with ocerride-ssrcdir) or to test fixes in foo.mk. In
either case, we do not need a top-level parallel build, because only one
package, foo, will be built.

So, even in this case, we should be able to use .NOTPARALLEL, no?

It also means that the kconfig rule I mentionned above, does not work
with top-level parallel builds, but that's OK: it's only used to spawn
kconfig configurators (menuconfig et al.), so it should not be used with
top-level parallel, and thus should require .NOTPARALLEL as well.

Meh, TLPB and shuffling are really uncovering issues everywhere... :-(

Regards,
Yann E. MORIN.

> just this which isn't doing anything
> unusual with multiple prerequisites that require a specific execution
> order like the re{install,build,configure} rules in pkg-generic.mk:
> 
> # Force olddefconfig again on -reconfigure
> $(1)-clean-for-reconfigure: $(1)-clean-kconfig-for-reconfigure
> 
> $(1)-clean-kconfig-for-reconfigure:
>     rm -f $$($(2)_DIR)/$$($(2)_KCONFIG_STAMP_DOTCONFIG)
> 
> >
> > Still, I think it is more easy to read...
> >
> > 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.  |
> > '------------------------------^-------^------------------^--------------------'

-- 
.-----------------.--------------------.------------------.--------------------.
|  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

Re: [Buildroot] [PATCH 1/1] package/pkg-generic.mk: fix rule order for re{install, build, configure}

[James Hilliard]

On Sun, Feb 12, 2023 at 3:57 AM Yann E. MORIN <yann.morin.1998@free.fr> wrote:
>
> James, All,
>
> On 2023-02-12 03:22 -0700, James Hilliard spake thusly:
> > On Sun, Feb 12, 2023 at 3:11 AM Yann E. MORIN <yann.morin.1998@free.fr> wrote:
> > >
> > > James, All,
> > >
> > > On 2023-02-12 03:02 -0700, James Hilliard spake thusly:
> > > > On Sun, Feb 12, 2023 at 2:42 AM Yann E. MORIN <yann.morin.1998@free.fr> wrote:
> > > > > On 2022-10-17 21:46 -0600, James Hilliard spake thusly:
> > > > > > 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.
> > > [--SNIP--]
> > > > > So, I'd like that we get another way to fix that, if possible...
> > > > I have unfortunately not found any other method to fix the execution
> > > > ordering...
> > >
> > > What about something along the lines of (untestd!):
> > >
> > >     $(1)-rebuild: $(1): $(1)-clean-for-rebuild
> > >     $(1)-rebuild: $(1)
> >
> > That would appear to be invalid syntax.
> >
> > >
> > > We already use such a construct in package/pkg-kconfig.mk, but only
> > > once, so this is not as if it were widespread either...
> >
> > I don't see anything like that there,
>
>     240:  $$(addprefix $(1)-,$$($(2)_KCONFIG_EDITORS)): $(1)-%: $$($(2)_DIR)/.kconfig_editor_%
>
> So, this looks like it is named "target pattern", and it works with a
> pattern (eh!).
>
>     $ cat Makefile
>      1  all: a
>      2
>      3  a:
>      4          @echo a
>      5
>      6  a-clean:
>      7          @sleep 1
>      8          @echo a-clean
>      9
>     10  a-rebuild: %-rebuild: %-clean
>     11  a-rebuild: a
>
> However, that does not work with parallel builds:
>
>     $ make
>     a
>
>     $ make a
>     a
>
>     $ make a-rebuild
>     a-clean
>     a
>
>     $ make -j a-rebuild
>     a
>     a-clean
>
> However, you stated in a previous reply:
>
>     So unlike the legal-info issue using .NOTPARALLEL would not work
>     here as these make targets do need to work with parallel builds.
>
> I don't think that's true. Indeed, foo-rebuild is usually done after a
> ful build, when iterating over the foo package, iether to test a new
> source tree (with ocerride-ssrcdir) or to test fixes in foo.mk. In
> either case, we do not need a top-level parallel build, because only one
> package, foo, will be built.
>
> So, even in this case, we should be able to use .NOTPARALLEL, no?

Breaking parallel build for foo-rebuild would be rather unexpected behavior
as foo-rebuild does work for full builds as well and one may default to using
foo-rebuild instead of a foo target for various reasons(to ensure that a package
is always rsynced from an overridden source directory for example).

I see no good reason to disable parallel build on it when simply ordering the
dependency execution order fixes the issue.

>
> It also means that the kconfig rule I mentionned above, does not work
> with top-level parallel builds, but that's OK: it's only used to spawn
> kconfig configurators (menuconfig et al.), so it should not be used with
> top-level parallel, and thus should require .NOTPARALLEL as well.

Might be better to use double colon rules there as well, they are more
explicit than using .NOTPARALLEL IMO as .NOTPARALLEL doesn't
specify an execution order, also it can potentially introduce accidental
performance regressions.

>
> Meh, TLPB and shuffling are really uncovering issues everywhere... :-(

Better to make them very obvious so they can be fixed IMO.

Once the known issues are fixed I can run shuffle mode on my autobuilders
to uncover remaining issues.

>
> Regards,
> Yann E. MORIN.
>
> > just this which isn't doing anything
> > unusual with multiple prerequisites that require a specific execution
> > order like the re{install,build,configure} rules in pkg-generic.mk:
> >
> > # Force olddefconfig again on -reconfigure
> > $(1)-clean-for-reconfigure: $(1)-clean-kconfig-for-reconfigure
> >
> > $(1)-clean-kconfig-for-reconfigure:
> >     rm -f $$($(2)_DIR)/$$($(2)_KCONFIG_STAMP_DOTCONFIG)
> >
> > >
> > > Still, I think it is more easy to read...
> > >
> > > 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.  |
> > > '------------------------------^-------^------------------^--------------------'
>
> --
> .-----------------.--------------------.------------------.--------------------.
> |  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

Re: [Buildroot] [PATCH 1/1] package/pkg-generic.mk: fix rule order for re{install, build, configure}

[Arnout Vandecappelle]

On 12/02/2023 10:42, Yann E. MORIN wrote:
> James, All,
> 
> Thanks again for working on such grunt work, it's much appreaciated!
> 
> On 2022-10-17 21:46 -0600, James Hilliard spake thusly:
>> These command rely on the clean operations being first so that the
>> stamp files being deleted will rebuild the targets.
>>
>> The execution ordering of the clean and rebuild 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.
> 
> I am not convinced that we should switch over to using double-colons.

  Me neither.


> As I reviewed your other related patch about legal-info [0], the make
> manual [1] suggests that double-colons are rally intended for cases
> where "the method used to update a target differs depending on which
> prerequisite files caused the update" and "double-colon rules really
> make sense are those where the order of executing the recipes would not
> matter".
> 
> So, we'd use a mechanism where order should not matter, to solve an
> issue with ordering. That'd be weird...
> 
> Yes, the manual states that "double-colon rules for a target are
> executed in the order they appear in the makefile", but that really
> looks like a side-effect of another goal.
> 
> Also, we'd need to carefully review all our Makefile to ensure that
> there are no other cases wehre ordering of prerequisites matter.

  Well, we need to do that regardless of which solution is found, right? Unless 
we just disable parallel builds and --shuffle entirely.

> We'd
> also have to be careful every time we introduce a new rule and remember
> to check whether it depends on the ordering prereauisites or not, adn
> worse yet, also whether we can afford the rule to be parallel-safe or
> not (see the explanation for that in the legal-info review).
> 
> Having colons and double-colons is going to be quite a headache... We
> already have issues with remembering that we need to use $$ to expand

  Yeah, maybe we should have some check-package thing to verify that $$ is used 
everywhere in a macro definition...

> variables in some places, and the rules there are relatively clean and
> explicit, but for the :: case, I'm afraid that's going to be a bit more
> complex.
> 
> We currently have a single location where we use double-colon:
>      support/kconfig/Makefile.br
> 
> which is added by our patch against kconfig:
>      support/kconfig/patches/10-br-build-system.patch
> 
> which dates back to 2010:
>      7c524dd0b683 Clean up our patches against kconfig
> 
> which got in three in 2007:
>      a665ed34960b - pull kconfig from linux-2.6.21.5
> 
> So, I'd like that we get another way to fix that, if possible...
> 
> Again, thanks a lot for spending efforts in that area, this is really
> great!
> 
> [0] https://lore.kernel.org/buildroot/20230212091914.GJ2796@scaer/
> [1] https://www.gnu.org/software/make/manual/make.html#Double_002dColon
> 
> Regards,
> Yann E. MORIN.
> 
>> Signed-off-by: James Hilliard <james.hilliard1@gmail.com>
>> ---
>>   package/pkg-generic.mk | 9 ++++++---
>>   1 file changed, 6 insertions(+), 3 deletions(-)
>>
>> diff --git a/package/pkg-generic.mk b/package/pkg-generic.mk
>> index f24e03a325..6cb461af90 100644
>> --- a/package/pkg-generic.mk
>> +++ b/package/pkg-generic.mk
>> @@ -1057,17 +1057,20 @@ endif
>>   			rm -f $$($(2)_TARGET_INSTALL_IMAGES)
>>   			rm -f $$($(2)_TARGET_INSTALL_HOST)
>>   
>> -$(1)-reinstall:		$(1)-clean-for-reinstall $(1)
>> +$(1)-reinstall::		$(1)-clean-for-reinstall
>> +$(1)-reinstall::		$(1)

  I may be stupid, but what we want is to have an ordering between 
clean-for-reinstall and $(1), right? So, let's use an ordering dependency?

$(1)-reinstall:		$(1)-clean-for-reinstall $(1)
$(1): 			$(1)-clean-for-reinstall


  Regards,
  Arnout

>>   
>>   $(1)-clean-for-rebuild: $(1)-clean-for-reinstall
>>   			rm -f $$($(2)_TARGET_BUILD)
>>   
>> -$(1)-rebuild:		$(1)-clean-for-rebuild $(1)
>> +$(1)-rebuild::		$(1)-clean-for-rebuild
>> +$(1)-rebuild::		$(1)
>>   
>>   $(1)-clean-for-reconfigure: $(1)-clean-for-rebuild
>>   			rm -f $$($(2)_TARGET_CONFIGURE)
>>   
>> -$(1)-reconfigure:	$(1)-clean-for-reconfigure $(1)
>> +$(1)-reconfigure::	$(1)-clean-for-reconfigure
>> +$(1)-reconfigure::	$(1)
>>   
>>   # define the PKG variable for all targets, containing the
>>   # uppercase package variable prefix
>> -- 
>> 2.34.1
>>
>> _______________________________________________
>> buildroot mailing list
>> buildroot@buildroot.org
>> https://lists.buildroot.org/mailman/listinfo/buildroot
> 
_______________________________________________
buildroot mailing list
buildroot@buildroot.org
https://lists.buildroot.org/mailman/listinfo/buildroot

Re: [Buildroot] [PATCH 1/1] package/pkg-generic.mk: fix rule order for re{install, build, configure}

[Arnout Vandecappelle]

On 14/02/2023 22:22, Arnout Vandecappelle wrote:
[1]
>  I may be stupid, but what we want is to have an ordering between 
> clean-for-reinstall and $(1), right? So, let's use an ordering dependency?
>
> $(1)-reinstall:        $(1)-clean-for-reinstall $(1)
> $(1):             $(1)-clean-for-reinstall


  Gah, I forgot the actual order-only bit...

$(1)-reinstall:   $(1)-clean-for-reinstall $(1)
$(1):           | $(1)-clean-for-reinstall


  Regards,
  Arnout


>
>
>  Regards,
>  Arnout
>
>>>     $(1)-clean-for-rebuild: $(1)-clean-for-reinstall
>>>               rm -f $$($(2)_TARGET_BUILD)
>>>   -$(1)-rebuild:        $(1)-clean-for-rebuild $(1)
>>> +$(1)-rebuild::        $(1)-clean-for-rebuild
>>> +$(1)-rebuild::        $(1)
>>>     $(1)-clean-for-reconfigure: $(1)-clean-for-rebuild
>>>               rm -f $$($(2)_TARGET_CONFIGURE)
>>>   -$(1)-reconfigure:    $(1)-clean-for-reconfigure $(1)
>>> +$(1)-reconfigure::    $(1)-clean-for-reconfigure
>>> +$(1)-reconfigure::    $(1)
>>>     # define the PKG variable for all targets, containing the
>>>   # uppercase package variable prefix
>>> -- 
>>> 2.34.1
>>>
>>> _______________________________________________
>>> buildroot mailing list
>>> buildroot@buildroot.org
>>> https://lists.buildroot.org/mailman/listinfo/buildroot
>>
_______________________________________________
buildroot mailing list
buildroot@buildroot.org
https://lists.buildroot.org/mailman/listinfo/buildroot

Re: [Buildroot] [PATCH 1/1] package/pkg-generic.mk: fix rule order for re{install, build, configure}

[James Hilliard]

On Tue, Feb 14, 2023 at 2:25 PM Arnout Vandecappelle <arnout@mind.be> wrote:
>
>
> On 14/02/2023 22:22, Arnout Vandecappelle wrote:
> [1]
> >  I may be stupid, but what we want is to have an ordering between
> > clean-for-reinstall and $(1), right? So, let's use an ordering dependency?
> >
> > $(1)-reinstall:        $(1)-clean-for-reinstall $(1)
> > $(1):             $(1)-clean-for-reinstall
>
>
>   Gah, I forgot the actual order-only bit...
>
> $(1)-reinstall:   $(1)-clean-for-reinstall $(1)
> $(1):           | $(1)-clean-for-reinstall

This doesn't seem to work, it ends up cleaning a bunch of additional targets
like the skeleton and toolchain that shouldn't be cleaned when doing a reinstall
for say a normal python package.

>
>
>   Regards,
>   Arnout
>
>
> >
> >
> >  Regards,
> >  Arnout
> >
> >>>     $(1)-clean-for-rebuild: $(1)-clean-for-reinstall
> >>>               rm -f $$($(2)_TARGET_BUILD)
> >>>   -$(1)-rebuild:        $(1)-clean-for-rebuild $(1)
> >>> +$(1)-rebuild::        $(1)-clean-for-rebuild
> >>> +$(1)-rebuild::        $(1)
> >>>     $(1)-clean-for-reconfigure: $(1)-clean-for-rebuild
> >>>               rm -f $$($(2)_TARGET_CONFIGURE)
> >>>   -$(1)-reconfigure:    $(1)-clean-for-reconfigure $(1)
> >>> +$(1)-reconfigure::    $(1)-clean-for-reconfigure
> >>> +$(1)-reconfigure::    $(1)
> >>>     # define the PKG variable for all targets, containing the
> >>>   # uppercase package variable prefix
> >>> --
> >>> 2.34.1
> >>>
> >>> _______________________________________________
> >>> buildroot mailing list
> >>> buildroot@buildroot.org
> >>> https://lists.buildroot.org/mailman/listinfo/buildroot
> >>
_______________________________________________
buildroot mailing list
buildroot@buildroot.org
https://lists.buildroot.org/mailman/listinfo/buildroot

Re: [Buildroot] [PATCH 1/1] package/pkg-generic.mk: fix rule order for re{install, build, configure}

[Arnout Vandecappelle via buildroot]

  Hi James,

On 18/10/2022 05:46, James Hilliard wrote:
> These command rely on the clean operations being first so that the
> stamp files being deleted will rebuild the targets.
> 
> The execution ordering of the clean and rebuild 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.

  As noted in earlier discussions, this feels a bit hackish and it's not clear 
if this behaviour is going to be kept in future versions of make.

  Since make 4.4, however, there's a better solution: it introduced a .WAIT 
phony target that enforces the ordering of dependencies. So I instead made a 
commit[1] that uses that approach.

  Regards,
  Arnout

[1] 
https://gitlab.com/buildroot.org/buildroot/-/commit/7e3105d5c8bd9ab31e728af9732a67d203c0a2e4

> 
> Signed-off-by: James Hilliard <james.hilliard1@gmail.com>
> ---
>   package/pkg-generic.mk | 9 ++++++---
>   1 file changed, 6 insertions(+), 3 deletions(-)
> 
> diff --git a/package/pkg-generic.mk b/package/pkg-generic.mk
> index f24e03a325..6cb461af90 100644
> --- a/package/pkg-generic.mk
> +++ b/package/pkg-generic.mk
> @@ -1057,17 +1057,20 @@ endif
>   			rm -f $$($(2)_TARGET_INSTALL_IMAGES)
>   			rm -f $$($(2)_TARGET_INSTALL_HOST)
>   
> -$(1)-reinstall:		$(1)-clean-for-reinstall $(1)
> +$(1)-reinstall::		$(1)-clean-for-reinstall
> +$(1)-reinstall::		$(1)
>   
>   $(1)-clean-for-rebuild: $(1)-clean-for-reinstall
>   			rm -f $$($(2)_TARGET_BUILD)
>   
> -$(1)-rebuild:		$(1)-clean-for-rebuild $(1)
> +$(1)-rebuild::		$(1)-clean-for-rebuild
> +$(1)-rebuild::		$(1)
>   
>   $(1)-clean-for-reconfigure: $(1)-clean-for-rebuild
>   			rm -f $$($(2)_TARGET_CONFIGURE)
>   
> -$(1)-reconfigure:	$(1)-clean-for-reconfigure $(1)
> +$(1)-reconfigure::	$(1)-clean-for-reconfigure
> +$(1)-reconfigure::	$(1)
>   
>   # define the PKG variable for all targets, containing the
>   # uppercase package variable prefix
_______________________________________________
buildroot mailing list
buildroot@buildroot.org
https://lists.buildroot.org/mailman/listinfo/buildroot

Re: [Buildroot] [PATCH 1/1] package/pkg-generic.mk: fix rule order for re{install, build, configure}

[Peter Korsgaard]

>>>>> "Arnout" == Arnout Vandecappelle via buildroot <buildroot@buildroot.org> writes:

 >  Hi James,
 > On 18/10/2022 05:46, James Hilliard wrote:
 >> These command rely on the clean operations being first so that the
 >> stamp files being deleted will rebuild the targets.
 >> The execution ordering of the clean and rebuild 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.

 >  As noted in earlier discussions, this feels a bit hackish and it's
 >  not clear if this behaviour is going to be kept in future versions of
 > make.

 >  Since make 4.4, however, there's a better solution: it introduced a
 >  .WAIT phony target that enforces the ordering of dependencies. So I
 > instead made a commit[1] that uses that approach.

Committed to 2023.02.x and 2023.08.x, thanks.

-- 
Bye, Peter Korsgaard
_______________________________________________
buildroot mailing list
buildroot@buildroot.org
https://lists.buildroot.org/mailman/listinfo/buildroot