Mehgugs / lacord

A low level, lightweight discord API library.
MIT License
6 stars 0 forks source link

luajit/5.1 support #8

Open JDaance opened 2 years ago

JDaance commented 2 years ago

Didnt notice the lua 5.3 requirement until luarocks install complained :)

I have a pretty big luajit/5.1 codebase that I would like to connect to a discord bot and its hard to tell at a glance how many > 5.1 specific language features this is running? I'm willing to hack a bit to get it working if its not anything major.

JDaance commented 2 years ago

I got the cdn avatar example running on luajit (the example uses deprecated init btw)

Replaced all

local _ENV = {}

with

local _ENV = {}
setfenv(1, _ENV)

And

--- Computes the FNV-1a 32bit hash of the given string.
-- @str str The input string.
-- @treturn integer The hash.
function hash(str)
    local hash = 2166136261
    for i = 1, #str do
        --hash = hash ~ str:byte(i)
        hash = bit.bxor(hash, str:byte(i))
        --hash = (hash * 16777619) & 0xffffffff
        hash = bit.band(hash * 16777619, 0xffffffff)
    end
    return hash
end

Not sure if the above is correct, cdn example probably does not use it

We will see what I run into

JDaance commented 2 years ago

Also got lacord-client running now after some more bitop changes and added utf8 dependency. I have a problem with intents that I made an issue about in that repo.

JDaance commented 2 years ago

Lacord-client hello world with message content up and running on luajit 👍

If I dont run into anything crazy while fleshing out my bot, the question becomes if youre interested in officially supporting it. Would make my life easier when upgrading, but I'm also used to maintaining hacked forks :)

The c file will need some ifdefs for lua < 5.3 as well, for my hack I just package.preloaded my way around it

Mehgugs commented 2 years ago

Yeah, I've been thinking about this off and on. The only big behavior based dependencies as far as I'm aware are: pcalls not interfering with yields (fixed in luajit not in vanilla 5.1) which is pretty integral to cqueues; native integer support which is required for the bitop calculations for stuff like intents and discord ID timestamp calculations. This is workable with luajits weird extensions for long integers; Stuff like table.pack is trivial to handle albeit a bit annoying.

The syntactic stuff like _ENV I could probably write a recompiled set of sources for and distribute, I'd rather not use setfenv here. Those util functions can probably be removed ( I'm not sure what the FNV implementations are for at this point 🤣). There's some scary trickery with the -lacord code too.

I won't look into this support for lacord client because that code is a mess and I need to refactor it first.

That c source file "archp" could be omitted completely from the luajit build, it's literally copying jit.os source code.

All in all, there's definitely space for this in the library but there's a few things I'd like to do which would need to be done first. I also need to weigh the extra maintenance luajit will bring: I've pretty nonchalantly assumed 5.3+ across the codebase. I do know however that luajit would make this library pretty snappy, and I'm interested in having that option available to people.

JDaance commented 2 years ago

The intent bit calculations are working for me translating them to luajits bitop without any special integer stuff. I dont know what the timestamp calculations are so cant verfify that. Me personally will only run this on luajit, puc 5.1 doesnt matter. I didnt even mock the arch part of the c file, only the version numbers. Maybe I should look how the arch part is used and move that to jit.os

And yes supporting luajit would make the code a bit uglier, and its kind of up to you in what way :)

I will be running my hack for this in production soon, with a very basic bot. Thanks for a (according to my limited testing) well made library

Mehgugs commented 2 years ago

The reason I mention bitfield operations is that nowadays discord bitfields are becoming quite fat (permissions for example) and I'm not sure how luajit handles integers made with the 1ULL syntax being passed into the bit module (it probably works but still something to consider).

And yes supporting luajit would make the code a bit uglier, and its kind of up to you in what way :)

I might make a jit branch for this purpose to avoid compat codegore 😂

This is how I'd mock archp, honestly I might just hardcode the platform as "unknown" in the identify step in future since I'm pretty sure discord just uses it for analytics and it isn't actually important, whether that means this module is discontinued or not is another matter.

