[Buildroot] [PATCH] Add target-clean makefile target

[Buildroot] [PATCH] Add target-clean makefile target

[Angelo Compagnucci]

This makefile target wipes the target folder and forces buildroot into rebuild it.
It's useful when you have changed the list of packages and the target
tree remains out of sync keeping old installed packages no longer needed.

Signed-off-by: Angelo Compagnucci <angelo.compagnucci@gmail.com>
---
 Makefile | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/Makefile b/Makefile
index 14fca2b..83dceb7 100644
--- a/Makefile
+++ b/Makefile
@@ -835,6 +835,11 @@ clean:
 		$(BUILD_DIR) $(BASE_DIR)/staging \
 		$(LEGAL_INFO_DIR)
 
+target-clean:
+	rm -rf $(TARGET_DIR)
+	find $(BUILD_DIR) -name ".stamp_target_installed" -exec rm {} \;
+	rm $(BUILD_DIR)/.root
+
 distclean: clean
 ifeq ($(DL_DIR),$(TOPDIR)/dl)
 	rm -rf $(DL_DIR)
@@ -848,6 +853,7 @@ help:
 	@echo 'Cleaning:'
 	@echo '  clean                  - delete all files created by build'
 	@echo '  distclean              - delete all non-source files (including .config)'
+	@echo '  target-clean           - delete all target files and forces reinstall'
 	@echo
 	@echo 'Build:'
 	@echo '  all                    - make world'
-- 
2.0.0

[Buildroot] [PATCH] Add target-clean makefile target

[Thomas Petazzoni]

Dear Angelo Compagnucci,

On Thu, 26 Jun 2014 12:29:11 +0200, Angelo Compagnucci wrote:
> This makefile target wipes the target folder and forces buildroot into rebuild it.
> It's useful when you have changed the list of packages and the target
> tree remains out of sync keeping old installed packages no longer needed.
> 
> Signed-off-by: Angelo Compagnucci <angelo.compagnucci@gmail.com>
> ---
>  Makefile | 6 ++++++
>  1 file changed, 6 insertions(+)
> 
> diff --git a/Makefile b/Makefile
> index 14fca2b..83dceb7 100644
> --- a/Makefile
> +++ b/Makefile
> @@ -835,6 +835,11 @@ clean:
>  		$(BUILD_DIR) $(BASE_DIR)/staging \
>  		$(LEGAL_INFO_DIR)
>  
> +target-clean:
> +	rm -rf $(TARGET_DIR)
> +	find $(BUILD_DIR) -name ".stamp_target_installed" -exec rm {} \;
> +	rm $(BUILD_DIR)/.root
> +
>  distclean: clean
>  ifeq ($(DL_DIR),$(TOPDIR)/dl)
>  	rm -rf $(DL_DIR)
> @@ -848,6 +853,7 @@ help:
>  	@echo 'Cleaning:'
>  	@echo '  clean                  - delete all files created by build'
>  	@echo '  distclean              - delete all non-source files (including .config)'
> +	@echo '  target-clean           - delete all target files and forces reinstall'
>  	@echo
>  	@echo 'Build:'
>  	@echo '  all                    - make world'

Thanks for this patch. However, until now, we've always rejected
similar patches, because they are potentially dangerous for users.
Users might be lead to think that they can do some changes in
"menuconfig", then do "make target-clean all" and get the updated
rootfs without rebuilding everything. This is obviously completely
wrong if the configuration of some packages is changed, if some
libraries are added/removed from the build, etc.

Therefore, I'm tempted to also reject this patch, but I'll wait for
other Buildroot developers to give their opinion.

Thanks,

Thomas
-- 
Thomas Petazzoni, CTO, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com

[Buildroot] [PATCH] Add target-clean makefile target

[Angelo Compagnucci]

Dear Thomas,

