Per-package host/target directory support

Quoted 1 line

Quoted 1 line

Quoted 1 line

Quoted 1 line

"Thomas" == Thomas Petazzoni <[email protected]> writes:

As we are going to move to per-package SDK, the location of CCACHE and therefore the definitions of HOSTCC and HOSTCXX need to be evaluated at the time of use and not at the time of assignment. Indeed, the value of HOST_DIR changes from one package to the other.

Therefore, we need to change from := to =.

In addition, while doing A := $(something) $(A) is possible, doing A = $(something) $(A) is not legal. So, instead of defining HOSTCC in terms of the current HOSTCC variable, we re-use HOSTCC_NOCCACHE instead.

Signed-off-by Thomas Petazzoni <[email protected]>

Acked-by "Yann E. MORIN" <[email protected]>

Quoted 1 line

Quoted 1 line

Quoted 1 line

Quoted 1 line

"Thomas" == Thomas Petazzoni <[email protected]> writes:

Inside the check_elf_has_rpath(), we check if the host binary has a correct RPATH, which should be either an absolute path to $(HOST_DIR)/lib, or a relative path using $ORIGIN. Those two conditions are checked in a single statements, but as we are going to add a third condition, let's split this up a bit:

- If we have a RPATH to $(HOST_DIR)/lib -> we're good, return 0
- If we have a RPATH to $ORIGIN/../lib -> we're good, return 0
- Otherwise, we will exit the loop, and return 1

Signed-off-by Thomas Petazzoni <[email protected]>

Acked-by "Yann E. MORIN" <[email protected]>

In the current code, the creation of the main output directories (BUILD_DIR, STAGING_DIR, HOST_DIR, TARGET_DIR, etc.) is done by a global "dirs" target. While this works fine in the current situation, it doesn't work well in a context where per-package host and target directories are used.

For example, with the current code and per-package host directories, the output/staging symbolic link ends up being created as a link to the per-package package sysroot directory of the first package being built, instead of the global sysroot.

This commit reworks the creation of those directories by having the package/pkg-generic.mk code ensure that the build directory, target directory, host directory, staging directory and binaries directory exist before they are needed.

Two new targets, host-finalize and staging-finalize are added in the main Makefile to create the compatibility symlinks for host and staging directories. They will be extended later with additional logic for per-package directories.

Thanks to those changes, the global "dirs" target is entirely removed.

Signed-off-by Thomas Petazzoni <[email protected]>

---
 Makefile               | 21 +++++++++------------
 fs/common.mk           |  1 +
 package/pkg-generic.mk |  7 ++++++-
 3 files changed, 16 insertions(+), 13 deletions(-)

Makefile +9 -12

