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

[Yann E. MORIN <yann.morin.1998@free.fr>]

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 <pkg>-{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
> <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)

> +	$(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.  |
'------------------------------^-------^------------------^--------------------'