2014-07-15 22:54 GMT+02:00 Thomas Petazzoni
<thomas.petazzoni@free-electrons.com>:
> Dear Angelo Compagnucci,
>
> On Thu, 26 Jun 2014 12:29:11 +0200, Angelo Compagnucci wrote:
>> This makefile target wipes the target folder and forces buildroot into rebuild it.
>> It's useful when you have changed the list of packages and the target
>> tree remains out of sync keeping old installed packages no longer needed.
>>
>> Signed-off-by: Angelo Compagnucci <angelo.compagnucci@gmail.com>
>> ---
>>  Makefile | 6 ++++++
>>  1 file changed, 6 insertions(+)
>>
>> diff --git a/Makefile b/Makefile
>> index 14fca2b..83dceb7 100644
>> --- a/Makefile
>> +++ b/Makefile
>> @@ -835,6 +835,11 @@ clean:
>>               $(BUILD_DIR) $(BASE_DIR)/staging \
>>               $(LEGAL_INFO_DIR)
>>
>> +target-clean:
>> +     rm -rf $(TARGET_DIR)
>> +     find $(BUILD_DIR) -name ".stamp_target_installed" -exec rm {} \;
>> +     rm $(BUILD_DIR)/.root
>> +
>>  distclean: clean
>>  ifeq ($(DL_DIR),$(TOPDIR)/dl)
>>       rm -rf $(DL_DIR)
>> @@ -848,6 +853,7 @@ help:
>>       @echo 'Cleaning:'
>>       @echo '  clean                  - delete all files created by build'
>>       @echo '  distclean              - delete all non-source files (including .config)'
>> +     @echo '  target-clean           - delete all target files and forces reinstall'
>>       @echo
>>       @echo 'Build:'
>>       @echo '  all                    - make world'
>
> Thanks for this patch. However, until now, we've always rejected
> similar patches, because they are potentially dangerous for users.
> Users might be lead to think that they can do some changes in
> "menuconfig", then do "make target-clean all" and get the updated
> rootfs without rebuilding everything. This is obviously completely
> wrong if the configuration of some packages is changed, if some
> libraries are added/removed from the build, etc.

I think it hurts so much to buildroot not having a clean way to
rebuild the rootfs after a changing. If this patch it's naive,
probably a corrected procedure should be documented somewhere.

Instead I think this patch is very useful (I use it everyday!), the
caveats should be documented in a proper place in the manual. Honestly
I haven't found a case in which cleaning and rebuilding rootfs this
way crashed my rootfs, but I admit that I'm not so confident with
buildroot internals.

It was only my honest attempt to be helpful!

>
> Therefore, I'm tempted to also reject this patch, but I'll wait for
> other Buildroot developers to give their opinion.
>
> Thanks,
>
> Thomas

Sincerly, Angelo
> --
> Thomas Petazzoni, CTO, Free Electrons
> Embedded Linux, Kernel and Android engineering
> http://free-electrons.com



-- 
Profile: http://it.linkedin.com/in/compagnucciangelo

[Buildroot] [PATCH] Add target-clean makefile target

[Thomas Petazzoni]

Dear Angelo Compagnucci,

On Tue, 15 Jul 2014 23:07:30 +0200, Angelo Compagnucci wrote:

> > Thanks for this patch. However, until now, we've always rejected
> > similar patches, because they are potentially dangerous for users.
> > Users might be lead to think that they can do some changes in
> > "menuconfig", then do "make target-clean all" and get the updated
> > rootfs without rebuilding everything. This is obviously completely
> > wrong if the configuration of some packages is changed, if some
> > libraries are added/removed from the build, etc.
> 
> I think it hurts so much to buildroot not having a clean way to
> rebuild the rootfs after a changing.

There is a clean way:

	make clean all

That's the only way that is 100% guaranteed to give the correct result.
Any other solution requires the user to have some deep understanding of
what (s)he is doing.

> If this patch it's  naive, probably a corrected procedure should be
> documented somewhere.
> 
> Instead I think this patch is very useful (I use it everyday!), the
> caveats should be documented in a proper place in the manual. Honestly
> I haven't found a case in which cleaning and rebuilding rootfs this
> way crashed my rootfs, but I admit that I'm not so confident with
> buildroot internals.

Scenario:

  1/ make menuconfig
  2/ Enable BR2_PACKAGE_GIT and BR2_PACKAGE_OPENSSL
  3/ Run make, use your rootfs, you're happy
  4/ make menuconfig
  5/ Disable BR2_PACKAGE_OPENSSL
  6/ Since you don't want to rebuild everything, you just run your new
     "make target-clean" thing.
  7/ Run your rootfs, and now Git fails to work, because it is linked
     against OpenSSL, but OpenSSL isn't installed in the rootfs.

