Page MenuHomePhabricator

Change the way SWIG is found, and fixed a typo in OS detection
ClosedPublic

Authored by ismailp on Nov 16 2014, 2:09 PM.

Details

Summary

SWIG is searched under certain paths within python script. CMake can
detect SWIG with find_package(SWIG). This is used iff user checks
LLDB_ENABLE_PYTHON_SCRIPTS_SWIG_API_GENERATION. If
buildSwigWrapperClasses.py does not receive swigExecutable argument,
then script will use its current search implementation.

This patch also fixes a typo in OS detection, where there is one too
many equals in the assignment for Darwin.

Diff Detail

Event Timeline

ismailp updated this revision to Diff 16278.Nov 16 2014, 2:09 PM
ismailp retitled this revision from to Change the way SWIG is found, and fixed a typo in OS detection.
ismailp updated this object.
ismailp edited the test plan for this revision. (Show Details)
ismailp added a reviewer: granata.enrico.
ismailp added a subscriber: Unknown Object (MLST).

I'll take a look at this on Monday. What does this fix exactly? Does it just mean that swig no longer has to be in system PATH because it can be found by CMake?

emaste added a subscriber: emaste.Nov 16 2014, 5:32 PM

On Darwin, if LLDB_ENABLE_PYTHON_SCRIPTS_SWIG_API_GENERATION is enabled, OS detection was failing. This is fixed in scripts/utilsOsType.py:80.

Fixing that didn't help; scripts/buildSwigWrapperClasses.py was not finding SWIG on Darwin. Instead of offloading finding SWIG in path to python, I thought adding find_package(SWIG) would make sense, because user has selected LLDB_ENABLE_PYTHON_SCRIPTS_SWIG_API_GENERATION in CMake.

emaste added inline comments.Nov 17 2014, 6:30 AM
scripts/utilsOsType.py
74–81

This is also broken for FreeBSD. I can take care of that and this Darwin fix in a moment.

Overall seems like a good idea - one thing I don't understand is marked above.

This could be useful for FreeBSD as well, as the swig 2.0 package installs swig as swig2 (or similar), and so swig is not found in the PATH.

scripts/CMakeLists.txt
12

I must be missing something -- I don't see how swigDir is used.

ismailp added inline comments.Nov 17 2014, 9:54 AM
scripts/CMakeLists.txt
12

CMake is setting that variable. That directory seems like to include skeleton/template/boilerplate files. buildSwigWrapperClasses.py, however, doesn't use it. I thought it might be useful in the future, and kept it. Perhaps it's best to remove it to prevent confusion.

emaste added inline comments.Nov 17 2014, 9:57 AM
scripts/CMakeLists.txt
12

Yeah, if there are no consumers yet I would leave it out for now. Or, add a comment that it's not yet used but will be soon if that's the case.

zturner edited edge metadata.Nov 17 2014, 10:00 AM

I would just leave it out until we have an actual need for it. No reason
to add complexity (however small) that nobody needs.

ismailp updated this revision to Diff 16343.Nov 18 2014, 10:27 AM
ismailp edited edge metadata.

Removed swigDir argument from both scripts/CMakeLists.txt, and scripts/buildSwigWrapperClasses.py.

Landed in 222262. Thanks! I can't seem to have Close Revision option, presumably because it wasn't accepted. Please do "Accept Revision" so that it can be closed.

emaste accepted this revision.Nov 18 2014, 2:03 PM
emaste added a reviewer: emaste.

BTW, if you include in the commit message

Differential Revision: http://reviews.llvm.org/D6290

it will auto-close when Phabricator sees the commit.

This revision is now accepted and ready to land.Nov 18 2014, 2:03 PM

Correct! My git-svn failed 3 times, then I checked out lldb svn repo,
and copied message from phabricator instead of original git commit.

ismailp closed this revision.Jan 23 2015, 12:34 PM

Closing. This has landed in r222262.