This is an archive of the discontinued LLVM Phabricator instance.

Differential D108515

[lldb/lua] Force Lua version to be 5.3
ClosedPublic

Authored by siger-young on Aug 21 2021, 4:59 PM.

Details

Reviewers
tammela
JDevlieghere
Commits
rG6de63b3ba5e2: [lldb/lua] Force Lua version to be 5.3
rG7bb42dc6b114: [lldb/lua] Force Lua version to be 5.3
Summary

Due to CMake cache, find_package in FindLuaAndSwig.cmake
will be ignored. This commit adds EXACT and REQUIRED flags
to it and removes find_package in Lua ScriptInterpreter.

Signed-off-by: Siger Yang <sigeryeung@gmail.com>

Diff Detail

Event Timeline

siger-young created this revision.Aug 21 2021, 4:59 PM
Herald added a subscriber: mgorny. · View Herald TranscriptAug 21 2021, 4:59 PM
siger-young requested review of this revision.Aug 21 2021, 4:59 PM
Herald added a project: Restricted Project. · View Herald TranscriptAug 21 2021, 4:59 PM
Herald added a subscriber: lldb-commits. · View Herald Transcript
Harbormaster completed remote builds in B120689: Diff 367972.Aug 21 2021, 5:33 PM
tammela accepted this revision.Aug 23 2021, 8:43 AM

LGTM

This revision is now accepted and ready to land.Aug 23 2021, 8:43 AM
JDevlieghere accepted this revision.Aug 29 2021, 11:02 PM

LGTM

Closed by commit rG7bb42dc6b114: [lldb/lua] Force Lua version to be 5.3 (authored by Siger Yang <sigeryeung@gmail.com>). · Explain WhySep 3 2021, 12:26 AM
This revision was automatically updated to reflect the committed changes.
Siger Yang <sigeryeung@gmail.com> added a commit: rG7bb42dc6b114: [lldb/lua] Force Lua version to be 5.3.
mstorsjo added a subscriber: mstorsjo.Sep 3 2021, 1:53 AM
mstorsjo added inline comments.
lldb/cmake/modules/FindLuaAndSwig.cmake
12

This breaks building in setups where SWIG is available, but not Lua. Previously this detected Lua and took it into use if both Lua and SWIG were available, and if not , proceeded without them.

mstorsjo added inline comments.Sep 3 2021, 2:20 AM
lldb/cmake/modules/FindLuaAndSwig.cmake
12

Just removing the REQUIRED from here seems to fix my build. Or I could just revert the patch to return to discussing how it should be done.

I'm doing either of them fairly soon in any case, to unbreak my build.

Siger Yang <sigeryeung@gmail.com> added a reverting change: rG49229bb92b4b: Revert "[lldb/lua] Force Lua version to be 5.3".Sep 3 2021, 2:32 AM
siger-young added inline comments.Sep 3 2021, 2:33 AM
lldb/cmake/modules/FindLuaAndSwig.cmake
12

I think removing the "REQUIRED" flags might work. I will revert the broken commit first.

tammela added inline comments.Sep 3 2021, 7:10 PM
lldb/cmake/modules/FindLuaAndSwig.cmake
12

That's look OK

siger-young reopened this revision.Sep 26 2021, 12:33 AM
This revision is now accepted and ready to land.Sep 26 2021, 12:33 AM
siger-young updated this revision to Diff 375086.Sep 26 2021, 12:33 AM

Remove "REQUIRED" flags when finding Lua at "FindLuaAndSwig.cmake".

Harbormaster completed remote builds in B125744: Diff 375086.Sep 26 2021, 12:36 AM
tammela accepted this revision.Sep 26 2021, 7:12 AM
Closed by commit rG6de63b3ba5e2: [lldb/lua] Force Lua version to be 5.3 (authored by Siger Yang <sigeryeung@gmail.com>). · Explain WhyOct 12 2021, 6:34 AM
This revision was automatically updated to reflect the committed changes.
Siger Yang <sigeryeung@gmail.com> added a commit: rG6de63b3ba5e2: [lldb/lua] Force Lua version to be 5.3.

Revision Contents

PathSize
lldb/
cmake/
modules/
2 lines
source/
Plugins/
ScriptInterpreter/
Lua/
2 lines

Diff 379012

lldb/cmake/modules/FindLuaAndSwig.cmake

#.rst: #.rst:
# FindLuaAndSwig # FindLuaAndSwig
# -------------- # --------------
# #
# Find Lua and SWIG as a whole. # Find Lua and SWIG as a whole.
if(LUA_LIBRARIES AND LUA_INCLUDE_DIR AND SWIG_EXECUTABLE) if(LUA_LIBRARIES AND LUA_INCLUDE_DIR AND SWIG_EXECUTABLE)
set(LUAANDSWIG_FOUND TRUE) set(LUAANDSWIG_FOUND TRUE)
else() else()
find_package(SWIG 3.0) find_package(SWIG 3.0)
if (SWIG_FOUND) if (SWIG_FOUND)
find_package(Lua 5.3) find_package(Lua 5.3 EXACT)
mstorsjoUnsubmitted
Not Done
ReplyInline Actions

This breaks building in setups where SWIG is available, but not Lua. Previously this detected Lua and took it into use if both Lua and SWIG were available, and if not , proceeded without them.

mstorsjo: This breaks building in setups where SWIG is available, but not Lua. Previously this detected…
siger-youngAuthorUnsubmitted
Done
ReplyInline Actions

I think removing the "REQUIRED" flags might work. I will revert the broken commit first.

siger-young: I think removing the "REQUIRED" flags might work. I will revert the broken commit first.
tammelaUnsubmitted
Not Done
ReplyInline Actions

That's look OK

tammela: That's look OK
mstorsjoUnsubmitted
Not Done
ReplyInline Actions

Just removing the REQUIRED from here seems to fix my build. Or I could just revert the patch to return to discussing how it should be done.

I'm doing either of them fairly soon in any case, to unbreak my build.

mstorsjo: Just removing the `REQUIRED` from here seems to fix my build. Or I could just revert the patch…
if(LUA_FOUND AND SWIG_FOUND) if(LUA_FOUND AND SWIG_FOUND)
mark_as_advanced( mark_as_advanced(
LUA_LIBRARIES LUA_LIBRARIES
LUA_INCLUDE_DIR LUA_INCLUDE_DIR
SWIG_EXECUTABLE) SWIG_EXECUTABLE)
endif() endif()
else() else()
message(STATUS "SWIG 3 or later is required for Lua support in LLDB but could not be found") message(STATUS "SWIG 3 or later is required for Lua support in LLDB but could not be found")
Show All 11 Lines

lldb/source/Plugins/ScriptInterpreter/Lua/CMakeLists.txt

find_package(Lua REQUIRED)
add_lldb_library(lldbPluginScriptInterpreterLua PLUGIN add_lldb_library(lldbPluginScriptInterpreterLua PLUGIN
Lua.cpp Lua.cpp
ScriptInterpreterLua.cpp ScriptInterpreterLua.cpp
LINK_LIBS LINK_LIBS
lldbCore lldbCore
lldbInterpreter lldbInterpreter
) )
target_include_directories(lldbPluginScriptInterpreterLua PUBLIC ${LUA_INCLUDE_DIR}) target_include_directories(lldbPluginScriptInterpreterLua PUBLIC ${LUA_INCLUDE_DIR})
target_link_libraries(lldbPluginScriptInterpreterLua INTERFACE ${LUA_LIBRARIES}) target_link_libraries(lldbPluginScriptInterpreterLua INTERFACE ${LUA_LIBRARIES})