(7) is due to the fact that Git was not rebuilt, so it still believes
that OpenSSL support is available. The scenario above is fairly simple,
but there are many, many more similar but more subtle scenarios to
screw things up.

Best regards,

Thomas
-- 
Thomas Petazzoni, CTO, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com

[Buildroot] [PATCH] Add target-clean makefile target

[Angelo Compagnucci]

Hi Thomas,

2014-07-15 23:33 GMT+02:00 Thomas Petazzoni
<thomas.petazzoni@free-electrons.com>:
> Dear Angelo Compagnucci,
>
> On Tue, 15 Jul 2014 23:07:30 +0200, Angelo Compagnucci wrote:
>
>> > Thanks for this patch. However, until now, we've always rejected
>> > similar patches, because they are potentially dangerous for users.
>> > Users might be lead to think that they can do some changes in
>> > "menuconfig", then do "make target-clean all" and get the updated
>> > rootfs without rebuilding everything. This is obviously completely
>> > wrong if the configuration of some packages is changed, if some
>> > libraries are added/removed from the build, etc.
>>
>> I think it hurts so much to buildroot not having a clean way to
>> rebuild the rootfs after a changing.
>
> There is a clean way:
>
>         make clean all
>
> That's the only way that is 100% guaranteed to give the correct result.
> Any other solution requires the user to have some deep understanding of
> what (s)he is doing.
>
>> If this patch it's  naive, probably a corrected procedure should be
>> documented somewhere.
>>
>> Instead I think this patch is very useful (I use it everyday!), the
>> caveats should be documented in a proper place in the manual. Honestly
>> I haven't found a case in which cleaning and rebuilding rootfs this
>> way crashed my rootfs, but I admit that I'm not so confident with
>> buildroot internals.
>
> Scenario:
>
>   1/ make menuconfig
>   2/ Enable BR2_PACKAGE_GIT and BR2_PACKAGE_OPENSSL
>   3/ Run make, use your rootfs, you're happy
>   4/ make menuconfig
>   5/ Disable BR2_PACKAGE_OPENSSL
>   6/ Since you don't want to rebuild everything, you just run your new
>      "make target-clean" thing.
>   7/ Run your rootfs, and now Git fails to work, because it is linked
>      against OpenSSL, but OpenSSL isn't installed in the rootfs.
>
> (7) is due to the fact that Git was not rebuilt, so it still believes
> that OpenSSL support is available. The scenario above is fairly simple,
> but there are many, many more similar but more subtle scenarios to
> screw things up.

Good catch, but this could be documented somewhere. I think that is
better to explain buildroot's users that they have to rebuild a
package when they mess it's dependencies instead of all the whole
rootfs! Compilation of a rootfs can take hours ...
Yes, I know, probably removing Openssl screws up a hundred of packages
and it's not practical to rebuild one by one, but I think this is a
corner case more than the rule.

And in the end, if their rootfs is not working, they can obviously
rely on the good old cleaning way.

Sincerly, Angelo.

>
> Best regards,
>
> Thomas
> --
> Thomas Petazzoni, CTO, Free Electrons
> Embedded Linux, Kernel and Android engineering
> http://free-electrons.com



-- 
Profile: http://it.linkedin.com/in/compagnucciangelo

[Buildroot] [PATCH] Add target-clean makefile target

[Thomas Petazzoni]

Dear Angelo Compagnucci,

On Wed, 16 Jul 2014 09:47:01 +0200, Angelo Compagnucci wrote:

> > (7) is due to the fact that Git was not rebuilt, so it still believes
> > that OpenSSL support is available. The scenario above is fairly simple,
> > but there are many, many more similar but more subtle scenarios to
> > screw things up.
> 
> Good catch, but this could be documented somewhere.

Right, but then a patch adding "make target-clean" should also be
responsible for adding the appropriate documentation :-)

> I think that is better to explain buildroot's users that they have to
> rebuild a package when they mess it's dependencies instead of all the
> whole rootfs! Compilation of a rootfs can take hours ...
> Yes, I know, probably removing Openssl screws up a hundred of packages
> and it's not practical to rebuild one by one, but I think this is a
> corner case more than the rule.

Not that much: any optional dependency in Buildroot will exhibit
exactly the same behavior, and there are hundreds if not thousands of
places were we rely on optional dependencies.