index 2819d44124..e675ac26aa 100644
@@ -572,10 +572,6 @@ $(foreach pkg,$(call UPPERCASE,$(PACKAGES)),\
 
 endif
 
-.PHONY: dirs
-dirs: $(BUILD_DIR) $(STAGING_DIR) $(BASE_TARGET_DIR) \
-	$(HOST_DIR) $(HOST_DIR_SYMLINK) $(BINARIES_DIR)
-
 $(BUILD_DIR)/buildroot-config/auto.conf: $(BR2_CONFIG)
 	$(MAKE1) $(EXTRAMAKEARGS) HOSTCC="$(HOSTCC_NOCCACHE)" HOSTCXX="$(HOSTCXX_NOCCACHE)" syncconfig
 

@@ -605,11 +601,6 @@ sdk: prepare-sdk $(BR2_TAR_HOST_DEPENDENCY)
 		--transform='s#^\.#$(BR2_SDK_PREFIX)#' \
 		-C $(HOST_DIR) "."
 
-# Populating the staging with the base directories is handled by the skeleton package
-$(STAGING_DIR):
-	@mkdir -p $(STAGING_DIR)
-	@ln -snf $(STAGING_DIR) $(BASE_DIR)/staging
-
 RSYNC_VCS_EXCLUSIONS = \
 	--exclude .svn --exclude .git --exclude .hg --exclude .bzr \
 	--exclude CVS

@@ -710,8 +701,14 @@ $(TARGETS_ROOTFS): target-finalize
 # Avoid the rootfs name leaking down the dependency chain
 target-finalize: ROOTFS=
 
+host-finalize: $(HOST_DIR_SYMLINK)
+
+.PHONY: staging-finalize
+staging-finalize:
+	@ln -snf $(STAGING_DIR) $(BASE_DIR)/staging
+
 .PHONY: target-finalize
-target-finalize: $(PACKAGES)
+target-finalize: $(PACKAGES) host-finalize
 	@$(call MESSAGE,"Finalizing target directory")
 	# Check files that are touched by more than one package
 	./support/scripts/check-uniq-files -t target $(BUILD_DIR)/packages-file-list.txt

@@ -782,7 +779,7 @@ endif
 	touch $(TARGET_DIR)/usr
 
 .PHONY: target-post-image
-target-post-image: $(TARGETS_ROOTFS) target-finalize
+target-post-image: $(TARGETS_ROOTFS) target-finalize staging-finalize
 	@rm -f $(ROOTFS_COMMON_TAR)
 	@$(foreach s, $(call qstrip,$(BR2_ROOTFS_POST_IMAGE_SCRIPT)), \
 		$(call MESSAGE,"Executing post-image script $(s)"); \

@@ -811,7 +808,7 @@ legal-info-prepare: $(LEGAL_INFO_DIR)
 	@cp $(BR2_CONFIG) $(LEGAL_INFO_DIR)/buildroot.config
 
 .PHONY: legal-info
-legal-info: dirs legal-info-clean legal-info-prepare $(foreach p,$(PACKAGES),$(p)-all-legal-info) \
+legal-info: legal-info-clean legal-info-prepare $(foreach p,$(PACKAGES),$(p)-all-legal-info) \
 		$(REDIST_SOURCES_DIR_TARGET) $(REDIST_SOURCES_DIR_HOST)
 	@cat support/legal-info/README.header >>$(LEGAL_REPORT)
 	@if [ -r $(LEGAL_WARNINGS) ]; then \

fs/common.mk +1 -0

index 2a5a202a89..96658428ba 100644
@@ -145,6 +145,7 @@ $$(BINARIES_DIR)/rootfs.$(1): ROOTFS=$(2)
 $$(BINARIES_DIR)/rootfs.$(1): FAKEROOT_SCRIPT=$$(ROOTFS_$(2)_DIR)/fakeroot
 $$(BINARIES_DIR)/rootfs.$(1): $$(ROOTFS_$(2)_DEPENDENCIES)
 	@$$(call MESSAGE,"Generating root filesystem image rootfs.$(1)")
+	mkdir -p $$(@D)
 	rm -rf $$(ROOTFS_$(2)_DIR)
 	mkdir -p $$(ROOTFS_$(2)_DIR)
 	echo '#!/bin/sh' > $$(FAKEROOT_SCRIPT)

package/pkg-generic.mk +6 -1

index f34f46afc8..309fd8cd48 100644
@@ -173,6 +173,7 @@ $(BUILD_DIR)/%/.stamp_extracted:
 $(BUILD_DIR)/%/.stamp_rsynced:
 	@$(call step_start,rsync)
 	@$(call MESSAGE,"Syncing from source dir $(SRCDIR)")
+	@mkdir -p $(@D)
 	$(foreach hook,$($(PKG)_PRE_RSYNC_HOOKS),$(call $(hook))$(sep))
 	@test -d $(SRCDIR) || (echo "ERROR: $(SRCDIR) does not exist" ; exit 1)
 	rsync -au --chmod=u=rwX,go=rX $(RSYNC_VCS_EXCLUSIONS) $($(PKG)_OVERRIDE_SRCDIR_RSYNC_EXCLUSIONS) $(call qstrip,$(SRCDIR))/ $(@D)

@@ -238,6 +239,7 @@ $(BUILD_DIR)/%/.stamp_built::
 $(BUILD_DIR)/%/.stamp_host_installed:
 	@$(call step_start,install-host)
 	@$(call MESSAGE,"Installing to host directory")
+	@mkdir -p $(HOST_DIR)
 	$(foreach hook,$($(PKG)_PRE_INSTALL_HOOKS),$(call $(hook))$(sep))
 	+$($(PKG)_INSTALL_CMDS)
 	$(foreach hook,$($(PKG)_POST_INSTALL_HOOKS),$(call $(hook))$(sep))

@@ -267,6 +269,7 @@ $(BUILD_DIR)/%/.stamp_host_installed:
 $(BUILD_DIR)/%/.stamp_staging_installed:
 	@$(call step_start,install-staging)
 	@$(call MESSAGE,"Installing to staging directory")
+	@mkdir -p $(STAGING_DIR)
 	$(foreach hook,$($(PKG)_PRE_INSTALL_STAGING_HOOKS),$(call $(hook))$(sep))
 	+$($(PKG)_INSTALL_STAGING_CMDS)
 	$(foreach hook,$($(PKG)_POST_INSTALL_STAGING_HOOKS),$(call $(hook))$(sep))

@@ -298,6 +301,7 @@ $(BUILD_DIR)/%/.stamp_staging_installed:
 # Install to images dir
 $(BUILD_DIR)/%/.stamp_images_installed:
 	@$(call step_start,install-image)
+	@mkdir -p $(BINARIES_DIR)
 	$(foreach hook,$($(PKG)_PRE_INSTALL_IMAGES_HOOKS),$(call $(hook))$(sep))
 	@$(call MESSAGE,"Installing to images directory")
 	+$($(PKG)_INSTALL_IMAGES_CMDS)

@@ -309,6 +313,7 @@ $(BUILD_DIR)/%/.stamp_images_installed:
 $(BUILD_DIR)/%/.stamp_target_installed:
 	@$(call step_start,install-target)
 	@$(call MESSAGE,"Installing to target")
+	@mkdir -p $(TARGET_DIR)
 	$(foreach hook,$($(PKG)_PRE_INSTALL_TARGET_HOOKS),$(call $(hook))$(sep))
 	+$($(PKG)_INSTALL_TARGET_CMDS)
 	$(if $(BR2_INIT_SYSTEMD),\

@@ -735,7 +740,7 @@ $$($(2)_TARGET_BUILD):	$$($(2)_TARGET_CONFIGURE)
 $(1)-configure:			$$($(2)_TARGET_CONFIGURE)
 $$($(2)_TARGET_CONFIGURE):	| $$($(2)_FINAL_DEPENDENCIES)
 
-$$($(2)_TARGET_SOURCE) $$($(2)_TARGET_RSYNC): | dirs prepare
+$$($(2)_TARGET_SOURCE) $$($(2)_TARGET_RSYNC): | prepare
 $$($(2)_TARGET_SOURCE) $$($(2)_TARGET_RSYNC): | dependencies
 
 ifeq ($$($(2)_OVERRIDE_SRCDIR),)

Signature · 1 line

2.19.1

Quoted 1 line

Quoted 1 line

Quoted 1 line

Quoted 1 line

"Thomas" == Thomas Petazzoni <[email protected]> writes:

In the current code, the creation of the main output directories (BUILD_DIR, STAGING_DIR, HOST_DIR, TARGET_DIR, etc.) is done by a global "dirs" target. While this works fine in the current situation, it doesn't work well in a context where per-package host and target directories are used.

For example, with the current code and per-package host directories, the output/staging symbolic link ends up being created as a link to the per-package package sysroot directory of the first package being built, instead of the global sysroot.

This commit reworks the creation of those directories by having the package/pkg-generic.mk code ensure that the build directory, target directory, host directory, staging directory and binaries directory exist before they are needed.

Two new targets, host-finalize and staging-finalize are added in the main Makefile to create the compatibility symlinks for host and staging directories. They will be extended later with additional logic for per-package directories.

Thanks to those changes, the global "dirs" target is entirely removed.

Signed-off-by Thomas Petazzoni <[email protected]>

Quoted 1 line

Quoted 1 line

Quoted 1 line

Quoted 1 line

"Thomas" == Thomas Petazzoni <[email protected]> writes:

In a follow-up commit, we will make the .NOTPARALLEL statement conditional on a Config.in option, so we need to move it further down.

Signed-off-by Thomas Petazzoni <[email protected]>

Reviewed-by "Yann E. MORIN" <[email protected]>

Quoted 1 line

Quoted 1 line

Quoted 1 line

Quoted 1 line

"Thomas" == Thomas Petazzoni <[email protected]> writes:

In commit 7e9870ce32d6329d9e3d602247fbe1709a2275a4 ("core: introduce intermediate BASE_TARGET_DIR variable"), the definition of TARGET_DIR_WARNING_FILE was changed to use $(BASE_TARGET_DIR) instead of $(TARGET_DIR).

However, this change is incompatible with per-package directories, and is in fact not needed.

With per-package directories, using $(BASE_TARGET_DIR) means that TARGET_DIR_WARNING_FILE is output/target/THIS_IS_NOT_YOUR_ROOT_FILESYSTEM. Due to this, when skeleton-init-common or skeleton-custom attempt to install it, it fails, because it should be installed to their package per-package target directory, and not the global output/target directory that doesn't exist yet. The failure looks like this:

/usr/bin/install -m 0644 support/misc/target-dir-warning.txt /home/thomas/projets/buildroot/output/target/THIS_IS_NOT_YOUR_ROOT_FILESYSTEM /usr/bin/install: cannot create regular file '/home/thomas/projets/buildroot/output/target/THIS_IS_NOT_YOUR_ROOT_FILESYSTEM': No such file or directory make[1]: *** [package/pkg-generic.mk:336: /home/thomas/projets/buildroot/output/build/skeleton-init-common/.stamp_target_installed] Error 1

TARGET_DIR_WARNING_FILE is used in three places:

- In skeleton-custom.mk and skeleton-init-common.mk, where as explained above, using $(TARGET_DIR) fixes the use of $(TARGET_DIR_WARNING_FILE) in the context of per-package target directories.

- In fs/common.mk, where it is used as argument to $(notdir ...) to retrieve just the name of the warning file. So in this case, we really don't care about the path of the file, just its name.

Signed-off-by Thomas Petazzoni <[email protected]>

Acked-by "Yann E. MORIN" <[email protected]>

This commit adjusts the logic in pkg-generic.mk that tweaks the *-config shell scripts installed by various libraries to make it compatible with per-package directories.

Signed-off-by Thomas Petazzoni <[email protected]>

Reviewed-by "Yann E. MORIN" <[email protected]>

---
 package/pkg-generic.mk | 7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)

package/pkg-generic.mk +4 -3

index 309fd8cd48..9b4db845b6 100644
@@ -275,12 +275,13 @@ $(BUILD_DIR)/%/.stamp_staging_installed:
 	$(foreach hook,$($(PKG)_POST_INSTALL_STAGING_HOOKS),$(call $(hook))$(sep))
 	$(Q)if test -n "$($(PKG)_CONFIG_SCRIPTS)" ; then \
 		$(call MESSAGE,"Fixing package configuration files") ;\
-			$(SED)  "s,$(BASE_DIR), at BASE_DIR@,g" \
-				-e "s,$(STAGING_DIR), at STAGING_DIR@,g" \
+			$(SED)  "s,$(HOST_DIR), at HOST_DIR@,g" \
+				-e "s,$(BASE_DIR), at BASE_DIR@,g" \
 				-e "s,^\(exec_\)\?prefix=.*,\1prefix=@STAGING_DIR@/usr,g" \
 				-e "s,-I/usr/,-I at STAGING_DIR@/usr/,g" \
 				-e "s,-L/usr/,-L at STAGING_DIR@/usr/,g" \
-				-e "s, at STAGING_DIR@,$(STAGING_DIR),g" \
+				-e 's, at STAGING_DIR@,$$(dirname $$0)/../..,g' \
+				-e 's, at HOST_DIR@,$$(dirname $$0)/../../../..,g' \
 				-e "s, at BASE_DIR@,$(BASE_DIR),g" \
 				$(addprefix $(STAGING_DIR)/usr/bin/,$($(PKG)_CONFIG_SCRIPTS)) ;\
 	fi

Signature · 1 line

2.19.1

Quoted 1 line

Quoted 1 line

Quoted 1 line

Quoted 1 line

"Thomas" == Thomas Petazzoni <[email protected]> writes:

This commit adjusts the logic in pkg-generic.mk that tweaks the *-config shell scripts installed by various libraries to make it compatible with per-package directories.

This requires two fixes:

- replacing $(STAGING_DIR) with a relative path from the config script to the staging directory, rather than using an absolute path of the staging directory.

Without this, a *-config script provided by package A, but called from package B per-package directory will return paths from package A per-package directory:

   $ ./output/per-package/mcrypt/host/usr/<tuple>/sysroot/usr/bin/libmcrypt-config --libs
   -L..../output/per-package/libmcrypt/host/usr/<tuple>/sysroot/usr/lib/

The libmcrypt-config script is installed by the libmcrypt package, and mcrypt is a package that depends on libmcrypt. When we call the libmcrypt-config script from the mcrypt per-package directory, it returns a -L flag that points to the libmcrypt per-package directory.

One might say: but this is OK, since the sysroot of the libmcrypt per-package directory also contains the libmcrypt library. This is true, but we encounter a more subtle issue: because -L paths are considered before standard paths, ld ends up finding libc.so in the libmcrypt per-package directory. This libc.so file is a linker script that looks like this:

GROUP ( /lib/libc.so.6 /usr/lib/libc_nonshared.a AS_NEEDED ( /lib/ld-linux.so.3 ) )

Normally, thanks to ld sysroot awareness, /lib/libc.so.6 in this script is re-interpreted according to the sysroot. But in this case, the library is *outside* the compiler sysroot. Remember: we are using the compiler/linker from the "mcrypt" per-package directory, but we found "libc.so.6" in the "libmcrypt" per-package directory.

   This causes the linker to really use the /lib/libc.so.6 from the
   host machine, obvisouly leading to a build failure such as:

   output/per-package/libgcrypt/host/opt/ext-toolchain/bin/../lib/gcc/nios2-linux-gnu/7.3.1/../../../../nios2-linux-gnu/bin/ld: cannot find /lib/libc.so.6
   output/per-package/libgcrypt/host/opt/ext-toolchain/bin/../lib/gcc/nios2-linux-gnu/7.3.1/../../../../nios2-linux-gnu/bin/ld: cannot find /usr/lib/libc_nonshared.a
   output/per-package/libgcrypt/host/opt/ext-toolchain/bin/../lib/gcc/nios2-linux-gnu/7.3.1/../../../../nios2-linux-gnu/bin/ld: cannot find /lib/ld-linux-nios2.so.1

- Some *-config scripts, such as the apr-1-config script, contain references to host tools:

CC=".../output/per-package/apr/hosr/bin/arm-linux-gcc" CCP=".../output/per-package/apr/hosr/bin/arm-linux-cpp"

We also want to replace those with proper relative paths. To achieve this, we need to also replace $(HOST_DIR) with a relative path. Since $(STAGING_DIR) is inside $(HOST_DIR), the first replacement of $(STAGING_DIR) by @STAGING_DIR@ is no longer needed:
replacing $(HOST_DIR) by @HOST_DIR@ is sufficient. We still need to replace @STAGING_DIR@ by the proper path though, as we introduce @STAGING_DIR@ references in exec_prefix and prefix variables, as well as -I and -L flags.

Signed-off-by Thomas Petazzoni <[email protected]>

Reviewed-by "Yann E. MORIN" <[email protected]>

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.

Signed-off-by Thomas Petazzoni <[email protected]>

---

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.

- We can support top-level parallel build properly, because a package only "sees" its own host directory and target directory, isolated from the build of other packages that can happen in parallel.

It works as follows:

- A new output/per-package/ directory is created, which will contain one sub-directory per package, and inside it, a "host" directory and a "target" directory:

output/per-package/busybox/target
output/per-package/busybox/host
output/per-package/host-fakeroot/target
output/per-package/host-fakeroot/host

This output/per-package/ directory is PER_PACKAGE_DIR.

- The global TARGET_DIR and HOST_DIR variable now automatically point to the per-package directory when PKG is defined. So whenever a package references $(HOST_DIR) or $(TARGET_DIR) in its build process, it effectively references the per-package host/target directories. Note that STAGING_DIR is a sub-dir of HOST_DIR, so it is handled as well.

- Of course, packages have dependencies, so those dependencies must be installed in the per-package host and target directories. To do so, we simply rsync (using hard links to save space and time) the host and target directories of the direct dependencies of the package to the current package host and target directories.

We only need to take care of direct dependencies (and not recursively all dependencies), because we accumulate into those per-package host and target directories the files installed by the dependencies. Note that this only works because we make the assumption that one package does *not* overwrite files installed by another package.

This is done for "extract dependencies" at the beginning of the extract step, and for "normal dependencies" at the beginning of the configure step.

This is basically enough to make per-package SDK and target work. The only gotcha is that at the end of the build, output/target and output/host are empty, which means that:

- The filesystem image creation code cannot work.

- We don't have a SDK to build code outside of Buildroot.

In order to fix this, this commit extends the target-finalize step so that it starts by populating output/target and output/host by rsync-ing into them the target and host directories of all packages listed in the $(PACKAGES) variable. It is necessary to do this sequentially in the target-finalize step and not in each package. Doing it in package installation means that it can be done in parallel. In that case, there is a chance that two rsyncs are creating the same hardlink or directory at the same time, which makes one of them fail.

Also, with this change to per-package directories, binaries built for the host may contain RPATH pointing to per-package directories. This commit therefore teaches the check-host-rpath script with the fact that RPATH pointing to a per-package directory are OK.

Signed-off-by Thomas Petazzoni <[email protected]>

---
 Config.in                        | 18 ++++++++++++++++
 Makefile                         | 36 +++++++++++++++++++++++++++-----
 package/pkg-generic.mk           |  5 ++++-
 package/pkg-utils.mk             | 21 +++++++++++++++++++
 support/scripts/check-host-rpath |  5 ++++-
 5 files changed, 78 insertions(+), 7 deletions(-)

Config.in +18 -0

index f965e9d6d8..2d8a07fda7 100644
@@ -696,6 +696,24 @@ config BR2_REPRODUCIBLE
 	  This is labeled as an experimental feature, as not all
 	  packages behave properly to ensure reproducibility.
 
+config BR2_PER_PACKAGE_DIRECTORIES
+	bool "Use per-package directories (experimental)"
+	help
+	  This option will change the build process of Buildroot
+	  package to use per-package target and host directories.
+
+	  This is useful for two related purposes:
+
+	    - Cleanly isolate the build of each package, so that a
+	      given package only "sees" the dependencies it has
+	      explicitly expressed, and not other packages that may
+	      have by chance been built before.
+
+	    - Enable top-level parallel build.
+
+	  This is labeled as an experimental feature, as not all
+	  packages behave properly with per-package directories.
+
 endmenu
 
 comment "Security Hardening Options"

Makefile +0 -0

index 23032988a5..35fe1b3644 100644

Makefile +5 -0

+
 # timezone and locale may affect build output
 ifeq ($(BR2_REPRODUCIBLE),y)
 export TZ = UTC

@@ -455,7 +462,11 @@ LZCAT := $(call qstrip,$(BR2_LZCAT))
 TAR_OPTIONS = $(call qstrip,$(BR2_TAR_OPTIONS)) -xf
 
 # packages compiled for the host go here
+ifeq ($(BR2_PER_PACKAGE_DIRECTORIES),y)
+HOST_DIR = $(if $(PKG),$(PER_PACKAGE_DIR)/$($(PKG)_NAME)/host,$(call qstrip,$(BR2_HOST_DIR)))
+else
 HOST_DIR := $(call qstrip,$(BR2_HOST_DIR))
+endif
 
 ifneq ($(HOST_DIR),$(BASE_DIR)/host)
 HOST_DIR_SYMLINK = $(BASE_DIR)/host

@@ -701,14 +712,28 @@ $(TARGETS_ROOTFS): target-finalize
 # Avoid the rootfs name leaking down the dependency chain

Makefile +2 -1

+endif
 
 .PHONY: staging-finalize
 staging-finalize:
 	@ln -snf $(STAGING_DIR) $(BASE_DIR)/staging
 
 .PHONY: target-finalize
-target-finalize: $(PACKAGES) host-finalize
+target-finalize: $(PACKAGES) $(TARGET_DIR) host-finalize

support/scripts/check-host-rpath +2 -0

 check_elf_has_rpath() {
     local file="${1}"
     local hostdir="${2}"
+    local perpackagedir="${3}"
     local rpath dir
 
     while read rpath; do

@@ -65,6 +67,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
         done
     done < <( readelf -d "${file}"                                              \
               |sed -r -e '/.* \(R(UN)?PATH\) +Library r(un)?path: \[(.+)\]$/!d' \

Quoted 6 lines

We only need to take care of direct dependencies (and not recursively all dependencies), because we accumulate into those per-package host and target directories the files installed by the dependencies. Note that this only works because we make the assumption that one package does *not* overwrite files installed by another package.

So that means that if BR2_PER_PACKAGE_DIRECTORIES=y, check-uniq-files should
give errors instead of warnings, no?

Quoted 7 lines

comment "Security Hardening Options"

Makefile +0 -0

index 23032988a5..35fe1b3644 100644
@@ -207,7 +207,8 @@ BINARIES_DIR := $(BASE_DIR)/images
 # The target directory is common to all packages,

Makefile BINARIES_DIR := $(BASE_DIR)/images · line 210

Quoted 4 lines

Makefile +1 -1

 # but there is one that is specific to each filesystem.
 BASE_TARGET_DIR := $(BASE_DIR)/target
-TARGET_DIR = $(if $(ROOTFS),$(ROOTFS_$(ROOTFS)_TARGET_DIR),$(BASE_TARGET_DIR))
+PER_PACKAGE_DIR := $(BASE_DIR)/per-package  

Perhaps it would be nice to add a preparatory patch that reorders the
definitions of BASE_TARGET_DIR, TARGET_DIR, STAGING_DIR and HOST_DIR so they are all together, and can be in a single BR2_PER_PACKAGE_DIRECTORIES condition.

Quoted 7 lines

Makefile +0 -0

 # initial definition so that 'make clean' works for most users, even without
 # .config. HOST_DIR will be overwritten later when .config is included.
 HOST_DIR := $(BASE_DIR)/host

@@ -246,6 +247,12 @@ endif
 #      make -j$((`getconf _NPROCESSORS_ONLN`+1))
 .NOTPARALLEL:
 

Makefile endif · line 254

Quoted 5 lines

Makefile +5 -0

+ifeq ($(BR2_PER_PACKAGE_DIRECTORIES),y)
+TARGET_DIR = $(if $(ROOTFS),$(ROOTFS_$(ROOTFS)_TARGET_DIR),$(if $(PKG),$(PER_PACKAGE_DIR)/$($(PKG)_NAME)/target,$(BASE_TARGET_DIR)))
+else
+TARGET_DIR = $(if $(ROOTFS),$(ROOTFS_$(ROOTFS)_TARGET_DIR),$(BASE_TARGET_DIR))
+endif  

Was there any reason to move the definition of TARGET_DIR?

Makefile $(TARGETS_ROOTFS): target-finalize · line 721

Quoted 8 lines

Makefile +7 -1

-host-finalize: $(HOST_DIR_SYMLINK)
+host-finalize: $(PACKAGES) $(HOST_DIR) $(HOST_DIR_SYMLINK)
+ifeq ($(BR2_PER_PACKAGE_DIRECTORIES),y)	
+	@$(call MESSAGE,"Creating global host directory")
+	$(foreach pkg,$(sort $(PACKAGES)),\
+		rsync -a --link-dest=$(PER_PACKAGE_DIR)/$(pkg)/host/ \
+		$(PER_PACKAGE_DIR)/$(pkg)/host/ \
+		$(HOST_DIR)$(sep))  

Don't you think it's worth factoring this into a pkg-utils macro? I'm thinking:

# rsync-per-package(packages,type,destdir)

# copy every per-package host or target dir (as determined by type) for packages
# (a list of packages) into destdir.
# copying is done by making hardlinks using rsync
define rsync-per-package
	$(foreach pkg,$(sort $(1)),\
		rsync -a --link-dest=$(PER_PACKAGE_DIR)/$(pkg)/$(2)/ \
		$(PER_PACKAGE_DIR)/$(pkg)/$(2)/ \
		$(3)$(sep))
endef

Makefile $(TARGETS_ROOTFS): target-finalize · line 739

Quoted 3 lines

Makefile +0 -0

 	@$(call MESSAGE,"Finalizing target directory")
 	# Check files that are touched by more than one package
 	./support/scripts/check-uniq-files -t target $(BUILD_DIR)/packages-file-list.txt  

Side note: if we make check-uniq-files return errors, we should probably also
move it to a per package step so the autobuilders can identify the failing package. But that can be done later.

Quoted 8 lines

# Remove duplicate and trailing '/' for proper match

Makefile +1 -1

@@ -20,7 +21,7 @@ main() {
     while read file; do
         is_elf "${file}" || continue
         elf_needs_rpath "${file}" "${hostdir}" || continue
-        check_elf_has_rpath "${file}" "${hostdir}" && continue
+        check_elf_has_rpath "${file}" "${hostdir}" "${perpackagedir}" && continue
         if [ ${ret} -eq 0 ]; then

Makefile elf_needs_rpath() {

Quoted 3 lines

Makefile +0 -0

             ret=1
             printf "***\n"

@@ -57,6 +58,7 @@ elf_needs_rpath() {  

I think there should be some explanation why elf_needs_rpath doesn't need to
check all per-package directories. Basically it's because any library that the executable may depend on must already have been rsynced into ${host_dir} which is the per-package host directory.

Hello Arnout,

Thanks for the review.

Quoted 7 lines · Arnout Vandecappelle

Quoted 6 lines

We only need to take care of direct dependencies (and not recursively all dependencies), because we accumulate into those per-package host and target directories the files installed by the dependencies. Note that this only works because we make the assumption that one package does *not* overwrite files installed by another package.

Quoted 2 lines

So that means that if BR2_PER_PACKAGE_DIRECTORIES=y, check-uniq-files should
give errors instead of warnings, no?

Possibly yes. But even as of today, check-uniq-files shows warnings for legitimate things that have been overwritten, such as .la files. So even if having check-uniq-files error out if files are overwritten is a desirable long-term goal, I'm not sure it's going to be reasonable to achieve this goal as part of this per-package SDK/target series.

Quoted 16 lines

Makefile BINARIES_DIR := $(BASE_DIR)/images · line 210

Quoted 12 lines

comment "Security Hardening Options"

Makefile +1 -1

index 23032988a5..35fe1b3644 100644
@@ -207,7 +207,8 @@ BINARIES_DIR := $(BASE_DIR)/images
 # The target directory is common to all packages,
 # but there is one that is specific to each filesystem.
 BASE_TARGET_DIR := $(BASE_DIR)/target
-TARGET_DIR = $(if $(ROOTFS),$(ROOTFS_$(ROOTFS)_TARGET_DIR),$(BASE_TARGET_DIR))
+PER_PACKAGE_DIR := $(BASE_DIR)/per-package  

Perhaps it would be nice to add a preparatory patch that reorders the
definitions of BASE_TARGET_DIR, TARGET_DIR, STAGING_DIR and HOST_DIR so they are all together, and can be in a single BR2_PER_PACKAGE_DIRECTORIES condition.

I'll have a look at what can I do, but the ordering of those variables is a bit messy/complicated.

Quoted 7 lines

Quoted 7 lines

Makefile +0 -0

 # initial definition so that 'make clean' works for most users, even without
 # .config. HOST_DIR will be overwritten later when .config is included.
 HOST_DIR := $(BASE_DIR)/host

@@ -246,6 +247,12 @@ endif
 #      make -j$((`getconf _NPROCESSORS_ONLN`+1))
 .NOTPARALLEL:
 

Quoted 7 lines

Makefile endif · line 254

Quoted 5 lines

Makefile +5 -0

+ifeq ($(BR2_PER_PACKAGE_DIRECTORIES),y)
+TARGET_DIR = $(if $(ROOTFS),$(ROOTFS_$(ROOTFS)_TARGET_DIR),$(if $(PKG),$(PER_PACKAGE_DIR)/$($(PKG)_NAME)/target,$(BASE_TARGET_DIR)))
+else
+TARGET_DIR = $(if $(ROOTFS),$(ROOTFS_$(ROOTFS)_TARGET_DIR),$(BASE_TARGET_DIR))
+endif  

Was there any reason to move the definition of TARGET_DIR?

Yes: TARGET_DIR was defined prior to the .config file being included. Now that its definition depends on BR2_PER_PACKAGE_DIRECTORIES, we need to move the TARGET_DIR definition after the .config file is included.

Quoted 12 lines

Makefile $(TARGETS_ROOTFS): target-finalize · line 721

Quoted 8 lines

Makefile +7 -1

-host-finalize: $(HOST_DIR_SYMLINK)
+host-finalize: $(PACKAGES) $(HOST_DIR) $(HOST_DIR_SYMLINK)
+ifeq ($(BR2_PER_PACKAGE_DIRECTORIES),y)	
+	@$(call MESSAGE,"Creating global host directory")
+	$(foreach pkg,$(sort $(PACKAGES)),\
+		rsync -a --link-dest=$(PER_PACKAGE_DIR)/$(pkg)/host/ \
+		$(PER_PACKAGE_DIR)/$(pkg)/host/ \
+		$(HOST_DIR)$(sep))  

Don't you think it's worth factoring this into a pkg-utils macro? I'm thinking:

# rsync-per-package(packages,type,destdir)

Quoted 9 lines

# copy every per-package host or target dir (as determined by type) for packages
# (a list of packages) into destdir.
# copying is done by making hardlinks using rsync
define rsync-per-package
	$(foreach pkg,$(sort $(1)),\
		rsync -a --link-dest=$(PER_PACKAGE_DIR)/$(pkg)/$(2)/ \
		$(PER_PACKAGE_DIR)/$(pkg)/$(2)/ \
		$(3)$(sep))
endef

Perhaps I could re-use the prepare-per-package-directory macro already in package/pkg-utils.mk, which is currently defined as:

ifeq ($(BR2_PER_PACKAGE_DIRECTORIES),y)
define prepare-per-package-directory

        mkdir -p $(HOST_DIR) $(TARGET_DIR)
        $(foreach pkg,$(1),\
                rsync -a --link-dest=$(PER_PACKAGE_DIR)/$(pkg)/host/ \
                $(PER_PACKAGE_DIR)/$(pkg)/host/ \
                $(HOST_DIR)$(sep))
        $(foreach pkg,$(1),\
                rsync -a --link-dest=$(PER_PACKAGE_DIR)/$(pkg)/target/ \
                $(PER_PACKAGE_DIR)/$(pkg)/target/ \
                $(TARGET_DIR)$(sep))

endef
endif

This macro is for now used to prepare the per-package directories during the build, but it could also be used to prepare the global directories at the end of the build. The only thing that we need is to separate it into two macros that do the host and target separately, since for the global directories, these are done in host-finalize and target-finalize respectively.

Quoted 8 lines

Makefile $(TARGETS_ROOTFS): target-finalize · line 739

Quoted 4 lines

Makefile +0 -0

 	@$(call MESSAGE,"Finalizing target directory")
 	# Check files that are touched by more than one package
 	./support/scripts/check-uniq-files -t target $(BUILD_DIR)/packages-file-list.txt  

Side note: if we make check-uniq-files return errors, we should probably also
move it to a per package step so the autobuilders can identify the failing package. But that can be done later.

Yes, let's not do everything in this series please :)

Quoted 17 lines

Makefile elf_needs_rpath() {

Quoted 12 lines

# Remove duplicate and trailing '/' for proper match

Makefile +1 -1

@@ -20,7 +21,7 @@ main() {
     while read file; do
         is_elf "${file}" || continue
         elf_needs_rpath "${file}" "${hostdir}" || continue
-        check_elf_has_rpath "${file}" "${hostdir}" && continue
+        check_elf_has_rpath "${file}" "${hostdir}" "${perpackagedir}" && continue
         if [ ${ret} -eq 0 ]; then
             ret=1
             printf "***\n"

@@ -57,6 +58,7 @@ elf_needs_rpath() {  

I think there should be some explanation why elf_needs_rpath doesn't need to
check all per-package directories. Basically it's because any library that the executable may depend on must already have been rsynced into ${host_dir} which is the per-package host directory.

It seems pretty obvious to me, no?

I mean the per-package host directory

contains all the stuff installed by the dependencies of the package, so it obviously contains all the necessary libraries.

But fair enough: where do you want to see this explanation ? In the commit log ? As a comment in the script itself ?

Once again, thanks for the review!

Thomas

Quoted 5 lines

Possibly yes. But even as of today, check-uniq-files shows warnings for legitimate things that have been overwritten, such as .la files. So even if having check-uniq-files error out if files are overwritten is a desirable long-term goal, I'm not sure it's going to be reasonable to achieve this goal as part of this per-package SDK/target series.

True. But on the other hand, the idea of this experimental, user-configurable
feature is that we get some time to debug it. That means we have to detect bugs, so we need to do this.

So, mark it as a side note as one of those things that still have to be done.

Quoted 6 lines

Quoted 3 lines

Perhaps it would be nice to add a preparatory patch that reorders the
definitions of BASE_TARGET_DIR, TARGET_DIR, STAGING_DIR and HOST_DIR so they are all together, and can be in a single BR2_PER_PACKAGE_DIRECTORIES condition.

I'll have a look at what can I do, but the ordering of those variables is a bit messy/complicated.

I think the only ordering constraint before this patch is HOST_DIR, which gets
defined twice (once before and once after including .config). The other ones don't have any complication, do they?

Quoted 3 lines

Yes: TARGET_DIR was defined prior to the .config file being included. Now that its definition depends on BR2_PER_PACKAGE_DIRECTORIES, we need to move the TARGET_DIR definition after the .config file is included.

All the more reason to move it in a separate patch :-)

I think the place where now HOST_DIR is defined a second time would be a good
location for doing this. TARGET_DIR should not be used anywhere outside of the BR2_HAVE_DOT_CONFIG condition (to be double-checked) - it should be BASE_TARGET_DIR instead, like is the case in the clean target.

Quoted 39 lines

Quoted 12 lines

Makefile $(TARGETS_ROOTFS): target-finalize · line 721

Quoted 8 lines

Makefile +7 -1

-host-finalize: $(HOST_DIR_SYMLINK)
+host-finalize: $(PACKAGES) $(HOST_DIR) $(HOST_DIR_SYMLINK)
+ifeq ($(BR2_PER_PACKAGE_DIRECTORIES),y)	
+	@$(call MESSAGE,"Creating global host directory")
+	$(foreach pkg,$(sort $(PACKAGES)),\
+		rsync -a --link-dest=$(PER_PACKAGE_DIR)/$(pkg)/host/ \
+		$(PER_PACKAGE_DIR)/$(pkg)/host/ \
+		$(HOST_DIR)$(sep))    

Don't you think it's worth factoring this into a pkg-utils macro? I'm thinking:

# rsync-per-package(packages,type,destdir)

Quoted 9 lines

# copy every per-package host or target dir (as determined by type) for packages
# (a list of packages) into destdir.
# copying is done by making hardlinks using rsync
define rsync-per-package
	$(foreach pkg,$(sort $(1)),\
		rsync -a --link-dest=$(PER_PACKAGE_DIR)/$(pkg)/$(2)/ \
		$(PER_PACKAGE_DIR)/$(pkg)/$(2)/ \
		$(3)$(sep))
endef  

Perhaps I could re-use the prepare-per-package-directory macro already in package/pkg-utils.mk, which is currently defined as:

ifeq ($(BR2_PER_PACKAGE_DIRECTORIES),y)
define prepare-per-package-directory

        mkdir -p $(HOST_DIR) $(TARGET_DIR)
        $(foreach pkg,$(1),\
                rsync -a --link-dest=$(PER_PACKAGE_DIR)/$(pkg)/host/ \
                $(PER_PACKAGE_DIR)/$(pkg)/host/ \
                $(HOST_DIR)$(sep))
        $(foreach pkg,$(1),\
                rsync -a --link-dest=$(PER_PACKAGE_DIR)/$(pkg)/target/ \
                $(PER_PACKAGE_DIR)/$(pkg)/target/ \
                $(TARGET_DIR)$(sep))

endef
endif

Quoted 6 lines

This macro is for now used to prepare the per-package directories during the build, but it could also be used to prepare the global directories at the end of the build. The only thing that we need is to separate it into two macros that do the host and target separately, since for the global directories, these are done in host-finalize and target-finalize respectively.

Well, yes, that's the same indeed... But you'll still want the
prepare-per-package-directory macro, I think, otherwise you'd have to call the inner macro twice everywhere (admittedly, "everywhere" is just in 2 places, or 3 if we add it to kconfig as well).

Quoted 5 lines

Quoted 3 lines

Side note: if we make check-uniq-files return errors, we should probably also
move it to a per package step so the autobuilders can identify the failing package. But that can be done later.

Yes, let's not do everything in this series please :)

Absolutely, but I guess you have a todo list somewhere, no?

Quoted 6 lines

Quoted 4 lines

I think there should be some explanation why elf_needs_rpath doesn't need to
check all per-package directories. Basically it's because any library that the executable may depend on must already have been rsynced into ${host_dir} which is the per-package host directory.

It seems pretty obvious to me, no?

I admit it was pretty late already, but still, it took me more than 10 minutes
to realize that.

Quoted 1 line

I mean the per-package host directory

That's the first problem already: it's hard to realize that HOST_DIR/host_dir
really is the per-package host directory. But I guess that that's a matter of being used to it.

That reminds me: perhaps a paragraph should be added to
docs/manual/adding-packages-generic.txt to explain that HOST_DIR, STAGING_DIR and TARGET_DIR point to the per-package directory when used within a package.

Quoted 2 lines

contains all the stuff installed by the dependencies of the package, so it obviously contains all the necessary libraries.

That's the second reason why it's not so obvious. When executable A links with
library B, it will typically have an rpath to the per-package directory of B.

It's not directly obvious that you can just as well check for the presence of the library in the per-package of A to know that it needs an rpath to the per-package of B.

Quoted 2 lines

But fair enough: where do you want to see this explanation ? In the commit log ? As a comment in the script itself ?

I think in the script itself. It could use a little bit more explanation in its
overall comment at the beginning of the file even without this patch.

The check-host-rpath script is not checking for the presence of libraries.

Nowhere it verifies that the NEEDED fields in host binaries match libraries found in the RPATH listed in the binaries.

This commit implements the core of the move to per-package SDK and

Makefile +9 -2

-host-finalize: $(HOST_DIR_SYMLINK)
+host-finalize: $(PACKAGES) $(HOST_DIR) $(HOST_DIR_SYMLINK)
+ifeq ($(BR2_PER_PACKAGE_DIRECTORIES),y)
+	@$(call MESSAGE,"Creating global host directory")
+	$(foreach pkg,$(sort $(PACKAGES)),\
+		rsync -a --link-dest=$(PER_PACKAGE_DIR)/$(pkg)/host/ \
+		$(PER_PACKAGE_DIR)/$(pkg)/host/ \
+		$(HOST_DIR)$(sep))
+endif
 
  .PHONY: staging-finalize
  staging-finalize:
  	@ln -snf $(STAGING_DIR) $(BASE_DIR)/staging
 
  .PHONY: target-finalize
-target-finalize: $(PACKAGES) host-finalize
+target-finalize: $(PACKAGES) $(TARGET_DIR) host-finalize

Makefile +3 -2

@@ -972,7 +997,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):
  	@mkdir -p $@
 
  # outputmakefile generates a Makefile in the output directory, if using a

@@ -1011,7 +1037,7 @@ printvars:
  clean:
  	rm -rf $(BASE_TARGET_DIR) $(BINARIES_DIR) $(HOST_DIR) $(HOST_DIR_SYMLINK) \
  		$(BUILD_DIR) $(BASE_DIR)/staging \
-		$(LEGAL_INFO_DIR) $(GRAPHS_DIR)
+		$(LEGAL_INFO_DIR) $(GRAPHS_DIR) $(PER_PACKAGE_DIR)
 
  .PHONY: distclean
  distclean: clean

package/pkg-generic.mk +4 -1

index 9b4db845b6..b1f0cdf34a 100644
@@ -98,7 +98,7 @@ GLOBAL_INSTRUMENTATION_HOOKS += check_bin_arch
  # have a proper DT_RPATH or DT_RUNPATH tag
  define check_host_rpath
  	$(if $(filter install-host,$(2)),\
-		$(if $(filter end,$(1)),support/scripts/check-host-rpath $(3) $(HOST_DIR)))
+		$(if $(filter end,$(1)),support/scripts/check-host-rpath $(3) $(HOST_DIR) $(PER_PACKAGE_DIR)))
  endef
  GLOBAL_INSTRUMENTATION_HOOKS += check_host_rpath
 

@@ -133,6 +133,7 @@ endif
  # Retrieve the archive
  $(BUILD_DIR)/%/.stamp_downloaded:
  	@$(call step_start,download)
+	$(call prepare-per-package-directory,$($(PKG)_FINAL_DOWNLOAD_DEPENDENCIES))
  	$(foreach hook,$($(PKG)_PRE_DOWNLOAD_HOOKS),$(call $(hook))$(sep))
  # Only show the download message if it isn't already downloaded
  	$(Q)for p in $($(PKG)_ALL_DOWNLOADS); do \

@@ -159,6 +160,7 @@ $(BUILD_DIR)/%/.stamp_actual_downloaded:
  $(BUILD_DIR)/%/.stamp_extracted:
  	@$(call step_start,extract)
  	@$(call MESSAGE,"Extracting")
+	$(call prepare-per-package-directory,$($(PKG)_FINAL_EXTRACT_DEPENDENCIES))
  	$(foreach hook,$($(PKG)_PRE_EXTRACT_HOOKS),$(call $(hook))$(sep))
  	$(Q)mkdir -p $(@D)
  	$($(PKG)_EXTRACT_CMDS)

@@ -219,6 +221,7 @@ $(foreach dir,$(call qstrip,$(BR2_GLOBAL_PATCH_DIR)),\
  $(BUILD_DIR)/%/.stamp_configured:
  	@$(call step_start,configure)
  	@$(call MESSAGE,"Configuring")
+	$(call prepare-per-package-directory,$($(PKG)_FINAL_DEPENDENCIES))
  	$(foreach hook,$($(PKG)_PRE_CONFIGURE_HOOKS),$(call $(hook))$(sep))
  	$($(PKG)_CONFIGURE_CMDS)
  	$(foreach hook,$($(PKG)_POST_CONFIGURE_HOOKS),$(call $(hook))$(sep))

package/pkg-utils.mk +21 -0

index bffd79dfb0..4ca2eebc95 100644
@@ -62,6 +62,27 @@ $$(error Package error: use $(2) instead of $(1). Please fix your .mk file)
  endif
  endef
 
+# This function prepares the per-package HOST_DIR and TARGET_DIR of
+# the current package, by rsync the host and target directories of the
+# dependencies of this package. The list of dependencies is passed as
+# argument, so that this function can be used to prepare with
+# different set of dependencies (download, extract, configure, etc.)
+#
+# $1: space-separated list of dependencies
+ifeq ($(BR2_PER_PACKAGE_DIRECTORIES),y)
+define prepare-per-package-directory
+	mkdir -p $(HOST_DIR) $(TARGET_DIR)
+	$(foreach pkg,$(1),\
+		rsync -a --link-dest=$(PER_PACKAGE_DIR)/$(pkg)/host/ \
+		$(PER_PACKAGE_DIR)/$(pkg)/host/ \
+		$(HOST_DIR)$(sep))
+	$(foreach pkg,$(1),\
+		rsync -a --link-dest=$(PER_PACKAGE_DIR)/$(pkg)/target/ \
+		$(PER_PACKAGE_DIR)/$(pkg)/target/ \
+		$(TARGET_DIR)$(sep))
+endef
+endif
+
  #
  # legal-info helper functions
  #

support/scripts/check-host-rpath +4 -1

index 6c5767da05..787a1763b0 100755
@@ -11,6 +11,7 @@ export LC_ALL=C
  main() {
      local pkg="${1}"
      local hostdir="${2}"
+    local perpackagedir="${3}"
      local file ret
 
      # Remove duplicate and trailing '/' for proper match

@@ -20,7 +21,7 @@ main() {
      while read file; do
          is_elf "${file}" || continue
          elf_needs_rpath "${file}" "${hostdir}" || continue
-        check_elf_has_rpath "${file}" "${hostdir}" && continue
+        check_elf_has_rpath "${file}" "${hostdir}" "${perpackagedir}" && continue
          if [ ${ret} -eq 0 ]; then
              ret=1
              printf "***\n"

@@ -57,6 +58,7 @@ elf_needs_rpath() {
  check_elf_has_rpath() {
      local file="${1}"
      local hostdir="${2}"
+    local perpackagedir="${3}"
      local rpath dir
 
      while read rpath; do

@@ -65,6 +67,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
          done
      done < <( readelf -d "${file}"                                              \
                |sed -r -e '/.* \(R(UN)?PATH\) +Library r(un)?path: \[(.+)\]$/!d' \

In an answer to v4 of this series I mentioned the issue I had with empty RPATH in some host-lib. After digging some more I found this happens during the prepare-sdk step (which I call in my wrapper script). Before this step, your suggested readelf output reports the correct pathes, however after, RPATH is empty. I compared this to a "standard" SDK, there RPATH is set to $ORIGIN/../lib or something similar.

Actually, an unrelated question arises: Why does the SDK need to build world, which includes all filesystem images, first? Should it not be sufficient to depend on $(PACKAGES) or now host-finalize only?

Hello Andreas,

Quoted 6 lines · Andreas Naumann

In an answer to v4 of this series I mentioned the issue I had with empty RPATH in some host-lib. After digging some more I found this happens during the prepare-sdk step (which I call in my wrapper script). Before this step, your suggested readelf output reports the correct pathes, however after, RPATH is empty. I compared this to a "standard" SDK, there RPATH is set to $ORIGIN/../lib or something similar.

So are you saying the issue still exists, and to reproduce, I simply need to do:

  $ make
  $ make prepare-sdk

of a build with BR2_PER_PACKAGE_DIRECTORIES=y ?

Quoted 4 lines

Actually, an unrelated question arises: Why does the SDK need to build world, which includes all filesystem images, first? Should it not be sufficient to depend on $(PACKAGES) or now host-finalize only?

Yes, it would be sufficient to build packages + host-finalization I believe.

Best regards,

Thomas

Hi,

Am 05.12.18 um 17:31 schrieb Thomas Petazzoni:

Quoted 14 lines

Hello Andreas,

Quoted 6 lines · Andreas Naumann

In an answer to v4 of this series I mentioned the issue I had with empty RPATH in some host-lib. After digging some more I found this happens during the prepare-sdk step (which I call in my wrapper script). Before this step, your suggested readelf output reports the correct pathes, however after, RPATH is empty. I compared this to a "standard" SDK, there RPATH is set to $ORIGIN/../lib or something similar.

So are you saying the issue still exists, and to reproduce, I simply need to do:

Quoted 4 lines

? $ make
? $ make prepare-sdk

of a build with BR2_PER_PACKAGE_DIRECTORIES=y ?

in an essence that's what I did. My setup has two externals and some custom defconfig, but that shouldnt matter since without the ppsh patches things work fine.

Quoted 8 lines

Quoted 5 lines

Actually, an unrelated question arises: Why does the SDK need to build world, which includes all filesystem images, first? Should it not be sufficient to depend on $(PACKAGES) or now host-finalize only?

Yes, it would be sufficient to build packages + host-finalization I believe.

Ok, I might submit a patch in this direction then..

regards,
Andreas

Quoted 5 lines

Best regards,

Thomas

_______________________________________________
buildroot mailing list
buildroot at busybox.net
http://lists.busybox.net/mailman/listinfo/buildroot

Just to make sure I just did the above using your ppsh-v6 branch with the wandboard_defconfig. The only additional change was to use the external linaro toolchain. After 'make'

   $ readelf -d output/host/sbin/mkfs.ext3
shows
   0x000000000000001d (RUNPATH)            Bibliothek runpath: 
[/local/gsrc/buildroot.upstream/output/per-package/host-e2fsprogs/host/lib]

which is already unexpected, shouldn't it show .../output/host/lib?

After 'make prepare-sdk'

   $ readelf -d output/host/sbin/mkfs.ext3
shows
   0x000000000000001d (RUNPATH)            Bibliothek runpath: []

Hello,

Quoted 3 lines · Andreas Naumann

Just to make sure I just did the above using your ppsh-v6 branch with the wandboard_defconfig. The only additional change was to use the external linaro toolchain. After 'make'

Thanks!

Quoted 6 lines

    $ readelf -d output/host/sbin/mkfs.ext3
shows
    0x000000000000001d (RUNPATH)            Bibliothek runpath:
[/local/gsrc/buildroot.upstream/output/per-package/host-e2fsprogs/host/lib]

which is already unexpected, shouldn't it show .../output/host/lib?

Nope, this is expected. During the build output/host/lib doesn't exist,

it's only populated at the very end of the build in host-finalize. So indeed the RPATH of binaries point to various per-package host/lib folders. This is totally expected.

Quoted 5 lines

After 'make prepare-sdk'

    $ readelf -d output/host/sbin/mkfs.ext3
shows
    0x000000000000001d (RUNPATH)            Bibliothek runpath: []

This is obviously not good :-)

Thanks for reporting that, I'll have a look. I guess it's simply the fixup of RPATH from absolute to relative that goes wrong.

Best regards,

Thomas

Quoted 1 line

Nope, this is expected. During the build output/host/lib doesn't exist,

of course

Quoted 3 lines

it's only populated at the very end of the build in host-finalize. So indeed the RPATH of binaries point to various per-package host/lib folders. This is totally expected.

Hmm, at this stage the host-finalize step was already run, so now output/host does exist.

Quoted 9 lines

Quoted 4 lines

After 'make prepare-sdk'

    $ readelf -d output/host/sbin/mkfs.ext3
shows
    0x000000000000001d (RUNPATH)            Bibliothek runpath: []  

This is obviously not good :-)

Thanks for reporting that, I'll have a look. I guess it's simply the fixup of RPATH from absolute to relative that goes wrong.

Would moving the fix-rpath step from prepare-sdk to host-finalize hurt?

Hello Andreas,

Quoted 10 lines · Andreas Naumann

Quoted 1 line

Nope, this is expected. During the build output/host/lib doesn't exist,

of course

Quoted 3 lines

it's only populated at the very end of the build in host-finalize. So indeed the RPATH of binaries point to various per-package host/lib folders. This is totally expected.

Hmm, at this stage the host-finalize step was already run, so now output/host does exist.

Yes, but the RPATH of host binaries are not fixed up in host-finalize. They are fixed up as part of prepare-sdk:

prepare-sdk: world

         @$(call MESSAGE,"Rendering the SDK relocatable")
         $(TOPDIR)/support/scripts/fix-rpath host
         $(TOPDIR)/support/scripts/fix-rpath staging

so it is totally expected that at the end of the build the host binaries carry those RPATH pointing to per-package host/lib folders. And it is not a problem: those folders exist, they contain the right libraries, so it's perfectly fine.

Quoted 7 lines

Quoted 7 lines

Quoted 4 lines

After 'make prepare-sdk'

     $ readelf -d output/host/sbin/mkfs.ext3
shows
     0x000000000000001d (RUNPATH)            Bibliothek runpath: []

This is obviously not good :-)

Quoted 4 lines

Quoted 2 lines

Thanks for reporting that, I'll have a look. I guess it's simply the fixup of RPATH from absolute to relative that goes wrong.

Would moving the fix-rpath step from prepare-sdk to host-finalize hurt?

It would not fix your problem: this fix-rpath step is currently broken as it doesn't understand how to rewrite those RPATHs that point to per-package host/lib.

So there are really two separate things:

- Fixing the fix-rpath logic so that it works with per-package host/lib RPATHs. This is mandatory. After doing that, at the end of the build, the RPATH would still point to per-package host/lib, but after prepare-sdk you will have again the correct relative RPATH you used to have before the per-package stuff.

- Moving fix-rpath at the end of the build so that instead of those weird per-package host/lib RPATH, you have nice relative RPATHs, without the need to run prepare-sdk. This is not mandatory at all, and would be just for convenience/beauty.

Best regards,

Thomas

Just to make sure I just did the above using your ppsh-v6 branch with the wandboard_defconfig. The only additional change was to use the external linaro toolchain. After 'make'

   $ readelf -d output/host/sbin/mkfs.ext3
shows
   0x000000000000001d (RUNPATH)            Bibliothek runpath: 
[/local/gsrc/buildroot.upstream/output/per-package/host-e2fsprogs/host/lib]

which is already unexpected, shouldn't it show .../output/host/lib? After 'make prepare-sdk'

   $ readelf -d output/host/sbin/mkfs.ext3
shows
   0x000000000000001d (RUNPATH)            Bibliothek runpath: []

Hello,

Arnout, Yann, see below some discussion where your input might be useful.

Quoted 8 lines · Andreas Naumann

Just to make sure I just did the above using your ppsh-v6 branch with the wandboard_defconfig. The only additional change was to use the external linaro toolchain. After 'make'

   $ readelf -d output/host/sbin/mkfs.ext3
shows
   0x000000000000001d (RUNPATH)            Bibliothek runpath:
[/local/gsrc/buildroot.upstream/output/per-package/host-e2fsprogs/host/lib]

Quoted 5 lines

which is already unexpected, shouldn't it show .../output/host/lib? After 'make prepare-sdk'

   $ readelf -d output/host/sbin/mkfs.ext3
shows
   0x000000000000001d (RUNPATH)            Bibliothek runpath: []

OK, so I investigated this issue, and now I understand where it comes from, but I'm not yet sure how to fix it properly.

Basically, *without* per-package directory enabled, the RPATH of binaries looks like this:

Library rpath: [/home/thomas/projets/buildroot/output/host/lib]

then, support/scripts/fix-rpath calls patchelf with the following invocation:

  /home/thomas/projets/buildroot/output/host/bin/patchelf --make-rpath-relative /home/thomas/projets/buildroot/output/host --relative-to-file /home/thomas/projets/buildroot/output/host/sbin/mke2fs

This works fine, and causes the resulting binary to have the following RPATH:

Library runpath: [$ORIGIN/../lib]

You can see that the original "Library rpath", which
is /home/thomas/projets/buildroot/output/host/lib is located *inside* the ROOTDIR passed as the --make-rpath-relative
option: /home/thomas/projets/buildroot/output/host.

Now, with per-package directory enabled, the RPATH of binaries when building looks like this:

Library rpath: [/home/thomas/projets/buildroot/output/per-package/host-e2fsprogs/host/lib]

So it is no longer within the ROOTDIR passed as patchelf's --make-rpath-relative argument.

The logic used by patchelf is explained in package/patchelf/0003-Add-option-to-make-the-rpath-relative-under-a-specif.patch:

====

RPATHDIR starts with "$ORIGIN":
The original build-system already took care of setting a relative RPATH, resolve it and test if it's valid (does exist)

RPATHDIR starts with ROOTDIR:
The original build-system added some absolute RPATH (absolute on the build machine). Test if it's valid (does exist).

ROOTDIR/RPATHDIR exists:
The original build-system already took care of setting an absolute RPATH (absolute in the final rootfs), resolve it and test if it's valid (does exist).

RPATHDIR points somewhere else:
(can be anywhere: build trees, staging tree, host location, non-existing location, etc.). Just discard such a path.

====

So, without per-package directory, we fall in case (2) above.

With per-package directory, we fall in case (4) above, and therefore the RPATH is discarded.

At this point, I am not sure at which level and how the issue should be fixed. Needs some thought. Input/ideas welcome.

In a similar context I once had following problem:
http://lists.busybox.net/pipermail/buildroot/2018-August/226955.html
This was for target binaries, rather than host, but for the below discussion it does not matter.

Looking back at this problem, taking into account the above comment from the patchelf patch, I would say that my problem would have been fixed if case (4) above would not discard the path.

If that change would be adopted, then it would also preserve the rpath '/home/thomas/projets/buildroot/output/per-package/host-e2fsprogs/host/lib'.

Of course, in your case you might actually want a different final rpath, i.e. rewrite it to the consolidated host directory. I think that in this case it would be better to read the rpath via patchelf, calculate the transformed rpath from our own script, then writing it back via patchelf, rather than adding more options into patchelf with e.g. transformation rules.

Quoted 4 lines

In a similar context I once had following problem:
http://lists.busybox.net/pipermail/buildroot/2018-August/226955.html
This was for target binaries, rather than host, but for the below discussion it does not matter.

I don't think you ever replied to my question in that thread:

Let's first find out why it is cleared. patchelf should check for each
directory in rpath if it is actually needed, and only remove it if it is not needed. So, what library do you have in /opt/foobar/lib, and is it really linked with the program?

Hm, I realize that we never thought about dlopen()ed libraries. Could that be
the cause? I guess that hasn't been a problem up to now because usually programs using dlopen() will use an absolute path (to a build-time or run-time configured plugin directory) rather than relying on RPATH.

Quoted 6 lines

Looking back at this problem, taking into account the above comment from the patchelf patch, I would say that my problem would have been fixed if case (4) above would not discard the path.

I think one motivation for removing redundant rpaths is to avoid having build
directories leaking into target binaries. Especially for reproducible builds this is important.

Quoted 5 lines

If that change would be adopted, then it would also preserve the rpath '/home/thomas/projets/buildroot/output/per-package/host-e2fsprogs/host/lib'.

Quoted 6 lines

Of course, in your case you might actually want a different final rpath, i.e. rewrite it to the consolidated host directory. I think that in this case it would be better to read the rpath via patchelf, calculate the transformed rpath from our own script, then writing it back via patchelf, rather than adding more options into patchelf with e.g. transformation rules.

The problem is that patchelf is rather slow, so running it twice is even slower...

El jue., 10 ene. 2019 a las 22:25, Arnout Vandecappelle (<[email protected]>) escribi?:

Quoted 16 lines · Thomas De Schampheleire

Quoted 4 lines

In a similar context I once had following problem:
http://lists.busybox.net/pipermail/buildroot/2018-August/226955.html
This was for target binaries, rather than host, but for the below discussion it does not matter.

I don't think you ever replied to my question in that thread:

Let's first find out why it is cleared. patchelf should check for each
directory in rpath if it is actually needed, and only remove it if it is not needed. So, what library do you have in /opt/foobar/lib, and is it really linked with the program?

Quoted 4 lines

Hm, I realize that we never thought about dlopen()ed libraries. Could that be
the cause? I guess that hasn't been a problem up to now because usually programs using dlopen() will use an absolute path (to a build-time or run-time configured plugin directory) rather than relying on RPATH.

Sorry that I did not seem to have replied on that question.

The binary in question did in fact link with the library, there is no dlopen used.
Some redacted output showing the initial rpath, which disappears after fix-rpath.

$ readelf -d output/target/opt/foobar/bin/fooprogram | rg 'NEEDED|RPATH'
...
 0x00000001 (NEEDED)                     Shared library: [libfoo.so.1]
...
 0x0000000f (RPATH)                      Library rpath:
[/../lib:/opt/foobar/lib:/home/tdescham/repo/isam/buildroot/output/target/opt/foobar/lib]

$ find output/target/ -name libfoo.so.1
output/target/opt/foobar/lib/libfoo.so.1

$ env HOST_DIR=output/host STAGING_DIR=output/staging
TARGET_DIR=output/target support/scripts/fix-rpath target

$ readelf -d output/target/opt/foobar/bin/fooprogram | rg 'NEEDED|RPATH'
...
 0x00000001 (NEEDED)                     Shared library: [libfoo.so.1]
...

Quoted 7 lines

Quoted 3 lines

Looking back at this problem, taking into account the above comment from the patchelf patch, I would say that my problem would have been fixed if case (4) above would not discard the path.

I think one motivation for removing redundant rpaths is to avoid having build
directories leaking into target binaries. Especially for reproducible builds this is important.

Ok, I can understand that.
But it should only remove directories which are not present in the target directory, e.g. if rpath contains /opt/foobar/lib then the script should check if output/target/opt/foobar/lib exists. If it does, then the rpath can remain, if it does not then it could be removed.

Quoted 10 lines

Quoted 9 lines

If that change would be adopted, then it would also preserve the rpath '/home/thomas/projets/buildroot/output/per-package/host-e2fsprogs/host/lib'.

Of course, in your case you might actually want a different final rpath, i.e. rewrite it to the consolidated host directory. I think that in this case it would be better to read the rpath via patchelf, calculate the transformed rpath from our own script, then writing it back via patchelf, rather than adding more options into patchelf with e.g. transformation rules.

Quoted 1 line

The problem is that patchelf is rather slow, so running it twice is even slower...

How so is it slow, can you clarify? Which times are you talking about?

Quoted 40 lines

El jue., 10 ene. 2019 a las 22:25, Arnout Vandecappelle (<[email protected]>) escribi?:

Quoted 16 lines · Thomas De Schampheleire

Quoted 4 lines

In a similar context I once had following problem:
http://lists.busybox.net/pipermail/buildroot/2018-August/226955.html
This was for target binaries, rather than host, but for the below discussion it does not matter.

I don't think you ever replied to my question in that thread:

Let's first find out why it is cleared. patchelf should check for each
directory in rpath if it is actually needed, and only remove it if it is not needed. So, what library do you have in /opt/foobar/lib, and is it really linked with the program?

Quoted 4 lines

Hm, I realize that we never thought about dlopen()ed libraries. Could that be
the cause? I guess that hasn't been a problem up to now because usually programs using dlopen() will use an absolute path (to a build-time or run-time configured plugin directory) rather than relying on RPATH.

Sorry that I did not seem to have replied on that question.

The binary in question did in fact link with the library, there is no dlopen used.
Some redacted output showing the initial rpath, which disappears after fix-rpath.

$ readelf -d output/target/opt/foobar/bin/fooprogram | rg 'NEEDED|RPATH'
...
 0x00000001 (NEEDED)                     Shared library: [libfoo.so.1]
...
 0x0000000f (RPATH)                      Library rpath:
[/../lib:/opt/foobar/lib:/home/tdescham/repo/isam/buildroot/output/target/opt/foobar/lib]

$ find output/target/ -name libfoo.so.1
output/target/opt/foobar/lib/libfoo.so.1

Quoted 7 lines

$ env HOST_DIR=output/host STAGING_DIR=output/staging
TARGET_DIR=output/target support/scripts/fix-rpath target

$ readelf -d output/target/opt/foobar/bin/fooprogram | rg 'NEEDED|RPATH'
...
 0x00000001 (NEEDED)                     Shared library: [libfoo.so.1]
...

OK, so that's a bug in our patchelf modification... At least, if libfoo.so.1
doesn't exist in output/target/lib or output/target/usr/lib.

If you run patchelf with --debug, it should tell you why the rpath got removed.

Quoted 10 lines

Quoted 9 lines

Quoted 5 lines

Looking back at this problem, taking into account the above comment from the patchelf patch, I would say that my problem would have been fixed if case (4) above would not discard the path.

I think one motivation for removing redundant rpaths is to avoid having build
directories leaking into target binaries. Especially for reproducible builds this is important.

Quoted 6 lines

Ok, I can understand that.
But it should only remove directories which are not present in the target directory, e.g. if rpath contains /opt/foobar/lib then the script should check if output/target/opt/foobar/lib exists. If it does, then the rpath can remain, if it does not then it could be removed.

Indeed, and that's what patchelf is supposed to do. Well, it also checks that
the library exists.

Quoted 10 lines

Quoted 10 lines

Quoted 9 lines

If that change would be adopted, then it would also preserve the rpath '/home/thomas/projets/buildroot/output/per-package/host-e2fsprogs/host/lib'.

Of course, in your case you might actually want a different final rpath, i.e. rewrite it to the consolidated host directory. I think that in this case it would be better to read the rpath via patchelf, calculate the transformed rpath from our own script, then writing it back via patchelf, rather than adding more options into patchelf with e.g. transformation rules.

Quoted 3 lines

Quoted 1 line

The problem is that patchelf is rather slow, so running it twice is even slower...

How so is it slow, can you clarify? Which times are you talking about?

I don't remember, but someone did measurements in 2017 and it was slow.

It seems the code already calls patchelf twice:

        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}"

so by saving the string returned in the first call, we can actually do a better transformation ourselves.

El dom., 13 ene. 2019 a las 23:10, Arnout Vandecappelle (<[email protected]>) escribi?:

Quoted 52 lines · Thomas De Schampheleire

Quoted 40 lines

El jue., 10 ene. 2019 a las 22:25, Arnout Vandecappelle (<[email protected]>) escribi?:

Quoted 16 lines · Thomas De Schampheleire

Quoted 4 lines

In a similar context I once had following problem:
http://lists.busybox.net/pipermail/buildroot/2018-August/226955.html
This was for target binaries, rather than host, but for the below discussion it does not matter.

I don't think you ever replied to my question in that thread:

Let's first find out why it is cleared. patchelf should check for each
directory in rpath if it is actually needed, and only remove it if it is not needed. So, what library do you have in /opt/foobar/lib, and is it really linked with the program?

Quoted 4 lines

Hm, I realize that we never thought about dlopen()ed libraries. Could that be
the cause? I guess that hasn't been a problem up to now because usually programs using dlopen() will use an absolute path (to a build-time or run-time configured plugin directory) rather than relying on RPATH.

Sorry that I did not seem to have replied on that question.

The binary in question did in fact link with the library, there is no dlopen used.
Some redacted output showing the initial rpath, which disappears after fix-rpath.

$ readelf -d output/target/opt/foobar/bin/fooprogram | rg 'NEEDED|RPATH'
...
 0x00000001 (NEEDED)                     Shared library: [libfoo.so.1]
...
 0x0000000f (RPATH)                      Library rpath:
[/../lib:/opt/foobar/lib:/home/tdescham/repo/isam/buildroot/output/target/opt/foobar/lib]

$ find output/target/ -name libfoo.so.1
output/target/opt/foobar/lib/libfoo.so.1

Quoted 7 lines

$ env HOST_DIR=output/host STAGING_DIR=output/staging
TARGET_DIR=output/target support/scripts/fix-rpath target

$ readelf -d output/target/opt/foobar/bin/fooprogram | rg 'NEEDED|RPATH'
...
 0x00000001 (NEEDED)                     Shared library: [libfoo.so.1]
...

Quoted 4 lines

OK, so that's a bug in our patchelf modification... At least, if libfoo.so.1
doesn't exist in output/target/lib or output/target/usr/lib.

If you run patchelf with --debug, it should tell you why the rpath got removed.

Part of the debug output for fooprogram:

patching ELF file `output/target/opt/foobar/bin/fooprogram' Kernel page size is 4096 bytes
removing directory '/../lib' from RPATH because it's not in rootdir keeping relative path of /home/tdescham/repo/isam/buildroot/output/target/opt/foobar/lib removing directory '/home/tdescham/repo/isam/buildroot/output/target/opt/foobar/lib' from RPATH because it's not in rootdir
new rpath is `m/repo/isam/buildroot/output/target/opt/foobar/lib'

The last line seems odd with the leading 'm'. I don't know if that is the problem, I will check it further.
Thanks for pointing to the --debug option.

El lun., 14 ene. 2019 a las 17:01, Thomas De Schampheleire (<[email protected]>) escribi?:

Quoted 72 lines

El dom., 13 ene. 2019 a las 23:10, Arnout Vandecappelle (<[email protected]>) escribi?:

Quoted 52 lines · Thomas De Schampheleire

Quoted 40 lines

El jue., 10 ene. 2019 a las 22:25, Arnout Vandecappelle (<[email protected]>) escribi?:

Quoted 16 lines · Thomas De Schampheleire

Quoted 4 lines

In a similar context I once had following problem:
http://lists.busybox.net/pipermail/buildroot/2018-August/226955.html
This was for target binaries, rather than host, but for the below discussion it does not matter.

I don't think you ever replied to my question in that thread:

Let's first find out why it is cleared. patchelf should check for each
directory in rpath if it is actually needed, and only remove it if it is not needed. So, what library do you have in /opt/foobar/lib, and is it really linked with the program?

Quoted 4 lines

Hm, I realize that we never thought about dlopen()ed libraries. Could that be
the cause? I guess that hasn't been a problem up to now because usually programs using dlopen() will use an absolute path (to a build-time or run-time configured plugin directory) rather than relying on RPATH.

Sorry that I did not seem to have replied on that question.

The binary in question did in fact link with the library, there is no dlopen used.
Some redacted output showing the initial rpath, which disappears after fix-rpath.

$ readelf -d output/target/opt/foobar/bin/fooprogram | rg 'NEEDED|RPATH'
...
 0x00000001 (NEEDED)                     Shared library: [libfoo.so.1]
...
 0x0000000f (RPATH)                      Library rpath:
[/../lib:/opt/foobar/lib:/home/tdescham/repo/isam/buildroot/output/target/opt/foobar/lib]

$ find output/target/ -name libfoo.so.1
output/target/opt/foobar/lib/libfoo.so.1

Quoted 7 lines

$ env HOST_DIR=output/host STAGING_DIR=output/staging
TARGET_DIR=output/target support/scripts/fix-rpath target

$ readelf -d output/target/opt/foobar/bin/fooprogram | rg 'NEEDED|RPATH'
...
 0x00000001 (NEEDED)                     Shared library: [libfoo.so.1]
...

Quoted 4 lines

OK, so that's a bug in our patchelf modification... At least, if libfoo.so.1
doesn't exist in output/target/lib or output/target/usr/lib.

If you run patchelf with --debug, it should tell you why the rpath got removed.

Part of the debug output for fooprogram:

patching ELF file `output/target/opt/foobar/bin/fooprogram' Kernel page size is 4096 bytes
removing directory '/../lib' from RPATH because it's not in rootdir keeping relative path of /home/tdescham/repo/isam/buildroot/output/target/opt/foobar/lib removing directory '/home/tdescham/repo/isam/buildroot/output/target/opt/foobar/lib' from RPATH because it's not in rootdir
new rpath is `m/repo/isam/buildroot/output/target/opt/foobar/lib'

Quoted 3 lines

The last line seems odd with the leading 'm'. I don't know if that is the problem, I will check it further.
Thanks for pointing to the --debug option.

Sorry for the confusion, this specific problem is caused by my calling fix-rpath with relative reference to the target directory. (the code in patchelf could perhaps be made more robust by resolving the input 'rootdir' with realpath as well.)

The actual output after fixing that parameter problem is:

output/host/bin/patchelf --debug --make-rpath-relative /home/tdescham/repo/isam/buildroot/output/target --no-standard-lib-dirs /home/tdescham/repo/isam/buildroot/output/target/opt/foobar/bin/fooprogram

patching ELF file `/home/tdescham/repo/isam/buildroot/output/target/opt/foobar/bin/fooprogram' Kernel page size is 4096 bytes
removing directory '/../lib' from RPATH because it's not in rootdir keeping relative path of /home/tdescham/repo/isam/buildroot/output/target/opt/foobar/lib removing directory '/home/tdescham/repo/isam/buildroot/output/target/opt/foobar/lib' from RPATH because it does not contain needed libs
new rpath is `/opt/foobar/lib'

So the logic above seems correct, but it is not correctly stored in the binary. After running patchelf, the rpath is cleared. I will investigate that further.

Quoted 7 lines

Just to make sure I just did the above using your ppsh-v6 branch with the wandboard_defconfig. The only additional change was to use the external linaro toolchain. After 'make'

   $ readelf -d output/host/sbin/mkfs.ext3
shows
   0x000000000000001d (RUNPATH)            Bibliothek runpath: 
[/local/gsrc/buildroot.upstream/output/per-package/host-e2fsprogs/host/lib]

OK, so I investigated this issue, and now I understand where it comes from, but I'm not yet sure how to fix it properly.

Now, with per-package directory enabled, the RPATH of binaries when
building looks like this:
Library rpath: [/home/thomas/projets/buildroot/output/per-package/host-e2fsprogs/host/lib]

So it is no longer within the ROOTDIR passed as patchelf's --make-rpath-relative argument.

At this point, I am not sure at which level and how the issue should be fixed. Needs some thought. Input/ideas welcome.

So, without per-package directory, we fall in case (2) above.

With per-package directory, we fall in case (4) above, and therefore the RPATH is discarded.

At this point, I am not sure at which level and how the issue should be fixed. Needs some thought. Input/ideas welcome.

Hello,

Quoted 7 lines · Thomas Petazzoni

So, without per-package directory, we fall in case (2) above.

With per-package directory, we fall in case (4) above, and therefore the RPATH is discarded.

At this point, I am not sure at which level and how the issue should be fixed. Needs some thought. Input/ideas welcome.

So for now, the way I fixed this is by rewriting the RPATH in fix-rpath so that they don't point to per-package host directories.

It is a bit
annoying that we end up rewriting RPATHs twice, but it is the easiest / most straightforward solution to be able to move forward with the whole per-package directory stuff.

Here is what I have locally:

support/scripts/fix-rpath +0 -7

index fa138ca15a..67bf1eb1b3 100755
@@ -127,14 +127,24 @@ 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}"

support/scripts/fix-rpath +8 -0

+        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[@]})
 
     # Restore patched patchelf utility

Best regards,

Thomas

With per-package folder support, top-level parallel build becomes safe, so we can enclose the .NOTPARALLEL statement in a !BR2_PER_PACKAGE_DIRECTORIES condition.

Signed-off-by Thomas Petazzoni <[email protected]>

---
 Makefile | 2 ++
 1 file changed, 2 insertions(+)

Makefile +1 -0

index 35fe1b3644..7bd7291cfd 100644
@@ -231,6 +231,7 @@ ifeq ($(filter $(noconfig_targets),$(MAKECMDGOALS)),)
 -include $(BR2_CONFIG)
 endif
 
+ifeq ($(BR2_PER_PACKAGE_DIRECTORIES),)
 # Parallel execution of this Makefile is disabled because it changes
 # the packages building order, that can be a problem for two reasons:
 # - If a package has an unspecified optional dependency and that

Makefile +1 -0

 .NOTPARALLEL:
+endif
 
 ifeq ($(BR2_PER_PACKAGE_DIRECTORIES),y)
 TARGET_DIR = $(if $(ROOTFS),$(ROOTFS_$(ROOTFS)_TARGET_DIR),$(if $(PKG),$(PER_PACKAGE_DIR)/$($(PKG)_NAME)/target,$(BASE_TARGET_DIR)))

Signature · 1 line

2.19.1

Libtool .la files unfortunately contain a number of absolute paths, which now refer to per-package directories. Due to this, when building package A, .la files may contain absolute paths referring to directories in package B per-package sysroot. This causes some -L flags referring to other sysroot from being added, which doesn't work as the linker no longer realizes that such paths are within its sysroot.

To fix this, we introduce a replacement step of .la files in the configure step, to make sure all paths refer to this package per-package directory.

Signed-off-by Thomas Petazzoni <[email protected]>

---
 package/pkg-generic.mk | 7 +++++++
 1 file changed, 7 insertions(+)

package/pkg-generic.mk +0 -0

index b1f0cdf34a..17909f4a61 100644

package/pkg-generic.mk +1 -0

 	$($(PKG)_CONFIGURE_CMDS)
 	$(foreach hook,$($(PKG)_POST_CONFIGURE_HOOKS),$(call $(hook))$(sep))

@@ -875,6 +881,7 @@ $$($(2)_TARGET_INSTALL_IMAGES):		PKG=$(2)
 $$($(2)_TARGET_INSTALL_HOST):		PKG=$(2)
 $$($(2)_TARGET_BUILD):			PKG=$(2)
 $$($(2)_TARGET_CONFIGURE):		PKG=$(2)
+$$($(2)_TARGET_CONFIGURE):		NAME=$(1)
 $$($(2)_TARGET_RSYNC):			SRCDIR=$$($(2)_OVERRIDE_SRCDIR)
 $$($(2)_TARGET_RSYNC):			PKG=$(2)
 $$($(2)_TARGET_PATCH):			PKG=$(2)

Signature · 1 line

2.19.1

Since I really want this topic to come to a conclusion sooner than later, and since the solution is good-enough for now:

Reviewed-by "Yann E. MORIN" <[email protected]>

I don't find it very clean that we shoe-horn such preparation in the generic rules; I'd rather we use a hook mechanism, which we can define for the prepare-per-package-directory macro to call.

Quoted 4 lines

Since I really want this topic to come to a conclusion sooner than later, and since the solution is good-enough for now:

Reviewed-by "Yann E. MORIN" <[email protected]>

Thanks. However, since I have changed a bit this patch for v7, I will not include your Reviewed-by.

Quoted 3 lines

I don't find it very clean that we shoe-horn such preparation in the generic rules; I'd rather we use a hook mechanism, which we can define for the prepare-per-package-directory macro to call.

I understand your proposal. However, I think that it's premature to introduce such a hook mechanism: for now there's only one such fixup to do. Let's wait until we have a few.

I have moved the code outside of the package step, into a helper function. It looks a bit nicer, and perhaps if we need to do other fixups this helper function can be extended to be the one doing all the per-package related fixups.

What do you think ?

Thomas

Signature · 3 lines

Thomas Petazzoni, CTO, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com

The pkg-kconfig infrastructure hijacks the regular chain of build steps to insert its own step to prepare the configuration of kconfig packages. This additional step may have dependencies of its own, such as host-flex, host-bison or toolchain.

In the context of per-package directory support, those dependencies must be copied to the per-package directory of the current package prior to doing the config preparation. This commit implements this logic by adding a call to prepare-per-package-directory at the right spot.

Reported-by Andreas Naumann <[email protected]>

Signed-off-by Thomas Petazzoni <[email protected]>

---
 package/pkg-kconfig.mk | 1 +
 1 file changed, 1 insertion(+)

package/pkg-kconfig.mk +1 -0

index d6c95b897e..bf43632adc 100644
@@ -113,6 +113,7 @@ endef
 # Since the file could be a defconfig file it needs to be expanded to a
 # full .config first.
 $$($(2)_DIR)/$$($(2)_KCONFIG_DOTCONFIG): $$($(2)_KCONFIG_FILE) $$($(2)_KCONFIG_FRAGMENT_FILES)
+	$$(call prepare-per-package-folder,$$($(2)_KCONFIG_DEPENDENCIES))
 	$$(Q)$$(if $$($(2)_KCONFIG_DEFCONFIG), \
 		$$($(2)_KCONFIG_MAKE) $$($(2)_KCONFIG_DEFCONFIG), \
 		$$(INSTALL) -m 0644 -D $$($(2)_KCONFIG_FILE) $$(@))

Signature · 1 line

2.19.1

The pkg-kconfig infrastructure hijacks the regular chain of build steps to insert its own step to prepare the configuration of kconfig packages. This additional step may have dependencies of its own, such as host-flex, host-bison or toolchain.

In the context of per-package directory support, those dependencies must be copied to the per-package directory of the current package prior to doing the config preparation. This commit implements this logic by adding a call to prepare-per-package-directory at the right spot.

Reported-by Andreas Naumann <[email protected]>

Signed-off-by Thomas Petazzoni <[email protected]>

---
  package/pkg-kconfig.mk | 1 +
  1 file changed, 1 insertion(+)

package/pkg-kconfig.mk +0 -0

index d6c95b897e..bf43632adc 100644

Hi Thomas,

Am 23.11.18 um 15:58 schrieb Thomas Petazzoni:

Quoted 17 lines

The pkg-kconfig infrastructure hijacks the regular chain of build steps to insert its own step to prepare the configuration of kconfig packages. This additional step may have dependencies of its own, such as host-flex, host-bison or toolchain.

In the context of per-package directory support, those dependencies must be copied to the per-package directory of the current package prior to doing the config preparation. This commit implements this logic by adding a call to prepare-per-package-directory at the right spot.

Reported-by Andreas Naumann <[email protected]>

Signed-off-by Thomas Petazzoni <[email protected]>

---
? package/pkg-kconfig.mk | 1 +
? 1 file changed, 1 insertion(+)

Quoted 10 lines

package/pkg-kconfig.mk +0 -0

index d6c95b897e..bf43632adc 100644
@@ -113,6 +113,7 @@ endef

? # Since the file could be a defconfig file it needs to be expanded to a ? # full .config first.
? $$($(2)_DIR)/$$($(2)_KCONFIG_DOTCONFIG): $$($(2)_KCONFIG_FILE) $$($(2)_KCONFIG_FRAGMENT_FILES)
+??? $$(call prepare-per-package-folder,$$($(2)_KCONFIG_DEPENDENCIES))

Quoted 4 lines

????? $$(Q)$$(if $$($(2)_KCONFIG_DEFCONFIG), \
????????? $$($(2)_KCONFIG_MAKE) $$($(2)_KCONFIG_DEFCONFIG), \
????????? $$(INSTALL) -m 0644 -D $$($(2)_KCONFIG_FILE) $$(@))

Finally I could test this and found an issue. Because I use ccache, the linux/uboot kconfig steps fail, because ccache apparently is not part of UBOOT/LINUX_KCONFIG_DEPENDENCIES.
Instead of conditionally adding it, my simple proposal would be to not use it, e.g.:

?# $(HOST_DIR)/lib/libncurses.so (which is not).? We don't actually ?# need any host-package for kconfig, so remove the HOSTCC/HOSTLDFLAGS ?# override again.

(quoted diff) +1 -1

-UBOOT_KCONFIG_OPTS = $(UBOOT_MAKE_OPTS) HOSTCC="$(HOSTCC)" HOSTLDFLAGS=""
+UBOOT_KCONFIG_OPTS = $(UBOOT_MAKE_OPTS) HOSTCC="$(HOSTCC_NOCCACHE)" 

HOSTLDFLAGS=""

So after this, I'm not sure if there really was a need for above fix since the fixup/.config creation may not even need the only kconfig dependencies set so far: host-flex and host-bison..

regards,
Andreas
_______________________________________________
buildroot mailing list
buildroot at busybox.net
http://lists.busybox.net/mailman/listinfo/buildroot

Quoted 7 lines

Quoted 7 lines

package/pkg-kconfig.mk +0 -0

index d6c95b897e..bf43632adc 100644
@@ -113,6 +113,7 @@ endef

? # Since the file could be a defconfig file it needs to be expanded to a ? # full .config first.

Quoted 3 lines

Quoted 3 lines

? $$($(2)_DIR)/$$($(2)_KCONFIG_DOTCONFIG): $$($(2)_KCONFIG_FILE) $$($(2)_KCONFIG_FRAGMENT_FILES)
+??? $$(call prepare-per-package-folder,$$($(2)_KCONFIG_DEPENDENCIES))

just realized that this doesn't match the function definition in pkg-utils.mk in this series (prepare-per-package-directories), thus is not executed at all. With that fixed ccache is, as part of toolchain, properly prepared and I no longer see the issue below.

Quoted 7 lines

Quoted 7 lines

package/pkg-kconfig.mk +0 -0

index d6c95b897e..bf43632adc 100644
@@ -113,6 +113,7 @@ endef

? # Since the file could be a defconfig file it needs to be expanded to a ? # full .config first.

Quoted 3 lines

Quoted 3 lines

? $$($(2)_DIR)/$$($(2)_KCONFIG_DOTCONFIG): $$($(2)_KCONFIG_FILE) $$($(2)_KCONFIG_FRAGMENT_FILES)
+??? $$(call prepare-per-package-folder,$$($(2)_KCONFIG_DEPENDENCIES))

just realized that this doesn't match the function definition in pkg-utils.mk in this series (prepare-per-package-directories), thus is not executed at all. With that fixed ccache is, as part of toolchain, properly prepared and I no longer see the issue below.