Use absolute paths to lib and lib64 icw $ORIGIN for the RPATH #2358

jdonners · 2017-11-30T14:47:24Z

Add %installdir/lib, %installdir/lib64 and $ORIGIN to RPATH. This works much better with shared libraries in non-standard locations, esp. python packages. See the discussion in #2076. This patch has been tested for building Tensorflow+python (using foss-2016b) and VTK (for intel-2016b) from scratch. Works without hacks in the easyconfig.

… $ORIGIN to RPATH. This works much better with shared libraries in non-standard locations, esp. python packages

…ldir is defined. Shuffled tests to match the changed rpath mechanism.

boegel · 2017-12-01T07:14:31Z

easybuild/framework/easyblock.py

        # prepare toolchain: load toolchain module and dependencies, set up build environment
-        self.toolchain.prepare(self.cfg['onlytcmod'], silent=self.silent, rpath_filter_dirs=self.rpath_filter_dirs)
+        self.toolchain.prepare(self.cfg['onlytcmod'], silent=self.silent, rpath_filter_dirs=self.rpath_filter_dirs, rpath_include_dirs=self.rpath_include_dirs)


line too long (max 120 chars), so wrap it, like so:

self.toolchain.prepare(self.cfg['onlytcmod'], silent=self.silent, rpath_filter_dirs=self.rpath_filter_dirs, rpath_include_dirs=self.rpath_include_dirs)

boegel · 2017-12-01T07:17:50Z

easybuild/scripts/rpath_args.py

+cmd_args_rpath = []
+if rpath_include:
+    for path in rpath_include:
+        cmd_args_rpath.append(flag_prefix + '-rpath=%s' % path)


This can be collapsed to a single line, since we're sure rpath_include is a list (possibly empty, but that's fine):

cmd_args_rpath = [flag_prefix + '-rpath=%s' % inc for inc in rpath_include]

boegel · 2017-12-01T07:17:54Z

easybuild/scripts/rpath_args.py

+if not rpath_include:
+    rpath_include = []
+else:
+    rpath_include = rpath_include.split(',')


positive logic is easier to follow (and maybe worth a comment to clarify possible None value):

if rpath_include: rpath_include = rpath_include.split(',') else: # take into account that rpath_include may be None rpath_include = []

boegel · 2017-12-01T07:19:04Z

easybuild/tools/toolchain/toolchain.py

+        rpath_include = ','.join(['%s' % d for d in rpath_include_dirs or []])
+        self.log.debug("Combined RPATH includes: '%s'", rpath_include)
+
+


avoid duplicate blank line, just one is enough

boegel · 2017-12-01T07:19:15Z

easybuild/scripts/rpath_args.py

+    for path in rpath_include:
+        cmd_args_rpath.append(flag_prefix + '-rpath=%s' % path)
+
+


avoid duplicate blank line, just one is enough

boegel · 2017-12-01T07:23:03Z

easybuild/tools/toolchain/toolchain.py

@@ -796,6 +797,12 @@ def prepare_rpath_wrappers(self, rpath_filter_dirs=None):
        rpath_filter = ','.join(rpath_filter + ['%s.*' % d for d in rpath_filter_dirs or []])
        self.log.debug("Combined RPATH filter: '%s'", rpath_filter)

+        # figure out list of patterns to use in rpath include
+        # rpath_include = build_option('rpath_include')
+        rpath_include = ','.join(['%s' % d for d in rpath_include_dirs or []])


'%s' % d doesn't do much, since the directories are already strings, no?

So this is equivalent with:

rpath_include = ','.join(rpath_include_dirs or [])

boegel · 2017-12-01T07:28:24Z

easybuild/tools/toolchain/toolchain.py

@@ -824,6 +831,7 @@ def prepare_rpath_wrappers(self, rpath_filter_dirs=None):
                    'orig_cmd': orig_cmd,
                    'python': sys.executable,
                    'rpath_args_py': rpath_args_py,
+                    'rpath_include': rpath_include,


Please keep this sorted alphabetically (i comes after f ;-)).

boegel · 2017-12-01T07:28:28Z

easybuild/tools/toolchain/toolchain.py

@@ -796,6 +797,12 @@ def prepare_rpath_wrappers(self, rpath_filter_dirs=None):
        rpath_filter = ','.join(rpath_filter + ['%s.*' % d for d in rpath_filter_dirs or []])
        self.log.debug("Combined RPATH filter: '%s'", rpath_filter)

+        # figure out list of patterns to use in rpath include
+        # rpath_include = build_option('rpath_include')


remove this commented out line?

boegel · 2017-12-01T07:29:39Z