By I tend to agree that we could provide the tool, provided that there
is sufficient documentation to explain how to use it correctly.

Best regards,

Thomas
-- 
Thomas Petazzoni, CTO, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com

[Buildroot] [PATCH] Add target-clean makefile target

[Angelo Compagnucci]

Dear Thomas,

2014-07-16 10:09 GMT+02:00 Thomas Petazzoni
<thomas.petazzoni@free-electrons.com>:
> Dear Angelo Compagnucci,
>
> On Wed, 16 Jul 2014 09:47:01 +0200, Angelo Compagnucci wrote:
>
>> > (7) is due to the fact that Git was not rebuilt, so it still believes
>> > that OpenSSL support is available. The scenario above is fairly simple,
>> > but there are many, many more similar but more subtle scenarios to
>> > screw things up.
>>
>> Good catch, but this could be documented somewhere.
>
> Right, but then a patch adding "make target-clean" should also be
> responsible for adding the appropriate documentation :-)

If there is hope to have target-clean accepted, then I'll be working
on the documentation as soon as possible!

>> I think that is better to explain buildroot's users that they have to
>> rebuild a package when they mess it's dependencies instead of all the
>> whole rootfs! Compilation of a rootfs can take hours ...
>> Yes, I know, probably removing Openssl screws up a hundred of packages
>> and it's not practical to rebuild one by one, but I think this is a
>> corner case more than the rule.
>
> Not that much: any optional dependency in Buildroot will exhibit
> exactly the same behavior, and there are hundreds if not thousands of
> places were we rely on optional dependencies.

Yes, I can understand the implications. Honestly, using buildroot
naively, I never encountered such behaviors, but I was not going
deeply into dependencies, only selecting and deselecting packages here
and there.
Buildroot served me well!

> By I tend to agree that we could provide the tool, provided that there
> is sufficient documentation to explain how to use it correctly.

Yes, it's exactly what I'm looking for.

>
> Best regards,
>
> Thomas
> --
> Thomas Petazzoni, CTO, Free Electrons
> Embedded Linux, Kernel and Android engineering
> http://free-electrons.com



-- 
Profile: http://it.linkedin.com/in/compagnucciangelo

[Buildroot] [PATCH] Add target-clean makefile target

[Arnout Vandecappelle]

On 16/07/14 10:29, Angelo Compagnucci wrote:
> Dear Thomas,
> 
> 2014-07-16 10:09 GMT+02:00 Thomas Petazzoni
> <thomas.petazzoni@free-electrons.com>:
>> Dear Angelo Compagnucci,
>>
>> On Wed, 16 Jul 2014 09:47:01 +0200, Angelo Compagnucci wrote:
>>
>>>> (7) is due to the fact that Git was not rebuilt, so it still believes
>>>> that OpenSSL support is available. The scenario above is fairly simple,
>>>> but there are many, many more similar but more subtle scenarios to
>>>> screw things up.
>>>
>>> Good catch, but this could be documented somewhere.
>>
>> Right, but then a patch adding "make target-clean" should also be
>> responsible for adding the appropriate documentation :-)
> 
> If there is hope to have target-clean accepted, then I'll be working
> on the documentation as soon as possible!

 I'd suggest to print a big fat warning at the end of target-clean. If it's
buried somewhere in the manual, chances are it will be missed.

 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] Add target-clean makefile target

[Thomas Petazzoni]

Dear Angelo Compagnucci,

On Thu, 26 Jun 2014 12:29:11 +0200, Angelo Compagnucci wrote:
> This makefile target wipes the target folder and forces buildroot into rebuild it.
> It's useful when you have changed the list of packages and the target
> tree remains out of sync keeping old installed packages no longer needed.
> 
> Signed-off-by: Angelo Compagnucci <angelo.compagnucci@gmail.com>
> ---
>  Makefile | 6 ++++++
>  1 file changed, 6 insertions(+)

Following the discussion, you received several suggestions to improve
the patch in order to make it acceptable. Therefore, I'll mark your
patch as "Changes Requested" in our patch tracking system, and we'll
wait for you to submit a new iteration that takes into account the
comments that were made during the discussion.

Thanks a lot!

Thomas
-- 
Thomas Petazzoni, CTO, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com