diff mbox

[4/4] bctoolbox: disable rpath

Message ID 20170214224402.8785-4-joerg.krause@embedded.rocks
State Accepted
Headers show

Commit Message

Jörg Krause Feb. 14, 2017, 10:44 p.m. UTC
By default, bctoolbox adds the rpath to the shared library. Prevent this
by setting `CMAKE_SKIP_RPATH` [1] to a true value.

[1] https://cmake.org/cmake/help/latest/variable/CMAKE_SKIP_RPATH.html

Signed-off-by: Jörg Krause <joerg.krause@embedded.rocks>
---
 package/bctoolbox/bctoolbox.mk | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

Comments

Baruch Siach Feb. 15, 2017, 5:28 a.m. UTC | #1
Hi Jörg,

On Tue, Feb 14, 2017 at 11:44:02PM +0100, Jörg Krause wrote:
> By default, bctoolbox adds the rpath to the shared library. Prevent this
> by setting `CMAKE_SKIP_RPATH` [1] to a true value.
> 
> [1] https://cmake.org/cmake/help/latest/variable/CMAKE_SKIP_RPATH.html
> 
> Signed-off-by: Jörg Krause <joerg.krause@embedded.rocks>
> ---
>  package/bctoolbox/bctoolbox.mk | 5 ++++-
>  1 file changed, 4 insertions(+), 1 deletion(-)
> 
> diff --git a/package/bctoolbox/bctoolbox.mk b/package/bctoolbox/bctoolbox.mk
> index 5eea0575f..76737aa3c 100644
> --- a/package/bctoolbox/bctoolbox.mk
> +++ b/package/bctoolbox/bctoolbox.mk
> @@ -11,11 +11,14 @@ BCTOOLBOX_LICENSE_FILES = COPYING
>  BCTOOLBOX_DEPENDENCIES = mbedtls
>  BCTOOLBOX_INSTALL_STAGING = YES
>  
> +# Set CMAKE_SKIP_RPATH to prevent bctoolbox from adding the rpath to
> +# shared library.
>  BCTOOLBOX_CONF_OPTS = \
>  	-DENABLE_STRICT=OFF \
>  	-DENABLE_TESTS_COMPONENT=OFF \
>  	-DENABLE_TESTS=OFF \
> -	-DGIT_EXECUTABLE=OFF
> +	-DGIT_EXECUTABLE=OFF \
> +	-DCMAKE_SKIP_RPATH=ON

Shouldn't we have this in package/pkg-cmake.mk? Samuel?

baruch
Samuel Martin Feb. 15, 2017, 6:12 a.m. UTC | #2
On Wed, Feb 15, 2017 at 6:28 AM, Baruch Siach <baruch@tkos.co.il> wrote:
> Hi Jörg,
>
> On Tue, Feb 14, 2017 at 11:44:02PM +0100, Jörg Krause wrote:
>> By default, bctoolbox adds the rpath to the shared library. Prevent this
>> by setting `CMAKE_SKIP_RPATH` [1] to a true value.
>>
>> [1] https://cmake.org/cmake/help/latest/variable/CMAKE_SKIP_RPATH.html
>>
>> Signed-off-by: Jörg Krause <joerg.krause@embedded.rocks>
>> ---
>>  package/bctoolbox/bctoolbox.mk | 5 ++++-
>>  1 file changed, 4 insertions(+), 1 deletion(-)
>>
>> diff --git a/package/bctoolbox/bctoolbox.mk b/package/bctoolbox/bctoolbox.mk
>> index 5eea0575f..76737aa3c 100644
>> --- a/package/bctoolbox/bctoolbox.mk
>> +++ b/package/bctoolbox/bctoolbox.mk
>> @@ -11,11 +11,14 @@ BCTOOLBOX_LICENSE_FILES = COPYING
>>  BCTOOLBOX_DEPENDENCIES = mbedtls
>>  BCTOOLBOX_INSTALL_STAGING = YES
>>
>> +# Set CMAKE_SKIP_RPATH to prevent bctoolbox from adding the rpath to
>> +# shared library.
>>  BCTOOLBOX_CONF_OPTS = \
>>       -DENABLE_STRICT=OFF \
>>       -DENABLE_TESTS_COMPONENT=OFF \
>>       -DENABLE_TESTS=OFF \
>> -     -DGIT_EXECUTABLE=OFF
>> +     -DGIT_EXECUTABLE=OFF \
>> +     -DCMAKE_SKIP_RPATH=ON
>
> Shouldn't we have this in package/pkg-cmake.mk? Samuel?

