[RFCv1,2/4] core: change host RPATH handling

Message ID 20171103160627.6468-3-thomas.petazzoni@free-electrons.com
State New
Headers show
Series
  • Per-package SDK and target directories
Related show

Commit Message

Thomas Petazzoni Nov. 3, 2017, 4:06 p.m.
Currently, our strategy for the RPATH of host binaries is as follows:

 - During the build, we force an absolute RPATH to be encoded into
   binaries, by passing -Wl,-rpath,$(HOST_DIR)/lib. This ensures that
   host binaries will find their libraries.

 - In the "make sdk" target, when preparing the SDK to be relocatable,
   we use patchelf to replace those absolute RPATHs by relative RPATHs
   that use $ORIGIN.

Unfortunately, the use of absolute RPATH during the build is not
compatible with the move to per-package SDK, where a different host
directory is used for the build of each package. Therefore, we need to
move to a different strategy for RPATH handling.

The new strategy is as follows:

 - During the build, we do not encode any RPATH into the host
   binaries. Instead, we specify LD_LIBRARY_PATH environment to point
   to the current HOST_DIR/lib directory (for the package being
   currently built).

 - At the end of the build, in order to make sure that binaries are
   usable without LD_LIBRARY_PATH, we fixup the host binaries to use
   $ORIGIN/../lib. This is more-or-less what was done previously in
   the "make sdk" target, except that instead of turning absolute
   paths into relative paths in the RPATH, we are simply setting the
   RPATH to $ORIGIN/../lib.

   In order to implement this, the fix-rpath script logic is adjusted
   for the "host" case.

An alternative strategy would have been to keep the
-Wl,-rpath,$(HOST_DIR) flag, and therefore the absolute RPATH in the
host binaries, and fix such RPATH at the end of the build of every
package. However, that would require calling fix-rpath after the
installation of every package, which is a bit expensive.

This change is independent from the per-package SDK functionality, and
could be applied separately.

Signed-off-by: Thomas Petazzoni <thomas.petazzoni@free-electrons.com>
---
 Makefile                  |  4 +++-
 package/Makefile.in       |  2 +-
 package/pkg-generic.mk    |  8 --------
 support/scripts/fix-rpath | 15 ++++++++++-----
 4 files changed, 14 insertions(+), 15 deletions(-)

Comments

Yann E. MORIN Nov. 5, 2017, 8:57 a.m. | #1
Thomas, All,

Here is a summary of our discussion on IRC...

On 2017-11-03 17:06 +0100, Thomas Petazzoni spake thusly:
> Currently, our strategy for the RPATH of host binaries is as follows:
> 
>  - During the build, we force an absolute RPATH to be encoded into
>    binaries, by passing -Wl,-rpath,$(HOST_DIR)/lib. This ensures that
>    host binaries will find their libraries.
> 
>  - In the "make sdk" target, when preparing the SDK to be relocatable,
>    we use patchelf to replace those absolute RPATHs by relative RPATHs
>    that use $ORIGIN.
> 
> Unfortunately, the use of absolute RPATH during the build is not
> compatible with the move to per-package SDK, where a different host
> directory is used for the build of each package. Therefore, we need to
> move to a different strategy for RPATH handling.
> 
> The new strategy is as follows:
> 
>  - During the build, we do not encode any RPATH into the host
>    binaries. Instead, we specify LD_LIBRARY_PATH environment to point
>    to the current HOST_DIR/lib directory (for the package being
>    currently built).

In the past, we explicitly got rid of LD_LIBRARY_PATH because it does
not work. See 34d081674a (core/pkg-infrastructures: remove LD_LIBRARY_PATH
from the environment).

Briefly, it does not work when we build a library that is also present
on the distro, which is used by a tool from the distro, and we build it
in an incompatibl way.

So, we'll have to come up with another solution, like your suggested
alternative.

>  - At the end of the build, in order to make sure that binaries are
>    usable without LD_LIBRARY_PATH, we fixup the host binaries to use
>    $ORIGIN/../lib. This is more-or-less what was done previously in
>    the "make sdk" target, except that instead of turning absolute
>    paths into relative paths in the RPATH, we are simply setting the
>    RPATH to $ORIGIN/../lib.
> 
>    In order to implement this, the fix-rpath script logic is adjusted
>    for the "host" case.
> 
> An alternative strategy would have been to keep the
> -Wl,-rpath,$(HOST_DIR) flag, and therefore the absolute RPATH in the
> host binaries, and fix such RPATH at the end of the build of every
> package. However, that would require calling fix-rpath after the
> installation of every package, which is a bit expensive.

I wonder how expensive that would be. It would be nice to time this.

