[Buildroot] [PATCH v7 3/8] core: implement per-package SDK and target
Yann E. MORIN
yann.morin.1998 at free.fr
Sun Dec 30 21:52:12 UTC 2018
Thomas, All,
I don;t have much to propose asa review, except very-minor issues.
On 2018-12-28 11:43 +0100, Thomas Petazzoni spake thusly:
> This commit implements the core of the move to per-package SDK and
> target directories. The main idea is that instead of having a global
> output/host and output/target in which all packages install files, we
> switch to per-package host and target directories, that only contain
> their explicit dependencies.
>
> There are two main benefits:
>
> - Packages will no longer discover dependencies that they do not
> explicitly indicate in their <pkg>_DEPENDENCIES variable.
Note that non-expressed dependencies may still be gathered, if they are
transitive dependencies.
[--SNIP--]
> Signed-off-by: Thomas Petazzoni <thomas.petazzoni at bootlin.com>
[--SNIP--]
> diff --git a/Makefile b/Makefile
> index 9de8e0c725..e01ec4c963 100644
> --- a/Makefile
> +++ b/Makefile
> @@ -205,6 +205,7 @@ BR_GRAPH_OUT := $(or $(BR2_GRAPH_OUT),pdf)
> BUILD_DIR := $(BASE_DIR)/build
> BINARIES_DIR := $(BASE_DIR)/images
> BASE_TARGET_DIR := $(BASE_DIR)/target
> +PER_PACKAGE_DIR := $(BASE_DIR)/per-package
Why don't you simply export this variable, like HOST_DIR and TARGET_DIR?
This would simplify calls to fix-rpath:
[--SNIP--]
> @@ -585,8 +587,8 @@ world: target-post-image
> .PHONY: prepare-sdk
> prepare-sdk: world
> @$(call MESSAGE,"Rendering the SDK relocatable")
> - $(TOPDIR)/support/scripts/fix-rpath host
> - $(TOPDIR)/support/scripts/fix-rpath staging
> + PER_PACKAGE_DIR=$(PER_PACKAGE_DIR) $(TOPDIR)/support/scripts/fix-rpath host
> + PER_PACKAGE_DIR=$(PER_PACKAGE_DIR) $(TOPDIR)/support/scripts/fix-rpath staging
... here.
[--SNIP--]
> @@ -973,7 +979,8 @@ savedefconfig: $(BUILD_DIR)/buildroot-config/conf prepare-kconfig
>
> # staging and target directories do NOT list these as
> # dependencies anywhere else
> -$(BUILD_DIR) $(BASE_TARGET_DIR) $(HOST_DIR) $(BINARIES_DIR) $(LEGAL_INFO_DIR) $(REDIST_SOURCES_DIR_TARGET) $(REDIST_SOURCES_DIR_HOST):
> +$(BUILD_DIR) $(BASE_TARGET_DIR) $(HOST_DIR) $(BINARIES_DIR) $(LEGAL_INFO_DIR) \
> + $(REDIST_SOURCES_DIR_TARGET) $(REDIST_SOURCES_DIR_HOST) $(PER_PACKAGE_DIR):
Don't ident this continuation line, it is misleading: the target of
the rule appear to be part of the commands.
[--SNIP--]
> diff --git a/support/scripts/check-host-rpath b/support/scripts/check-host-rpath
> index c8939569e2..d903a82958 100755
> --- a/support/scripts/check-host-rpath
> +++ b/support/scripts/check-host-rpath
As I was saying on IRC: when we are not using per-package directories,
then I was a bit surprised to see that this script (and fix_rpath,
below) still worked, when the support for PPD is not optional in it.
What confused me, was that PER_PACKAGE_DIR is always used, even when
BR2_PER_PACKAGE_DIRECTORIES is unset.
But as you pointed to on IRC, PER_PACKAGE_DIR is also always defined to
an non-empty string, that either points to a valid location when
BR2_PER_PACKAGE_DIRECTORIES=y, or points to a non-existent location
otherwise.
> @@ -77,6 +93,7 @@ check_elf_has_rpath() {
> dir="$( sed -r -e 's:/+:/:g; s:/$::;' <<<"${dir}" )"
> [ "${dir}" = "${hostdir}/lib" ] && return 0
> [ "${dir}" = "\$ORIGIN/../lib" ] && return 0
> + [[ ${dir} =~ ${perpackagedir}/[^/]*/host/lib ]] && return 0
^^^^^^^
That would also match the string '//' so maybe we want:
${perpackagedir}/([^/]+/)?host/lib
[--SNIP--]
> diff --git a/support/scripts/fix-rpath b/support/scripts/fix-rpath
> index fa138ca15a..926767a54f 100755
> --- a/support/scripts/fix-rpath
> +++ b/support/scripts/fix-rpath
> @@ -127,14 +127,29 @@ main() {
>
> while read file ; do
> # check if it's an ELF file
> - if ${PATCHELF} --print-rpath "${file}" > /dev/null 2>&1; then
> - # make files writable if necessary
> - changed=$(chmod -c u+w "${file}")
> - # 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}"
> + 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}@")
Ditto?
> + if test "${rpath}" != "${changed_rpath}" ; then
> + ${PATCHELF} --set-rpath ${changed_rpath} "${file}"
Can't you do that unconditioanlly? If it's changed, we need to set it;
if it's not changed, that set it to the initial value anyway...
> +
> + # 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[@]})
>
> # Restore patched patchelf utility
> --
> 2.20.1
>
--
.-----------------.--------------------.------------------.--------------------.
| Yann E. MORIN | Real-Time Embedded | /"\ ASCII RIBBON | Erics' conspiracy: |
| +33 662 376 056 | Software Designer | \ / CAMPAIGN | ___ |
| +33 223 225 172 `------------.-------: X AGAINST | \e/ There is no |
| http://ymorin.is-a-geek.org/ | _/*\_ | / \ HTML MAIL | v conspiracy. |
'------------------------------^-------^------------------^--------------------'
More information about the buildroot
mailing list