Use absolute paths to lib and lib64 icw $ORIGIN for the RPATH by jdonners · Pull Request #2358 · easybuilders/easybuild-framework

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

+
        # 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

+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

+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

+        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

+    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


+        # 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

                    '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

        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

        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


        # 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

+        # 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

boegel mentioned this pull request Apr 23, 2025

allow specifying location for RPATH wrapper scripts via rpath_wrappers_dir + also pass rpath_include_dirs when preparing build environment for extensions #4596

Merged

		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)

Conversation

jdonners commented Nov 30, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

boegel commented Dec 8, 2017

Uh oh!

boegel commented Dec 10, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants