Patchwork [RFC,v3] chrpath.bbclass: strip common parent directories from rpath

login
register
mail settings
Submitter Andreas Oberritter
Date July 2, 2013, 9:50 a.m.
Message ID <1372758642-6302-1-git-send-email-obi@opendreambox.org>
Download mbox | patch
Permalink /patch/52789/
State Superseded
Headers show

Comments

Andreas Oberritter - July 2, 2013, 9:50 a.m.
Allows to use shorter TMPDIRs in corner cases, e.g. with native
perl modules, having a deep directory structure.

The problem is that the original absolute rpath may be shorter
than the newly generated relative rpath.

The failing rpaths were '/ab/cde/tmp/sysroots/x86_64-linux/usr/lib'
(old) and '$ORIGIN/../../../../../../../../../usr/lib' (new) for
'/ab/cde/tmp/sysroots/x86_64-linux/usr/lib/perl-native/perl/5.14.3/auto/XML/Parser/Expat/Expat.so'

The new rpath is '$ORIGIN/../../../../../../..'.

The new code doesn't just compare libdir, because I guess it
should also work if only parts of libdir share a parent directory
with the binary.

This patch also adds a check for len(basedir) > 0, because I think
basedir is empty in the cross-compile case, in which case
rpath.find(basedir) would always return 0, but I'm unsure whether
I understood that part of the code correctly or not.

