[Buildroot] [PATCH 13/15] Makefile: Rsync global {TARGET, HOST}_DIR using exclusion file list
Herve Codina
herve.codina at bootlin.com
Fri Jun 25 12:00:23 UTC 2021
On Thu, 24 Jun 2021 22:20:10 +0200
"Yann E. MORIN" <yann.morin.1998 at free.fr> 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
> > <PKG>_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 <herve.codina at bootlin.com>
> > ---
> > 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
More information about the buildroot
mailing list