Also note that, even if the overhead is noticeable, this would be
compensated by the mere fact that we are now doing a parallel build, so
we would end up winning.

Regards,
Yann E. MORIN.

> This change is independent from the per-package SDK functionality, and
> could be applied separately.
> 
> Signed-off-by: Thomas Petazzoni <thomas.petazzoni@free-electrons.com>
> ---
>  Makefile                  |  4 +++-
>  package/Makefile.in       |  2 +-
>  package/pkg-generic.mk    |  8 --------
>  support/scripts/fix-rpath | 15 ++++++++++-----
>  4 files changed, 14 insertions(+), 15 deletions(-)
> 
> diff --git a/Makefile b/Makefile
> index 79db7fe48a..5496273329 100644
> --- a/Makefile
> +++ b/Makefile
> @@ -231,6 +231,8 @@ LEGAL_MANIFEST_CSV_HOST = $(LEGAL_INFO_DIR)/host-manifest.csv
>  LEGAL_WARNINGS = $(LEGAL_INFO_DIR)/.warnings
>  LEGAL_REPORT = $(LEGAL_INFO_DIR)/README
>  
> +export LD_LIBRARY_PATH = $(if $(PKG),$(HOST_DIR)/lib)
> +
>  ################################################################################
>  #
>  # staging and target directories do NOT list these as
> @@ -558,7 +560,6 @@ world: target-post-image
>  .PHONY: sdk
>  sdk: world
>  	@$(call MESSAGE,"Rendering the SDK relocatable")
> -	$(TOPDIR)/support/scripts/fix-rpath host
>  	$(TOPDIR)/support/scripts/fix-rpath staging
>  	$(INSTALL) -m 755 $(TOPDIR)/support/misc/relocate-sdk.sh $(HOST_DIR)/relocate-sdk.sh
>  	echo $(HOST_DIR) > $(HOST_DIR)/share/buildroot/sdk-location
> @@ -679,6 +680,7 @@ $(TARGETS_ROOTFS): target-finalize
>  .PHONY: target-finalize
>  target-finalize: $(PACKAGES)
>  	@$(call MESSAGE,"Finalizing target directory")
> +	$(TOPDIR)/support/scripts/fix-rpath host
>  	$(foreach hook,$(TARGET_FINALIZE_HOOKS),$($(hook))$(sep))
>  	rm -rf $(TARGET_DIR)/usr/include $(TARGET_DIR)/usr/share/aclocal \
>  		$(TARGET_DIR)/usr/lib/pkgconfig $(TARGET_DIR)/usr/share/pkgconfig \
> diff --git a/package/Makefile.in b/package/Makefile.in
> index a1a5316051..e94a75c230 100644
> --- a/package/Makefile.in
> +++ b/package/Makefile.in
> @@ -220,7 +220,7 @@ HOST_CPPFLAGS  = -I$(HOST_DIR)/include
>  HOST_CFLAGS   ?= -O2
>  HOST_CFLAGS   += $(HOST_CPPFLAGS)
>  HOST_CXXFLAGS += $(HOST_CFLAGS)
> -HOST_LDFLAGS  += -L$(HOST_DIR)/lib -Wl,-rpath,$(HOST_DIR)/lib
> +HOST_LDFLAGS  += -L$(HOST_DIR)/lib
>  
>  # The macros below are taken from linux 4.11 and adapted slightly.
>  # Copy more when needed.
> diff --git a/package/pkg-generic.mk b/package/pkg-generic.mk
> index cca94ba338..82f8c06821 100644
> --- a/package/pkg-generic.mk
> +++ b/package/pkg-generic.mk
> @@ -105,14 +105,6 @@ endef
>  
>  GLOBAL_INSTRUMENTATION_HOOKS += check_bin_arch
>  
> -# This hook checks that host packages that need libraries that we build
> -# 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)))
> -endef
> -GLOBAL_INSTRUMENTATION_HOOKS += check_host_rpath
> -
>  define step_check_build_dir_one
>  	if [ -d $(2) ]; then \
>  		printf "%s: installs files in %s\n" $(1) $(2) >&2; \
> diff --git a/support/scripts/fix-rpath b/support/scripts/fix-rpath
> index 15705a3b0d..eed751f5da 100755
> --- a/support/scripts/fix-rpath
> +++ b/support/scripts/fix-rpath
> @@ -61,7 +61,7 @@ main() {
>      local rootdir
>      local tree="${1}"
>      local find_args=( )
> -    local sanitize_extra_args=( )
> +    local sanitize_args=( )
>  
>      if ! "${PATCHELF}" --version > /dev/null 2>&1; then
>  	echo "Error: can't execute patchelf utility '${PATCHELF}'"
> @@ -89,7 +89,8 @@ main() {
>              cp "${PATCHELF}" "${PATCHELF}.__to_be_patched"
>  
>              # we always want $ORIGIN-based rpaths to make it relocatable.
> -            sanitize_extra_args+=( "--relative-to-file" )
> +            sanitize_args+=( "--set-rpath" )
> +            sanitize_args+=( "\$ORIGIN/../lib" )
>              ;;
>  
>          staging)
> @@ -101,14 +102,18 @@ main() {
>              done
>  
>              # should be like for the target tree below
> -            sanitize_extra_args+=( "--no-standard-lib-dirs" )
> +            sanitize_args+=( "--no-standard-lib-dirs" )
> +            sanitize_args+=( "--make-rpath-relative" )
> +            sanitize_args+=( "${rootdir}" )
>              ;;
>  
>          target)
>              rootdir="${TARGET_DIR}"
>              # we don't want $ORIGIN-based rpaths but absolute paths without rootdir.
>              # we also want to remove rpaths pointing to /lib or /usr/lib.
> -            sanitize_extra_args+=( "--no-standard-lib-dirs" )
> +            sanitize_args+=( "--no-standard-lib-dirs" )
> +            sanitize_args+=( "--make-rpath-relative" )
> +            sanitize_args+=( "${rootdir}" )
>              ;;
>  
>          *)
> @@ -125,7 +130,7 @@ main() {
>              # 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}"
> +            ${PATCHELF} ${sanitize_args[@]} "${file}"
>              # restore the original permission
>              test "${changed}" != "" && chmod u-w "${file}"
>          fi
> -- 
> 2.13.6
>
Thomas Petazzoni Nov. 5, 2017, 2:44 p.m. | #2
Hello,

On Sun, 5 Nov 2017 09:57:56 +0100, Yann E. MORIN wrote:

> In the past, we explicitly got rid of LD_LIBRARY_PATH because it does
> not work. See 34d081674a (core/pkg-infrastructures: remove LD_LIBRARY_PATH
> from the environment).
> 
> Briefly, it does not work when we build a library that is also present
> on the distro, which is used by a tool from the distro, and we build it
> in an incompatibl way.
> 
> So, we'll have to come up with another solution, like your suggested
> alternative.
> 
> >  - At the end of the build, in order to make sure that binaries are
> >    usable without LD_LIBRARY_PATH, we fixup the host binaries to use
> >    $ORIGIN/../lib. This is more-or-less what was done previously in
> >    the "make sdk" target, except that instead of turning absolute
> >    paths into relative paths in the RPATH, we are simply setting the
> >    RPATH to $ORIGIN/../lib.
> > 
> >    In order to implement this, the fix-rpath script logic is adjusted
> >    for the "host" case.
> > 
> > An alternative strategy would have been to keep the
> > -Wl,-rpath,$(HOST_DIR) flag, and therefore the absolute RPATH in the
> > host binaries, and fix such RPATH at the end of the build of every
> > package. However, that would require calling fix-rpath after the
> > installation of every package, which is a bit expensive.  
> 
> I wonder how expensive that would be. It would be nice to time this.
> 
> Also note that, even if the overhead is noticeable, this would be
> compensated by the mere fact that we are now doing a parallel build, so
> we would end up winning.

Thanks for the feedback. We already discussed this on IRC, and I agree.
I'll have a look at implementing the solution of doing the patchelf
adding $ORIGIN/../lib as an RPATH directly after the installation of
each package.

Thanks!

Thomas
Arnout Vandecappelle Nov. 5, 2017, 2:48 p.m. | #3
On 05-11-17 15:44, Thomas Petazzoni wrote:
> Hello,
> 
> On Sun, 5 Nov 2017 09:57:56 +0100, Yann E. MORIN wrote:
> 
>> In the past, we explicitly got rid of LD_LIBRARY_PATH because it does
>> not work. See 34d081674a (core/pkg-infrastructures: remove LD_LIBRARY_PATH
>> from the environment).
>>
>> Briefly, it does not work when we build a library that is also present
>> on the distro, which is used by a tool from the distro, and we build it
>> in an incompatibl way.
>>
>> So, we'll have to come up with another solution, like your suggested
>> alternative.
>>
>>>  - At the end of the build, in order to make sure that binaries are
>>>    usable without LD_LIBRARY_PATH, we fixup the host binaries to use
>>>    $ORIGIN/../lib. This is more-or-less what was done previously in
>>>    the "make sdk" target, except that instead of turning absolute
>>>    paths into relative paths in the RPATH, we are simply setting the
>>>    RPATH to $ORIGIN/../lib.
>>>
>>>    In order to implement this, the fix-rpath script logic is adjusted
>>>    for the "host" case.
>>>
>>> An alternative strategy would have been to keep the
>>> -Wl,-rpath,$(HOST_DIR) flag, and therefore the absolute RPATH in the
>>> host binaries, and fix such RPATH at the end of the build of every
>>> package. However, that would require calling fix-rpath after the
>>> installation of every package, which is a bit expensive.  
>>
>> I wonder how expensive that would be. It would be nice to time this.
>>
>> Also note that, even if the overhead is noticeable, this would be
>> compensated by the mere fact that we are now doing a parallel build, so
>> we would end up winning.
> 
> Thanks for the feedback. We already discussed this on IRC, and I agree.
> I'll have a look at implementing the solution of doing the patchelf
> adding $ORIGIN/../lib as an RPATH directly after the installation of
> each package.

 Note that according to Wolfgang's measurements (if I remember correctly), doing
fix-rpath on the host dir didn't take too long, but doing it in staging does.
There aren't that many host files.

 Regards,
 Arnout
Arnout Vandecappelle Nov. 7, 2017, 9:15 p.m. | #4
On 03-11-17 17:06, Thomas Petazzoni wrote:
> Currently, our strategy for the RPATH of host binaries is as follows:
> 
>  - During the build, we force an absolute RPATH to be encoded into
>    binaries, by passing -Wl,-rpath,$(HOST_DIR)/lib. This ensures that
>    host binaries will find their libraries.
> 
>  - In the "make sdk" target, when preparing the SDK to be relocatable,
>    we use patchelf to replace those absolute RPATHs by relative RPATHs
>    that use $ORIGIN.
> 
> Unfortunately, the use of absolute RPATH during the build is not
> compatible with the move to per-package SDK, where a different host
> directory is used for the build of each package. Therefore, we need to
> move to a different strategy for RPATH handling.
> 
> The new strategy is as follows:
> 
>  - During the build, we do not encode any RPATH into the host
>    binaries. Instead, we specify LD_LIBRARY_PATH environment to point
>    to the current HOST_DIR/lib directory (for the package being
>    currently built).
> 
>  - At the end of the build, in order to make sure that binaries are
>    usable without LD_LIBRARY_PATH, we fixup the host binaries to use
>    $ORIGIN/../lib. This is more-or-less what was done previously in
>    the "make sdk" target, except that instead of turning absolute
>    paths into relative paths in the RPATH, we are simply setting the
>    RPATH to $ORIGIN/../lib.
> 
>    In order to implement this, the fix-rpath script logic is adjusted
>    for the "host" case.
> 
> An alternative strategy would have been to keep the
> -Wl,-rpath,$(HOST_DIR) flag, and therefore the absolute RPATH in the
> host binaries, and fix such RPATH at the end of the build of every
> package. However, that would require calling fix-rpath after the
> installation of every package, which is a bit expensive.

 As discussed, this would be the preferred approach after all.

> 
> This change is independent from the per-package SDK functionality, and
> could be applied separately.

 Shouldn't this type of comment be below the --- ?

> 
> Signed-off-by: Thomas Petazzoni <thomas.petazzoni@free-electrons.com>
> ---
[snip]
> @@ -89,7 +89,8 @@ main() {
>              cp "${PATCHELF}" "${PATCHELF}.__to_be_patched"
>  
>              # we always want $ORIGIN-based rpaths to make it relocatable.
> -            sanitize_extra_args+=( "--relative-to-file" )
> +            sanitize_args+=( "--set-rpath" )
> +            sanitize_args+=( "\$ORIGIN/../lib" )

 One advantage of --make-rpath-relative over --set-rpath is that the former will
also remove rpaths that are not needed (which I guess is often the case for host
tools). OTOH for host tools this is not really important. And it doesn't even
save size, patchelf doesn't shift sections to the left (IIRC). But it does save
a tiny amount of startup time - for gcc, this could have an impact on overall
build time.


 Regards,
 Arnout

[snip]
Thomas Petazzoni Nov. 7, 2017, 9:22 p.m. | #5
Hello,

On Tue, 7 Nov 2017 22:15:38 +0100, Arnout Vandecappelle wrote:

> > An alternative strategy would have been to keep the
> > -Wl,-rpath,$(HOST_DIR) flag, and therefore the absolute RPATH in the
> > host binaries, and fix such RPATH at the end of the build of every
> > package. However, that would require calling fix-rpath after the
> > installation of every package, which is a bit expensive.  
> 
>  As discussed, this would be the preferred approach after all.

Yes, agreed.

> > This change is independent from the per-package SDK functionality, and
> > could be applied separately.  
> 
>  Shouldn't this type of comment be below the --- ?

Well, it's not really that I expected the v1 of the patches to be
merged :-)

> > @@ -89,7 +89,8 @@ main() {
> >              cp "${PATCHELF}" "${PATCHELF}.__to_be_patched"
> >  
> >              # we always want $ORIGIN-based rpaths to make it relocatable.
> > -            sanitize_extra_args+=( "--relative-to-file" )
> > +            sanitize_args+=( "--set-rpath" )
> > +            sanitize_args+=( "\$ORIGIN/../lib" )  
> 
>  One advantage of --make-rpath-relative over --set-rpath is that the former will
> also remove rpaths that are not needed (which I guess is often the case for host
> tools).

Well, --set-rpath sets the RPATH, so I believe it entirely removes any
existing RPATH and replaces them all by just $ORIGIN/../lib. So I don't
see how it could be more "efficient", since we're down to a single
RPATH, which is the only rpath that is needed.

Perhaps the only problem that I can think of is if some host binaries
do have libraries installed in non-standard locations, and really need
a custom absolute RPATH such
as /home/foo/buildroot/output/per-package/host/lib/baz/ to be turned
into $ORIGIN/../lib/baz/. However, I don't think we have such
non-standard library locations for host packages, so I simply ignored
this possible problem for now. It is a problem that can be fixed later
on if needed by adjusting the host rpath mangling logic.

Best regards,

Thomas
Thomas Petazzoni Nov. 7, 2017, 10:41 p.m. | #6
Hello,

On Sun, 5 Nov 2017 09:57:56 +0100, Yann E. MORIN wrote:

> > An alternative strategy would have been to keep the
> > -Wl,-rpath,$(HOST_DIR) flag, and therefore the absolute RPATH in the
> > host binaries, and fix such RPATH at the end of the build of every
> > package. However, that would require calling fix-rpath after the
> > installation of every package, which is a bit expensive.  
> 
> I wonder how expensive that would be. It would be nice to time this.

OK, to time this, I did a quick hacky shell script
(http://code.bulix.org/uboevd-223829). What it does is:

 - Generate a mix of ELF files and random data files (half of the files
   are ELF files, half are random data files). This is the "generate"
   target of the script.

 - Fixup rpath to $ORIGIN/../lib if not already fixed. We don't want to
   fixup files repeatedly, as we don't want to duplicate files over and
   over by breaking hard links. So we really want to set the RPATH only
   once per binary.

Results are as follows:

$ ./rpath-evaluation.sh generate 0 500
... generate 250 random data files, 250 ELF programs with a crappy
rpath ...

$ time ./rpath-evaluation.sh fixup

real	0m0.976s
user	0m0.546s
sys	0m0.457s
$ time ./rpath-evaluation.sh fixup

real	0m0.610s
user	0m0.393s
sys	0m0.234s

So the first fixup pass, which actually fixes the RPATH of 250 binaries
takes about one second. The next fixup pass doesn't do anything, except
checking that all binaries already have a fixed RPATH.

So we see that it will take ~600ms to scan 500 files, provided half
of them are ELF files, verify that they already have the correct RPATH.

To me, this sounds very reasonable. What do you think?

Thomas
Arnout Vandecappelle Nov. 7, 2017, 11:45 p.m. | #7
On 07-11-17 22:22, Thomas Petazzoni wrote:
> Hello,
> 
> On Tue, 7 Nov 2017 22:15:38 +0100, Arnout Vandecappelle wrote:
[snip]
>>  One advantage of --make-rpath-relative over --set-rpath is that the former will
>> also remove rpaths that are not needed (which I guess is often the case for host
>> tools).
> 
> Well, --set-rpath sets the RPATH, so I believe it entirely removes any
> existing RPATH and replaces them all by just $ORIGIN/../lib. So I don't
> see how it could be more "efficient", since we're down to a single
> RPATH, which is the only rpath that is needed.

 In many cases, no rpath is needed because it doesn't link with any
Buildroot-built libraries. In that case, no rpath is slightly more efficient for
application startup.

> 
> Perhaps the only problem that I can think of is if some host binaries
> do have libraries installed in non-standard locations, and really need
> a custom absolute RPATH such
> as /home/foo/buildroot/output/per-package/host/lib/baz/ to be turned
> into $ORIGIN/../lib/baz/. However, I don't think we have such
> non-standard library locations for host packages, so I simply ignored
> this possible problem for now. It is a problem that can be fixed later
> on if needed by adjusting the host rpath mangling logic.

 I've taken a quick look at what I have in my build dirs, and there is no .so
file in another location except for dlopen()'d stuff.

 Regards,
 Arnout
Arnout Vandecappelle Nov. 7, 2017, 11:50 p.m. | #8
On 07-11-17 23:41, Thomas Petazzoni wrote:
> Hello,
> 
> On Sun, 5 Nov 2017 09:57:56 +0100, Yann E. MORIN wrote:
> 
>>> An alternative strategy would have been to keep the
>>> -Wl,-rpath,$(HOST_DIR) flag, and therefore the absolute RPATH in the
>>> host binaries, and fix such RPATH at the end of the build of every
>>> package. However, that would require calling fix-rpath after the
>>> installation of every package, which is a bit expensive.  
>>
>> I wonder how expensive that would be. It would be nice to time this.
> 
> OK, to time this, I did a quick hacky shell script
> (http://code.bulix.org/uboevd-223829). What it does is:
> 
>  - Generate a mix of ELF files and random data files (half of the files
>    are ELF files, half are random data files). This is the "generate"
>    target of the script.
> 
>  - Fixup rpath to $ORIGIN/../lib if not already fixed. We don't want to
>    fixup files repeatedly, as we don't want to duplicate files over and
>    over by breaking hard links. So we really want to set the RPATH only
>    once per binary.
> 
> Results are as follows:
> 
> $ ./rpath-evaluation.sh generate 0 500
> ... generate 250 random data files, 250 ELF programs with a crappy
> rpath ...
> 
> $ time ./rpath-evaluation.sh fixup
> 
> real	0m0.976s
> user	0m0.546s
> sys	0m0.457s
> $ time ./rpath-evaluation.sh fixup
> 
> real	0m0.610s
> user	0m0.393s
> sys	0m0.234s
> 
> So the first fixup pass, which actually fixes the RPATH of 250 binaries
> takes about one second. The next fixup pass doesn't do anything, except
> checking that all binaries already have a fixed RPATH.
> 
> So we see that it will take ~600ms to scan 500 files, provided half
> of them are ELF files, verify that they already have the correct RPATH.
> 
> To me, this sounds very reasonable. What do you think?

 500 files is not a lot if you include staging as well, so I've repeated the
experiment with 50000 files and on a HDD. First pass takes 3m34, second pass
3m43. Hm, perhaps I should have left my computer alone for a while during this
experiment :-).

 Anyway, I think the bottom line is that the fixup script should avoid going
over staging. But $(HOST_DIR)/{bin,sbin,lib} should be enough anyway.

 Regards,
 Arnout
Thomas Petazzoni Nov. 8, 2017, 8:55 a.m. | #9
Hello,

On Wed, 8 Nov 2017 00:50:15 +0100, Arnout Vandecappelle wrote:

> > To me, this sounds very reasonable. What do you think?  
> 
>  500 files is not a lot if you include staging as well

Why would I include staging? I don't see at all why fixing up RPATH in
staging after each package build would be necessary.

> so I've repeated the experiment with 50000 files and on a HDD. First
> pass takes 3m34, second pass 3m43. Hm, perhaps I should have left my
> computer alone for a while during this experiment :-).

It's not very logical that the second pass is longer than the first
pass, since the first pass does do patchelf invocations per file, while
the second pass does only one patchelf invocation per file.

>  Anyway, I think the bottom line is that the fixup script should
> avoid going over staging.

Why would it need to go over staging?

> But $(HOST_DIR)/{bin,sbin,lib} should be enough anyway.

Yes, that was my intent. If needed, we could optimize it by only doing
the fixup at the end of a host package installation. But since we have
a few target packages installing host stuff (toolchain, qt, etc.), it
would require them to explicitly state that they need host rpath
fixups. Perhaps it's easiest for now to just do the rpath fixup after
every package installation.

Best regards,

Thomas
Arnout Vandecappelle Nov. 8, 2017, 10:55 a.m. | #10
On 08-11-17 09:55, Thomas Petazzoni wrote:
> Hello,
> 
> On Wed, 8 Nov 2017 00:50:15 +0100, Arnout Vandecappelle wrote:
> 
>>> To me, this sounds very reasonable. What do you think?  
>>
>>  500 files is not a lot if you include staging as well
> 
> Why would I include staging? I don't see at all why fixing up RPATH in
> staging after each package build would be necessary.

 The simplest way would be to do find $(HOST_DIR) -type f but that includes staging.

> 
>> so I've repeated the experiment with 50000 files and on a HDD. First
>> pass takes 3m34, second pass 3m43. Hm, perhaps I should have left my
>> computer alone for a while during this experiment :-).
> 
> It's not very logical that the second pass is longer than the first
> pass, since the first pass does do patchelf invocations per file, while
> the second pass does only one patchelf invocation per file.
> 
>>  Anyway, I think the bottom line is that the fixup script should
>> avoid going over staging.
> 
> Why would it need to go over staging?
> 
>> But $(HOST_DIR)/{bin,sbin,lib} should be enough anyway.

 So this is not true, we also need $(HOST_DIR)/$(GNU_TARGET_NAME)/{bin,lib}.
So then it's perhaps easier to just -prune staging.

> Yes, that was my intent. If needed, we could optimize it by only doing
> the fixup at the end of a host package installation. But since we have
> a few target packages installing host stuff (toolchain, qt, etc.), it
> would require them to explicitly state that they need host rpath
> fixups. Perhaps it's easiest for now to just do the rpath fixup after
> every package installation.

 After every package installation would be better, yes.

 Regards,
 Arnout


> 
> Best regards,
> 
> Thomas
>
Thomas Petazzoni Nov. 8, 2017, 12:30 p.m. | #11
Hello,

On Wed, 8 Nov 2017 11:55:19 +0100, Arnout Vandecappelle wrote:

> >>> To me, this sounds very reasonable. What do you think?    
> >>
> >>  500 files is not a lot if you include staging as well  
> > 
> > Why would I include staging? I don't see at all why fixing up RPATH in
> > staging after each package build would be necessary.  
> 
>  The simplest way would be to do find $(HOST_DIR) -type f but that includes staging.

Hum, you didn't reply to my question, which was: "Why would we need to
fixup RPATHs in STAGING_DIR" ?

> >> But $(HOST_DIR)/{bin,sbin,lib} should be enough anyway.  
> 
>  So this is not true, we also need $(HOST_DIR)/$(GNU_TARGET_NAME)/{bin,lib}.
> So then it's perhaps easier to just -prune staging.

Absolutely.

> > Yes, that was my intent. If needed, we could optimize it by only doing
> > the fixup at the end of a host package installation. But since we have
> > a few target packages installing host stuff (toolchain, qt, etc.), it
> > would require them to explicitly state that they need host rpath
> > fixups. Perhaps it's easiest for now to just do the rpath fixup after
> > every package installation.  
> 
>  After every package installation would be better, yes.

ACK.

Thomas
Arnout Vandecappelle Nov. 8, 2017, 12:38 p.m. | #12
On 08-11-17 13:30, Thomas Petazzoni wrote:
> Hello,
> 
> On Wed, 8 Nov 2017 11:55:19 +0100, Arnout Vandecappelle wrote:
> 
>>>>> To me, this sounds very reasonable. What do you think?    
>>>>
>>>>  500 files is not a lot if you include staging as well  
>>>
>>> Why would I include staging? I don't see at all why fixing up RPATH in
>>> staging after each package build would be necessary.  
>>
>>  The simplest way would be to do find $(HOST_DIR) -type f but that includes staging.
> 
> Hum, you didn't reply to my question, which was: "Why would we need to
> fixup RPATHs in STAGING_DIR" ?

 We don't, obviously. However, if we naively go through HOST_DIR we will include
STAGING_DIR. I thought that was obvious :-)

 Regards,
 Arnout

