Page MenuHomePhabricator

[lldb] Move swig call from python code to cmake
ClosedPublic

Authored by hhb on Sep 24 2019, 8:26 PM.

Diff Detail

Repository
rL LLVM

Event Timeline

hhb created this revision.Sep 24 2019, 8:26 PM
Herald added a project: Restricted Project. · View Herald TranscriptSep 24 2019, 8:26 PM
hhb updated this revision to Diff 221664.Sep 24 2019, 8:31 PM

Fix a typo

hhb updated this revision to Diff 221667.Sep 24 2019, 8:39 PM

Format

mgorny accepted this revision.Sep 24 2019, 10:36 PM

Thanks! That's a great job, and certainly saves me some trouble in pushing my patch forward, though I'm kinda surprised we don't need site-packages path there after all.

Please wait for others to review it as well.

This revision is now accepted and ready to land.Sep 24 2019, 10:36 PM
hhb added a comment.Sep 24 2019, 10:43 PM

I'm kinda surprised we don't need site-packages path there after all.

Yes that's surprising. Seems like it is better to move the install() call in your change to lldb/CMakeLists.txt. To be together with the call to finishSwigWrapperClasses.py.

labath accepted this revision.Sep 25 2019, 2:35 AM

Yay, tons of nasty python code going down the drain. This looks fine, but please take it slowly when landing these patches. Waiting a day or two before proceeding with the next patch will make it easier to fix things up if any of them cause problems for some people.

Thanks! That's a great job, and certainly saves me some trouble in pushing my patch forward, though I'm kinda surprised we don't need site-packages path there after all.

I guess that's because this script puts its output in a temporary location, and the "finish" script then copies it to the site-packages folder. I think this was mainly done because we were doing some post-processing of the swig-generated code, which we don't do any more thankfully, so it may be possible to have this put the code straight into the final location, but it's better to take things one slowly.

This revision was automatically updated to reflect the committed changes.
Herald added a project: Restricted Project. · View Herald TranscriptSep 25 2019, 9:35 AM
xiaobai added inline comments.
lldb/trunk/scripts/CMakeLists.txt
5

Doesn't this now include the lldb-private headers now? Is that intended?

hhb marked an inline comment as done.Sep 25 2019, 10:57 AM
hhb added inline comments.
lldb/trunk/scripts/CMakeLists.txt
5

Oh sorry. Will send a change to fix. Is there an easy way to exclude a pattern in file() call?

xiaobai added inline comments.Sep 25 2019, 11:00 AM
lldb/trunk/scripts/CMakeLists.txt
5

You can use EXCLUDE but I'm not sure that works with file(GLOB). It might be better just to enumerate all the headers you want to use instead of globbing.

JDevlieghere added inline comments.Sep 30 2019, 9:34 AM
lldb/trunk/scripts/CMakeLists.txt
5

+1