Page MenuHomePhabricator

Make sure to use lib instead of lib64 for LLDB_LIB_DIR
ClosedPublic

Authored by fjricci on Apr 13 2016, 11:31 AM.

Details

Summary

$(lldb -P)/../../ is assumed to be the lldb library directory
by the test suite. However, it is possible that the python
libs would be installed in build/lib64 instead of build/lib.
Since liblldb.so is always installed in lib,
make sure this is always used as LLDB_LIB_DIR.

In cases where the python libs were already in build/lib, this
patch will not affect LLDB_LIB_DIR.

Diff Detail

Repository
rL LLVM

Event Timeline

fjricci updated this revision to Diff 53599.Apr 13 2016, 11:31 AM
fjricci retitled this revision from to Make sure to use lib instead of lib64 for LLDB_LIB_DIR.
fjricci updated this object.
fjricci added reviewers: zturner, vharron, emaste.
fjricci added subscribers: sas, lldb-commits.
labath added a subscriber: labath.Apr 14 2016, 1:40 AM

Actually, the destination for liblldb.so depends on the LLVM_LIBDIR_SUFFIX cmake variable. If you're on a system which likes to shove things into lib64, maybe you could follow suit and define the variable when you build llvm/lldb. (otherwise, this patch will break the logic for anyone who does use the variable.) Would that work for your use case?

When I use LLVM_LIBDIR_SUFFIX=64, it looks like the python _lldb.so symlink still points to build/lib/liblldb.so, instead of using the lib64 directory. So if I do this, none of the tests will run on the check-lldb target, since importing lldb in python fails.

Perhaps this symlink is the bug that should be fixed? However, it appears that finish-swig-Python-LLDB.sh already has the correct logic for this, so it's not immediately clear to me why the symlink is incorrect (if this seems like a bug we want fixed, I'll investigate):

ln -s "../../../liblldb${SOEXT}" _lldb.so

When I use LLVM_LIBDIR_SUFFIX=64, it looks like the python _lldb.so symlink still points to build/lib/liblldb.so, instead of using the lib64 directory. So if I do this, none of the tests will run on the check-lldb target, since importing lldb in python fails.

Perhaps this symlink is the bug that should be fixed? However, it appears that finish-swig-Python-LLDB.sh already has the correct logic for this, so it's not immediately clear to me why the symlink is incorrect (if this seems like a bug we want fixed, I'll investigate):

ln -s "../../../liblldb${SOEXT}" _lldb.so

Yes, I believe this would be a better fix for your problem.

Note that finish-swig-Python-LLDB.sh is not used anymore in the cmake build. It's replacement is finishSwigPythonLLDB.py so you'll need to look for the problem there.

So I looked at finishSwigPythonLLDB.py, and it does look like that's where the bug is. This script sets the end of the symlink path with:

strSrc = os.path.join("lib", "liblldb" + strLibFileExtn)

Note here that "lib" is hardcoded, and doesn't use the LLVM_LIBDIR_SUFFIX, which is why the symlink is broken. The only way I can think of to communicate this suffix from cmake into python would be by having cmake set an environment variable (presumably with the same name). Does this seem reasonable, or do you think there's a better solution?

The solution from finish-swig-Python-LLDB.sh would also probably work (enforcing that liblldb.so and python2.7/site-packages/lldb be in the same directory). That solution actually would be more consistent with the way the unit test suite works anyway. It does seem less flexible though.

zturner edited edge metadata.Apr 15 2016, 5:15 PM
zturner added a subscriber: zturner.

Don't use an environment variable, add a command line flag to the script
instead

Yes, please use a command-line argument to pass that information into python.

fjricci updated this revision to Diff 54077.Apr 18 2016, 9:58 AM
fjricci edited edge metadata.

Pass the lldb lib directory to python swig scripts

zturner accepted this revision.Apr 18 2016, 10:58 AM
zturner edited edge metadata.
This revision is now accepted and ready to land.Apr 18 2016, 10:58 AM
This revision was automatically updated to reflect the committed changes.