test/framework/toolchain.py

        self.assertEqual(ec, 0)
        expected = '\n'.join([
            "CMD_ARGS=('foo.o')",
-            "RPATH_ARGS='--disable-new-dtags -rpath=$ORIGIN/../lib -rpath=$ORIGIN/../lib64'",
+            "RPATH_ARGS='--disable-new-dtags -rpath=%s/lib -rpath=%s/lib64 -rpath=$ORIGIN'" % (self.test_prefix, self.test_prefix),


"RPATH_ARGS='--disable-new-dtags -rpath=%(p)s/lib -rpath=%(p)s/lib64 -rpath=$ORIGIN'" % {'p': self.test_prefix},

boegel · 2017-12-01T07:29:41Z

test/framework/toolchain.py

@@ -1066,61 +1066,66 @@ def test_rpath_args_script(self):
        script = find_eb_script('rpath_args.py')

        # simplest possible compiler command
-        out, ec = run_cmd("%s gcc '' -c foo.c" % script, simple=False)
+        out, ec = run_cmd("%s gcc '' '%s/lib,%s/lib64,$ORIGIN' -c foo.c" % (script, self.test_prefix, self.test_prefix), simple=False)


Construct the argument separately, since you're using it in multiple tests.
This also avoids making the line too long.

rpath_inc = '%(prefix)s/lib,%(prefix)s/lib64,$ORIGIN' % {'prefix': self.test_prefix} out, ec = run_cmd("%s gcc '' '%s' -c foo.c" % (script, rpath_inc), simple=False)

And then reuse rpath_inc below too.

boegel · 2017-12-07T14:28:19Z

easybuild/framework/easyblock.py

+        # see also https://linux.die.net/man/8/ld-linux;
+        self.rpath_include_dirs.append(self.installdir+'/lib')
+        self.rpath_include_dirs.append(self.installdir+'/lib64')
+        self.rpath_include_dirs.append('$ORIGIN')


@jdonners Shouldn't we also retain $ORIGIN/../lib and $ORIGIN/../lib64 in here? That's not necessarily the same as <installdir>/lib and <installdir>/lib64, is it?

@boegel indeed they're not necessarily the same. Conservatively, I thought it's better to add fewer paths to the RPATH. I'd expect that packages that install shared objects in non-standard locations, also somehow make these directories findable in some wrapper scripts. There's no guarantee that that is indeed the case. On the other side, it still is an experimental feature..

For now, I'd stick to also included $ORIGIIN/../lib and $ORIGIN/../lib64.

It indeed makes sense to limit the amount of paths we inject, but 3 or 5 isn't going to make a big difference I think.

The order may be important though (w.r.t. speeding up the loader): we should probably list the most common locations first?

@jdonners Fixed in sara-nl#1

…s is used

…ected by RPATH compiler wrapper

boegel · 2017-12-08T17:39:46Z

@jdonners Please review/merge sara-nl#1, with that included this is good to go imho.

enhance test to catch fixed bug + re-instate injection of $ORIGIN/../lib and $ORIGIN/../lib64

boegel · 2017-12-10T07:16:44Z

Going in, thanks @jdonners!

John Donners added 4 commits November 30, 2017 15:32

Change to RPATH injection. Add %installdir/lib, %installdir/lib64 and…

c483704

… $ORIGIN to RPATH. This works much better with shared libraries in non-standard locations, esp. python packages

Added comma

0a74817

Adjusted tests to new rpath_args.py

d510fbb

Include paths are now passed down from easyblock.py, where the instal…

2150c55

…ldir is defined. Shuffled tests to match the changed rpath mechanism.

easybuilders deleted a comment from boegelbot Dec 1, 2017

boegel added this to the 3.5.0 milestone Dec 1, 2017

boegel requested changes Dec 1, 2017

View reviewed changes

boegel added the enhancement label Dec 1, 2017

John Donners added 3 commits December 6, 2017 10:43

Changed accordingly

623aacb

Fixed typo

9ce6d57

Improved python code

2d8ed12

easybuilders deleted a comment from boegelbot Dec 7, 2017

boegel reviewed Dec 7, 2017

View reviewed changes

boegel added 2 commits December 8, 2017 17:39

enhance test_toy_rpath to make sure correct list of rpath filter path…

3470201

…s is used

re-instate $ORIGIN/../lib and $ORIGIN/../lib64 in RPATH locations inj…

eb5edab

…ected by RPATH compiler wrapper

boegel mentioned this pull request Dec 8, 2017

enhance test to catch fixed bug + re-instate injection of $ORIGIN/../lib and $ORIGIN/../lib64 sara-nl/easybuild-framework#1

Merged

Merge pull request #1 from boegel/abs_rpath

25577b3

enhance test to catch fixed bug + re-instate injection of $ORIGIN/../lib and $ORIGIN/../lib64

boegel approved these changes Dec 10, 2017

View reviewed changes

boegel merged commit e86fa6f into easybuilders:develop Dec 10, 2017

boegel mentioned this pull request May 8, 2018

RPATH with Python and virtualenv #2076

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Use absolute paths to lib and lib64 icw $ORIGIN for the RPATH #2358

Use absolute paths to lib and lib64 icw $ORIGIN for the RPATH #2358

jdonners commented Nov 30, 2017

boegel Dec 1, 2017

boegel Dec 1, 2017

boegel Dec 1, 2017

boegel Dec 1, 2017

boegel Dec 1, 2017

boegel Dec 1, 2017

boegel Dec 1, 2017

boegel Dec 1, 2017

boegel Dec 1, 2017

boegel Dec 1, 2017

boegel Dec 7, 2017

jdonners Dec 8, 2017

boegel Dec 8, 2017

boegel Dec 8, 2017

boegel commented Dec 8, 2017

boegel commented Dec 10, 2017

		rpath_include = ','.join(['%s' % d for d in rpath_include_dirs or []])
		self.log.debug("Combined RPATH includes: '%s'", rpath_include)

		for path in rpath_include:
		cmd_args_rpath.append(flag_prefix + '-rpath=%s' % path)

Use absolute paths to lib and lib64 icw $ORIGIN for the RPATH #2358

Use absolute paths to lib and lib64 icw $ORIGIN for the RPATH #2358

Conversation

jdonners commented Nov 30, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

boegel commented Dec 8, 2017

boegel commented Dec 10, 2017