search

cloudius-systems / osv-apps

OSv Applications
135 stars 75 forks source link

lua: add lua support #21

Closed parasitid closed 6 years ago

penberg commented 9 years ago

Looks good to me. @nyh, comments?

geraldo-netto commented 6 years ago

Dear Friends,

If I understood correctly, we already have a Lua integration: https://github.com/cloudius-systems/osv/tree/master/modules/lua

Maybe we could produce some documentation on how to run lua scripts on it?

Kind Regards, Geraldo Netto

wkozaczuk commented 6 years ago

Sure. I am not familiar with lua myself so if you feel up to the task please go ahead.

Cli module is good example. Therefore this issue might be obsolete.

Sent from my iPhone

On Mar 3, 2018, at 08:58, Geraldo Netto notifications@github.com wrote:

Dear Friends,

If I understood correctly, we already have a Lua integration: https://github.com/cloudius-systems/osv/tree/master/modules/lua

Maybe we could produce some documentation on how to run lua scripts on it?

Kind Regards, Geraldo Netto

— You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub, or mute the thread.

nyh commented 6 years ago

Indeed, we already have a Lua module for several years, and the patches in this pull request are mostly unneeded either (e.g., I fixed __longjmp_chk() in 1f6fd50afe26ebfc1d99864058d87f8a58ee9a6b, before this pull request but apparently after gby fixed it in his patch on which this PR is based), and moreover, seem incomplete, e.g., they assume some "hello.so", but the patch doesn't seem to include anything like that. I don't know what I'm missing.

It would be nice to have a modules/lua/README explaining better how to use it and how to run some example Lua code. But this pull request doesn't seem to provide this. So I'm closing it. Feel free to reopen if I missed something.

jaromil commented 6 years ago

Hi! Have you actually read the patch that was offered here? your LUA module is not building with FORTIFY_SOURCE nor is working well once built (as other modules included in OSv but never tested). The PR by @parasitid fixed that and detailed in its README. I understand the effort of including whatever package may exceed your capacity, but at least its recommendable to give a proper read to the contributions that go into the direction of fixing issues.

nyh commented 6 years ago

@jaromil I'll respond to you in kind: have you actually read my reply?

I did note he's fixing __longjmp_chk() which is needed by FORTIFY_SOURCE, but I said that I already fixed it, in a different way, in July 2014. Yes, his PR is newer but it is based on an earlier 2014 patch by GBY, so I am wondering if it's still necessary, since it fixes the same thing already fixed. Did you verify it is still broken? Any idea what is wrong with my fix?

Also, the title of the PR was "add Lua support", but obviously this PR doesn't do it (as both I and you mentioned, we already had Lua support)... Finally, this patch seems to rely on / mention "hello.so", which we don't have, and I have no idea what it is.

Is it your contention that this 3-year-old PR is still relevant and should not be closed?

jaromil commented 6 years ago

thanks for the clarifications! I was just worried since I am working on a lua-based interpreter compiled with fortify_source and planning also to have it working in osv when possible. did not notice you have already fixed the issue. cheers

geraldo-netto commented 6 years ago

Dear Friends,

Just in case, I have created a module that does call a Lua script with the classic hello world message Of course, it can be adapted to any other Lua script More details here: https://groups.google.com/forum/#!topic/osv-dev/NeTsVRTyP6k

Hope this helps :)

Kind Regards, Geraldo Netto