Patchwork [v2,2/3] package: fix generic extract target for top-level parallel make

login
register
mail settings
Submitter Fabio Porcedda
Date July 18, 2013, 9:12 a.m.
Message ID <1374138746-23279-3-git-send-email-fabio.porcedda@gmail.com>
Download mbox | patch
Permalink /patch/260033/
State RFC
Headers show

Comments

Fabio Porcedda - July 18, 2013, 9:12 a.m.
To be able to use top-level parallel make we must don't depend in a rule
on the order of evaluation of the prerequisites, so instead of reling on
the left to right ordering of evaluation of the prerequisites add
an explicit rule to describe the dependencies.

Add a rule to specify that the $(2)_TARGET_EXTRACT target depends
on $(2)_TARGET_SOURCE target.

Signed-off-by: Fabio Porcedda <fabio.porcedda@gmail.com>
---
 package/pkg-generic.mk | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)
Arnout Vandecappelle - Aug. 21, 2013, 7:24 p.m.
On 18/07/13 11:12, Fabio Porcedda wrote:
> To be able to use top-level parallel make we must don't depend in a rule
> on the order of evaluation of the prerequisites, so instead of reling on
> the left to right ordering of evaluation of the prerequisites add
> an explicit rule to describe the dependencies.
>
> Add a rule to specify that the $(2)_TARGET_EXTRACT target depends
> on $(2)_TARGET_SOURCE target.
>
> Signed-off-by: Fabio Porcedda <fabio.porcedda@gmail.com>
> ---
>   package/pkg-generic.mk | 4 ++--
>   1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/package/pkg-generic.mk b/package/pkg-generic.mk
> index 668f011..f29ea99 100644
> --- a/package/pkg-generic.mk
> +++ b/package/pkg-generic.mk
> @@ -391,8 +391,8 @@ $(1)-configure:		$(1)-patch $(1)-depends \
>
>   $(1)-patch:		$(1)-extract $$($(2)_TARGET_PATCH)
>
> -$(1)-extract:		$(1)-source \
> -			$$($(2)_TARGET_EXTRACT)
> +$$($(2)_TARGET_EXTRACT):	$$($(2)_TARGET_SOURCE)
> +$(1)-extract:			$$($(2)_TARGET_EXTRACT)
>
>   $(1)-depends:		$$($(2)_DEPENDENCIES)
>
>

  On second observation, I don't really like this change, because it 
makes the extract and patch parts asymmetrical with the others. I would 
prefer one patch that changes it for all the targets. But then, the 
behaviour does change, because rebuilding one package will also trigger a 
rebuild of the packages that depend on it. Which may be a good thing, of 
course...

  Also, I think it would be nicer / clearer to put these dependencies in 
the %-rules at the top of the file, rather than specifying them per package.

  Regards,
  Arnout
Fabio Porcedda - Aug. 22, 2013, 7:44 a.m.
On Wed, Aug 21, 2013 at 9:24 PM, Arnout Vandecappelle <arnout@mind.be> wrote:
> On 18/07/13 11:12, Fabio Porcedda wrote:
>>
>> To be able to use top-level parallel make we must don't depend in a rule
>> on the order of evaluation of the prerequisites, so instead of reling on
>> the left to right ordering of evaluation of the prerequisites add
>> an explicit rule to describe the dependencies.
>>
>> Add a rule to specify that the $(2)_TARGET_EXTRACT target depends
>> on $(2)_TARGET_SOURCE target.
>>
>> Signed-off-by: Fabio Porcedda <fabio.porcedda@gmail.com>
>> ---
>>   package/pkg-generic.mk | 4 ++--
>>   1 file changed, 2 insertions(+), 2 deletions(-)
>>
>> diff --git a/package/pkg-generic.mk b/package/pkg-generic.mk
>> index 668f011..f29ea99 100644
>> --- a/package/pkg-generic.mk
>> +++ b/package/pkg-generic.mk
>> @@ -391,8 +391,8 @@ $(1)-configure:             $(1)-patch $(1)-depends \
>>
>>   $(1)-patch:           $(1)-extract $$($(2)_TARGET_PATCH)
>>
>> -$(1)-extract:          $(1)-source \
>> -                       $$($(2)_TARGET_EXTRACT)
>> +$$($(2)_TARGET_EXTRACT):       $$($(2)_TARGET_SOURCE)
>> +$(1)-extract:                  $$($(2)_TARGET_EXTRACT)
>>
>>   $(1)-depends:         $$($(2)_DEPENDENCIES)
>>
>>
>
>  On second observation, I don't really like this change, because it makes
> the extract and patch parts asymmetrical with the others. I would prefer one
> patch that changes it for all the targets. But then, the behaviour does
> change, because rebuilding one package will also trigger a rebuild of the
> packages that depend on it. Which may be a good thing, of course...

Do you mean a single patch that changes all the targets? IMHO the
patch becomes too complex, but if is the preferred way i'm fine with
that.

To be able to change the others targets i need to add stamp file for
every target inside $$($(2)_DEPENDENCIES,
i need to do that because a file cannot depends on a non existing file.
If there is any chance that such modification is going to be accepted
i restart to work on the second part.

>  Also, I think it would be nicer / clearer to put these dependencies in the
> %-rules at the top of the file, rather than specifying them per package.

Do you mean to put together all those rules between the %-rules
section and inner-generic-package function?
or to scatter them in the %-rules section?

Best regards
Arnout Vandecappelle - Aug. 22, 2013, 3:59 p.m.
On 22/08/13 09:44, Fabio Porcedda wrote:
> On Wed, Aug 21, 2013 at 9:24 PM, Arnout Vandecappelle <arnout@mind.be> wrote:
>> On 18/07/13 11:12, Fabio Porcedda wrote:
>>>
[snip]
>>>
>>>    $(1)-patch:           $(1)-extract $$($(2)_TARGET_PATCH)
>>>
>>> -$(1)-extract:          $(1)-source \
>>> -                       $$($(2)_TARGET_EXTRACT)
>>> +$$($(2)_TARGET_EXTRACT):       $$($(2)_TARGET_SOURCE)
>>> +$(1)-extract:                  $$($(2)_TARGET_EXTRACT)
>>>
>>>    $(1)-depends:         $$($(2)_DEPENDENCIES)
>>>
>>>
>>
>>   On second observation, I don't really like this change, because it makes
>> the extract and patch parts asymmetrical with the others. I would prefer one
>> patch that changes it for all the targets. But then, the behaviour does
>> change, because rebuilding one package will also trigger a rebuild of the
>> packages that depend on it. Which may be a good thing, of course...
>
> Do you mean a single patch that changes all the targets? IMHO the
> patch becomes too complex, but if is the preferred way i'm fine with
> that.

  Yes. I estimate it will modify about 50 lines, so I don't see a problem.

>
> To be able to change the others targets i need to add stamp file for
> every target inside $$($(2)_DEPENDENCIES,
> i need to do that because a file cannot depends on a non existing file.

  That's not true. Take the following Makefile:

----------------
%.source:
	touch $@

%.extract: %.source
	touch $@

%.config: %.extract
	touch $@

%.build: %.config
	touch $@

X_DEPS = y z

x.config: $(X_DEPS)
x: x.build

Y_DEPS = z

y.config: $(Y_DEPS)
y: y.build

z.config: $(Z_DEPS)
z: z.build

.PRECIOUS: %.source %.extract
----------------

  Type 'make x', and all the build, config, extract, source files will be 
touched in the right order.

  The .PRECIOUS line may not be needed in practice, I added it here 
because there are no explicit rules involving *.source and *.extract, 
therefore these files will be deleted after a successful build.


> If there is any chance that such modification is going to be accepted
> i restart to work on the second part.
>
>>   Also, I think it would be nicer / clearer to put these dependencies in the
>> %-rules at the top of the file, rather than specifying them per package.
>
> Do you mean to put together all those rules between the %-rules
> section and inner-generic-package function?
> or to scatter them in the %-rules section?

  No, I mean to do it like the example above: use the % rules to specify 
the dependencies between the stamp files, rather than an explicit rule 
for each package.

  Regards,
  Arnout


>
> Best regards
>
Thomas Petazzoni - Aug. 23, 2013, 7:36 a.m.
Dear Arnout Vandecappelle,

On Wed, 21 Aug 2013 21:24:19 +0200, Arnout Vandecappelle wrote:

>   On second observation, I don't really like this change, because it 
> makes the extract and patch parts asymmetrical with the others. I would 
> prefer one patch that changes it for all the targets. But then, the 
> behaviour does change, because rebuilding one package will also trigger a 
> rebuild of the packages that depend on it. Which may be a good thing, of 
> course...

When I did some experiments on top-level parallel build some years ago,
it is also one of the problem I had seen. Today, if you decide to
rebuild "libfoo", it rebuilds libfoo only and that's it. If libfoo is
used by something else, it's your responsibility to rebuild this
something else. Of course, this is all possible because the chain of
dependencies uses virtual targets and not stamp files.

If we use stamp files directly for the dependencies between the build
steps, then rebuilding a package will automatically rebuild all its
reverse dependencies. I believe this will be very very annoying for
people who just want to test a small change in a library, for example,
and make the whole OVERRIDE_SRCDIR thing used for development quite
useless.

Best regards,

Thomas
Fabio Porcedda - Aug. 23, 2013, 8 a.m.
On Fri, Aug 23, 2013 at 9:36 AM, Thomas Petazzoni
<thomas.petazzoni@free-electrons.com> wrote:
> Dear Arnout Vandecappelle,
>
> On Wed, 21 Aug 2013 21:24:19 +0200, Arnout Vandecappelle wrote:
>
>>   On second observation, I don't really like this change, because it
>> makes the extract and patch parts asymmetrical with the others. I would
>> prefer one patch that changes it for all the targets. But then, the
>> behaviour does change, because rebuilding one package will also trigger a
>> rebuild of the packages that depend on it. Which may be a good thing, of
>> course...
>
> When I did some experiments on top-level parallel build some years ago,
> it is also one of the problem I had seen. Today, if you decide to
> rebuild "libfoo", it rebuilds libfoo only and that's it. If libfoo is
> used by something else, it's your responsibility to rebuild this
> something else. Of course, this is all possible because the chain of
> dependencies uses virtual targets and not stamp files.

For this problem there is a simple solution, to use a
"order-only-prerequisites", because
a target is not rebuild if is older than a "order-only-prerequisites".

http://www.gnu.org/software/make/manual/html_node/Prerequisite-Types.html#Prerequisite-Types

Best regards
Thomas Petazzoni - Aug. 23, 2013, 11:14 a.m.
Dear Fabio Porcedda,

On Fri, 23 Aug 2013 10:00:33 +0200, Fabio Porcedda wrote:

> > When I did some experiments on top-level parallel build some years ago,
> > it is also one of the problem I had seen. Today, if you decide to
> > rebuild "libfoo", it rebuilds libfoo only and that's it. If libfoo is
> > used by something else, it's your responsibility to rebuild this
> > something else. Of course, this is all possible because the chain of
> > dependencies uses virtual targets and not stamp files.
> 
> For this problem there is a simple solution, to use a
> "order-only-prerequisites", because
> a target is not rebuild if is older than a "order-only-prerequisites".
> 
> http://www.gnu.org/software/make/manual/html_node/Prerequisite-Types.html#Prerequisite-Types

Ah, yes, indeed!

Thanks,

Thomas
Fabio Porcedda - Aug. 23, 2013, 11:31 a.m.
On Thu, Aug 22, 2013 at 5:59 PM, Arnout Vandecappelle <arnout@mind.be> wrote:
> On 22/08/13 09:44, Fabio Porcedda wrote:
>>
>> On Wed, Aug 21, 2013 at 9:24 PM, Arnout Vandecappelle <arnout@mind.be>
>> wrote:
>>>
>>> On 18/07/13 11:12, Fabio Porcedda wrote:
>>>>
>>>>
> [snip]
>
>>>>
>>>>    $(1)-patch:           $(1)-extract $$($(2)_TARGET_PATCH)
>>>>
>>>> -$(1)-extract:          $(1)-source \
>>>> -                       $$($(2)_TARGET_EXTRACT)
>>>> +$$($(2)_TARGET_EXTRACT):       $$($(2)_TARGET_SOURCE)
>>>> +$(1)-extract:                  $$($(2)_TARGET_EXTRACT)
>>>>
>>>>    $(1)-depends:         $$($(2)_DEPENDENCIES)
>>>>
>>>>
>>>
>>>   On second observation, I don't really like this change, because it
>>> makes
>>> the extract and patch parts asymmetrical with the others. I would prefer
>>> one
>>> patch that changes it for all the targets. But then, the behaviour does
>>> change, because rebuilding one package will also trigger a rebuild of the
>>> packages that depend on it. Which may be a good thing, of course...
>>
>>
>> Do you mean a single patch that changes all the targets? IMHO the
>> patch becomes too complex, but if is the preferred way i'm fine with
>> that.
>
>
>  Yes. I estimate it will modify about 50 lines, so I don't see a problem.
>
>
>>
>> To be able to change the others targets i need to add stamp file for
>> every target inside $$($(2)_DEPENDENCIES,
>> i need to do that because a file cannot depends on a non existing file.
>
>
>  That's not true. Take the following Makefile:
>
> ----------------
> %.source:
>         touch $@
>
> %.extract: %.source
>         touch $@
>
> %.config: %.extract
>         touch $@
>
> %.build: %.config
>         touch $@
>
> X_DEPS = y z
>
> x.config: $(X_DEPS)
> x: x.build
>
> Y_DEPS = z
>
> y.config: $(Y_DEPS)
> y: y.build
>
> z.config: $(Z_DEPS)
> z: z.build
>
> .PRECIOUS: %.source %.extract
> ----------------
>
>  Type 'make x', and all the build, config, extract, source files will be
> touched in the right order.
>
>  The .PRECIOUS line may not be needed in practice, I added it here because
> there are no explicit rules involving *.source and *.extract, therefore
> these files will be deleted after a successful build.
>
>
>
>> If there is any chance that such modification is going to be accepted
>> i restart to work on the second part.
>>
>>>   Also, I think it would be nicer / clearer to put these dependencies in
>>> the
>>> %-rules at the top of the file, rather than specifying them per package.
>>
>>
>> Do you mean to put together all those rules between the %-rules
>> section and inner-generic-package function?
>> or to scatter them in the %-rules section?
>
>
>  No, I mean to do it like the example above: use the % rules to specify the
> dependencies between the stamp files, rather than an explicit rule for each
> package.
>

Thanks for the suggestions, I'll look into it.

Regards
Fabio Porcedda - Aug. 26, 2013, 8:29 a.m.
On Thu, Aug 22, 2013 at 5:59 PM, Arnout Vandecappelle <arnout@mind.be> wrote:
> On 22/08/13 09:44, Fabio Porcedda wrote:
>>
>> On Wed, Aug 21, 2013 at 9:24 PM, Arnout Vandecappelle <arnout@mind.be>
>> wrote:
>>>
>>> On 18/07/13 11:12, Fabio Porcedda wrote:
>>>>
>>>>
> [snip]
>
>>>>
>>>>    $(1)-patch:           $(1)-extract $$($(2)_TARGET_PATCH)
>>>>
>>>> -$(1)-extract:          $(1)-source \
>>>> -                       $$($(2)_TARGET_EXTRACT)
>>>> +$$($(2)_TARGET_EXTRACT):       $$($(2)_TARGET_SOURCE)
>>>> +$(1)-extract:                  $$($(2)_TARGET_EXTRACT)
>>>>
>>>>    $(1)-depends:         $$($(2)_DEPENDENCIES)
>>>>
>>>>
>>>
>>>   On second observation, I don't really like this change, because it
>>> makes
>>> the extract and patch parts asymmetrical with the others. I would prefer
>>> one
>>> patch that changes it for all the targets. But then, the behaviour does
>>> change, because rebuilding one package will also trigger a rebuild of the
>>> packages that depend on it. Which may be a good thing, of course...
>>
>>
>> Do you mean a single patch that changes all the targets? IMHO the
>> patch becomes too complex, but if is the preferred way i'm fine with
>> that.
>
>
>  Yes. I estimate it will modify about 50 lines, so I don't see a problem.
>
>
>>
>> To be able to change the others targets i need to add stamp file for
>> every target inside $$($(2)_DEPENDENCIES,
>> i need to do that because a file cannot depends on a non existing file.
>
>
>  That's not true. Take the following Makefile:
>
> ----------------
> %.source:
>         touch $@
>
> %.extract: %.source
>         touch $@
>
> %.config: %.extract
>         touch $@
>
> %.build: %.config
>         touch $@
>
> X_DEPS = y z
>
> x.config: $(X_DEPS)

The problem with this solution is that the "x.config" target is
reevaluated every time because it does not depends on a virtual
target, the only solution that i found is to add a stamp file for
every dependencies.
Using virtual target as dependency is fine only with virtual targets.

Regards
Arnout Vandecappelle - Aug. 27, 2013, 6:01 a.m.
On 08/26/13 10:29, Fabio Porcedda wrote:
> On Thu, Aug 22, 2013 at 5:59 PM, Arnout Vandecappelle <arnout@mind.be> wrote:
>> On 22/08/13 09:44, Fabio Porcedda wrote:
[snip]
>>> To be able to change the others targets i need to add stamp file for
>>> every target inside $$($(2)_DEPENDENCIES,
>>> i need to do that because a file cannot depends on a non existing file.
>>
>>
>>   That's not true. Take the following Makefile:
>>
>> ----------------
>> %.source:
>>          touch $@
>>
>> %.extract: %.source
>>          touch $@
>>
>> %.config: %.extract
>>          touch $@
>>
>> %.build: %.config
>>          touch $@
>>
>> X_DEPS = y z
>>
>> x.config: $(X_DEPS)
>
> The problem with this solution is that the "x.config" target is
> reevaluated every time because it does not depends on a virtual
> target, the only solution that i found is to add a stamp file for
> every dependencies.
> Using virtual target as dependency is fine only with virtual targets.

  Ah yes, you're right.

  However, using the order-only rule that you mentioned in reply to 
Thomas solves the problem. So

x.config: | $(X_DEPS)


  Regards,
  Arnout
Fabio Porcedda - Aug. 28, 2013, 8:26 a.m.
On Tue, Aug 27, 2013 at 8:01 AM, Arnout Vandecappelle <arnout@mind.be> wrote:
> On 08/26/13 10:29, Fabio Porcedda wrote:
>>
>> On Thu, Aug 22, 2013 at 5:59 PM, Arnout Vandecappelle <arnout@mind.be>
>> wrote:
>>>
>>> On 22/08/13 09:44, Fabio Porcedda wrote:
>
> [snip]
>
>>>> To be able to change the others targets i need to add stamp file for
>>>> every target inside $$($(2)_DEPENDENCIES,
>>>> i need to do that because a file cannot depends on a non existing file.
>>>
>>>
>>>
>>>   That's not true. Take the following Makefile:
>>>
>>> ----------------
>>> %.source:
>>>          touch $@
>>>
>>> %.extract: %.source
>>>          touch $@
>>>
>>> %.config: %.extract
>>>          touch $@
>>>
>>> %.build: %.config
>>>          touch $@
>>>
>>> X_DEPS = y z
>>>
>>> x.config: $(X_DEPS)
>>
>>
>> The problem with this solution is that the "x.config" target is
>> reevaluated every time because it does not depends on a virtual
>> target, the only solution that i found is to add a stamp file for
>> every dependencies.
>> Using virtual target as dependency is fine only with virtual targets.
>
>
>  Ah yes, you're right.
>
>  However, using the order-only rule that you mentioned in reply to Thomas
> solves the problem. So
>
> x.config: | $(X_DEPS)

That's great, I didn't know that.
I'm working on the v3.

Regards
Fabio Porcedda

Patch

diff --git a/package/pkg-generic.mk b/package/pkg-generic.mk
index 668f011..f29ea99 100644
--- a/package/pkg-generic.mk
+++ b/package/pkg-generic.mk
@@ -391,8 +391,8 @@  $(1)-configure:		$(1)-patch $(1)-depends \
 
 $(1)-patch:		$(1)-extract $$($(2)_TARGET_PATCH)
 
-$(1)-extract:		$(1)-source \
-			$$($(2)_TARGET_EXTRACT)
+$$($(2)_TARGET_EXTRACT):	$$($(2)_TARGET_SOURCE)
+$(1)-extract:			$$($(2)_TARGET_EXTRACT)
 
 $(1)-depends:		$$($(2)_DEPENDENCIES)