| Message ID | 1403778551-31435-1-git-send-email-angelo.compagnucci@gmail.com |
|---|---|
| State | Superseded |
| Headers | show |
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
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
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
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
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
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
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
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
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'
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(+)