* [Buildroot] [git commit branch/next] support/scripts/fix-rpath: parallelize patching files
@ 2023-08-06 21:27 Thomas Petazzoni via buildroot
2023-08-07 21:02 ` Yann E. MORIN
0 siblings, 1 reply; 2+ messages in thread
From: Thomas Petazzoni via buildroot @ 2023-08-06 21:27 UTC (permalink / raw)
To: buildroot
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.
Signed-off-by: Victor Dumas <dumasv.dev@gmail.com>
[Thomas: take into account the suggestion of Quentin Schulz to pass
PARALLEL_JOBS through the environment down to the fix-rpath script]
Signed-off-by: Thomas Petazzoni <thomas.petazzoni@bootlin.com>
---
Makefile | 12 ++++++---
support/scripts/fix-rpath | 67 +++++++++++++++++++++++++++--------------------
2 files changed, 48 insertions(+), 31 deletions(-)
diff --git a/Makefile b/Makefile
index 18e337006b..e5aae17602 100644
--- a/Makefile
+++ b/Makefile
@@ -594,8 +594,12 @@ world: target-post-image
.PHONY: prepare-sdk
prepare-sdk: world
@$(call MESSAGE,"Rendering the SDK relocatable")
- PER_PACKAGE_DIR=$(PER_PACKAGE_DIR) $(TOPDIR)/support/scripts/fix-rpath host
- PER_PACKAGE_DIR=$(PER_PACKAGE_DIR) $(TOPDIR)/support/scripts/fix-rpath staging
+ PARALLEL_JOBS=$(PARALLEL_JOBS) \
+ PER_PACKAGE_DIR=$(PER_PACKAGE_DIR) \
+ $(TOPDIR)/support/scripts/fix-rpath host
+ PARALLEL_JOBS=$(PARALLEL_JOBS) \
+ PER_PACKAGE_DIR=$(PER_PACKAGE_DIR) \
+ $(TOPDIR)/support/scripts/fix-rpath staging
$(INSTALL) -m 755 $(TOPDIR)/support/misc/relocate-sdk.sh $(HOST_DIR)/relocate-sdk.sh
mkdir -p $(HOST_DIR)/share/buildroot
echo $(HOST_DIR) > $(HOST_DIR)/share/buildroot/sdk-location
@@ -765,7 +769,9 @@ endif
ln -sf ../usr/lib/os-release $(TARGET_DIR)/etc
@$(call MESSAGE,"Sanitizing RPATH in target tree")
- PER_PACKAGE_DIR=$(PER_PACKAGE_DIR) $(TOPDIR)/support/scripts/fix-rpath target
+ PARALLEL_JOBS=$(PARALLEL_JOBS) \
+ PER_PACKAGE_DIR=$(PER_PACKAGE_DIR) \
+ $(TOPDIR)/support/scripts/fix-rpath target
# For a merged /usr, ensure that /lib, /bin and /sbin and their /usr
# counterparts are appropriately setup as symlinks ones to the others.
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
@@ -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() {
+ PATCHELF="${1}"
+ rootdir="${2}"
+ sanitize_extra_args="${3}"
+ file="${4}"
+
+ # check if it's an ELF file
+ rpath=$(${PATCHELF} --print-rpath "${file}" 2>&1)
+ if test $? -ne 0 ; then
+ return 0
+ fi
+
+ # make files writable if necessary
+ changed=$(chmod -c u+w "${file}")
+
+ # With per-package directory support, most RPATH of host
+ # binaries will point to per-package directories. This won't
+ # work with the --make-rpath-relative ${rootdir} invocation as
+ # the per-package host directory is not within ${rootdir}. So,
+ # we rewrite all RPATHs pointing to per-package directories so
+ # that they point to the global host directry.
+ changed_rpath=$(echo ${rpath} | sed "s@${PER_PACKAGE_DIR}/[^/]\+/host@${HOST_DIR}@")
+ if test "${rpath}" != "${changed_rpath}" ; then
+ ${PATCHELF} --set-rpath ${changed_rpath} "${file}"
+ fi
+
+ # call patchelf to sanitize the rpath
+ ${PATCHELF} --make-rpath-relative "${rootdir}" ${sanitize_extra_args[@]} "${file}"
+ # restore the original permission
+ test "${changed}" != "" && chmod u-w "${file}"
+}
+
main() {
local rootdir
local tree="${1}"
@@ -123,34 +157,11 @@ main() {
;;
esac
- find_args+=( "-type" "f" "-print" )
-
- while read file ; do
- # check if it's an ELF file
- rpath=$(${PATCHELF} --print-rpath "${file}" 2>&1)
- if test $? -ne 0 ; then
- continue
- fi
-
- # make files writable if necessary
- changed=$(chmod -c u+w "${file}")
-
- # With per-package directory support, most RPATH of host
- # binaries will point to per-package directories. This won't
- # work with the --make-rpath-relative ${rootdir} invocation as
- # the per-package host directory is not within ${rootdir}. So,
- # we rewrite all RPATHs pointing to per-package directories so
- # that they point to the global host directry.
- changed_rpath=$(echo ${rpath} | sed "s@${PER_PACKAGE_DIR}/[^/]\+/host@${HOST_DIR}@")
- if test "${rpath}" != "${changed_rpath}" ; then
- ${PATCHELF} --set-rpath ${changed_rpath} "${file}"
- fi
-
- # call patchelf to sanitize the rpath
- ${PATCHELF} --make-rpath-relative "${rootdir}" ${sanitize_extra_args[@]} "${file}"
- # restore the original permission
- test "${changed}" != "" && chmod u-w "${file}"
- done < <(find "${rootdir}" ${find_args[@]})
+ find_args+=( "-type" "f" "-print0" )
+
+ export -f patch_file
+ # Limit the number of cores used
+ find "${rootdir}" ${find_args[@]} | xargs -0 -r -P ${PARALLEL_JOBS} -I {} bash -c "patch_file '${PATCHELF}' '${rootdir}' '${sanitize_extra_args}' $@" _ {}
# 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
^ permalink raw reply related [flat|nested] 2+ messages in thread* Re: [Buildroot] [git commit branch/next] support/scripts/fix-rpath: parallelize patching files
2023-08-06 21:27 [Buildroot] [git commit branch/next] support/scripts/fix-rpath: parallelize patching files Thomas Petazzoni via buildroot
@ 2023-08-07 21:02 ` Yann E. MORIN
0 siblings, 0 replies; 2+ messages in thread
From: Yann E. MORIN @ 2023-08-07 21:02 UTC (permalink / raw)
To: Victor Dumas; +Cc: buildroot
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
^ permalink raw reply [flat|nested] 2+ messages in thread
end of thread, other threads:[~2023-08-07 21:02 UTC | newest]
Thread overview: 2+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2023-08-06 21:27 [Buildroot] [git commit branch/next] support/scripts/fix-rpath: parallelize patching files Thomas Petazzoni via buildroot
2023-08-07 21:02 ` Yann E. MORIN
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.