[Buildroot] [PATCHv2 01/10] toolchain-external: clarify rsync excludes in copy_toolchain_sysroot

[Thomas De Schampheleire]

On Tue, Feb 7, 2017 at 11:26 AM, Romain Naour <romain.naour at gmail.com> wrote:
> Hi Thomas,
>
> Le 04/02/2017 à 14:42, Thomas De Schampheleire a écrit :
>> From: Thomas De Schampheleire <thomas.de_schampheleire at nokia.com>
>>
>> The copy_toolchain_sysroot helper features a complex rsync loop that copies
>> various directories from the extracted toolchain to the staging directory.
>> The complexity mainly stems from the fact that we support multilib toolchain
>> tarballs but only copy one of the multilib variants into staging.
>>
>> Increase understandability of this logic by explicitly restricting the
>> rsync excludes to the iteration of the for loop they are relevant for.
>> Additionally, update the function comment.
>>
>> Note: all attempts to reduce duplication between both rsync while keeping
>> things nice and readable failed. One has to be extremely careful regarding
>> line continuation, indentation, and single vs double quoting. In the end, a
>> split up rsync seemed most clean.
>>
>> Signed-off-by: Thomas De Schampheleire <thomas.de_schampheleire at nokia.com>
>> ---
>> v2: no changes
>>
>>  toolchain/helpers.mk | 26 ++++++++++++++++++--------
>>  1 file changed, 18 insertions(+), 8 deletions(-)
>>
>> diff --git a/toolchain/helpers.mk b/toolchain/helpers.mk
>> index 72e7292..06dc197 100644
>> --- a/toolchain/helpers.mk
>> +++ b/toolchain/helpers.mk
>> @@ -39,11 +39,16 @@ copy_toolchain_lib_root = \
>>  # dir. The operation of this function is rendered a little bit
>>  # complicated by the support for multilib toolchains.
>>  #
>> -# We start by copying etc, lib, sbin and usr from the sysroot of the
>> -# selected architecture variant (as pointed by ARCH_SYSROOT_DIR). This
>> -# allows to import into the staging directory the C library and
>> -# companion libraries for the correct architecture variant. We
>> -# explictly only copy etc, lib, sbin and usr since other directories
>> +# We start by copying etc, 'lib', sbin, usr and usr/'lib' from the
>> +# sysroot of the selected architecture variant (as pointed to by
>> +# ARCH_SYSROOT_DIR). This allows to import into the staging directory
>> +# the C library and companion libraries for the correct architecture
>> +# variant. 'lib' may not be literally 'lib' but could be something else,
>> +# e.g. lib32-fp (as determined by ARCH_LIB_DIR) and we only want to copy
>> +# that lib directory and no other. When copying usr, we therefore need
>> +# to be extra careful not to include usr/lib* but we _do_ want to
>> +# include usr/libexec.
>> +# We are selective in the directories we copy since other directories
>>  # might exist for other architecture variants (on Codesourcery
>>  # toolchain, the sysroot for the default architecture variant contains
>>  # the armv4t and thumb2 subdirectories, which are the sysroot for the
>> @@ -90,9 +95,14 @@ copy_toolchain_sysroot = \
>>       SUPPORT_LIB_DIR="$(strip $5)" ; \
>>       for i in etc $${ARCH_LIB_DIR} sbin usr usr/$${ARCH_LIB_DIR}; do \
>>               if [ -d $${ARCH_SYSROOT_DIR}/$$i ] ; then \
>> -                     rsync -au --chmod=u=rwX,go=rX --exclude 'usr/lib/locale' \
>> -                             --include '/libexec*/' --exclude '/lib*/' \
>> -                             $${ARCH_SYSROOT_DIR}/$$i/ $(STAGING_DIR)/$$i/ ; \
>> +                     if [ "$$i" = "usr" ]; then \
>> +                             rsync -au --chmod=u=rwX,go=rX --exclude 'usr/lib/locale'\
>> +                                     --include '/libexec*/' --exclude '/lib*/' \
>> +                                     $${ARCH_SYSROOT_DIR}/$$i/ $(STAGING_DIR)/$$i/ ; \
>> +                     else \
>> +                             rsync -au --chmod=u=rwX,go=rX --exclude 'usr/lib/locale'\
>
> Not related to your patch but I'm wondering if --exclude 'usr/lib/locale' is
> still relevant. It was added to rsync with commit [1] but in seems to be already
> excluded while extracting the toolchain (BR2_TOOLCHAIN_EXTERNAL_DOWNLOAD=y case).
> Maybe we still need it for BR2_TOOLCHAIN_EXTERNAL_PREINSTALLED=y case.
>
> Also, in "$$i" != "usr" case it's seems not needed.

I wasn't too sure about this 'usr/lib/locale' thing so did not touch it.
Note that the exclusion does not contain a leading slash, which means
that 'usr/lib/locale' is matched anywhere in the path, so e.g.
usr/share/foo/usr/lib/locale would also be excluded. If we are sure
that the intention is to only match in the beginning, then the code
can indeed be simplified.