add support for RPATH linking (REVIEW)#1942

boegel · 2016-10-04T13:17:25Z

(continuation of the work started in #1613 by @rjeschmi)

With --rpath enabled, EasyBuild will generate a bash wrapper script for each compiler and linker command (e.g. icc, icpc, ifort, ld and ld.gold) during the toolchain prepare phase, where a couple of things are hardcoded into:

the name of the command being wrapped
the path to the original command
the path to a Python utility script (rpath_args.py)
the path to a log file specific to the wrapper script

rpath_args.py takes care of determining which -rpath option should be passed down to the original command $RPATH), based on the specified command line arguments for that command.
It may also filter/tweak the specified command line arguments that will actually be passed down to the original command ($CMD_ARGS).
The bash wrapper script will run rpath_args.py, evaluate the statements it prints out (which define $RPATH and $CMD_ARGS), and then call the original command using $RPATH and $CMD_ARGS).

The location of these wrapper scripts is prepended to $PATH to they get preference over the compiler/linker commands they wrap around.

The sanity check was extended to check the output of ldd and readelf -d on all dynamically linked executables and libraries (that reside in bin, lib and lib64 subdirectories) in case --rpath was used.

TODO:

[ ] more testing

in combination with option that avoids that $LD_LIBRARY_PATH is defined by generated modules (TODO)
[ ] fix the FIXMEs in rpath_args.py
[ ] end-to-end test for --rpath
[ ] documentation

Feedback welcome!

cc @geimer, @pescobar

…pare_rpath_wrapper (WIP)

…a generic script

…path

…r it

boegel · 2016-11-11T15:32:29Z

OK, time for one more update...

With the current state combined with easybuilders/easybuild-easyblocks#1031 (and easybuilders/easybuild-easyblocks#1030, but I think that's mostly irrelevant), eb --rpath can build the whole foss/2016a stack + all dependencies for OpenFOAM 3.0.0 (i.e. CMake, Boost, METIS, ...) without issues! \o/
The corresponding list of modules:

M4/1.4.17
Autoconf/2.69
Automake/1.15
libtool/2.4.6
flex/2.5.39
Autotools/20150215
Bison/3.0.4
zlib/1.2.8
binutils/2.25
GCCcore/4.9.3
M4/1.4.17-GCCcore-4.9.3
flex/2.5.39-GCCcore-4.9.3
Bison/3.0.4-GCCcore-4.9.3
zlib/1.2.8-GCCcore-4.9.3
binutils/2.25-GCCcore-4.9.3
GCC/4.9.3-2.25
OpenBLAS/0.2.15-GCC-4.9.3-2.25-LAPACK-3.6.0
numactl/2.0.11-GCC-4.9.3-2.25
hwloc/1.11.2-GCC-4.9.3-2.25
OpenMPI/1.10.2-GCC-4.9.3-2.25
gompi/2016a
FFTW/3.3.4-gompi-2016a
ScaLAPACK/2.0.2-gompi-2016a-OpenBLAS-0.2.15-LAPACK-3.6.0
foss/2016a
bzip2/1.0.6-foss-2016a
flex/2.5.39-foss-2016a
zlib/1.2.8-foss-2016a
M4/1.4.17-foss-2016a
ncurses/6.0-foss-2016a
SCOTCH/6.0.4-foss-2016a
CMake/3.4.3-foss-2016a
Boost/1.60.0-foss-2016a
Bison/3.0.4-foss-2016a
libreadline/6.3-foss-2016a
METIS/5.1.0-foss-2016a-32bitIDX

The OpenFOAM build itself is still running...

So, unless further testing reveals any serious problems, this is the version that is going to go in under --experimental in EasyBuild v3.0.

@wpoely86 do you mind giving this implementation a code review?

Once this is merged, I'll open up an issue to track any problems that pop up with the --rpath support that is included in EasyBuild v3.0.

…by default

wpoely86 · 2016-10-04T14:44:35Z

easybuild/framework/easyblock.py

