Patchwork cmake.bbclass: set the rpath for binaries build with cmake to ${libdir}

login
register
mail settings
Submitter Simon Busch
Date May 4, 2011, 3:50 p.m.
Message ID <1304524228-25642-1-git-send-email-morphis@gravedo.de>
Download mbox | patch
Permalink /patch/3193/
State Superseded
Headers show

Comments

Simon Busch - May 4, 2011, 3:50 p.m.
In the default configuration cmake does not set a rpath for its builded binaries. This
leads to errors at runtime when the binaries not find their needed runtime libraries.

Signed-off-by: Simon Busch <morphis@gravedo.de>
---
 classes/cmake.bbclass |    4 ++++
 1 files changed, 4 insertions(+), 0 deletions(-)
Phil Blundell - May 4, 2011, 4:09 p.m.
On Wed, 2011-05-04 at 17:50 +0200, Simon Busch wrote:
> +# We need to set the rpath to the correct directory as cmake does not provide any
> +# directory as rpath by default
> +  echo "set( CMAKE_INSTALL_RPATH ${libdir} )" >> ${WORKDIR}/toolchain.cmake

Shouldn't this be inside some kind of guard for native?  You don't want
rpath in target binaries, do you?

p.
Simon Busch - May 4, 2011, 4:19 p.m.
On 04.05.2011 18:09, Phil Blundell wrote:
> On Wed, 2011-05-04 at 17:50 +0200, Simon Busch wrote:
>> +# We need to set the rpath to the correct directory as cmake does not provide any
>> +# directory as rpath by default
>> +  echo "set( CMAKE_INSTALL_RPATH ${libdir} )" >> ${WORKDIR}/toolchain.cmake
> 
> Shouldn't this be inside some kind of guard for native?  You don't want
> rpath in target binaries, do you?

Thanks for the hint!

You mean something like:

if not bb.data.inherits_class('native', d)
   and not bb.data.inherits_class('cross', d):

[...]

?

regards,
Simon
Phil Blundell - May 4, 2011, 4:54 p.m.
On Wed, 2011-05-04 at 18:19 +0200, Simon Busch wrote:
> On 04.05.2011 18:09, Phil Blundell wrote:
> > On Wed, 2011-05-04 at 17:50 +0200, Simon Busch wrote:
> >> +# We need to set the rpath to the correct directory as cmake does not provide any
> >> +# directory as rpath by default
> >> +  echo "set( CMAKE_INSTALL_RPATH ${libdir} )" >> ${WORKDIR}/toolchain.cmake
> > 
> > Shouldn't this be inside some kind of guard for native?  You don't want
> > rpath in target binaries, do you?
> 
> Thanks for the hint!
> 
> You mean something like:
> 
> if not bb.data.inherits_class('native', d)
>    and not bb.data.inherits_class('cross', d):

I think you have the sense backwards but yes, that's the general idea.
Though I must admit I don't quite understand why the rpath setting from
BUILD_LDFLAGS is not turning up in cmake's final link.  It might be
worth investigating why that isn't working for you in case this patch is
just papering over some deeper problem.

p.
Simon Busch - May 4, 2011, 5 p.m.
On 04.05.2011 18:54, Phil Blundell wrote:
> On Wed, 2011-05-04 at 18:19 +0200, Simon Busch wrote:
>> On 04.05.2011 18:09, Phil Blundell wrote:
>>> On Wed, 2011-05-04 at 17:50 +0200, Simon Busch wrote:
>>>> +# We need to set the rpath to the correct directory as cmake does not provide any
>>>> +# directory as rpath by default
>>>> +  echo "set( CMAKE_INSTALL_RPATH ${libdir} )" >> ${WORKDIR}/toolchain.cmake
>>>
>>> Shouldn't this be inside some kind of guard for native?  You don't want
>>> rpath in target binaries, do you?
>>
>> Thanks for the hint!
>>
>> You mean something like:
>>
>> if not bb.data.inherits_class('native', d)
>>    and not bb.data.inherits_class('cross', d):
> 
> I think you have the sense backwards but yes, that's the general idea.
> Though I must admit I don't quite understand why the rpath setting from
> BUILD_LDFLAGS is not turning up in cmake's final link.  It might be
> worth investigating why that isn't working for you in case this patch is
> just papering over some deeper problem.

I alreay searched through the cmake documentation and it says that the
rpath is set with the content of the CMAKE_INSTALL_RPATH variable. The
default value of CMAKE_INSTALL_RPATH is ""[1]. As currently nothing sets
it to another value the rpath is never set during the build for the
resulting binaries. I send another patch which does the solution
mentioned above the right way.

regards,
Simon

[1]: http://www.cmake.org/Wiki/CMake_RPATH_handling
Phil Blundell - May 5, 2011, 9:50 a.m.
On Wed, 2011-05-04 at 19:00 +0200, Simon Busch wrote:
> I alreay searched through the cmake documentation and it says that the
> rpath is set with the content of the CMAKE_INSTALL_RPATH variable. The
> default value of CMAKE_INSTALL_RPATH is ""[1]. As currently nothing sets
> it to another value the rpath is never set during the build for the
> resulting binaries. I send another patch which does the solution
> mentioned above the right way.

