From mboxrd@z Thu Jan 1 00:00:00 1970 From: Herve Codina Date: Fri, 25 Jun 2021 14:00:23 +0200 Subject: [Buildroot] [PATCH 13/15] Makefile: Rsync global {TARGET, HOST}_DIR using exclusion file list In-Reply-To: <20210624202010.GA2852@scaer> References: <20210621141130.48654-1-herve.codina@bootlin.com> <20210621141130.48654-14-herve.codina@bootlin.com> <20210624202010.GA2852@scaer> Message-ID: <20210625140023.04e02ada@fedora> List-Id: MIME-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit To: buildroot@busybox.net On Thu, 24 Jun 2021 22:20:10 +0200 "Yann E. MORIN" wrote: > 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: > Ok, thanks. > > 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) Changed. > > > + $(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)) Changed > > 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 \ > ) Changed. > > > .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. Changed > > 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. > I missed this use case. You you think it can help to keep in mind if (1) I add it in my v2 my cover letter or (2) I add it in this patch commit log Herv? -- Herv? Codina, Bootlin Embedded Linux and Kernel engineering https://bootlin.com