Page MenuHomePhabricator

meson: add a option for selecting lua interpreter
ClosedPublic

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

Details

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
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.
akanad created this revision.Jan 9 2019, 12:26 AM
akanad requested review of this revision.Jan 9 2019, 12:26 AM
zmike added a reviewer: q66.Jan 9 2019, 9:38 AM
bu5hm4n requested changes to this revision.Jan 9 2019, 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.Jan 9 2019, 11:34 AM
akanad added a comment.Jan 9 2019, 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.Jan 9 2019, 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.Jan 9 2019, 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.Jan 9 2019, 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.Jan 10 2019, 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.Jan 10 2019, 2:43 AM

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

akanad added a comment.EditedJan 10 2019, 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.Jan 10 2019, 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.Jan 10 2019, 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.Jan 10 2019, 6:31 PM

modify the default as luajit

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

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

meson.build
290

@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
9–10

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

10–36

Noep, only luajit is broken...

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

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

This revision now requires changes to proceed.Jan 12 2019, 3:14 AM
akanad updated this revision to Diff 18586.Jan 17 2019, 5:59 PM

rebase and fix a osx case.

With '-Dlua-interpretor=lua' I get the build failure:

../src/lib/evas/filters/evas_filter_parser.c: In function ‘_lua_class_create’:
../src/lib/evas/filters/evas_filter_parser.c:2378:4: warning: implicit declaration of function ‘luaL_register’; did you mean ‘lua_register’? [-Wimplicit-function-declaration]
    luaL_register(L, NULL, meta);
    ^~~~~~~~~~~~~
    lua_register
bu5hm4n requested changes to this revision.Jan 26 2019, 8:44 AM
This revision now requires changes to proceed.Jan 26 2019, 8:44 AM
akanad updated this revision to Diff 18930.Jan 27 2019, 5:34 PM

modify to print info messages

akanad updated this revision to Diff 18931.Jan 27 2019, 5:36 PM

fix a little error

I would like to know the exact reason why a error comes up in your environment cause I don't encounter the error.
woud you please make sure lua(not luajit) is installed on your system?
and then the error comes again while configuring, please attach a log file located on ${builddir}/meson-logs/meson-log.txt.

That is my version

Lua 5.3.5 Copyright (C) 1994-2018 Lua.org, PUC-Rio

meson configures just fine.

And the corresponding 'lua has been found: "Message: lua(version :>=5.1.0) for lua intepreter has been found"'

To me it seems that we aren't able to run with lua5.3/5.2 at all, since we use api which is only available in lua5.1...

Have you checked "ENABLE_LUA_OLD" and what it does ?

akanad updated this revision to Diff 19081.Jan 30 2019, 6:15 PM

modify to find lua libraries under version 5.3.0

That is my version

Lua 5.3.5 Copyright (C) 1994-2018 Lua.org, PUC-Rio

meson configures just fine.

And the corresponding 'lua has been found: "Message: lua(version :>=5.1.0) for lua intepreter has been found"'

To me it seems that we aren't able to run with lua5.3/5.2 at all, since we use api which is only available in lua5.1...

Have you checked "ENABLE_LUA_OLD" and what it does ?

I had a look at LUA_OLD stuffs but I didn't realize that AC_CHECK_LIB at that time.
I just checked current changes under lua libraries of various versions and they don't give me any error.

bu5hm4n requested changes to this revision.Feb 7 2019, 9:22 AM
col:$ORIGIN/../../static_libs/libunibreak:$ORIGIN/../../modules/evas' -Wl,-rpath-link,/home/marcel/git/efl/build/src/lib/evas:/home/marcel/git/efl/build/src/lib/eina:/home/marcel/git/efl/
build/src/lib/eo:/home/marcel/git/efl/build/src/lib/ector:/home/marcel/git/efl/build/src/lib/ector/software:/home/marcel/git/efl/build/src/lib/efl:/home/marcel/git/efl/build/src/static_li
bs/triangulator:/home/marcel/git/efl/build/src/static_libs/freetype:/home/marcel/git/efl/build/src/static_libs/draw:/home/marcel/git/efl/build/src/lib/emile:/home/marcel/git/efl/build/src
/static_libs/lz4:/home/marcel/git/efl/build/src/lib/eet:/home/marcel/git/efl/build/src/lib/ecore:/home/marcel/git/efl/build/src/static_libs/buildsystem:/home/marcel/git/efl/build/src/wayl
and_protocol:/home/marcel/git/efl/build/src/static_libs/libunibreak:/home/marcel/git/efl/build/src/modules/evas
/bin/ld: src/lib/evas_goal/c61fe7f@@evas@sha/.._evas_filters_evas_filter_parser.c.o: in function `_lua_class_create':
/home/marcel/git/efl/build/../src/lib/evas/filters/evas_filter_parser.c:2378: undefined reference to `luaL_register'
/bin/ld: /home/marcel/git/efl/build/../src/lib/evas/filters/evas_filter_parser.c:2386: undefined reference to `luaL_register'
collect2: error: ld returned 1 exit status
[711/2871] Compiling C object 'src/mod...c@@wayland@sha/wayland_imcontext.c.o'.
ninja: build stopped: subcommand failed.

Again please check ENABLE_LUA_OLD.

The checks you have added are quite useless, since there is a bunch of functions that change again and again, efl has a compatibility mode, which can be used here, which is enabled if you define ENABLE_LUA_OLD ...

This revision now requires changes to proceed.Feb 7 2019, 9:22 AM
akanad updated this revision to Diff 19269.Feb 10 2019, 5:09 PM

yup, you are right.

bu5hm4n added inline comments.Feb 13 2019, 12:13 AM
src/lib/evas/filters/meson.build
26

Why is there still this check ?

akanad updated this revision to Diff 19395.Feb 14 2019, 2:25 AM

I hope it's fine.

I should have taken a look at cafully.
anyway, as far as I think, I fix this patch as a point you said.(ENABLE_LUA_OLD stuff)
and I tried to translate a autotools logic below to meson one.
however, I am not sure the thing is needed.

<-----
AC_MSG_CHECKING([whether lua_newstate() is in liblua])
AC_CHECK_LIB([lua], [lua_newstate]

[have_lua="yes"
 EFL_ADD_LIBS([$1], [-llua])]

----->

bu5hm4n accepted this revision.Feb 21 2019, 11:29 AM

Looks better now, however, i will remove the part where you check the lua library itself. Lua itself comes with a .pc file, if someone decides to ship without this, then its IMO the problem of this platform and not ours, we rely on .pc files.

This revision was not accepted when it landed; it landed in state Needs Review.Feb 21 2019, 11:49 AM
Closed by commit rEFL56a91961ce07: meson: add a option for selecting lua interpreter (authored by WhiskyKiloSq, committed by Marcel Hollerbach <mail@marcel-hollerbach.de>). · Explain Why
This revision was automatically updated to reflect the committed changes.