Oh right, I see.  So cmake is actually stripping the rpaths during the
install step unless you tell it that you want them included?

In that case, yeah, your plan seems reasonable.

p.
Simon Busch - May 5, 2011, 11:21 a.m.
On 05.05.2011 11:50, Phil Blundell wrote:
> On Wed, 2011-05-04 at 19:00 +0200, Simon Busch wrote:
>> I alreay searched through the cmake documentation and it says that the
>> rpath is set with the content of the CMAKE_INSTALL_RPATH variable. The
>> default value of CMAKE_INSTALL_RPATH is ""[1]. As currently nothing sets
>> it to another value the rpath is never set during the build for the
>> resulting binaries. I send another patch which does the solution
>> mentioned above the right way.
> 
> Oh right, I see.  So cmake is actually stripping the rpaths during the
> install step unless you tell it that you want them included?
>
> In that case, yeah, your plan seems reasonable.

Ok, so you are fine with the patch?

regards,
Simon
Phil Blundell - May 5, 2011, 11:34 a.m.
On Thu, 2011-05-05 at 13:21 +0200, Simon Busch wrote:
> On 05.05.2011 11:50, Phil Blundell wrote:
> > On Wed, 2011-05-04 at 19:00 +0200, Simon Busch wrote:
> >> I alreay searched through the cmake documentation and it says that the
> >> rpath is set with the content of the CMAKE_INSTALL_RPATH variable. The
> >> default value of CMAKE_INSTALL_RPATH is ""[1]. As currently nothing sets
> >> it to another value the rpath is never set during the build for the
> >> resulting binaries. I send another patch which does the solution
> >> mentioned above the right way.
> > 
> > Oh right, I see.  So cmake is actually stripping the rpaths during the
> > install step unless you tell it that you want them included?
> >
> > In that case, yeah, your plan seems reasonable.
> 
> Ok, so you are fine with the patch?

Yup.  My only remaining concern was that it seemed a bit unwholesome to
do:

+  echo "set( CMAKE_INSTALL_RPATH ${OECMAKE_RPATH} )" >> ${WORKDIR}/toolchain.cmake

when OECMAKE_RPATH might be empty, and maybe it'd have been better to
bracket that echo with "if [ -n ${OECMAKE_RPATH} ] ...".  But if you've
tested it and cmake does the right thing then I guess it's fine as it
stands.

Acked-by: Phil Blundell <philb@gnu.org>

p.
Simon Busch - May 5, 2011, 11:44 a.m.
On 05.05.2011 13:34, Phil Blundell wrote:
> On Thu, 2011-05-05 at 13:21 +0200, Simon Busch wrote:
>> On 05.05.2011 11:50, Phil Blundell wrote:
>>> On Wed, 2011-05-04 at 19:00 +0200, Simon Busch wrote:
>>>> I alreay searched through the cmake documentation and it says that the
>>>> rpath is set with the content of the CMAKE_INSTALL_RPATH variable. The
>>>> default value of CMAKE_INSTALL_RPATH is ""[1]. As currently nothing sets
>>>> it to another value the rpath is never set during the build for the
>>>> resulting binaries. I send another patch which does the solution
>>>> mentioned above the right way.
>>>
>>> Oh right, I see.  So cmake is actually stripping the rpaths during the
>>> install step unless you tell it that you want them included?
>>>
>>> In that case, yeah, your plan seems reasonable.
>>
>> Ok, so you are fine with the patch?
> 
> Yup.  My only remaining concern was that it seemed a bit unwholesome to
> do:
> 
> +  echo "set( CMAKE_INSTALL_RPATH ${OECMAKE_RPATH} )" >> ${WORKDIR}/toolchain.cmake
> 
> when OECMAKE_RPATH might be empty, and maybe it'd have been better to
> bracket that echo with "if [ -n ${OECMAKE_RPATH} ] ...".  But if you've
> tested it and cmake does the right thing then I guess it's fine as it
> stands.
> 
> Acked-by: Phil Blundell <philb@gnu.org>

Ok, thanks for your help. Patch is now pushed but somehow cgit got the
line breaks wrong while local git is showing them correctly ...

regards,
Simon

Patch

diff --git a/classes/cmake.bbclass b/classes/cmake.bbclass
index 40fadea..5f7066b 100644
--- a/classes/cmake.bbclass
+++ b/classes/cmake.bbclass
@@ -44,6 +44,10 @@  cmake_do_generate_toolchain_file() {
   echo "set( CMAKE_FIND_ROOT_PATH_MODE_INCLUDE ONLY )" >> ${WORKDIR}/toolchain.cmake
 # Use qt.conf settings
   echo "set( ENV{QT_CONF_PATH} ${WORKDIR}/qt.conf )" >> ${WORKDIR}/toolchain.cmake
+
+# We need to set the rpath to the correct directory as cmake does not provide any
+# directory as rpath by default
+  echo "set( CMAKE_INSTALL_RPATH ${libdir} )" >> ${WORKDIR}/toolchain.cmake
 }
 
 addtask generate_toolchain_file after do_patch before do_configure