> 
>>>> But $(HOST_DIR)/{bin,sbin,lib} should be enough anyway.  
>>
>>  So this is not true, we also need $(HOST_DIR)/$(GNU_TARGET_NAME)/{bin,lib}.
>> So then it's perhaps easier to just -prune staging.
> 
> Absolutely.
> 
>>> Yes, that was my intent. If needed, we could optimize it by only doing
>>> the fixup at the end of a host package installation. But since we have
>>> a few target packages installing host stuff (toolchain, qt, etc.), it
>>> would require them to explicitly state that they need host rpath
>>> fixups. Perhaps it's easiest for now to just do the rpath fixup after
>>> every package installation.  
>>
>>  After every package installation would be better, yes.
> 
> ACK.
> 
> Thomas
>

Patch

diff --git a/Makefile b/Makefile
index 79db7fe48a..5496273329 100644
--- a/Makefile
+++ b/Makefile
@@ -231,6 +231,8 @@  LEGAL_MANIFEST_CSV_HOST = $(LEGAL_INFO_DIR)/host-manifest.csv
 LEGAL_WARNINGS = $(LEGAL_INFO_DIR)/.warnings
 LEGAL_REPORT = $(LEGAL_INFO_DIR)/README
 
+export LD_LIBRARY_PATH = $(if $(PKG),$(HOST_DIR)/lib)
+
 ################################################################################
 #
 # staging and target directories do NOT list these as
