From mboxrd@z Thu Jan  1 00:00:00 1970
From: Yann E. MORIN <yann.morin.1998@free.fr>
Date: Thu, 24 Jun 2021 22:34:10 +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: <20210624203410.GC2852@scaer>
List-Id: <buildroot.busybox.net>
MIME-Version: 1.0
Content-Type: text/plain; charset="us-ascii"
Content-Transfer-Encoding: 7bit
To: buildroot@busybox.net

Herv?, All,

Additional revview I forgot previously...

On 2021-06-24 22:20 +0200, Yann E. MORIN spake thusly:
> 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?

Now I have seen the next patch, it makes sense. Yet, it should be
explained here nonetheless. ;-)

[--SNIP--]
> > +	$(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))
[--SNIP--]
> 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.

And to begin with, why are we generating an _exclude_ list, rather than
an _include_ list?

I.e. rather than rsync everything except what we don't want, why not
just rsync just what we want, and generate a list of filters about
exactly just the files installed by a package?

I.e. in previous patch, crea create '+' filters rather than '-' filters.
Also, I think using the full-length filter 'include' is nicer than the
shorter '+'.

Probably something like:

    LC_ALL=C comm -1 \
        $($(PKG)_DIR)/.files-final-rsync$(2).before \
        $($(PKG)_DIR)/.files-final-rsync$(2).after \
        | sed -r -e 's/^[^,]+,./include /' \
        > $($(PKG)_DIR)/.files-final-rsync$(2).exclude_rsync

(Totally untested, slippery code, excersise caution.)

Regards,
Yann E. MORIN.

-- 
.-----------------.--------------------.------------------.--------------------.
|  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.  |
'------------------------------^-------^------------------^--------------------'