local mj mi, rl = jit.version:match"LuaJIT (%d+)%.(%d+)%.(%d+)"
package.preload['lacord.util.archp'] = {
   jit = true, -- If I was writing this code in lacord I'd probably add a jit flag (all this module does is provide a string to the shard)
   os = jit.os,
   arch = jit.arch, 
   lua = {
      vm = jit.version_num, 
      release = jit.version, 
      major = mj, 
      minor = mi, 
      release_num = rl,
   }

}
JDaance commented 2 years ago

Thanks for that snippet! This is how my weird preamble now looks to run hacked lacord + client outside of luarocks and without the .c file:



---------------------------------------------------------------------------------------------------
-- INIT LACORD
---------------------------------------------------------------------------------------------------

package.path = package.path .. ";./lib/lacord/lacord/lua/?.lua"
package.path = package.path .. ";./lib/lacord/lacord/lua/?/init.lua"
package.preload["lacord.util.archp"] = function()
    local mj, mi, rl = jit.version:match("LuaJIT (%d+)%.(%d+)%.(%d+)")
    return { 
        jit = true,
        os = jit.os, 
        arch = jit.arch, 
        lua = { 
            vm = jit.version_num, 
            release = jit.version, 
            major = mj, 
            minor = mi, 
            release_num = rl, 
        } 
    }
end
package.preload["lacord.cli"] = function()
    return require("lacord/util/cli_default")
end

for link, target in pairs {
    -- stole these from rockspec
    ["lacord.x.client"] = "lua/lacord-client/client.lua",
    ["lacord.x.event"] = "lua/lacord-client/event.lua",
    ["lacord.x.queue"] = "lua/lacord-client/queue.lua",
    ["lacord.x.timer"] = "lua/lacord-client/timer.lua",
    ["lacord.x.cqswrap"] = "lua/lacord-client/cqswrap.lua",

    ["lacord.x.components"] = "lua/lacord-client/interactions/components.lua",
    ["lacord.x.command"] = "lua/lacord-client/interactions/command.lua",
    ["lacord.x.command.context"] = "lua/lacord-client/interactions/context.lua",
    ["lacord.x.command.arguments"] = "lua/lacord-client/interactions/arguments.lua",
    ["lacord.x.command.context.specs"] = "lua/lacord-client/interactions/context-specs.lua"
} do
    package.preload[link] = function()
        return dofile("lib/lacord/lacord-client/" .. target)
    end
end

---------------------------------------------------------------------------------------------------
``
Mehgugs commented 2 years ago

Okay, well I will definitely look into decoupling lacord-client from luarocks source maps in the future 😄 . I'm currently looking at April 30th as my "deadline" for when lacord / lacord-client should be stable. I cannot promise built in luajit support in that time because I suspect discord are sitting on some complex features surrounding slash command permissions that i'll have to get my head around.

I'd really appreciate feedback on things you think are superfluous / needed in lacord-client. I'm going to gut it internally at some point, but try to keep the user api as close as I can.

JDaance commented 2 years ago

Sounds good to me! My use case so far is very light, so I wont test so many features perhaps. On the other hand any api changes wont affect me much so you may gut it from all ends as far as I am concerned :+1:

Mehgugs commented 2 years ago

I was able to compile archp for luajit I just needed to check for 501 versions, since they dont define LUA_VERSION_MAJOR et al.

I'll probably remove this module alltogether though.

NB. Compiling for luajit requires you to manually make and install it from what I can see, on targets compatible with cqueues this is really easy to do but I noticed the apt distribution didn't put a .so on my system as far I could tell.

Mehgugs commented 2 years ago

I've basically finished this for lacord, I just need to get lua-http and cqueues compiled for luajit so I can test and then I'll push the branch.

Mehgugs commented 2 years ago

@JDaance how do you build your libraries/dependencies out of curiosity. Would an alternative to luarocks be helpful for luajit users?

JDaance commented 2 years ago

@JDaance how do you build your libraries/dependencies out of curiosity. Would an alternative to luarocks be helpful for luajit users?

For this bot I use luarocks in a docker container. Here is the actual dockerfile that I use to run the bot both in development and production: https://gist.github.com/JDaance/7ed965c43d08981de9139576c3c01cd4

And for now I run lacord directly in the lua tree since I had to modify it, my first attempt was just to luarocks install it.