@@ -558,7 +560,6 @@  world: target-post-image
 .PHONY: sdk
 sdk: world
 	@$(call MESSAGE,"Rendering the SDK relocatable")
-	$(TOPDIR)/support/scripts/fix-rpath host
 	$(TOPDIR)/support/scripts/fix-rpath staging
 	$(INSTALL) -m 755 $(TOPDIR)/support/misc/relocate-sdk.sh $(HOST_DIR)/relocate-sdk.sh
 	echo $(HOST_DIR) > $(HOST_DIR)/share/buildroot/sdk-location
@@ -679,6 +680,7 @@  $(TARGETS_ROOTFS): target-finalize
 .PHONY: target-finalize
 target-finalize: $(PACKAGES)
 	@$(call MESSAGE,"Finalizing target directory")
+	$(TOPDIR)/support/scripts/fix-rpath host
 	$(foreach hook,$(TARGET_FINALIZE_HOOKS),$($(hook))$(sep))
 	rm -rf $(TARGET_DIR)/usr/include $(TARGET_DIR)/usr/share/aclocal \
 		$(TARGET_DIR)/usr/lib/pkgconfig $(TARGET_DIR)/usr/share/pkgconfig \
diff --git a/package/Makefile.in b/package/Makefile.in
index a1a5316051..e94a75c230 100644
--- a/package/Makefile.in
+++ b/package/Makefile.in
@@ -220,7 +220,7 @@  HOST_CPPFLAGS  = -I$(HOST_DIR)/include
 HOST_CFLAGS   ?= -O2
 HOST_CFLAGS   += $(HOST_CPPFLAGS)
 HOST_CXXFLAGS += $(HOST_CFLAGS)