+        dirpaths = [
+            os.path.join(self.installdir, 'bin'),
+            os.path.join(self.installdir, 'lib'),
+            os.path.join(self.installdir, 'lib64'),


some software (think OpenFOAM) doesn't follow this. In principle, you need to search everywhere.

I have reworked this so sanity_check_rpath (now a public method) accepts an argument rpath_dirs so we can specialise this in easyblocks (e.g. for OpenFOAM) as needed. We can also consider adding an easyconfig parameter like sanity_check_rpath_dirs in addition.

wpoely86 · 2016-10-04T15:57:50Z

easybuild/framework/easyblock.py

+                    # ELF 64-bit LSB shared object, x86-64, version 1 (SYSV), dynamically linked, not stripped
+                    if "dynamically linked" in out:
+                        # check whether all required libraries are found via 'ldd'
+                        out, ec = run_cmd("ldd %s" % path, simple=False)


it's not because a library is found, that we are using the correct one. A library can be both installed in the system and by EB (zlib, ...)

I don't think his intention was to check this very deeply. It is just a sanity check.

I agree that a good check is needed before this goes into production. It is pretty difficult though, since EB doesn't enforce linking only within EB.

You would need to have information about each dependency library to ensure those are linked against the correct EB version and ignore ones that are OS deps. I don't think this is reliable given current information in the easyconfigs.

Sure, but at least the library is either found via RPATH or in a default system location.

This is the best we can do in terms of a sanity check for RPATH imho, there's no guarantee here indeed...

I'm open to suggestions on how to check this more strictly (while keeping it relatively lightweight).

I would at the minimum print a warning about linkage to non EB paths. We can whitelist all glibc things but any other library needs to be looked after. It's not necessarily an error, but it needs to be checked.

OK, but I won't block this PR for that, we can do that as a follow-up, since it'll take a while before we get the white-listed right probably.

Also, that's sort of orthogonal to --rpath anyway, we should always be doing that to catch unintended linking to system libraries (but then we also need to take into account --filter-deps, which further complicates things).

I opened an issue for this, see #1991

wpoely86 · 2016-10-04T16:01:31Z

easybuild/scripts/rpath_args.py

@@ -0,0 +1,80 @@
+#!/usr/bin/env python


That doesn't work, you can only pass a single argument to a shebang.

And anyway, this doesn't matter much, we call the script with python rpath_args.py ... anyway (i.e. with the python that is used to run eb, i.e. sys.executable).

env passes the arguments. env python -E will just work.

Hmm, ok, seems like it does work.

But still, this is irrelevant, since we call this script with python rpath_args.py, bypassing the shebang entirely...

wpoely86 · 2016-10-04T16:02:15Z

easybuild/scripts/rpath_args.py

+    return res
+
+
+cmd = sys.argv[1]


put in if __main__

wpoely86 · 2016-10-04T16:06:38Z

easybuild/scripts/rpath_args.py

+
+# output: statement to define $RPATH
+if lib_paths:
+    print "export RPATH='%s=%s'" % (rpath_flag, ':'.join(sorted(lib_paths)))


use print( ) (py3)

wpoely86 · 2016-11-12T13:39:35Z

easybuild/scripts/rpath_args.py

+# add -rpath flags in front
+cmd_args = cmd_args_rpath + cmd_args
+
+if not version_mode:


what's the meaning of this version mode anyway?

see above, commands like gcc -v should not get broken by injecting -rpath flags

wpoely86 · 2016-11-12T13:41:03Z

easybuild/scripts/rpath_wrapper_template.sh

+
+# logging function
+function log {
+    echo "($$) [$(date "+%%Y-%%m-%%d %%H:%%M:%%S")] $1" >> %(rpath_wrapper_log)s


explain why the %% are here.

It does seem like it might be more obvious if the file had an extension other than .sh (.sh.in, or template or something). Is there not a saner template system for python used already by EB?

OK, will do.

@rjeschmi I like your suggestion to name it .sh.in, I'll apply that change.

wpoely86 · 2016-11-12T13:41:50Z

easybuild/scripts/rpath_wrapper_template.sh

@@ -0,0 +1,26 @@
+#!/bin/bash


why both a bash and python wrapper?

because iterating of the command line arguments in bash is damn nasty to implement imho, it can be done way cleaner in a Python script (which is easier to unit test too).

so again, why both a bash and python wrapper? Use a purely python one? Forks are costly.

I'm not sure that calling the original compiler/linker command is going to work when called from within a Python shell, I suspect that may lead to problems.

Total overhead of this bash+Python wrapper approach is 10-15%, which is very acceptable imho, you'll barely notice.

wpoely86 · 2016-11-12T13:49:51Z

easybuild/tools/filetools.py

    return var_defs, hits


+def find_eb_script(script_name):


lets cache this?

Pfft, I don't think that's worth the effort. find_eb_script is called once for the bash wrapper script, once for rpath_args.py per installation (by toolchain.prepare()). Caching isn't going to make a significant difference here (even when multiple installations are being done in one session).

wpoely86 · 2016-11-12T13:55:17Z

easybuild/tools/toolchain/toolchain.py

+        """Return list of relevant compilers for this toolchain"""
+
+        if self.name == DUMMY_TOOLCHAIN_NAME:
+            c_comps = ['gcc', 'g++']


I guess we could, but if we don't the RPATH sanity check should fail on builds done with dummy, and I haven't seen that happening.
We can do this when the need arises?

We could also (always!) wrap cc and c++ with a script that breaks the compilation, so we know when these are used. Because if they are, the compilers provided by a non-dummy toolchain won't be used...

Also fine by me. Some projects use cc/c++ before gcc/g++

rjeschmi

will work on some testing...

rjeschmi · 2016-11-12T16:25:13Z

easybuild/framework/easyblock.py

+                    # ELF 64-bit LSB shared object, x86-64, version 1 (SYSV), dynamically linked, not stripped
+                    if "dynamically linked" in out:
+                        # check whether all required libraries are found via 'ldd'
+                        out, ec = run_cmd("ldd %s" % path, simple=False)


I don't think his intention was to check this very deeply. It is just a sanity check.

I agree that a good check is needed before this goes into production. It is pretty difficult though, since EB doesn't enforce linking only within EB.

You would need to have information about each dependency library to ensure those are linked against the correct EB version and ignore ones that are OS deps. I don't think this is reliable given current information in the easyconfigs.

rjeschmi · 2016-11-12T16:34:54Z

easybuild/scripts/rpath_wrapper_template.sh

+
+# logging function
+function log {
+    echo "($$) [$(date "+%%Y-%%m-%%d %%H:%%M:%%S")] $1" >> %(rpath_wrapper_log)s


It does seem like it might be more obvious if the file had an extension other than .sh (.sh.in, or template or something). Is there not a saner template system for python used already by EB?

rjeschmi · 2016-11-12T16:36:31Z

easybuild/scripts/rpath_args.py

+    arg = args[idx]
+
+    # if command is run in 'version check' mode, make sure we don't include *any* -rpath arguments
+    if arg in ['-v', '-V', '--version', '-dumpversion']:


I think these options are related to flags that some build sent to ld or ld.gold and are being caught. The version mode below I think is to catch the bug related to rpath GCC that was noticed elsewhere.

…script

@rjeschmi

…owing @rjeschmi's suggestion

boegel · 2016-11-12T21:32:26Z

@wpoely86 I consider this good to go now. This is certainly not 100% finished, but that's exactly why we've pushed this under --experimental.

It will allow people to easily provide feedback on what we have in place now though, since it will be included in the upcoming EasyBuild v3.0.

Please submit an approved review, so this can be merged?

geimer · 2016-11-13T00:53:42Z

easybuild/scripts/rpath_args.py

+    rpath_filter = rpath_filter.split(',')
+else:
+    # don't RPATH system library locations by default
+    rpath_filter = ['/lib.*', '/usr/lib.*']


Remember that on a multiarch system (Debian, Ubuntu), system libraries may also be located in /usr/lib/x86_64-linux-gnu (and i386-linux-gnu, and aarch64-linux-gnu, and armhf-linux-gnu, and ...).

@geimer that's fine, those paths will match the /usr/lib* regex, so we don't inject -rpath flags for those locations, which is as intended

…rpath filter in options.py

…lter in toolchain.prepare

boegel · 2016-11-13T13:03:52Z

@wpoely86 now updated to included build dir and tmpdir in RPATH filter (passed down from prepare_step to toolchain.prepare)

wpoely86 · 2016-11-13T13:49:10Z

easybuild/framework/easyblock.py


+        # list of paths to include in RPATH filter;
+        # only include builddir if we're not building in installation directory
+        if os.getenv('TMPDIR'):


shouldn't we better use tempfile.tempdir?

I think you mean tempfile.gettempdir(), but sure, good point.

boegel · 2016-11-13T14:30:59Z

Thanks for the thorough review @wpoely86 and @rjeschmi!

EricDeveaud · 2016-11-16T09:27:19Z

Hello,

maybee it will looks a really dumb comment, but why don't include LIBRARY_PATH and LD_RUN_PATH export in modulefiles

we use this kind of approach on our current module setup.
egg boost C modulefile defines the following

those environment varaiables are natively used by the linker.

cixi:~ > module show boost/1_61_0
cixi:~ > show boost
-------------------------------------------------------------------
/local/gensoft2/devmodules/boost/1_61_0:

module-whatis    Set environnement for boost (1_61_0) 
module-whatis    topic_3316 
prepend-path     LIBRARY_PATH /local/gensoft2/lib/boost/1_61_0/lib 
prepend-path     LD_RUN_PATH /local/gensoft2/lib/boost/1_61_0/lib 
prepend-path     -d   LDFLAGS -L/local/gensoft2/lib/boost/1_61_0/lib 
prepend-path     CMAKE_LIBRARY_PATH /local/gensoft2/lib/boost/1_61_0/lib 
prepend-path     C_INCLUDE_PATH /local/gensoft2/lib/boost/1_61_0/include 
prepend-path     CPLUS_INCLUDE_PATH /local/gensoft2/lib/boost/1_61_0/include 
prepend-path     CMAKE_INCLUDE_PATH /local/gensoft2/lib/boost/1_61_0/include 
prepend-path     -d   CPPFLAGS -I/local/gensoft2/lib/boost/1_61_0/include 
setenv           BOOST_ROOT /local/gensoft2/lib/boost/1_61_0 
-------------------------------------------------------------------

loading boost and compiling versus this one provide binaries with accurate boost rpath
eg ufasta, part of masurca-3.1.3 (this one was compiled versus boost-1_61_1 and gcc-4.9.0

/local/gensoft2/exe/masurca/3.1.3/bin/ufasta:
        linux-vdso.so.1 =>  (0x00007ffc461dc000)
        libboost_regex.so.1.61.0 => /local/gensoft2/lib/boost/1_61_0/lib/libboost_regex.so.1.61.0 (0x00007f50efdef000)
        libbz2.so.1 => /lib64/libbz2.so.1 (0x00000034fb800000)
        libz.so.1 => /lib64/libz.so.1 (0x00000034f6800000)
        librt.so.1 => /lib64/librt.so.1 (0x00000034f6000000)
        libpthread.so.0 => /lib64/libpthread.so.0 (0x00000034f5800000)
        libstdc++.so.6 => /local/gensoft2/exe/gcc/4.9.0/lib64/libstdc++.so.6 (0x00007f50efacc000)
        libm.so.6 => /lib64/libm.so.6 (0x00000034f6400000)
        libgcc_s.so.1 => /local/gensoft2/exe/gcc/4.9.0/lib64/libgcc_s.so.1 (0x00007f50ef8b5000)
        libc.so.6 => /lib64/libc.so.6 (0x00000034f5400000)
        libicudata.so.42 => /usr/lib64/libicudata.so.42 (0x00007f50ee76a000)
        libicui18n.so.42 => /usr/lib64/libicui18n.so.42 (0x00007f50ee3d3000)
        libicuuc.so.42 => /usr/lib64/libicuuc.so.42 (0x00007f50ee081000)
        /lib64/ld-linux-x86-64.so.2 (0x00000034f5000000)

NB there is a "corner case" using this approach, when some rpath instructions are set in upstream compilation mechanism. as ld in this case ignores LD_RUN_PATH and LD_LIBRARY_PATH (man ld explain it pretty well)

edit: typo

boegel · 2016-11-23T21:08:25Z

@EricDeveaud let's follow up on the LD_RUN_PATH suggestion in easybuilders/easybuild#282

rjeschmi and others added 30 commits January 29, 2016 06:29

adding ld_wrapper.sh as provided

8e41d66

clean up some NSC variables

2dadc9d

almost not working version. ld load problems

d71b391

somewhat working version. Still need to get right binary called

5c2b6e5

update wrapper

f83ab79

working on expanding rpath to envvars

1ee637d

don't use a separate option group for --rpath

7f8f6c1

Merge branch 'develop' into rpath

f925252

move putting ld wrapper script in place into toolchain.prepare

6055f05

rework symlink_compilers into symlink_commads, start fleshing out pre…

a2e08dd

…pare_rpath_wrapper (WIP)

fix typo

4911df9

Merge branch 'develop' into rpath

02e2294

implement embedded fallthrough RPATH ld wrapper script

895c19a

add --rpath-debug configure option

d08caf9

generate dedicated wrapper script rather than installing symlinks to …

55b282f

…a generic script

use (dummy) Python script to filter cmd args & determine value for -r…

1e19406

…path

pass down cmd to rpath_args.py, add -Wl,

3cc103b

rpath flags depends on cmd

a57fdc9

Merge branch 'develop' into rpath

1ab9379

make rpath wrapper script log to file

76b91a8

fix logging of original args

a59638b

fix typo

b8e71ae

only enable logging in rpath wrapper under --debug

3933299

hard specify path to Python interpreter to use for rpath_args script

07e7041

make rpath wrapper script exit on any errors

81ec273

complete initial version of rpath_args script, together with tests fo…

714bb24

…r it

resolve to real path for rpath arguments

7c99002

Merge branch 'fix_travis' into rpath

dff89e3

add missing import in rpath_args script

ed82887

implement sanity checking of RPATH linking in binaries/libraries

44a41cc

boegel changed the title ~~add support for RPATH linking (WIP)~~ add support for RPATH linking (REVIEW) Nov 11, 2016

boegel added 4 commits November 12, 2016 10:26

require --experimental for --rpath

173121c

implement support for --rpath-filter, filter out /lib* and /usr/lib* …

3e5e602

…by default

correctly grab 'rpath_filter' build option

6385728

--rpath required --experimental in toy test

51ded8c

wpoely86 requested changes Nov 12, 2016

View reviewed changes

make sanity_check_rpath method public, add rpath_dirs argument

14688be

rjeschmi reviewed Nov 12, 2016

View reviewed changes

boegel added 5 commits November 12, 2016 21:43

fix minor remarks, add license header + description in rpath wrapper …

ade8336

…script

rename rpath_wrapper_template.sh to rpath_wrapper_template.sh.in foll…

84cebe4

…owing @rjeschmi's suggestion

interpet values in --rpath-filter as regexes

81b47a4

drop comment in template wrapper script that breaks templating

4ea6a34

avoid including % in comment in rpath wrapper script

0ff6284

rjeschmi approved these changes Nov 12, 2016

View reviewed changes

geimer reviewed Nov 13, 2016

View reviewed changes

boegel added 2 commits November 13, 2016 11:48

compile rpath filter to a single regex in rpath_args.py, set default …

3f809b2

…rpath filter in options.py

include build dir and tmpdir in RPATH filter, define default RPATH fi…

583a408

…lter in toolchain.prepare

wpoely86 reviewed Nov 13, 2016

View reviewed changes

include tmpdir in RPATH filter via tempfile.gettempdir()

2d38cd8

wpoely86 approved these changes Nov 13, 2016

View reviewed changes

boegel mentioned this pull request Nov 13, 2016

known issues with --rpath support #1992

Open

18 tasks

boegel merged commit 6c87b1f into easybuilders:develop Nov 14, 2016

boegel deleted the rpath branch November 14, 2016 23:47

Conversation

boegel commented Oct 4, 2016

Uh oh!

boegel commented Nov 11, 2016

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!

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!

Choose a reason for hiding this comment

Uh oh!

rjeschmi left a comment

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 Nov 12, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

boegel commented Nov 13, 2016

Uh oh!

EricDeveaud commented Nov 16, 2016 •

edited

Loading