[Buildroot] [PATCH 13/15] Makefile: Rsync global {TARGET, HOST}_DIR using exclusion file list

[Herve Codina <herve.codina@bootlin.com>]

On Thu, 24 Jun 2021 22:20:10 +0200
"Yann E. MORIN" <yann.morin.1998@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@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