Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

split plugin lua macros to separate header #5161

Merged
merged 7 commits into from
Jan 1, 2025

Conversation

ab9rf
Copy link
Member

@ab9rf ab9rf commented Dec 31, 2024

this will reduce the number of translation units that require the includes from the lua dependency

Copy link
Member

@myk002 myk002 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Don't forget to get rid of those lines I just added in Plugins.cmake:

        # since PluginManager currently uses Lua headers, even when Lua is not used
        get_target_property(lua_INCLUDES lua INTERFACE_INCLUDE_DIRECTORIES)
        target_include_directories(${PLUGIN_NAME} PRIVATE ${lua_INCLUDES})

plugins/cxxrandom.cpp Outdated Show resolved Hide resolved
@myk002
Copy link
Member

myk002 commented Jan 1, 2025

Maybe also add a comment to plugins/examples/skeleton.cpp about which header files to include if the plugin makes use of Lua

@ab9rf
Copy link
Member Author

ab9rf commented Jan 1, 2025

Don't forget to get rid of those lines I just added in Plugins.cmake:

        # since PluginManager currently uses Lua headers, even when Lua is not used
        get_target_property(lua_INCLUDES lua INTERFACE_INCLUDE_DIRECTORIES)
        target_include_directories(${PLUGIN_NAME} PRIVATE ${lua_INCLUDES})

my base branch for this PR is before that was merged, so i either have to rebase, or cherry-pick that commit so i can reverse it. it'll be a bit

@myk002
Copy link
Member

myk002 commented Jan 1, 2025

you can also merge develop into this branch

@ab9rf
Copy link
Member Author

ab9rf commented Jan 1, 2025

you can also merge develop into this branch

i tried that but it made a mess of the PR

@ab9rf ab9rf force-pushed the plugin-lua-headers branch from af802e2 to 20246c4 Compare January 1, 2025 03:45
@ab9rf
Copy link
Member Author

ab9rf commented Jan 1, 2025

need to add lua include to add-spatter because it uses interposes, which have a dependency on DataFuncs that probably isn't absolutely necessary but will need more work to unwind

eventExample also had problems, but there we can just remove the unnecessary headers, which included VTableInterpose.h, Export.h, and Console.h. removed

i can't figure out how to get add-spatter to compile with backing out the change in Plugins.cmake, so i'm backing that out for now, we can catch that subsequently

@myk002
Copy link
Member

myk002 commented Jan 1, 2025

i can't figure out how to get add-spatter to compile with backing out the change in Plugins.cmake, so i'm backing that out for now, we can catch that subsequently

If it's just add-spatter, we can include Lua headers for just that build target in cmake by adding this to plugins/CMakeLists.txt:

    if(BUILD_PLUGINS)
        # remove when VTableInterpose.h no longer has a dependency on DataFuncs.h
        get_target_property(lua_INCLUDES lua INTERFACE_INCLUDE_DIRECTORIES)
        target_include_directories(add-spatter PRIVATE ${lua_INCLUDES})
    endif()

can be added just after dfhack_plugin(add-spatter add-spatter.cpp)

@ab9rf ab9rf force-pushed the plugin-lua-headers branch from 19c0c01 to 88b84a7 Compare January 1, 2025 15:33
ab9rf added 3 commits January 1, 2025 09:46
now only add-spatter and spectate, due to `VTableInterpose.h`

VTableInterpose.h needs `return_type<T>::is_method` whch is defined in `DataFuncs.h` which requires lua headers due to using `Lua::Push` in an inlined method
accomplished by moving the `return_type` trait definition from `DataFuncs.h` to `DataDefs.h`

also fixed a few minor style issues
solely so the skeleton compiles without requiring lua libraries

also expanded the comment
@myk002 myk002 merged commit bb871a5 into DFHack:develop Jan 1, 2025
14 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
No open projects
Status: Done
Development

Successfully merging this pull request may close these issues.

2 participants