fix: Use luajit by default when run APISIX CLI #3335

fukiki · 2021-01-19T04:43:35Z

Signed-off-by: fukiki one.fhh@163.com

What this PR does / why we need it:

When run APISIX, use LuaJIT or Lua 5.1 by default.

If the local lua version is not 5.1, use openresty-luajit as default.

Fix : #3281

Pre-submission checklist:

Did you explain what problem does this PR solve? Or what new features have been added?
Have you added corresponding test cases?
Have you modified the corresponding document?
Is this PR backward compatible? If it is not backward compatible, please discuss on the mailing list first

membphis · 2021-01-19T05:00:48Z

Makefile

@@ -56,6 +57,9 @@ ifeq ($(LUAROCKS_VER),luarocks 3.)
 else
 	luarocks install rockspec/apisix-master-0.rockspec --tree=deps --only-deps --local
 endif
+ifneq ($(LUA_VER),Lua 5.1)
+	sed -i '' -e "s|#!/usr/bin/env lua|#!/usr/bin/env $(LUAJIT_DIR)/bin/luajit|g" ./bin/apisix


this is a hack way. it should not be enough.

here is another way, we can make a try:

#!/bin/sh # find the openresty, eg: which openresty ... # find the luajit of openresty ... # run the CLI of APISIX $luajit -e '...'

@membphis
I don't understand very clearly.
Do you mean to create a new shell script to judge whether to use lua5.1 or luajit when running the CLI of APISIX?
It could run the CLI of APISIX selectively: lua ./bin/apisix or luajit -e ./bin/apisix?

we can use a shell script to write the CLI, it'll to call luajit ./bin/apisix

fukiki · 2021-01-20T06:51:15Z

@membphis Could you review commit 065a44 for me? Thank you.

By the way, about CI error: lua: /usr/bin/apisix.lua:3: unexpected symbol near '-' , the file apisix.lua is the same with old file apisix, no change, and I didn't find any reason about it, could you help me?

membphis

can we put this file bin/apisix.lua to apisix/cli?

fukiki · 2021-01-20T09:35:37Z

can we put this file bin/apisix.lua to apisix/cli?

apisix.lua is a part of cli, put it into apisix/cli will be better. Thanks.
I will modify it.

Makefile

apisix/cli/apisix.lua

membphis · 2021-01-21T03:52:02Z

please merge the code of master branch, CI failed ^_^

bin/apisix

fukiki · 2021-01-21T09:57:45Z

@membphis @tokers Please take a look when you have time.

membphis · 2021-01-21T10:44:34Z

bin/apisix

-- modify the load path to load our dependencies
-package.cpath = pkg_cpath .. pkg_cpath_org
-package.path  = pkg_path .. pkg_path_org
+if [[ "$LUA_VERSION" =~ "Lua 5.1" && $USE_LUA ]]; then


this is wrong. we should always use luajit of openresty

membphis · 2021-01-21T10:44:36Z

bin/apisix

-- modify the load path to load our dependencies
-package.cpath = pkg_cpath .. pkg_cpath_org
-package.path  = pkg_path .. pkg_path_org
+if [[ "$LUA_VERSION" =~ "Lua 5.1" && $USE_LUA ]]; then


this is wrong. we should always use luajit of openresty

if we always use luajit of openresty , it will be a big chage for old users . so we think maybe we should leave a way for old users ?

this is fine for the old users, sure for this

membphis · 2021-01-22T01:50:27Z

bin/apisix

-                  .. apisix_home .. "/deps/lib/lua/5.1/?.so;;"
-local pkg_path = apisix_home .. "/deps/share/lua/5.1/?.lua;;"
+# find the openresty
+OR_EXEC=$(which openresty)


This step may fail, and we need to do some exception checks.

If the openresty command is not found, you can actively determine whether there is this directory in /usr/local/openresty.

thrown an error if failed to find the openresty

eg:

https://github.com/apache/apisix/blob/master/Makefile#L36

membphis

LGTM, ci failed, need you to process it

membphis · 2021-01-27T06:47:04Z

@spacewander told me that, this is a bug of openresty 1.15 or 1.17 .

do you have any suggestions about this?

