Re: [Buildroot] [git commit branch/next] support/scripts/fix-rpath: parallelize patching files

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

Victor, All,

On 2023-08-06 23:27 +0200, Thomas Petazzoni via buildroot spake thusly:
> commit: https://git.buildroot.net/buildroot/commit/?id=134900401f0831f893ddd4f358e499f548d66433
> branch: https://git.buildroot.net/buildroot/commit/?id=refs/heads/next
> 
> Using "xargs" instead of "while read" loop allows for the patching of
> files to be parallelized. This significantly reduces the amount of
> time it takes to fix all the paths.  On a larger RFS(~300MB) this
> script was taking 5 minutes, it now only takes about 30s on a 12 core
> machine.

This commit is not working, and breaks fix-rpath. See below...

[--SNIP--]
> diff --git a/support/scripts/fix-rpath b/support/scripts/fix-rpath
> index 3e67e770e5..a9b348e189 100755
> --- a/support/scripts/fix-rpath
> +++ b/support/scripts/fix-rpath

there are tons of shellcheck errors in that script, which is what
prompted me to look into that and notice the breakage.

    $ ./utils/docker-run shellcheck support/scripts/fix-rpath
    In support/scripts/fix-rpath line 56:
    : ${PATCHELF:=${HOST_DIR}/bin/patchelf}
      ^-- SC2223: This default assignment may cause DoS due to globbing. Quote it.


    In support/scripts/fix-rpath line 84:
        changed_rpath=$(echo ${rpath} | sed "s@${PER_PACKAGE_DIR}/[^/]\+/host@${HOST_DIR}@")
                        ^-- SC2001: See if you can use ${variable//search/replace} instead.
                             ^------^ SC2086: Double quote to prevent globbing and word splitting.

    Did you mean:
        changed_rpath=$(echo "${rpath}" | sed "s@${PER_PACKAGE_DIR}/[^/]\+/host@${HOST_DIR}@")


    In support/scripts/fix-rpath line 86:
            ${PATCHELF} --set-rpath ${changed_rpath} "${file}"
                                    ^--------------^ SC2086: Double quote to prevent globbing and word splitting.

    Did you mean:
            ${PATCHELF} --set-rpath "${changed_rpath}" "${file}"


    In support/scripts/fix-rpath line 90:
        ${PATCHELF} --make-rpath-relative "${rootdir}" ${sanitize_extra_args[@]} "${file}"
                                                       ^-----------------------^ SC2068: Double quote array expansions to avoid re-splitting elements.


    In support/scripts/fix-rpath line 164:
        find "${rootdir}" ${find_args[@]} | xargs -0 -r -P ${PARALLEL_JOBS} -I {} bash -c "patch_file '${PATCHELF}' '${rootdir}' '${sanitize_extra_args}' $@" _ {}
                          ^-------------^ SC2068: Double quote array expansions to avoid re-splitting elements.
                                                           ^--------------^ SC2086: Double quote to prevent globbing and word splitting.
                                                                                                                                  ^--------------------^ SC2128: Expanding an array without an index only gives the first element.
                                                                                                                                                          ^-- SC2145: Argument mixes string and array. Use * or separate argument.

    Did you mean:
        find "${rootdir}" ${find_args[@]} | xargs -0 -r -P "${PARALLEL_JOBS}" -I {} bash -c "patch_file '${PATCHELF}' '${rootdir}' '${sanitize_extra_args}' $@" _ {}


    In support/scripts/fix-rpath line 173:
    main ${@}
         ^--^ SC2068: Double quote array expansions to avoid re-splitting elements.

    For more information:
      https://www.shellcheck.net/wiki/SC2068 -- Double quote array expansions to ...
      https://www.shellcheck.net/wiki/SC2145 -- Argument mixes string and array. ...
      https://www.shellcheck.net/wiki/SC2128 -- Expanding an array without an ind...

So a few of those reported issues are harmless, or at least were OK-ish
before the parallelisation.

> @@ -46,6 +46,8 @@ Environment:
>      TOOLCHAIN_EXTERNAL_DOWNLOAD_INSTALL_DIR
>                   (default HOST_DIR/opt/ext-toolchain)
>  
> +    PARALLEL_JOBS number of parallel jobs to run
> +
>  Returns:         0 if success or 1 in case of error
>  
>  EOF
> @@ -58,6 +60,38 @@ HOST_EXCLUDEPATHS="/share/terminfo"
>  STAGING_EXCLUDEPATHS="/usr/include /usr/share/terminfo"
>  TARGET_EXCLUDEPATHS="/lib/firmware"
>  
> +patch_file() {

I've stuck a 'set -x' here to see how this function was called and what
it did, once it got called from a sub-bash, and I also tweaked the xargs
call with '-t -P1'  (verbose and mt parralel, just to have a clean
output). Here's what it says for the first entry:

    bash -c "patch_file '/home/ymorin/dev/buildroot/O/next/host/bin/patchelf' '/home/ymorin/dev/buildroot/O/next/target' '--no-standard-lib-dirs' target" _ /home/ymorin/dev/buildroot/O/next/target/lib/udev/cdrom_id
    + PATCHELF=/home/ymorin/dev/buildroot/O/next/host/bin/patchelf
    + rootdir=/home/ymorin/dev/buildroot/O/next/target
    + sanitize_extra_args=--no-standard-lib-dirs
    + file=target
    ++ /home/ymorin/dev/buildroot/O/next/host/bin/patchelf --print-rpath target
    + rpath='patchelf: getting info about '''target''': No such file or directory'
    + test 1 -ne 0
    + return 0

So what's going on here? Why does it look at 'target', and not at the
actual file 'cdrom_id' ?

That's because it is actually told to look at 'target'. See that the $@
has been replaced by 'target'. See below [0]...

> +    PATCHELF="${1}"
> +    rootdir="${2}"
> +    sanitize_extra_args="${3}"

In the caller, this is an array, but here it's a string. That's not
going to cut it, but it works by accident: we only ever put a single
item in the array, so we're lucky. But we can't rely on luck alone.

[--SNIP--]
> @@ -123,34 +157,11 @@ main() {
[--SNIP--]

(I've split the following line so that it is easier to read and comment on).

> +    find "${rootdir}" ${find_args[@]} \
> +    | xargs -0 -r -P ${PARALLEL_JOBS} -I {} \
> +            bash -c "patch_file '${PATCHELF}' '${rootdir}' '${sanitize_extra_args}' $@" _ {}
                                                                                       ^^
That $@ is double-quoted, so it is expanded in the context of the
caller, i.e. it expands to the postional parameters passed when calling
main(), the current function, i.e.... 'target', as we noticed in the
log, above...

I've fixed all this and will push that shortly.

Regards,
Yann E. MORIN.

>  
>      # Restore patched patchelf utility
>      test "${tree}" = "host" && mv "${PATCHELF}.__to_be_patched" "${PATCHELF}"
> _______________________________________________
> buildroot mailing list
> buildroot@buildroot.org
> https://lists.buildroot.org/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.  |
'------------------------------^-------^------------------^--------------------'
_______________________________________________
buildroot mailing list
buildroot@buildroot.org
https://lists.buildroot.org/mailman/listinfo/buildroot