| Message ID | 1413880974-30138-1-git-send-email-sojka@merica.cz |
|---|---|
| State | Accepted |
| Commit | 202bd70dfb7fbf2eb92bedb68e571426e32c65ae |
| Headers | show |
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@busybox.net > http://lists.busybox.net/mailman/listinfo/buildroot
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
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
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
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
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
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
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
>>>>> "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.
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)
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(+)