-HOST_LDFLAGS  += -L$(HOST_DIR)/lib -Wl,-rpath,$(HOST_DIR)/lib
+HOST_LDFLAGS  += -L$(HOST_DIR)/lib
 
 # The macros below are taken from linux 4.11 and adapted slightly.
 # Copy more when needed.
diff --git a/package/pkg-generic.mk b/package/pkg-generic.mk
index cca94ba338..82f8c06821 100644
--- a/package/pkg-generic.mk
+++ b/package/pkg-generic.mk
@@ -105,14 +105,6 @@  endef
 
 GLOBAL_INSTRUMENTATION_HOOKS += check_bin_arch
 
-# This hook checks that host packages that need libraries that we build
-# 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)))
-endef
-GLOBAL_INSTRUMENTATION_HOOKS += check_host_rpath
-
 define step_check_build_dir_one
 	if [ -d $(2) ]; then \
 		printf "%s: installs files in %s\n" $(1) $(2) >&2; \
diff --git a/support/scripts/fix-rpath b/support/scripts/fix-rpath
index 15705a3b0d..eed751f5da 100755
--- a/support/scripts/fix-rpath
+++ b/support/scripts/fix-rpath
@@ -61,7 +61,7 @@  main() {
     local rootdir
     local tree="${1}"
     local find_args=( )
-    local sanitize_extra_args=( )
+    local sanitize_args=( )
 
     if ! "${PATCHELF}" --version > /dev/null 2>&1; then
 	echo "Error: can't execute patchelf utility '${PATCHELF}'"
@@ -89,7 +89,8 @@  main() {
             cp "${PATCHELF}" "${PATCHELF}.__to_be_patched"
 
             # we always want $ORIGIN-based rpaths to make it relocatable.
-            sanitize_extra_args+=( "--relative-to-file" )
+            sanitize_args+=( "--set-rpath" )
+            sanitize_args+=( "\$ORIGIN/../lib" )
             ;;
 
         staging)
@@ -101,14 +102,18 @@  main() {
             done
 
             # should be like for the target tree below
-            sanitize_extra_args+=( "--no-standard-lib-dirs" )
+            sanitize_args+=( "--no-standard-lib-dirs" )
+            sanitize_args+=( "--make-rpath-relative" )
+            sanitize_args+=( "${rootdir}" )
             ;;
 
         target)
             rootdir="${TARGET_DIR}"
             # we don't want $ORIGIN-based rpaths but absolute paths without rootdir.
             # we also want to remove rpaths pointing to /lib or /usr/lib.
-            sanitize_extra_args+=( "--no-standard-lib-dirs" )
+            sanitize_args+=( "--no-standard-lib-dirs" )
+            sanitize_args+=( "--make-rpath-relative" )
+            sanitize_args+=( "${rootdir}" )
             ;;
 
         *)
@@ -125,7 +130,7 @@  main() {
             # 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}"
+            ${PATCHELF} ${sanitize_args[@]} "${file}"
             # restore the original permission
             test "${changed}" != "" && chmod u-w "${file}"
         fi