luajit: lj_asm_x86.h:2819: asm_loop_fixup: Assertion `((intptr_t)target & 15) == 0' failed.
./bin/apisix: line 40: 24266 Aborted                 (core dumped) $LUAJIT_BIN $APISIX_LUA $*
Error: Process completed with exit code 134.

spacewander · 2021-01-27T06:51:57Z

The problem occurs with openresty-debug. Since people might use openresty-debug package, the robust way is to avoid using LuaJIT unless you are using OpenResty 1.19.

fukiki · 2021-01-27T07:39:32Z

About the bug of openresty-debug, there are two suggestions:

Add conditions: when OpenResty version is not 1.19 , allow to use Lua 5.1
Modify document: introduce the bug , explain the reasons and solutions

membphis · 2021-01-27T08:21:10Z

About the bug of openresty-debug, there are two suggestions:

Add conditions: when OpenResty version is not 1.19 , allow to use Lua 5.1

Modify document: introduce the bug , explain the reasons and solutions

I think this is fine. you can make a try ^_^

tokers · 2021-01-27T11:19:07Z

@fukiki I think the title can be changed :), we only want to run APISIX CLI with LuaJIT.

fukiki · 2021-01-28T03:01:16Z

@fukiki I think the title can be changed :), we only want to run APISIX CLI with LuaJIT.

About the bug of openresty-debug, only using OpenResty 1.19 not Lua 5.1?

#3335 (comment)
#3335 (comment)

fukiki · 2021-01-29T03:22:00Z

About the bug of openresty-debug, there are two suggestions:

Add conditions: when OpenResty version is not 1.19 , allow to use Lua 5.1

Modify document: introduce the bug , explain the reasons and solutions

I think this is fine. you can make a try ^_^

@membphis I've already done, could you review my pr please? Than you.

membphis · 2021-01-29T03:25:57Z

@membphis I've already done, could you review my pr please? Than you.

let me take a look. many thx for your nice job

apisix/cli/ngx_tpl.lua

membphis · 2021-01-29T03:32:12Z

@fukiki I think we can merge this PR soon ^_^

fukiki · 2021-01-29T04:27:25Z

@membphis The lua_package_path of stream has already updated.

liuxiran · 2021-01-29T08:06:38Z

so cool @fukiki ~!

Since all checks have passed, pls take a look again at your convenience

@membphis @tokers @imjoey @Fuchange

membphis

LGTM, except minor style things

@tokers do you have time to look at this PR?

doc/zh-cn/install-dependencies.md

membphis · 2021-01-29T14:39:09Z

doc/install-dependencies.md

@@ -35,6 +35,8 @@

 - If you want use Tengine instead of OpenResty, please take a look at this installation step script [Install Tengine at Ubuntu](../.travis/linux_tengine_runner.sh).

+- By default Apache APISIX runs with luajit of OpenResty 1.19 (priority) or Lua 5.1. If you run in to an issue `luajit: lj_asm_x86.h:2819: asm_loop_fixup: Assertion '((intptr_t)target & 15) == 0' failed`, it is caused by to the compatibility of OpenResty version. OpenResty 1.19 is recommended, please take a look at this installation step script [linux-install-openresty](../utils/linux-install-openresty.sh).


Apache APISIX runs with luajit -> Apache APISIX runs with LuaJIT

@membphis I submitted about the style things.

tokers · 2021-01-29T14:41:13Z

Makefile

@@ -35,6 +35,8 @@ ifeq ($(OR_EXEC), )
 ifeq ("$(wildcard /usr/local/openresty-debug/bin/openresty)", "")
 	@echo "ERROR: OpenResty not found. You have to install OpenResty and add the binary file to PATH before install Apache APISIX."
 	exit 1
+else


What about echoing a warn message here?

@tokers The warn message will be better, I've submitted.

tokers · 2021-01-29T14:44:02Z

LGTM, except minor style things

@tokers do you have time to look at this PR?

Just reviewed.

Signed-off-by: fukiki <one.fhh@163.com>

membphis · 2021-01-29T15:57:59Z

@fukiki many thx, merge already

fukiki changed the title ~~fix:Use luajit or lua 5.1 by default when run APISIX~~ fix: Use luajit or lua 5.1 by default when run APISIX Jan 19, 2021

membphis reviewed Jan 19, 2021

View reviewed changes

fukiki force-pushed the apisix-run-deps branch 7 times, most recently from 32bbf86 to 065a44e Compare January 20, 2021 03:01

membphis reviewed Jan 20, 2021

View reviewed changes

fukiki force-pushed the apisix-run-deps branch 2 times, most recently from 2f04024 to c0d7384 Compare January 20, 2021 10:14

membphis reviewed Jan 20, 2021

View reviewed changes

Makefile Outdated Show resolved Hide resolved

imjoey reviewed Jan 21, 2021

View reviewed changes

apisix/cli/apisix.lua Outdated Show resolved Hide resolved

fukiki force-pushed the apisix-run-deps branch 3 times, most recently from ae0ab58 to e44bdfc Compare January 21, 2021 02:40

tokers reviewed Jan 21, 2021

View reviewed changes

bin/apisix Show resolved Hide resolved

fukiki force-pushed the apisix-run-deps branch 2 times, most recently from 612eccc to 519b4f6 Compare January 21, 2021 08:45

membphis requested changes Jan 21, 2021

View reviewed changes

fukiki force-pushed the apisix-run-deps branch from 519b4f6 to 8911df5 Compare January 22, 2021 00:58

membphis reviewed Jan 22, 2021

View reviewed changes

fukiki force-pushed the apisix-run-deps branch 2 times, most recently from 208613a to 5583dac Compare January 22, 2021 03:28

membphis reviewed Jan 22, 2021

View reviewed changes

fukiki reopened this Jan 26, 2021

fukiki force-pushed the apisix-run-deps branch from ef3847e to 70e5eaa Compare January 28, 2021 02:52

fukiki force-pushed the apisix-run-deps branch 2 times, most recently from 531e38b to 818fe47 Compare January 28, 2021 09:27

fukiki changed the title ~~fix: Use luajit or lua 5.1 by default when run APISIX~~ fix: Use luajit by default when run APISIX CLI Jan 29, 2021

membphis reviewed Jan 29, 2021

View reviewed changes

apisix/cli/ngx_tpl.lua Show resolved Hide resolved

apisix/cli/ngx_tpl.lua Show resolved Hide resolved

fukiki force-pushed the apisix-run-deps branch 2 times, most recently from 6b70000 to ac25353 Compare January 29, 2021 03:52

fukiki force-pushed the apisix-run-deps branch from ac25353 to a4fd95e Compare January 29, 2021 12:16

membphis approved these changes Jan 29, 2021

View reviewed changes

tokers reviewed Jan 29, 2021

View reviewed changes

fix:Use luajit by default when run apisix

2a032d6

Signed-off-by: fukiki <one.fhh@163.com>

fukiki force-pushed the apisix-run-deps branch from a4fd95e to 2a032d6 Compare January 29, 2021 15:00

tokers approved these changes Jan 29, 2021

View reviewed changes

membphis merged commit 98be489 into apache:master Jan 29, 2021

fukiki deleted the apisix-run-deps branch January 29, 2021 16:11

imjoey mentioned this pull request Feb 1, 2021

TestBalancer_roundrobin_with_weight failed apache/apisix-dashboard#1401

Closed

		@@ -35,6 +35,8 @@

		- If you want use Tengine instead of OpenResty, please take a look at this installation step script [Install Tengine at Ubuntu](../.travis/linux_tengine_runner.sh).

		- By default Apache APISIX runs with luajit of OpenResty 1.19 (priority) or Lua 5.1. If you run in to an issue `luajit: lj_asm_x86.h:2819: asm_loop_fixup: Assertion '((intptr_t)target & 15) == 0' failed`, it is caused by to the compatibility of OpenResty version. OpenResty 1.19 is recommended, please take a look at this installation step script [linux-install-openresty](../utils/linux-install-openresty.sh).

fix: Use luajit by default when run APISIX CLI #3335

fix: Use luajit by default when run APISIX CLI #3335

Conversation

fukiki commented Jan 19, 2021 • edited Loading

What this PR does / why we need it:

Pre-submission checklist:

membphis Jan 19, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fukiki commented Jan 20, 2021

membphis left a comment

Choose a reason for hiding this comment

fukiki commented Jan 20, 2021

membphis commented Jan 21, 2021 • edited Loading

fukiki commented Jan 21, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

membphis left a comment

Choose a reason for hiding this comment

membphis commented Jan 27, 2021

spacewander commented Jan 27, 2021

fukiki commented Jan 27, 2021

membphis commented Jan 27, 2021

tokers commented Jan 27, 2021

fukiki commented Jan 28, 2021

fukiki commented Jan 29, 2021

membphis commented Jan 29, 2021

membphis commented Jan 29, 2021

fukiki commented Jan 29, 2021

liuxiran commented Jan 29, 2021

membphis left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tokers commented Jan 29, 2021

membphis commented Jan 29, 2021

fukiki commented Jan 19, 2021 •

edited

Loading

membphis Jan 19, 2021 •

edited

Loading

membphis commented Jan 21, 2021 •

edited

Loading