For host package, we may want to enable rpath.
For target package, I think we should not set/force this option by the
infra because some packages may install some kind-of private libraries
elsewhere than in /usr/lib and set RPATH to in their binaries to find
them (e.g.: sudo or iptables, though they are cmake-based packages.).

Regards,
Thomas Petazzoni Feb. 15, 2017, 8:56 a.m. UTC | #3
Hello,

On Wed, 15 Feb 2017 07:28:46 +0200, Baruch Siach wrote:
> Hi Jörg,
> 
> On Tue, Feb 14, 2017 at 11:44:02PM +0100, Jörg Krause wrote:
> > By default, bctoolbox adds the rpath to the shared library. Prevent this
> > by setting `CMAKE_SKIP_RPATH` [1] to a true value.
> > 
> > [1] https://cmake.org/cmake/help/latest/variable/CMAKE_SKIP_RPATH.html
> > 
> > Signed-off-by: Jörg Krause <joerg.krause@embedded.rocks>
> > ---
> >  package/bctoolbox/bctoolbox.mk | 5 ++++-
> >  1 file changed, 4 insertions(+), 1 deletion(-)
> > 
> > diff --git a/package/bctoolbox/bctoolbox.mk b/package/bctoolbox/bctoolbox.mk
> > index 5eea0575f..76737aa3c 100644
> > --- a/package/bctoolbox/bctoolbox.mk
> > +++ b/package/bctoolbox/bctoolbox.mk
> > @@ -11,11 +11,14 @@ BCTOOLBOX_LICENSE_FILES = COPYING
> >  BCTOOLBOX_DEPENDENCIES = mbedtls
> >  BCTOOLBOX_INSTALL_STAGING = YES
> >  
> > +# Set CMAKE_SKIP_RPATH to prevent bctoolbox from adding the rpath to
> > +# shared library.
> >  BCTOOLBOX_CONF_OPTS = \
> >  	-DENABLE_STRICT=OFF \
> >  	-DENABLE_TESTS_COMPONENT=OFF \
> >  	-DENABLE_TESTS=OFF \
> > -	-DGIT_EXECUTABLE=OFF
> > +	-DGIT_EXECUTABLE=OFF \
> > +	-DCMAKE_SKIP_RPATH=ON  
> 
> Shouldn't we have this in package/pkg-cmake.mk? Samuel?

Indeed, it's a generic CMake option, so it should be in
package/pkg-cmake.mk IMO.

Worth mentioning that there is both CMAKE_SKIP_RPATH and
CMAKE_SKIP_INSTALL_RPATH. The former skips adding the rpath during both
the build and install steps, which prevents from running the binary
from the build directory. The latter only skips the rpath during the
install step. But since we're cross-compiling, running on the host
machine from the build directory anyway doesn't make sense. So
CMAKE_SKIP_RPATH is OK.

See https://cmake.org/cmake/help/v3.0/variable/CMAKE_SKIP_RPATH.html.

Thomas
Thomas Petazzoni Feb. 15, 2017, 8:58 a.m. UTC | #4
Hello,

On Wed, 15 Feb 2017 07:12:28 +0100, Samuel Martin wrote:

> For target package, I think we should not set/force this option by the
> infra because some packages may install some kind-of private libraries
> elsewhere than in /usr/lib and set RPATH to in their binaries to find
> them (e.g.: sudo or iptables, though they are cmake-based packages.).

Hum, I didn't think about this, and you're probably right. If
CMAKE_SKIP_RPATH skips *all* rpath information, including some custom
one like pointing to /usr/lib/<foo>/, then we have to keep it indeed.

Thomas
Jörg Krause Feb. 19, 2017, 3:07 p.m. UTC | #5
Hi,

On Wed, 2017-02-15 at 09:56 +0100, Thomas Petazzoni wrote:
> Hello,
> 
> On Wed, 15 Feb 2017 07:28:46 +0200, Baruch Siach wrote:
> > Hi Jörg,
> > 
> > On Tue, Feb 14, 2017 at 11:44:02PM +0100, Jörg Krause wrote:
> > > By default, bctoolbox adds the rpath to the shared library.
> > > Prevent this
> > > by setting `CMAKE_SKIP_RPATH` [1] to a true value.
> > > 
> > > [1] https://cmake.org/cmake/help/latest/variable/CMAKE_SKIP_RPATH
> > > .html
> > > 
> > > Signed-off-by: Jörg Krause <joerg.krause@embedded.rocks>
> > > ---
> > >  package/bctoolbox/bctoolbox.mk | 5 ++++-
> > >  1 file changed, 4 insertions(+), 1 deletion(-)
> > > 
> > > diff --git a/package/bctoolbox/bctoolbox.mk
> > > b/package/bctoolbox/bctoolbox.mk
> > > index 5eea0575f..76737aa3c 100644
> > > --- a/package/bctoolbox/bctoolbox.mk
> > > +++ b/package/bctoolbox/bctoolbox.mk
> > > @@ -11,11 +11,14 @@ BCTOOLBOX_LICENSE_FILES = COPYING
> > >  BCTOOLBOX_DEPENDENCIES = mbedtls
> > >  BCTOOLBOX_INSTALL_STAGING = YES
> > >  
> > > +# Set CMAKE_SKIP_RPATH to prevent bctoolbox from adding the
> > > rpath to
> > > +# shared library.
> > >  BCTOOLBOX_CONF_OPTS = \
> > >  	-DENABLE_STRICT=OFF \
> > >  	-DENABLE_TESTS_COMPONENT=OFF \
> > >  	-DENABLE_TESTS=OFF \
> > > -	-DGIT_EXECUTABLE=OFF
> > > +	-DGIT_EXECUTABLE=OFF \
> > > +	-DCMAKE_SKIP_RPATH=ON  
> > 
> > Shouldn't we have this in package/pkg-cmake.mk? Samuel?
> 
> Indeed, it's a generic CMake option, so it should be in
> package/pkg-cmake.mk IMO.

opencv and opencv3 both set this option on a package level.

> Worth mentioning that there is both CMAKE_SKIP_RPATH and
> CMAKE_SKIP_INSTALL_RPATH. The former skips adding the rpath during
> both
> the build and install steps, which prevents from running the binary
> from the build directory. The latter only skips the rpath during the
> install step. But since we're cross-compiling, running on the host
> machine from the build directory anyway doesn't make sense. So
> CMAKE_SKIP_RPATH is OK.
> 
> See https://cmake.org/cmake/help/v3.0/variable/CMAKE_SKIP_RPATH.html.

Samuel argues not to force this option by the infrastructure. I am 	
not sure how do we handle this properly. So, what shall we do?

Best regards,
Jörg Krause
Thomas Petazzoni Feb. 19, 2017, 5:44 p.m. UTC | #6
Hello,

On Sun, 19 Feb 2017 16:07:35 +0100, Jörg Krause wrote:

> > Worth mentioning that there is both CMAKE_SKIP_RPATH and
> > CMAKE_SKIP_INSTALL_RPATH. The former skips adding the rpath during
> > both
> > the build and install steps, which prevents from running the binary
> > from the build directory. The latter only skips the rpath during the
> > install step. But since we're cross-compiling, running on the host
> > machine from the build directory anyway doesn't make sense. So
> > CMAKE_SKIP_RPATH is OK.
> > 
> > See https://cmake.org/cmake/help/v3.0/variable/CMAKE_SKIP_RPATH.html.  
> 
> Samuel argues not to force this option by the infrastructure. I am 	
> not sure how do we handle this properly. So, what shall we do?

As I said in my second e-mail (sent after reading Samuel's answer),
Samuel is probably correct. Skipping the rpath in *all* cases may not
be correct, so we have to do it at the per-package level, like your
patch does.

Thomas
diff mbox

Patch

diff --git a/package/bctoolbox/bctoolbox.mk b/package/bctoolbox/bctoolbox.mk
index 5eea0575f..76737aa3c 100644
--- a/package/bctoolbox/bctoolbox.mk
+++ b/package/bctoolbox/bctoolbox.mk
@@ -11,11 +11,14 @@  BCTOOLBOX_LICENSE_FILES = COPYING
 BCTOOLBOX_DEPENDENCIES = mbedtls
 BCTOOLBOX_INSTALL_STAGING = YES
 
+# Set CMAKE_SKIP_RPATH to prevent bctoolbox from adding the rpath to
+# shared library.
 BCTOOLBOX_CONF_OPTS = \
 	-DENABLE_STRICT=OFF \
 	-DENABLE_TESTS_COMPONENT=OFF \
 	-DENABLE_TESTS=OFF \
-	-DGIT_EXECUTABLE=OFF
+	-DGIT_EXECUTABLE=OFF \
+	-DCMAKE_SKIP_RPATH=ON
 
 # bctoolbox can be build with mbedTLS or PolarSSL support. If both
 # libraries are present, mbedTLS is preferred over PolarSSL.