From mboxrd@z Thu Jan 1 00:00:00 1970 From: Yann E. MORIN Date: Thu, 24 Jun 2021 22:20:10 +0200 Subject: [Buildroot] [PATCH 13/15] Makefile: Rsync global {TARGET, HOST}_DIR using exclusion file list In-Reply-To: <20210621141130.48654-14-herve.codina@bootlin.com> References: <20210621141130.48654-1-herve.codina@bootlin.com> <20210621141130.48654-14-herve.codina@bootlin.com> Message-ID: <20210624202010.GA2852@scaer> List-Id: MIME-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit To: buildroot@busybox.net Herv?, All, Nitpick, applicable to most of your patches in this series: there should not be an uppercase in the title, after the colon sign, and the tense should be imperative, e.g: Makefile: rsync global {TARGET,HOST}_DIR using exclusion file list Makefile: break hardlinks in global {TARGET,HOST}_DIR on per-package build package/pkg-generic.mk: fix per-package -{reconfigure, rebuild, reinstall} On 2021-06-21 16:11 +0200, Herve Codina spake thusly: > On a per-package build, rsync final {TARGET,HOST}_DIR using > exclusion file list computed for each packages. > As we rsync only files generated by each packages, we need to > to do this rsync recursively on each dependencies to collect > all needed files. This is done based on existing > _FINAL_RECURSIVE_DEPENDENCIES I am not sure I understand why this is needed... One thing that comes to mind is speedup. Indeed, now that we can ensure that packages only install new files and don;t change existing files, we can speedup the final aggregation by just handling the new files. But since we are using hardlinks, the aggregation is rather fast... Can you expand on the rationale in the commit log when you respin, please? > Signed-off-by: Herve Codina > --- > Makefile | 24 ++++++++++++++++++++++-- > 1 file changed, 22 insertions(+), 2 deletions(-) > > diff --git a/Makefile b/Makefile > index 472af5a318..50bc6b4503 100644 > --- a/Makefile > +++ b/Makefile > @@ -740,10 +740,28 @@ TARGET_DIR_FILES_LISTS = $(sort $(wildcard $(BUILD_DIR)/*/.files-list.txt)) > HOST_DIR_FILES_LISTS = $(sort $(wildcard $(BUILD_DIR)/*/.files-list-host.txt)) > STAGING_DIR_FILES_LISTS = $(sort $(wildcard $(BUILD_DIR)/*/.files-list-staging.txt)) > > +ifeq ($(BR2_PER_PACKAGE_DIRECTORIES),y) > +# rsync the contents of per-package directories > +# $1: space-separated list of packages to rsync from > +# $2: 'host' or 'target' > +# $3: destination directory > +# $4: exclude file list to use > +define per-package-rsync-delta > + mkdir -p $(3) $(Q)mkdir -p $(3) > + $(foreach pkg,$(1),\ > + rsync -a --link-dest=$(PER_PACKAGE_DIR)/$(pkg)/$(strip $(2))/ \ > + --filter='merge $($(call UPPERCASE,$(pkg))_DIR)/$(strip $(4))' \ > + $(PER_PACKAGE_DIR)/$(pkg)/$(strip $(2))/ \ > + $(3)$(sep)) I would read it more easily if the lines after rsync are indented furhter, to represent the fact that they are stilll rsync arguments. Also, proably it would be nice to have this quiet by default. Indeed, with rather-deep dependency chains, there can be quite a lot of rsync lines... $(foreach pkg,$(1),\ $(Q)rsync -a --link-dest=$(PER_PACKAGE_DIR)/$(pkg)/$(strip $(2))/ \ --filter='merge $($(call UPPERCASE,$(pkg))_DIR)/$(strip $(4))' \ $(PER_PACKAGE_DIR)/$(pkg)/$(strip $(2))/ \ $(3)$(sep)) Furthermore, this rsync filter is non-obvious. It would be nice to explain the commit log that the list is already a proper filer, not just a list. Additionally, in the commit that generates that list, the commit log should also mention that a list of filters is created, not a list of filenames. > +endef > +endif > + > .PHONY: host-finalize > host-finalize: $(PACKAGES) $(HOST_DIR) $(HOST_DIR_SYMLINK) > @$(call MESSAGE,"Finalizing host directory") > - $(call per-package-rsync,$(sort $(PACKAGES)),host,$(HOST_DIR)) > + $(call per-package-rsync-delta, $(sort $(foreach pkg, $(PACKAGES), \ > + $(pkg) $($(call UPPERCASE,$(pkg))_FINAL_RECURSIVE_DEPENDENCIES))), \ > + host,$(HOST_DIR),.files-final-rsync-host.exclude_rsync) This is unparseable by a human... :-( $(call per-package-rsync-delta, \ $(sort $(foreach pkg, $(PACKAGES), \ $(pkg) $($(call UPPERCASE,$(pkg))_FINAL_RECURSIVE_DEPENDENCIES)) \ ), \ host, \ $(HOST_DIR), \ .files-final-rsync-host.exclude_rsync \ ) > .PHONY: staging-finalize > staging-finalize: $(STAGING_DIR_SYMLINK) > @@ -751,7 +769,9 @@ staging-finalize: $(STAGING_DIR_SYMLINK) > .PHONY: target-finalize > target-finalize: $(PACKAGES) $(TARGET_DIR) host-finalize > @$(call MESSAGE,"Finalizing target directory") > - $(call per-package-rsync,$(sort $(PACKAGES)),target,$(TARGET_DIR)) > + $(call per-package-rsync-delta, $(sort $(foreach pkg, $(PACKAGES), \ > + $(pkg) $($(call UPPERCASE,$(pkg))_FINAL_RECURSIVE_DEPENDENCIES))), \ > + target,$(TARGET_DIR),.files-final-rsync.exclude_rsync) Ditto. But now that I review this patch, I notice that there is still a hole in our file-overwrite detection. It only handles files modified by a package, that comes from it dependencies. However, if two packages in two different depednency chains, install the same file, then we're still toast. For example: A --> B --> C `-> D If C and D install the same file, then we don't detect the conflict, and we can't easil predict which the final file will come from... Of course, such a detection will have to be *in addition* to what this series already does, so this new issue does not invalidate this series. Regards, Yann E. MORIN. > $(foreach hook,$(TARGET_FINALIZE_HOOKS),$($(hook))$(sep)) > rm -rf $(TARGET_DIR)/usr/include $(TARGET_DIR)/usr/share/aclocal \ > $(TARGET_DIR)/usr/lib/pkgconfig $(TARGET_DIR)/usr/share/pkgconfig \ > -- > 2.31.1 > > _______________________________________________ > buildroot mailing list > buildroot at busybox.net > http://lists.busybox.net/mailman/listinfo/buildroot -- .-----------------.--------------------.------------------.--------------------. | Yann E. MORIN | Real-Time Embedded | /"\ ASCII RIBBON | Erics' conspiracy: | | +33 662 376 056 | Software Designer | \ / CAMPAIGN | ___ | | +33 561 099 427 `------------.-------: X AGAINST | \e/ There is no | | http://ymorin.is-a-geek.org/ | _/*\_ | / \ HTML MAIL | v conspiracy. | '------------------------------^-------^------------------^--------------------'