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

On Thu, 24 Jun 2021 22:20:10 +0200
"Yann E. MORIN" [off-list ref] 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:

quoted

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?

quoted

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.

quoted

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

quoted

+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.

quoted

 .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