[Buildroot] [PATCH] Complain loudly if BR2_GLOBAL_PATCH_DIR does not exit

[Buildroot] [PATCH] Complain loudly if BR2_GLOBAL_PATCH_DIR does not exit

[Michal Sojka]

If the BR2_GLOBAL_PATCH_DIR is misspelled in the .config or if somebody
renamed the existing directory with patches, buildroot happily builds
everything but without the patches. As this can lead to surprising
results, it is better to fail and give a user a message that
BR2_GLOBAL_PATCH_DIR is wrongly configured.

Signed-off-by: Michal Sojka <sojka@merica.cz>
---
 package/pkg-generic.mk | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/package/pkg-generic.mk b/package/pkg-generic.mk
index 259ee02..46d92aa 100644
--- a/package/pkg-generic.mk
+++ b/package/pkg-generic.mk
@@ -162,6 +162,11 @@ $(BUILD_DIR)/%/.stamp_patched:
 	$(Q)touch $@
 	@$(call step_end,patch)
 
+# Check that all directories specified in BR2_GLOBAL_PATCH_DIR exist.
+$(foreach dir,$(call qstrip,$(BR2_GLOBAL_PATCH_DIR)),\
+	$(if $(wildcard $(dir)),,\
+		$(error BR2_GLOBAL_PATCH_DIR contains nonexistent directory $(dir))))
+
 # Configure
 $(BUILD_DIR)/%/.stamp_configured:
 	@$(call step_start,configure)
-- 
2.1.1

[Buildroot] [PATCH] Complain loudly if BR2_GLOBAL_PATCH_DIR does not exit

[Yann E. MORIN]

Michal, All,

On 2014-10-21 10:42 +0200, Michal Sojka spake thusly:
> If the BR2_GLOBAL_PATCH_DIR is misspelled in the .config or if somebody
> renamed the existing directory with patches, buildroot happily builds
> everything but without the patches. As this can lead to surprising
> results,

Indeed... :-/

> it is better to fail and give a user a message that
> BR2_GLOBAL_PATCH_DIR is wrongly configured.
> 
> Signed-off-by: Michal Sojka <sojka@merica.cz>

Reviewed-by: "Yann E. MORIN" <yann.morin.1998@free.fr>

Regards,
Yann E. MORIN.

> ---
>  package/pkg-generic.mk | 5 +++++
>  1 file changed, 5 insertions(+)
> 
> diff --git a/package/pkg-generic.mk b/package/pkg-generic.mk
> index 259ee02..46d92aa 100644
> --- a/package/pkg-generic.mk
> +++ b/package/pkg-generic.mk
> @@ -162,6 +162,11 @@ $(BUILD_DIR)/%/.stamp_patched:
>  	$(Q)touch $@
>  	@$(call step_end,patch)
>  
> +# Check that all directories specified in BR2_GLOBAL_PATCH_DIR exist.
> +$(foreach dir,$(call qstrip,$(BR2_GLOBAL_PATCH_DIR)),\
> +	$(if $(wildcard $(dir)),,\
> +		$(error BR2_GLOBAL_PATCH_DIR contains nonexistent directory $(dir))))
> +
>  # Configure
>  $(BUILD_DIR)/%/.stamp_configured:
>  	@$(call step_start,configure)
> -- 
> 2.1.1
> 
> _______________________________________________
> buildroot mailing list
> buildroot at busybox.net
> http://lists.busybox.net/mailman/listinfo/buildroot

