| Message ID | 20210621141130.48654-4-herve.codina@bootlin.com |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | Overwritten file detection and fixes, one more step to TLP build | expand |
On Mon, 21 Jun 2021 16:11:18 +0200 Herve Codina <herve.codina@bootlin.com> wrote: > Some packages (autotools for instance) install documentation > files using install-info. This program adds an entry in > the Info directory file (share/info/dir) and this causes > TARGET_DIR and/or HOST_DIR overwrite. > > In order to avoid this overwrite this patch removes the Info > directory file right after any installation. > > Signed-off-by: Herve Codina <herve.codina@bootlin.com> > --- > package/pkg-generic.mk | 3 +++ > 1 file changed, 3 insertions(+) > > diff --git a/package/pkg-generic.mk b/package/pkg-generic.mk > index bb9ff4150a..2499c94746 100644 > --- a/package/pkg-generic.mk > +++ b/package/pkg-generic.mk > @@ -280,6 +280,7 @@ $(BUILD_DIR)/%/.stamp_host_installed: > $(foreach hook,$($(PKG)_PRE_INSTALL_HOOKS),$(call $(hook))$(sep)) > +$($(PKG)_INSTALL_CMDS) > $(foreach hook,$($(PKG)_POST_INSTALL_HOOKS),$(call $(hook))$(sep)) > + $(Q) rm -f $(HOST_DIR)/share/info/dir No space between $(Q) and rm. This should perhaps use $(RM) in fact. However, I'm not a huge fan of having this right in the middle of the infrastructure. It feels like a small detail that gets handled in the middle of super generic infrastructure code. The issue is that I don't really have a good alternative proposal :-/ Instead of removing that file, ignore it in the overwrite detection, perhaps? Thomas
Hi, On Mon, 21 Jun 2021 22:51:20 +0200 Thomas Petazzoni <thomas.petazzoni@bootlin.com> wrote: > On Mon, 21 Jun 2021 16:11:18 +0200 > Herve Codina <herve.codina@bootlin.com> wrote: > > > Some packages (autotools for instance) install documentation > > files using install-info. This program adds an entry in > > the Info directory file (share/info/dir) and this causes > > TARGET_DIR and/or HOST_DIR overwrite. > > > > In order to avoid this overwrite this patch removes the Info > > directory file right after any installation. > > > > Signed-off-by: Herve Codina <herve.codina@bootlin.com> > > --- > > package/pkg-generic.mk | 3 +++ > > 1 file changed, 3 insertions(+) > > > > diff --git a/package/pkg-generic.mk b/package/pkg-generic.mk > > index bb9ff4150a..2499c94746 100644 > > --- a/package/pkg-generic.mk > > +++ b/package/pkg-generic.mk > > @@ -280,6 +280,7 @@ $(BUILD_DIR)/%/.stamp_host_installed: > > $(foreach hook,$($(PKG)_PRE_INSTALL_HOOKS),$(call $(hook))$(sep)) > > +$($(PKG)_INSTALL_CMDS) > > $(foreach hook,$($(PKG)_POST_INSTALL_HOOKS),$(call $(hook))$(sep)) > > + $(Q) rm -f $(HOST_DIR)/share/info/dir > > No space between $(Q) and rm. This should perhaps use $(RM) in fact. Space removed > > However, I'm not a huge fan of having this right in the middle of the > infrastructure. It feels like a small detail that gets handled in the > middle of super generic infrastructure code. > > The issue is that I don't really have a good alternative proposal :-/ Maybe using a macro defined closed to fixup-libtool-files and calling this macro here instead of '$(Q)rm ...' will help. Do you think it will be better ? > > Instead of removing that file, ignore it in the overwrite detection, > perhaps? This add a little complexity in overwrite detection (filter out) and I prefer having overwrite detection quite stupid. It checks for overwrites without any exception. Adding exception now in the detection mechanism is opening the door to more and more exceptions. Hervé
On Tue, 22 Jun 2021 10:43:43 +0200 Herve Codina <herve.codina@bootlin.com> wrote: > > However, I'm not a huge fan of having this right in the middle of the > > infrastructure. It feels like a small detail that gets handled in the > > middle of super generic infrastructure code. > > > > The issue is that I don't really have a good alternative proposal :-/ > > Maybe using a macro defined closed to fixup-libtool-files and calling > this macro here instead of '$(Q)rm ...' will help. > Do you think it will be better ? Either a macro, or a list of files that are removed, perhaps? > This add a little complexity in overwrite detection (filter out) and > I prefer having overwrite detection quite stupid. It checks for > overwrites without any exception. > Adding exception now in the detection mechanism is opening the door to > more and more exceptions. Yes, I understand your argument. As I stated: I'm also not sure which solution to propose here. I was just not a big fan of this removal of one specific file, there, in the middle of a highly generic piece of infrastructure. Thomas
Thomas, Hervé, All, On 2021-06-22 11:34 +0200, Thomas Petazzoni spake thusly: > On Tue, 22 Jun 2021 10:43:43 +0200 > Herve Codina <herve.codina@bootlin.com> wrote: > > > However, I'm not a huge fan of having this right in the middle of the > > > infrastructure. It feels like a small detail that gets handled in the > > > middle of super generic infrastructure code. > > > > > > The issue is that I don't really have a good alternative proposal :-/ I too am not too fond of this, but I don't have an obvious better solution in mind... > > Maybe using a macro defined closed to fixup-libtool-files and calling > > this macro here instead of '$(Q)rm ...' will help. > > Do you think it will be better ? > > Either a macro, or a list of files that are removed, perhaps? I think a list+macro would be a better solution. Alternatively, we could append to post-install hooks, like we do for pre-configure hooks in the autotools infra for autoreconf et al., for example... # Outside generic-package-inner: # $1: base directory (target, staging, host) define remove-conflicting-useless-files $(Q)$(RM) -rf $(patsubst %, $(1)/%, $($(PKG)_DROP_FILES_OR_DIRS)) endef define REMOVE_CONFLICTING_USELESS_FILES_IN_HOST $(call remove-conflicting-useless-files,$(HOST_DIR)) endef define REMOVE_CONFLICTING_USELESS_FILES_IN_STAGING $(call remove-conflicting-useless-files,$(STAGING_DIR)) endef define REMOVE_CONFLICTING_USELESS_FILES_IN_TARGET $(call remove-conflicting-useless-files,$(TARGET_DIR)) endef # In generic-package-inner: $(2)_DROP_FILES_OR_DIRS += /share/info/dir # For host packages: $(2)_POST_INSTALL_HOOKS += REMOVE_CONFLICTING_USELESS_FILES_IN_HOST # For target packages: $(2)_POST_INSTALL_STAGING_HOOKS += REMOVE_CONFLICTING_USELESS_FILES_IN_STAGING $(2)_POST_INSTALL_TARGET_HOOKS += REMOVE_CONFLICTING_USELESS_FILES_IN_TARGET This way, it also paves the way to add other hooks to actually fix some files instead of removing them... For example, assume that packages (e.g. 'wonders') can register themselves against another package (e.g. 'manager') by appending a line to a text file, like so (over-simplified, of course): echo "name=wonders path=/usr/lib/wonders/v42" >>/usr/share/manager/registry Then we could have a hook that detects that, extracts the delta installed by the package, stash that somewhere in staging, and have a target-finalise hook that aggregates all the packages in the end. > > This add a little complexity in overwrite detection (filter out) and > > I prefer having overwrite detection quite stupid. It checks for > > overwrites without any exception. > > Adding exception now in the detection mechanism is opening the door to > > more and more exceptions. > > Yes, I understand your argument. As I stated: I'm also not sure which > solution to propose here. I was just not a big fan of this removal of > one specific file, there, in the middle of a highly generic piece of > infrastructure. Yes, this bothered me too... Regards, Yann E. MORIN. > Thomas > -- > Thomas Petazzoni, co-owner and CEO, Bootlin > Embedded Linux and Kernel engineering > https://bootlin.com > _______________________________________________ > buildroot mailing list > buildroot@busybox.net > http://lists.busybox.net/mailman/listinfo/buildroot
Hi, On Tue, 22 Jun 2021 22:18:18 +0200 "Yann E. MORIN" <yann.morin.1998@free.fr> wrote: > I think a list+macro would be a better solution. > > Alternatively, we could append to post-install hooks, like we do for > pre-configure hooks in the autotools infra for autoreconf et al., for > example... > > # Outside generic-package-inner: > > # $1: base directory (target, staging, host) > define remove-conflicting-useless-files > $(Q)$(RM) -rf $(patsubst %, $(1)/%, $($(PKG)_DROP_FILES_OR_DIRS)) > endef > define REMOVE_CONFLICTING_USELESS_FILES_IN_HOST > $(call remove-conflicting-useless-files,$(HOST_DIR)) > endef > define REMOVE_CONFLICTING_USELESS_FILES_IN_STAGING > $(call remove-conflicting-useless-files,$(STAGING_DIR)) > endef > define REMOVE_CONFLICTING_USELESS_FILES_IN_TARGET > $(call remove-conflicting-useless-files,$(TARGET_DIR)) > endef > > > # In generic-package-inner: > > $(2)_DROP_FILES_OR_DIRS += /share/info/dir > > # For host packages: > $(2)_POST_INSTALL_HOOKS += REMOVE_CONFLICTING_USELESS_FILES_IN_HOST > > # For target packages: > $(2)_POST_INSTALL_STAGING_HOOKS += REMOVE_CONFLICTING_USELESS_FILES_IN_STAGING > $(2)_POST_INSTALL_TARGET_HOOKS += REMOVE_CONFLICTING_USELESS_FILES_IN_TARGET > > This way, it also paves the way to add other hooks to actually fix some > files instead of removing them... > > For example, assume that packages (e.g. 'wonders') can register > themselves against another package (e.g. 'manager') by appending a > line to a text file, like so (over-simplified, of course): > > echo "name=wonders path=/usr/lib/wonders/v42" >>/usr/share/manager/registry > > Then we could have a hook that detects that, extracts the delta > installed by the package, stash that somewhere in staging, and have a > target-finalise hook that aggregates all the packages in the end. > I like this solution. If it is okay for everyone, I will rework my patch accordingly. Thomas, your opinion ? Regards, Hervé
diff --git a/package/pkg-generic.mk b/package/pkg-generic.mk index bb9ff4150a..2499c94746 100644 --- a/package/pkg-generic.mk +++ b/package/pkg-generic.mk @@ -280,6 +280,7 @@ $(BUILD_DIR)/%/.stamp_host_installed: $(foreach hook,$($(PKG)_PRE_INSTALL_HOOKS),$(call $(hook))$(sep)) +$($(PKG)_INSTALL_CMDS) $(foreach hook,$($(PKG)_POST_INSTALL_HOOKS),$(call $(hook))$(sep)) + $(Q) rm -f $(HOST_DIR)/share/info/dir @$(call step_end,install-host) $(Q)touch $@ @@ -309,6 +310,7 @@ $(BUILD_DIR)/%/.stamp_staging_installed: $(foreach hook,$($(PKG)_PRE_INSTALL_STAGING_HOOKS),$(call $(hook))$(sep)) +$($(PKG)_INSTALL_STAGING_CMDS) $(foreach hook,$($(PKG)_POST_INSTALL_STAGING_HOOKS),$(call $(hook))$(sep)) + $(Q) rm -f $(STAGING_DIR)/share/info/dir $(Q)if test -n "$($(PKG)_CONFIG_SCRIPTS)" ; then \ $(call MESSAGE,"Fixing package configuration files") ;\ $(SED) "s,$(HOST_DIR),@HOST_DIR@,g" \ @@ -368,6 +370,7 @@ $(BUILD_DIR)/%/.stamp_target_installed: $(or $($(PKG)_INSTALL_INIT_OPENRC), \ $($(PKG)_INSTALL_INIT_SYSV))) $(foreach hook,$($(PKG)_POST_INSTALL_TARGET_HOOKS),$(call $(hook))$(sep)) + $(Q) rm -f $(TARGET_DIR)/share/info/dir $(Q)if test -n "$($(PKG)_CONFIG_SCRIPTS)" ; then \ $(RM) -f $(addprefix $(TARGET_DIR)/usr/bin/,$($(PKG)_CONFIG_SCRIPTS)) ; \ fi
Some packages (autotools for instance) install documentation files using install-info. This program adds an entry in the Info directory file (share/info/dir) and this causes TARGET_DIR and/or HOST_DIR overwrite. In order to avoid this overwrite this patch removes the Info directory file right after any installation. Signed-off-by: Herve Codina <herve.codina@bootlin.com> --- package/pkg-generic.mk | 3 +++ 1 file changed, 3 insertions(+)