Page MenuHomePhabricator

meson: add a option for selecting lua interpreter
Needs RevisionPublic

Authored by akanad on Wed, Jan 9, 12:26 AM.

Details

Reviewers
bu5hm4n
q66
Summary

this patch is for selecting lua interpreter such as luajit, lua51
and in addition, little more changes to unify lua dependency over efl

Diff Detail

Repository
rEFL core/efl
Branch
arcpatch-D7564
Lint
No Linters Available
Unit
No Unit Test Coverage
Build Status
Buildable 8761
Build 7724: arc lint + arc unit
akanad created this revision.Wed, Jan 9, 12:26 AM
akanad requested review of this revision.Wed, Jan 9, 12:26 AM
zmike added a reviewer: q66.Wed, Jan 9, 9:38 AM
bu5hm4n requested changes to this revision.Wed, Jan 9, 11:34 AM

Mhmmmm i don't know if it is so good to leak the name of the depency into a option, on arch for example i don't even have lua51 arround, is it *really* only lua5.1 support, or is it also working with lua5.2 ?

This revision now requires changes to proceed.Wed, Jan 9, 11:34 AM
akanad added a comment.Wed, Jan 9, 6:25 PM

IMO, what you said is not the kind of a problem. because the things that we are doing on meson.build are the kind of dependencies.
we don't say that it's a problem to leak a name of window systems such as 'wayland', 'x', don't we?
also luajit has a compatibility on both ABI/API level for lua5.1.
that is the reason why I thought that this kind of configuration could be adapted.

q66 requested changes to this revision.Wed, Jan 9, 6:40 PM

elua only supports luajit.

Also, I intend to remove support for both luajit and 5.1 in near future and unify everything under 5.3, but that will take some significant effort.

q66 added a comment.Wed, Jan 9, 6:44 PM

Also, it's not true that 5.1 is API/ABI compatible with luajit; it only applies the other way around, i.e. luajit is a superset of 5.1 functionality. While elua itself might compile with 5.1 (because it doesn't really use any C API calls beyond what 5.1 provides), it will be effectively useless because all the elua scripts rely on runtime luajit functionality (primarily the FFI)

akanad added a comment.Wed, Jan 9, 6:53 PM

Yeah, I got it.
and I have another question about edje.
as you know, edje also has a lua dependency, so I can say that which means it only works in luajit environment too?

bu5hm4n accepted this revision.Thu, Jan 10, 1:36 AM

@akanad The problem i see here is: when we update the dependency (like @q66 proposed above), a whole config option will be dropped and a new one will be added. Which is nothing that i want to have in the build options. The difference to the display protocol here is, the version of the wayland package or x11 package does not leak through.
A option where you can choose between lua or luajit is fine. The problem is the lua51 vs. lua52 vs. lua53.

(Additionally, can you rename the variable to lua, having a variable luajit that is a dependency object from the dependency lua, is a bit weird :))

q66 added a comment.Thu, Jan 10, 2:43 AM

my proposal involves keeping support for *only* 5.3, so a build option will be unnecessary

akanad added a comment.EditedThu, Jan 10, 3:06 AM

I remembered that I saw something like 'lua51' while using autogen.sh.
and I just found it on m4/efl_lua_old.m4. (regarding to --enable-lua-old)

how do you think about the kind of a patch to provide a lua-interpreter option as 'lua-old' and 'luajit'
if user select 'lua-old' as a interpreter then check lua dep from system package manager by names like a way it does on m4/efl_lua_old.m4
(of course, lua-old should be used, except elua)

I think that we don't have to think the proposal unless it exist. once it comes, then we can remove the thing that I am saying.

I remembered that I saw something like 'lua51' while using autogen.sh.
and I just found it on m4/efl_lua_old.m4. (regarding to --enable-lua-old)

how do you think about the kind of a patch to provide a lua-interpreter option as 'lua-old' and 'luajit'
if user select 'lua-old' as a interpreter then check lua dep from system package manager by names like a way it does on m4/efl_lua_old.m4
(of course, lua-old should be used, except elua)

I think that we don't have to think the proposal unless it exist. once it comes, then we can remove the thing that I am saying.

Sounds fine, i would just have the two values called 'lua' and 'luajit', but thats just my two cents :)

good~! I will write another changes soon : )

q66 added a comment.Thu, Jan 10, 4:35 AM

Yes, have those two values for now, and if "lua" is selected, disable elua entirely. I will update it as necessary when luajit is removed.

akanad updated this revision to Diff 18352.Thu, Jan 10, 6:20 PM

would you please have a look this : )

In D7564#133360, @q66 wrote:

Yes, have those two values for now, and if "lua" is selected, disable elua entirely. I will update it as necessary when luajit is removed.

I just saw this comment after posting another changes like just above.
I changed it like elua uses hardcoded 'luajit' and the option affects to edje/evas only.

akanad updated this revision to Diff 18353.Thu, Jan 10, 6:31 PM

modify the default as luajit

bu5hm4n added a subscriber: raster.Fri, Jan 11, 1:36 AM

Concept looks good and perfetly fine, just those little nitpicks :)

meson.build
283

@q66 @raster does someone know if we really need lua as a public dependency of edje ? Its also with autotools like that, i am wondering why...

src/lib/evas/filters/meson.build
31

this should also check what happens if luajit is really the option value

32

Noep, only luajit is broken...

bu5hm4n requested changes to this revision.Sat, Jan 12, 3:14 AM

Setting it to this state so it is out of my queue :)

This revision now requires changes to proceed.Sat, Jan 12, 3:14 AM