-- 
.-----------------.--------------------.------------------.--------------------.
|  Yann E. MORIN  | Real-Time Embedded | /"\ ASCII RIBBON | Erics' conspiracy: |
| +33 662 376 056 | Software  Designer | \ / CAMPAIGN     |  ___               |
| +33 223 225 172 `------------.-------:  X  AGAINST      |  \e/  There is no  |
| http://ymorin.is-a-geek.org/ | _/*\_ | / \ HTML MAIL    |   v   conspiracy.  |
'------------------------------^-------^------------------^--------------------'

[Buildroot] [PATCH] Complain loudly if BR2_GLOBAL_PATCH_DIR does not exit

[Maxime Hadjinlian]

Hi Michal, Yann, all

On Tue, Oct 21, 2014 at 6:58 PM, Yann E. MORIN <yann.morin.1998@free.fr> wrote:
> Michal, All,
>
> On 2014-10-21 10:42 +0200, Michal Sojka spake thusly:
>> If the BR2_GLOBAL_PATCH_DIR is misspelled in the .config or if somebody
>> renamed the existing directory with patches, buildroot happily builds
>> everything but without the patches. As this can lead to surprising
>> results,
>> ---
>>  package/pkg-generic.mk | 5 +++++
>>  1 file changed, 5 insertions(+)
>>
>> diff --git a/package/pkg-generic.mk b/package/pkg-generic.mk
>> index 259ee02..46d92aa 100644
>> --- a/package/pkg-generic.mk
>> +++ b/package/pkg-generic.mk
>> @@ -162,6 +162,11 @@ $(BUILD_DIR)/%/.stamp_patched:
>>       $(Q)touch $@
>>       @$(call step_end,patch)
>>
>> +# Check that all directories specified in BR2_GLOBAL_PATCH_DIR exist.
>> +$(foreach dir,$(call qstrip,$(BR2_GLOBAL_PATCH_DIR)),\
>> +     $(if $(wildcard $(dir)),,\
>> +             $(error BR2_GLOBAL_PATCH_DIR contains nonexistent directory $(dir))))
>> +
On a small note, since we already do a '[...] test -d $${D} [...]',
why don't you simply add an else condition with the error message ?
>>  # Configure
>>  $(BUILD_DIR)/%/.stamp_configured:
>>       @$(call step_start,configure)
>> --
>> 2.1.1

[Buildroot] [PATCH] Complain loudly if BR2_GLOBAL_PATCH_DIR does not exit

[Michal Sojka]

On Wed, Oct 22 2014, Maxime Hadjinlian wrote:
> Hi Michal, Yann, all
>
> On Tue, Oct 21, 2014 at 6:58 PM, Yann E. MORIN <yann.morin.1998@free.fr> wrote:
>> Michal, All,
>>
>> On 2014-10-21 10:42 +0200, Michal Sojka spake thusly:
>>> If the BR2_GLOBAL_PATCH_DIR is misspelled in the .config or if somebody
>>> renamed the existing directory with patches, buildroot happily builds
>>> everything but without the patches. As this can lead to surprising
>>> results,
>>> ---
>>>  package/pkg-generic.mk | 5 +++++
>>>  1 file changed, 5 insertions(+)
>>>
>>> diff --git a/package/pkg-generic.mk b/package/pkg-generic.mk
>>> index 259ee02..46d92aa 100644
>>> --- a/package/pkg-generic.mk
>>> +++ b/package/pkg-generic.mk
>>> @@ -162,6 +162,11 @@ $(BUILD_DIR)/%/.stamp_patched:
>>>       $(Q)touch $@
>>>       @$(call step_end,patch)
>>>
>>> +# Check that all directories specified in BR2_GLOBAL_PATCH_DIR exist.
>>> +$(foreach dir,$(call qstrip,$(BR2_GLOBAL_PATCH_DIR)),\
>>> +     $(if $(wildcard $(dir)),,\
>>> +             $(error BR2_GLOBAL_PATCH_DIR contains nonexistent directory $(dir))))
>>> +
> On a small note, since we already do a '[...] test -d $${D} [...]',
> why don't you simply add an else condition with the error message ?

Because $(D) is in fact $(BR2_GLOBAL_PATCH_DIR)/$(RAWNAME), which is not
required to exist for every package.

Best regards,
-Michal

[Buildroot] [PATCH] Complain loudly if BR2_GLOBAL_PATCH_DIR does not exit

[Maxime Hadjinlian]

On Wed, Oct 22, 2014 at 1:43 PM, Michal Sojka <sojka@merica.cz> wrote:
> On Wed, Oct 22 2014, Maxime Hadjinlian wrote:
>> Hi Michal, Yann, all
>>
>> On Tue, Oct 21, 2014 at 6:58 PM, Yann E. MORIN <yann.morin.1998@free.fr> wrote:
>>> Michal, All,
>>>
>>> On 2014-10-21 10:42 +0200, Michal Sojka spake thusly:
>>>> If the BR2_GLOBAL_PATCH_DIR is misspelled in the .config or if somebody
>>>> renamed the existing directory with patches, buildroot happily builds
>>>> everything but without the patches. As this can lead to surprising
>>>> results,
>>>> ---
>>>>  package/pkg-generic.mk | 5 +++++
>>>>  1 file changed, 5 insertions(+)
>>>>
>>>> diff --git a/package/pkg-generic.mk b/package/pkg-generic.mk
>>>> index 259ee02..46d92aa 100644
>>>> --- a/package/pkg-generic.mk
>>>> +++ b/package/pkg-generic.mk
>>>> @@ -162,6 +162,11 @@ $(BUILD_DIR)/%/.stamp_patched:
>>>>       $(Q)touch $@
>>>>       @$(call step_end,patch)
>>>>
>>>> +# Check that all directories specified in BR2_GLOBAL_PATCH_DIR exist.
>>>> +$(foreach dir,$(call qstrip,$(BR2_GLOBAL_PATCH_DIR)),\
>>>> +     $(if $(wildcard $(dir)),,\
>>>> +             $(error BR2_GLOBAL_PATCH_DIR contains nonexistent directory $(dir))))
>>>> +
>> On a small note, since we already do a '[...] test -d $${D} [...]',
>> why don't you simply add an else condition with the error message ?
>
> Because $(D) is in fact $(BR2_GLOBAL_PATCH_DIR)/$(RAWNAME), which is not
> required to exist for every package.
Ah yes indeed. Sorry.
Other question, why don't put that bit of code at the beginning of the
target stamp_patched ? Or at least in the target stamp_patched ?
>
> Best regards,
> -Michal

[Buildroot] [PATCH] Complain loudly if BR2_GLOBAL_PATCH_DIR does not exit

[Michal Sojka]

On Wed, Oct 22 2014, Maxime Hadjinlian wrote:
> On Wed, Oct 22, 2014 at 1:43 PM, Michal Sojka <sojka@merica.cz> wrote:
>> On Wed, Oct 22 2014, Maxime Hadjinlian wrote:
>>> Hi Michal, Yann, all
>>>
>>> On Tue, Oct 21, 2014 at 6:58 PM, Yann E. MORIN <yann.morin.1998@free.fr> wrote:
>>>> Michal, All,
>>>>
>>>> On 2014-10-21 10:42 +0200, Michal Sojka spake thusly:
>>>>> If the BR2_GLOBAL_PATCH_DIR is misspelled in the .config or if somebody
>>>>> renamed the existing directory with patches, buildroot happily builds
>>>>> everything but without the patches. As this can lead to surprising
>>>>> results,
>>>>> ---
>>>>>  package/pkg-generic.mk | 5 +++++
>>>>>  1 file changed, 5 insertions(+)
>>>>>
>>>>> diff --git a/package/pkg-generic.mk b/package/pkg-generic.mk
>>>>> index 259ee02..46d92aa 100644
>>>>> --- a/package/pkg-generic.mk
>>>>> +++ b/package/pkg-generic.mk
>>>>> @@ -162,6 +162,11 @@ $(BUILD_DIR)/%/.stamp_patched:
>>>>>       $(Q)touch $@
>>>>>       @$(call step_end,patch)
>>>>>
>>>>> +# Check that all directories specified in BR2_GLOBAL_PATCH_DIR exist.
>>>>> +$(foreach dir,$(call qstrip,$(BR2_GLOBAL_PATCH_DIR)),\
>>>>> +     $(if $(wildcard $(dir)),,\
>>>>> +             $(error BR2_GLOBAL_PATCH_DIR contains nonexistent directory $(dir))))
>>>>> +
>>> On a small note, since we already do a '[...] test -d $${D} [...]',
>>> why don't you simply add an else condition with the error message ?
>>
>> Because $(D) is in fact $(BR2_GLOBAL_PATCH_DIR)/$(RAWNAME), which is not
>> required to exist for every package.
> Ah yes indeed. Sorry.
> Other question, why don't put that bit of code at the beginning of the
> target stamp_patched ? Or at least in the target stamp_patched ?

Because the same check would be performed many times for every package.
This way, things are checked only once, while the Makefile is read.

-Michal

[Buildroot] [PATCH] Complain loudly if BR2_GLOBAL_PATCH_DIR does not exit

[Maxime Hadjinlian]

On Wed, Oct 22, 2014 at 4:01 PM, Michal Sojka <sojka@merica.cz> wrote:
> On Wed, Oct 22 2014, Maxime Hadjinlian wrote:
>> On Wed, Oct 22, 2014 at 1:43 PM, Michal Sojka <sojka@merica.cz> wrote:
>>> On Wed, Oct 22 2014, Maxime Hadjinlian wrote:
>>>> Hi Michal, Yann, all
>>>>
>>>> On Tue, Oct 21, 2014 at 6:58 PM, Yann E. MORIN <yann.morin.1998@free.fr> wrote:
>>>>> Michal, All,
>>>>>
>>>>> On 2014-10-21 10:42 +0200, Michal Sojka spake thusly:
>>>>>> If the BR2_GLOBAL_PATCH_DIR is misspelled in the .config or if somebody
>>>>>> renamed the existing directory with patches, buildroot happily builds
>>>>>> everything but without the patches. As this can lead to surprising
>>>>>> results,
>>>>>> ---
>>>>>>  package/pkg-generic.mk | 5 +++++
>>>>>>  1 file changed, 5 insertions(+)
>>>>>>
>>>>>> diff --git a/package/pkg-generic.mk b/package/pkg-generic.mk
>>>>>> index 259ee02..46d92aa 100644
>>>>>> --- a/package/pkg-generic.mk
>>>>>> +++ b/package/pkg-generic.mk
>>>>>> @@ -162,6 +162,11 @@ $(BUILD_DIR)/%/.stamp_patched:
>>>>>>       $(Q)touch $@
>>>>>>       @$(call step_end,patch)
>>>>>>
>>>>>> +# Check that all directories specified in BR2_GLOBAL_PATCH_DIR exist.
>>>>>> +$(foreach dir,$(call qstrip,$(BR2_GLOBAL_PATCH_DIR)),\
>>>>>> +     $(if $(wildcard $(dir)),,\
>>>>>> +             $(error BR2_GLOBAL_PATCH_DIR contains nonexistent directory $(dir))))
>>>>>> +
>>>> On a small note, since we already do a '[...] test -d $${D} [...]',
>>>> why don't you simply add an else condition with the error message ?
>>>
>>> Because $(D) is in fact $(BR2_GLOBAL_PATCH_DIR)/$(RAWNAME), which is not
>>> required to exist for every package.
>> Ah yes indeed. Sorry.
>> Other question, why don't put that bit of code at the beginning of the
>> target stamp_patched ? Or at least in the target stamp_patched ?
>
> Because the same check would be performed many times for every package.
> This way, things are checked only once, while the Makefile is read.
Well. As you may have seen, we already execute the stamp-patched
target for every package, we do some action, even call a script. So...
I don't think it would matter much to add this check for every
package. And IMHO, it would be better suited in that block of code.
But I don't know what would be preferred in this case.
>
> -Michal

[Buildroot] [PATCH] Complain loudly if BR2_GLOBAL_PATCH_DIR does not exit

[Arnout Vandecappelle]

On 22/10/14 13:54, Maxime Hadjinlian wrote:
> Other question, why don't put that bit of code at the beginning of the
> target stamp_patched ? Or at least in the target stamp_patched ?

 Because we prefer to have checks for the validity of the .config immediately,
before the build is started. Even a 'make savedefconfig' or 'make graph-depends'
will trigger the error. For this particular one it is not so important, but for
other checks it is really useful to have it before the build begins and not
after half an hour of building.

 Note that this is currently not always the case, e.g. dependencies.sh, external
toolchain validity. But if possible, I prefer .config checks to be done in make.


 Regards,
 Arnout


-- 
Arnout Vandecappelle                          arnout at mind be
Senior Embedded Software Architect            +32-16-286500
Essensium/Mind                                http://www.mind.be
G.Geenslaan 9, 3001 Leuven, Belgium           BE 872 984 063 RPR Leuven
LinkedIn profile: http://www.linkedin.com/in/arnoutvandecappelle
GPG fingerprint:  7CB5 E4CC 6C2E EFD4 6E3D A754 F963 ECAB 2450 2F1F

[Buildroot] [PATCH] Complain loudly if BR2_GLOBAL_PATCH_DIR does not exit

[Maxime Hadjinlian]

Hi Arnout, Michal, Yann, all,

On Wed, Oct 22, 2014 at 6:35 PM, Arnout Vandecappelle <arnout@mind.be> wrote:
> On 22/10/14 13:54, Maxime Hadjinlian wrote:
>> Other question, why don't put that bit of code at the beginning of the
>> target stamp_patched ? Or at least in the target stamp_patched ?
>
>  Because we prefer to have checks for the validity of the .config immediately,
> before the build is started. Even a 'make savedefconfig' or 'make graph-depends'
> will trigger the error. For this particular one it is not so important, but for
> other checks it is really useful to have it before the build begins and not
> after half an hour of building.
Understood. Thanks for clearing that up.
>
>  Note that this is currently not always the case, e.g. dependencies.sh, external
> toolchain validity. But if possible, I prefer .config checks to be done in make.
>
>
>  Regards,
>  Arnout
>
>
> --
> Arnout Vandecappelle                          arnout at mind be
> Senior Embedded Software Architect            +32-16-286500
> Essensium/Mind                                http://www.mind.be
> G.Geenslaan 9, 3001 Leuven, Belgium           BE 872 984 063 RPR Leuven
> LinkedIn profile: http://www.linkedin.com/in/arnoutvandecappelle
> GPG fingerprint:  7CB5 E4CC 6C2E EFD4 6E3D A754 F963 ECAB 2450 2F1F

[Buildroot] [PATCH] Complain loudly if BR2_GLOBAL_PATCH_DIR does not exit

[Peter Korsgaard]

>>>>> "Michal" == Michal Sojka <sojka@merica.cz> writes:

 > If the BR2_GLOBAL_PATCH_DIR is misspelled in the .config or if somebody
 > renamed the existing directory with patches, buildroot happily builds
 > everything but without the patches. As this can lead to surprising
 > results, it is better to fail and give a user a message that
 > BR2_GLOBAL_PATCH_DIR is wrongly configured.

 > Signed-off-by: Michal Sojka <sojka@merica.cz>

Committed, thanks.

-- 
Bye, Peter Korsgaard