[YOCTO #3989]

Signed-off-by: Andreas Oberritter <obi@opendreambox.org>
---
v2: Fixed reassembly of libpath
v3: Really fixed reassembly of libpath:
    libpath = '/' + '/'.join(libdirs[nparents + 1:])

 I've been using this version of the patch since about four months ago
 and didn't notice any problems.

 meta/classes/chrpath.bbclass | 17 +++++++++++++++--
 1 file changed, 15 insertions(+), 2 deletions(-)
Phil Blundell - July 30, 2013, 3:24 p.m.
On Tue, 2013-07-02 at 11:50 +0200, Andreas Oberritter wrote:
> Allows to use shorter TMPDIRs in corner cases, e.g. with native
> perl modules, having a deep directory structure.
> 
> The problem is that the original absolute rpath may be shorter
> than the newly generated relative rpath.

Can we not just fix chrpath to cope with the case where the new path is
longer than the old one?  It seems a bit sad to require
ever-more-convoluted workarounds for what is essentially a tool problem.

p.
Khem Raj - July 30, 2013, 3:27 p.m.
On Jul 30, 2013, at 8:24 AM, Phil Blundell <pb@pbcl.net> wrote:

> On Tue, 2013-07-02 at 11:50 +0200, Andreas Oberritter wrote:
>> Allows to use shorter TMPDIRs in corner cases, e.g. with native
>> perl modules, having a deep directory structure.
>> 
>> The problem is that the original absolute rpath may be shorter
>> than the newly generated relative rpath.
> 
> Can we not just fix chrpath to cope with the case where the new path is
> longer than the old one?  It seems a bit sad to require
> ever-more-convoluted workarounds for what is essentially a tool problem.
> 

another option is to dump chrpath in favour of something which does it elegantly - patchelf
I had propsed patch set for that last year here

http://git.openembedded.org/openembedded-core-contrib/log/?h=kraj/patchelf


> p.
> 
> 
> _______________________________________________
> Openembedded-core mailing list
> Openembedded-core@lists.openembedded.org
> http://lists.openembedded.org/mailman/listinfo/openembedded-core
Phil Blundell - July 30, 2013, 3:34 p.m.
On Tue, 2013-07-30 at 08:27 -0700, Khem Raj wrote:
> On Jul 30, 2013, at 8:24 AM, Phil Blundell <pb@pbcl.net> wrote:
> 
> > On Tue, 2013-07-02 at 11:50 +0200, Andreas Oberritter wrote:
> >> Allows to use shorter TMPDIRs in corner cases, e.g. with native
> >> perl modules, having a deep directory structure.
> >> 
> >> The problem is that the original absolute rpath may be shorter
> >> than the newly generated relative rpath.
> > 
> > Can we not just fix chrpath to cope with the case where the new path is
> > longer than the old one?  It seems a bit sad to require
> > ever-more-convoluted workarounds for what is essentially a tool problem.
> > 
> 
> another option is to dump chrpath in favour of something which does it elegantly - patchelf

Right, or that.  I've never used patchelf myself but if it works better
than chrpath (which isn't an especially high bar to set) then that
sounds like a decent enough option.

p.
Andreas Oberritter - July 30, 2013, 6:47 p.m.
On 30.07.2013 17:34, Phil Blundell wrote:
> On Tue, 2013-07-30 at 08:27 -0700, Khem Raj wrote:
>> On Jul 30, 2013, at 8:24 AM, Phil Blundell <pb@pbcl.net> wrote:
>>
>>> On Tue, 2013-07-02 at 11:50 +0200, Andreas Oberritter wrote:
>>>> Allows to use shorter TMPDIRs in corner cases, e.g. with native
>>>> perl modules, having a deep directory structure.
>>>>
>>>> The problem is that the original absolute rpath may be shorter
>>>> than the newly generated relative rpath.
>>>
>>> Can we not just fix chrpath to cope with the case where the new path is
>>> longer than the old one?  It seems a bit sad to require
>>> ever-more-convoluted workarounds for what is essentially a tool problem.
>>>

I guess this can be done equally well in a later patch. I don't really
like this workaround either, but it improves the current situation
surprisingly well.

>>
>> another option is to dump chrpath in favour of something which does it elegantly - patchelf
> 
> Right, or that.  I've never used patchelf myself but if it works better
> than chrpath (which isn't an especially high bar to set) then that
> sounds like a decent enough option.

I won't have the time to work on either improving chrpath or integrating
patchelf. If Khem has a working solution, then let's integrate it.

AFAIR, the reasoning against patchelf was the bad availability across
distributions and it being a prerequisite for native builds. Today,
there's still no patchelf package in current releases of Debian or Ubuntu.

Regards,
Andreas
Trevor Woerner - July 31, 2013, 11:25 p.m.
On 30 July 2013 14:47, Andreas Oberritter <obi@opendreambox.org> wrote:
> AFAIR, the reasoning against patchelf was the bad availability across
> distributions and it being a prerequisite for native builds. Today,
> there's still no patchelf package in current releases of Debian or Ubuntu.

pseudo isn't part of most distributions either; but that doesn't mean
it can't be an important part of OE/Yocto :-) Couldn't patchelf-native
be built as part of the "getting your system ready to build" phase?

Khem, don't forget to add it to buildtools-tarball, the SDKs, and the
build applicance(s).

Patch

diff --git a/meta/classes/chrpath.bbclass b/meta/classes/chrpath.bbclass
index 0c7ab77..9255aa7 100644
--- a/meta/classes/chrpath.bbclass
+++ b/meta/classes/chrpath.bbclass
@@ -55,9 +55,22 @@  def process_dir (directory, d):
                 rpath =  os.path.normpath(rpath)
                 # If the rpath shares a root with base_prefix determine a new dynamic rpath from the
                 # base_prefix shared root
-                if rpath.find(basedir) != -1:
-                    depth = fpath.partition(basedir)[2].count('/')
+                if len(basedir) > 0 and rpath.find(basedir) != -1:
+                    relfpath = fpath.partition(basedir)[2].strip()
                     libpath = rpath.partition(basedir)[2].strip()
+                    depth = relfpath.count('/')
+                    # Compare common parent directories to reduce the length of the new rpath.
+                    # Ignore the first (empty) element.
+                    libdirs = libpath.split('/')
+                    relfdirs = relfpath.split('/')
+                    nparents = 0
+                    for i in range(1, min(len(libdirs), len(relfdirs))):
+                        if libdirs[i] != relfdirs[i]:
+                            break
+                        nparents += 1
+                    if nparents > 0:
+                        libpath = '/' + '/'.join(libdirs[nparents + 1:])
+                        depth -= nparents
                 # otherwise (i.e. cross packages) determine a shared root based on the TMPDIR
                 # NOTE: This will not work reliably for cross packages, particularly in the case
                 # where your TMPDIR is a short path (i.e. /usr/poky) as chrpath cannot insert an