Skip to content

lua: Fixes and improvements #1520

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

Open
wants to merge 18 commits into
base: master
Choose a base branch
from
Open

lua: Fixes and improvements #1520

wants to merge 18 commits into from

Conversation

PRESFIL
Copy link

@PRESFIL PRESFIL commented Jun 14, 2025

Description

  • Fixed some critical bugs in Lua subsystem, that caused crashes and freeze rtorrent
  • Make rtorrent.lua accessible at start
    • Add rtorrent.lua to rtorrent's distribution
  • rtorrent.lua
    • Autocall Replaced with Target-object, Autocall_config renamed to Autocall
    • Autocalls now can be copied to variable
    • shor-circuiting of old autocall_config with global environment _G removed
    • Added aliases for d.*, f.*, p.*, t.*, load.* commands
    • Added new assignment semantic
    • Added insert_lua_method() to insert lua functions as rtorrent's methods
  • Fix: print= shadowed by native Lua print()
  • Some minor fixes

@PRESFIL
Copy link
Author

PRESFIL commented Jun 14, 2025

@kannibalox, review, please.

@PRESFIL
Copy link
Author

PRESFIL commented Jun 15, 2025

@rakshasa, could you please review.

@rakshasa
Copy link
Owner

I'll review it later.

@PRESFIL
Copy link
Author

PRESFIL commented Jun 18, 2025

I thought in the last PR (#1260) it wasn't very clear how to try the new functionality and added a example config doc/rtorrent.rc.lua-example converted from doc/rtorrent.rc-example.

@PRESFIL
Copy link
Author

PRESFIL commented Jun 20, 2025

Checked the build with Lua in Manjaro and Nixos.

Change of package for Nixos:

{
  configureflags = [ 
    "--with-lua"
  ];

  nativeBuildInputs = [ 
    lua5_4_compat 
  ];

  buildInputs = [ 
    lua5_4_compat 
  ];
}

Changes for Manjaro's PKGBUILD:

deps = (lua)
makedeps = (LUA)
./configure --with-lua

Perhaps deps and nativeBuildInputs changes can be thrown away.

@PRESFIL
Copy link
Author

PRESFIL commented Jun 22, 2025

@rakshasa, is there anything that I should add so that it is easier for you to test this branch?

@PRESFIL
Copy link
Author

PRESFIL commented Jun 22, 2025

I tried to remove the need to overried the LUA_PATH environment variable (see 2ebac20) to test changes during development, but I do not understand Autoconf.

This can be implemented like in vifm (1, 2) and in addition to LUA_PATH it could activate RAK_ENABLE_DEBUG and RAK_ENABLE_WERROR.

@rakshasa
Copy link
Owner

@rakshasa, is there anything that I should add so that it is easier for you to test this branch?

I'm just in the middle of some complicated tracker/http/dht threading work so don't have the bandwidth to figure out LUA atm.

The review will be done, but it might be a couple weeks at least.

PRESFIL added 13 commits July 5, 2025 07:36
- missing `pos = end + 1` pos caused infinite loop

- there was no processing of the last element after the last ';', i.e. it was
  assumed that `lua_path` always has ';' in the last character
rtorrent doesn't have floating point data type
Trying `rc.print()` causes redirection to native global Lua `print()`, which
prints to stdout. I don't see any proc of shor-circuiting `autocall_config`
with global environment `_G`.
…tocall

Deep-copy `__namestack` decouples all instances of Autocall, previously it
worked as singleton. This allows to store call-chains as variables, aliases.

Target-object allows to avoid duplication of the target-parameter as required
with Autocall by storing them as variables.

Autocall_config renamed to Autocall for brevity.
Updated @kannibalox's method to pass lua-functions as rtorrent's methods.
Additional step to test in development (non-packaged) mode:

- In packaged mode, rtorrent knows where to search rtorrent.lua, but in
  project mode, rtorrent.lua located in a non-standard location.
  Therefore run rtorrent with following environment variabler set:

  LUA_PATH=<full path to rtorrent project dir>/lua/?.lua
@PRESFIL
Copy link
Author

PRESFIL commented Jul 5, 2025

Rebased.

@rakshasa
Copy link
Owner

rakshasa commented Jul 7, 2025

Looks like using config.h is not solvable, so let's just use -DLUA_DATADIR=\"$(datadir)/lua\" as initially done.

@rakshasa
Copy link
Owner

rakshasa commented Jul 7, 2025

Or rather, have a define in config.h: #define LUA_DATADIR PACKAGE_DATADIR "/lua"

And pass PACKAGE_DATADIR using CPPFLAGS, as this allows the user to override the lua directory.

@PRESFIL
Copy link
Author

PRESFIL commented Jul 7, 2025

config.h is autogenerated file, i can put #define directly to lua.h.

@rakshasa
Copy link
Owner

rakshasa commented Jul 7, 2025

config.h is autogenerated file, i can put #define directly to lua.h.

No, that misses the purpose. It's supposed to be changeable by the user when compiling, and as such belongs in config.h.

Preferably it should be exposed as an option to configure, however that isn't necessary to add atm.

@@ -284,6 +284,7 @@ AC_DEFUN([TORRENT_WITH_LUA], [
AX_LUA_LIBS
AX_LUA_HEADERS
AC_DEFINE(HAVE_LUA, 1, Use LUA.)
AC_DEFINE_UNQUOTED(LUA_DATADIR, ["${prefix}/share/rtorrent/lua"], Use LUA.)
Copy link
Owner

Choose a reason for hiding this comment

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

This should be AC_DEFINE(LUA_DATADIR, [PACKAGE_DATADIR "/lua"], [LUA data directory]).

Copy link
Author

Choose a reason for hiding this comment

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

Done

@PRESFIL
Copy link
Author

PRESFIL commented Jul 8, 2025

Is there anything that needs to be finished?

@PRESFIL
Copy link
Author

PRESFIL commented Jul 13, 2025

Ping

@PRESFIL
Copy link
Author

PRESFIL commented Jul 18, 2025

@rakshasa , Is there anything that needs to be finished?

@rakshasa
Copy link
Owner

rakshasa commented Aug 2, 2025

I'll have to test it first, so should get a proper review done soon.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants