| Message ID | 20200430095249.782597-6-thomas.petazzoni@bootlin.com |
|---|---|
| State | Accepted |
| Headers | show |
| Series | Overwritten file detection, improvements to file listing logic | expand |
On 2020-04-30 11:52 +0200, Thomas Petazzoni spake thusly: > This commits reworks the pkg_size logic to no longer use the > GLOBAL_INSTRUMENTATION_HOOKS mechanism, but instead be directly > implemented within the configure step and install step. > > The problem with the current implementation in the > GLOBAL_INSTRUMENTATION_HOOKS is that we only capture what is installed > in $(HOST_DIR) during the "host installation step", what is installed > in $(TARGET_DIR) during the "target installation step" and what is > installed in "$(STAGING_DIR)" during the staging installation step. > > While this seems reasonable in principle, it is in fact not completely > true. For example, "toolchain", which is a target package, installs > tons of files in $(HOST_DIR). "qt5base", which is also a target > package, also installs things in $(HOST_DIR). Same with the "syslinux" > package. > > The idea behind this patch is pretty simple: > > - At the beginning of the configure step, right after the per-package > directories have been populated (if BR2_PER_PACKAGE_DIRECTORIES=y), > we capture the state of the HOST_DIR, TARGET_DIR and STAGING_DIR. > > - At the end of all install steps (which is possible thanks to the > newly introduced "install" step), we capture again the state of > HOST_DIR, TARGET_DIR and STAGING_DIR, and compare it to what we > have saved at the configure step. I don't understand wy tht can't be achieved in the instrumentation hooks mechanism: $(if $(filter start-configure,$(1)-$(2)),\ $(call step_pkg_size_before,$(TARGET_DIR))\ $(call step_pkg_size_before,$(STAGING_DIR),-staging)\ $(call step_pkg_size_before,$(HOST_DIR),-host)) $(if $(filter end-install,$(1)-$(2)),\ $(call step_pkg_size_after,$(TARGET_DIR))\ $(call step_pkg_size_after,$(STAGING_DIR),-staging)\ $(call step_pkg_size_after,$(HOST_DIR),-host)) Of course, it means you have to add a call the instrumentation hooks from the new 'install' step. > So regardless of whether a file has been installed in $(HOST_DIR) > during the target or staging installation steps of a target package, > or if a host package has installed a file in $(TARGET_DIR), we will > detect it. > > The pkg_size_before and pkg_size_after macros are intentionally left > where they are (even if they now fall in the middle of the > GLOBAL_INSTRUMENTATION_HOOKS implementations) to minimize the diffstat > and facilitate review. > > Note that we also have to change check_bin_arch to be explicitly > called from the install step rather than through a > GLOBAL_INSTRUMENTATION_HOOKS as it depends on the .files-list.txt file > produced by the pkg_size_after function. And that would solve this as well. Regards, Yann E. MORIN. > Signed-off-by: Thomas Petazzoni <thomas.petazzoni@bootlin.com> > --- > package/pkg-generic.mk | 42 ++++++++++++++---------------------------- > 1 file changed, 14 insertions(+), 28 deletions(-) > > diff --git a/package/pkg-generic.mk b/package/pkg-generic.mk > index efdc32e355..3fb1e5f8c3 100644 > --- a/package/pkg-generic.mk > +++ b/package/pkg-generic.mk > @@ -59,7 +59,7 @@ GLOBAL_INSTRUMENTATION_HOOKS += step_time > > # $(1): base directory to search in > # $(2): suffix of file (optional) > -define step_pkg_size_before > +define pkg_size_before > cd $(1); \ > LC_ALL=C find . \( -type f -o -type l \) -printf '%T@:%i:%#m:%y:%s,%p\n' \ > | LC_ALL=C sort > $($(PKG)_DIR)/.files-list$(2).before > @@ -67,7 +67,7 @@ endef > > # $(1): base directory to search in > # $(2): suffix of file (optional) > -define step_pkg_size_after > +define pkg_size_after > cd $(1); \ > LC_ALL=C find . \( -type f -o -type l \) -printf '%T@:%i:%#m:%y:%s,%p\n' \ > | LC_ALL=C sort > $($(PKG)_DIR)/.files-list$(2).after > @@ -80,35 +80,14 @@ define step_pkg_size_after > rm -f $($(PKG)_DIR)/.files-list$(2).after > endef > > -define step_pkg_size > - $(if $(filter start-install-target,$(1)-$(2)),\ > - $(call step_pkg_size_before,$(TARGET_DIR))) > - $(if $(filter start-install-staging,$(1)-$(2)),\ > - $(call step_pkg_size_before,$(STAGING_DIR),-staging)) > - $(if $(filter start-install-host,$(1)-$(2)),\ > - $(call step_pkg_size_before,$(HOST_DIR),-host)) > - > - $(if $(filter end-install-target,$(1)-$(2)),\ > - $(call step_pkg_size_after,$(TARGET_DIR))) > - $(if $(filter end-install-staging,$(1)-$(2)),\ > - $(call step_pkg_size_after,$(STAGING_DIR),-staging)) > - $(if $(filter end-install-host,$(1)-$(2)),\ > - $(call step_pkg_size_after,$(HOST_DIR),-host)) > -endef > -GLOBAL_INSTRUMENTATION_HOOKS += step_pkg_size > - > -# Relies on step_pkg_size, so must be after > define check_bin_arch > - $(if $(filter end-install-target,$(1)-$(2)),\ > - support/scripts/check-bin-arch -p $(3) \ > - -l $($(PKG)_DIR)/.files-list.txt \ > - $(foreach i,$($(PKG)_BIN_ARCH_EXCLUDE),-i "$(i)") \ > - -r $(TARGET_READELF) \ > - -a $(BR2_READELF_ARCH_NAME)) > + support/scripts/check-bin-arch -p $($(PKG)_NAME) \ > + -l $($(PKG)_DIR)/.files-list.txt \ > + $(foreach i,$($(PKG)_BIN_ARCH_EXCLUDE),-i "$(i)") \ > + -r $(TARGET_READELF) \ > + -a $(BR2_READELF_ARCH_NAME) > endef > > -GLOBAL_INSTRUMENTATION_HOOKS += check_bin_arch > - > # This hook checks that host packages that need libraries that we build > # have a proper DT_RPATH or DT_RUNPATH tag > define check_host_rpath > @@ -253,6 +232,9 @@ $(BUILD_DIR)/%/.stamp_configured: > @$(call step_start,configure) > @$(call MESSAGE,"Configuring") > $(call prepare-per-package-directory,$($(PKG)_FINAL_DEPENDENCIES)) > + @$(call pkg_size_before,$(TARGET_DIR)) > + @$(call pkg_size_before,$(STAGING_DIR),-staging) > + @$(call pkg_size_before,$(HOST_DIR),-host) > $(call fixup-libtool-files,$(NAME),$(STAGING_DIR)) > $(foreach hook,$($(PKG)_PRE_CONFIGURE_HOOKS),$(call $(hook))$(sep)) > $($(PKG)_CONFIGURE_CMDS) > @@ -374,6 +356,10 @@ $(BUILD_DIR)/%/.stamp_target_installed: > # Final installation step, completed when all installation steps > # (host, images, staging, target) have completed > $(BUILD_DIR)/%/.stamp_installed: > + @$(call pkg_size_after,$(TARGET_DIR)) > + @$(call pkg_size_after,$(STAGING_DIR),-staging) > + @$(call pkg_size_after,$(HOST_DIR),-host) > + @$(call check_bin_arch) > $(Q)touch $@ > > # Remove package sources > -- > 2.25.4 >
Hello, Thanks for taking the time to review this stuff! On Fri, 1 May 2020 22:44:41 +0200 "Yann E. MORIN" <yann.morin.1998@free.fr> wrote: > I don't understand wy tht can't be achieved in the instrumentation hooks > mechanism: > > $(if $(filter start-configure,$(1)-$(2)),\ > $(call step_pkg_size_before,$(TARGET_DIR))\ > $(call step_pkg_size_before,$(STAGING_DIR),-staging)\ > $(call step_pkg_size_before,$(HOST_DIR),-host)) > > $(if $(filter end-install,$(1)-$(2)),\ > $(call step_pkg_size_after,$(TARGET_DIR))\ > $(call step_pkg_size_after,$(STAGING_DIR),-staging)\ > $(call step_pkg_size_after,$(HOST_DIR),-host)) > > Of course, it means you have to add a call the instrumentation hooks > from the new 'install' step. This wouldn't work for the configure step. See the code: $(BUILD_DIR)/%/.stamp_configured: @$(call step_start,configure) @$(call MESSAGE,"Configuring") $(call prepare-per-package-directory,$($(PKG)_FINAL_DEPENDENCIES)) The hooks are called *before* we populate the per package directories. But we need to capture the list of files *after* we populate the per package directories. In addition, I don't really understand why we use those global instrumentation hooks within the generic infra itself. It is just a lot clearer to me to make those function calls directly than to have this indirection through hooks. For me, it's like using hooks for the configure, build or install steps of a generic package. For example, do we typically do: ifeq ($(something),y) define FOO_BUILD_THIS ... endef endif define FOO_BUILD_CMDS ... do stuff ... $(FOO_BUILD_THIS) endef or this: define FOO_BUILD_CMDS ... do stuff ... endef ifeq ($(something),y) define FOO_BUILD_THIS ... endef FOO_POST_BUILD_HOOKS += FOO_BUILD_THIS endef We typically do the former, because there is no point in using hooks in this situation. Hooks are useful to inject some logic to a step of logic you don't have control over. When you have control over that step of logic, hooks are kind of useless. That being said, if you have a proposal to solve the ordering of things in the configure step between the instrumentation hook and the per-package directory population, I'm definitely fine with changing to use instrumentation hooks. Thanks! Thomas
diff --git a/package/pkg-generic.mk b/package/pkg-generic.mk index efdc32e355..3fb1e5f8c3 100644 --- a/package/pkg-generic.mk +++ b/package/pkg-generic.mk @@ -59,7 +59,7 @@ GLOBAL_INSTRUMENTATION_HOOKS += step_time # $(1): base directory to search in # $(2): suffix of file (optional) -define step_pkg_size_before +define pkg_size_before cd $(1); \ LC_ALL=C find . \( -type f -o -type l \) -printf '%T@:%i:%#m:%y:%s,%p\n' \ | LC_ALL=C sort > $($(PKG)_DIR)/.files-list$(2).before @@ -67,7 +67,7 @@ endef # $(1): base directory to search in # $(2): suffix of file (optional) -define step_pkg_size_after +define pkg_size_after cd $(1); \ LC_ALL=C find . \( -type f -o -type l \) -printf '%T@:%i:%#m:%y:%s,%p\n' \ | LC_ALL=C sort > $($(PKG)_DIR)/.files-list$(2).after @@ -80,35 +80,14 @@ define step_pkg_size_after rm -f $($(PKG)_DIR)/.files-list$(2).after endef -define step_pkg_size - $(if $(filter start-install-target,$(1)-$(2)),\ - $(call step_pkg_size_before,$(TARGET_DIR))) - $(if $(filter start-install-staging,$(1)-$(2)),\ - $(call step_pkg_size_before,$(STAGING_DIR),-staging)) - $(if $(filter start-install-host,$(1)-$(2)),\ - $(call step_pkg_size_before,$(HOST_DIR),-host)) - - $(if $(filter end-install-target,$(1)-$(2)),\ - $(call step_pkg_size_after,$(TARGET_DIR))) - $(if $(filter end-install-staging,$(1)-$(2)),\ - $(call step_pkg_size_after,$(STAGING_DIR),-staging)) - $(if $(filter end-install-host,$(1)-$(2)),\ - $(call step_pkg_size_after,$(HOST_DIR),-host)) -endef -GLOBAL_INSTRUMENTATION_HOOKS += step_pkg_size - -# Relies on step_pkg_size, so must be after define check_bin_arch - $(if $(filter end-install-target,$(1)-$(2)),\ - support/scripts/check-bin-arch -p $(3) \ - -l $($(PKG)_DIR)/.files-list.txt \ - $(foreach i,$($(PKG)_BIN_ARCH_EXCLUDE),-i "$(i)") \ - -r $(TARGET_READELF) \ - -a $(BR2_READELF_ARCH_NAME)) + support/scripts/check-bin-arch -p $($(PKG)_NAME) \ + -l $($(PKG)_DIR)/.files-list.txt \ + $(foreach i,$($(PKG)_BIN_ARCH_EXCLUDE),-i "$(i)") \ + -r $(TARGET_READELF) \ + -a $(BR2_READELF_ARCH_NAME) endef -GLOBAL_INSTRUMENTATION_HOOKS += check_bin_arch - # This hook checks that host packages that need libraries that we build # have a proper DT_RPATH or DT_RUNPATH tag define check_host_rpath @@ -253,6 +232,9 @@ $(BUILD_DIR)/%/.stamp_configured: @$(call step_start,configure) @$(call MESSAGE,"Configuring") $(call prepare-per-package-directory,$($(PKG)_FINAL_DEPENDENCIES)) + @$(call pkg_size_before,$(TARGET_DIR)) + @$(call pkg_size_before,$(STAGING_DIR),-staging) + @$(call pkg_size_before,$(HOST_DIR),-host) $(call fixup-libtool-files,$(NAME),$(STAGING_DIR)) $(foreach hook,$($(PKG)_PRE_CONFIGURE_HOOKS),$(call $(hook))$(sep)) $($(PKG)_CONFIGURE_CMDS) @@ -374,6 +356,10 @@ $(BUILD_DIR)/%/.stamp_target_installed: # Final installation step, completed when all installation steps # (host, images, staging, target) have completed $(BUILD_DIR)/%/.stamp_installed: + @$(call pkg_size_after,$(TARGET_DIR)) + @$(call pkg_size_after,$(STAGING_DIR),-staging) + @$(call pkg_size_after,$(HOST_DIR),-host) + @$(call check_bin_arch) $(Q)touch $@ # Remove package sources
This commits reworks the pkg_size logic to no longer use the GLOBAL_INSTRUMENTATION_HOOKS mechanism, but instead be directly implemented within the configure step and install step. The problem with the current implementation in the GLOBAL_INSTRUMENTATION_HOOKS is that we only capture what is installed in $(HOST_DIR) during the "host installation step", what is installed in $(TARGET_DIR) during the "target installation step" and what is installed in "$(STAGING_DIR)" during the staging installation step. While this seems reasonable in principle, it is in fact not completely true. For example, "toolchain", which is a target package, installs tons of files in $(HOST_DIR). "qt5base", which is also a target package, also installs things in $(HOST_DIR). Same with the "syslinux" package. The idea behind this patch is pretty simple: - At the beginning of the configure step, right after the per-package directories have been populated (if BR2_PER_PACKAGE_DIRECTORIES=y), we capture the state of the HOST_DIR, TARGET_DIR and STAGING_DIR. - At the end of all install steps (which is possible thanks to the newly introduced "install" step), we capture again the state of HOST_DIR, TARGET_DIR and STAGING_DIR, and compare it to what we have saved at the configure step. So regardless of whether a file has been installed in $(HOST_DIR) during the target or staging installation steps of a target package, or if a host package has installed a file in $(TARGET_DIR), we will detect it. The pkg_size_before and pkg_size_after macros are intentionally left where they are (even if they now fall in the middle of the GLOBAL_INSTRUMENTATION_HOOKS implementations) to minimize the diffstat and facilitate review. Note that we also have to change check_bin_arch to be explicitly called from the install step rather than through a GLOBAL_INSTRUMENTATION_HOOKS as it depends on the .files-list.txt file produced by the pkg_size_after function. Signed-off-by: Thomas Petazzoni <thomas.petazzoni@bootlin.com> --- package/pkg-generic.mk | 42 ++++++++++++++---------------------------- 1 file changed, 14 insertions(+), 28 deletions(-)