Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feature: remove lua dir. #1351

Merged
merged 7 commits into from
Mar 31, 2020
Merged

feature: remove lua dir. #1351

merged 7 commits into from
Mar 31, 2020

Conversation

@moonming moonming changed the title feature: remove lua dir. [WIP]feature: remove lua dir. Mar 27, 2020
Copy link
Member

@membphis membphis left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we need to update the t/APISIX.pm too

bin/apisix Outdated Show resolved Hide resolved
Copy link
Contributor

@chnliyong chnliyong left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@membphis @moonming This PR's code seems install the apisix code in directory $PKG_ROOT/apisix/, not $PKG_ROOT/(I just use PKG_ROOT to represent the root package search path), is this right?
We should confirm which method is better.
I prefer treat apisix as a regular rocks lib and the target to be installed in $PKG_ROOT:

$ tree -L 1 deps/share/lua/5.1/
deps/share/lua/5.1/
├── apisix
├── apisix.lua
├── jsonschema
├── jsonschema.lua
├── net
├── opentracing
├── protoc.lua
├── resty
├── tinyyaml.lua
└── typeof.lua

Take this for example: the apisix.lua and apisix directory under this directory, not add a directory level named apisix, which would look like:

deps/share/lua/5.1/
├── apisix
│      ├── apisix
│      └── apisix.lua
│......

bin/apisix Outdated
local pkg_path = apisix_home .. "/deps/share/lua/5.1/apisix/lua/?.lua;"
.. "/usr/local/share/lua/5.1/apisix/lua/?.lua;"
local pkg_path = apisix_home .. "/deps/share/lua/5.1/apisix/?.lua;"
.. "/usr/local/share/lua/5.1/apisix/?.lua;"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

All packages begin at apisix_home, this line looks redundant.

Contributing.md Show resolved Hide resolved
@membphis
Copy link
Member

I prefer treat apisix as a regular rocks lib and the target to be installed in $PKG_ROOT:

I agree with you ^_^

@moonming
Copy link
Member Author

moonming commented Mar 28, 2020 via email

Makefile Outdated Show resolved Hide resolved
@membphis
Copy link
Member

Will this conflict with other luarocks libraries?

No conflict.

@moonming moonming force-pushed the remove-lua-dir branch 2 times, most recently from 5f46cb5 to 17f730a Compare March 28, 2020 08:16
@membphis
Copy link
Member

lua/apisix.lua → apisix.lua

How about lua/apisix.lua → apisix/init.lua ?

@moonming
Copy link
Member Author

lua/apisix.lua → apisix.lua

How about lua/apisix.lua → apisix/init.lua ?

good idea

@moonming moonming changed the title [WIP]feature: remove lua dir. feature: remove lua dir. Mar 29, 2020
@dabue
Copy link
Contributor

dabue commented Mar 29, 2020 via email

@moonming moonming force-pushed the remove-lua-dir branch 2 times, most recently from f290920 to 1e93f11 Compare March 30, 2020 13:57
@@ -25,7 +25,7 @@ add_block_preprocessor(sub {

my $init_by_lua_block = <<_EOC_;
require "resty.core"
apisix = require("apisix")
apisix = require("init")
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is wrong, we should use the original name "apisix".

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

./?/init.lua; is the standard Lua source code search path.

please take a look at this:

$ cat ../mm.patch
diff --git a/bin/apisix b/bin/apisix
index e27da893..c393536e 100755
--- a/bin/apisix
+++ b/bin/apisix
@@ -132,7 +132,7 @@ stream {

     init_by_lua_block {
         require "resty.core"
-        apisix = require("init")
+        apisix = require("apisix")
         apisix.stream_init()
     }

@@ -261,7 +261,7 @@ http {

     init_by_lua_block {
         require "resty.core"
-        apisix = require("init")
+        apisix = require("apisix")

         local dns_resolver = { {% for _, dns_addr in ipairs(dns_resolver or {}) do %} "{*dns_addr*}", {% end %} }
         local args = {
diff --git a/dashboard b/dashboard
index cfb3ee7b..5ce7c37c 160000
--- a/dashboard
+++ b/dashboard
@@ -1 +1 @@
-Subproject commit cfb3ee7b8721076975c1deaff3e52da3ea4a312a
+Subproject commit 5ce7c37c758f6365da042333d7909c12c0ab6714-dirty
diff --git a/t/APISIX.pm b/t/APISIX.pm
index e1a336a8..6b191524 100644
--- a/t/APISIX.pm
+++ b/t/APISIX.pm
@@ -107,7 +107,7 @@ _EOC_

     my $stream_enable = $block->stream_enable;
     my $stream_config = $block->stream_config // <<_EOC_;
-    lua_package_path "$apisix_home/deps/share/lua/5.1/?.lua;$apisix_home/apisix/?.lua;$apisix_home/t/?.lua;./?.lua;;";
+    lua_package_path "$apisix_home/deps/share/lua/5.1/?.lua;$apisix_home/apisix/?.lua;$apisix_home/t/?.lua;./?.lua;./?/init.lua;;";
     lua_package_cpath "$apisix_home/deps/lib/lua/5.1/?.so;$apisix_home/deps/lib64/lua/5.1/?.so;./?.so;;";

     lua_socket_log_errors off;
@@ -127,7 +127,7 @@ _EOC_

         require "resty.core"

-        apisix = require("init")
+        apisix = require("apisix")
         apisix.stream_init()
     }

@@ -177,7 +177,7 @@ _EOC_

     require "resty.core"

-    apisix = require("init")
+    apisix = require("apisix")
     local args = {
         dns_resolver = $dns_addrs_tbl_str,
     }
@@ -186,7 +186,7 @@ _EOC_

     my $http_config = $block->http_config // '';
     $http_config .= <<_EOC_;
-    lua_package_path "$apisix_home/deps/share/lua/5.1/?.lua;$apisix_home/apisix/?.lua;$apisix_home/t/?.lua;./?.lua;;";
+    lua_package_path "$apisix_home/deps/share/lua/5.1/?.lua;$apisix_home/apisix/?.lua;$apisix_home/t/?.lua;./?.lua;./?/init.lua;;";
     lua_package_cpath "$apisix_home/deps/lib/lua/5.1/?.so;$apisix_home/deps/lib64/lua/5.1/?.so;./?.so;;";

     lua_shared_dict plugin-limit-req     10m;
@@ -216,7 +216,7 @@ _EOC_
     }

     init_worker_by_lua_block {
-        require("init").http_init_worker()
+        require("apisix").http_init_worker()
     }

     # fake server, only for test
diff --git a/t/admin/balancer.t b/t/admin/balancer.t
index d88f19bf..0be70dbd 100644
--- a/t/admin/balancer.t
+++ b/t/admin/balancer.t
@@ -25,7 +25,7 @@ add_block_preprocessor(sub {

     my $init_by_lua_block = <<_EOC_;
     require "resty.core"
-    apisix = require("init")
+    apisix = require("apisix")
     apisix.http_init()

     function test(route, ctx, count)
diff --git a/t/admin/health-check.t b/t/admin/health-check.t
index 1020a30d..8680768e 100644
--- a/t/admin/health-check.t
+++ b/t/admin/health-check.t
@@ -27,7 +27,7 @@ add_block_preprocessor(sub {

     my $init_by_lua_block = <<_EOC_;
     require "resty.core"
-    apisix = require("init")
+    apisix = require("apisix")
     apisix.http_init()

     json = require("cjson.safe")

@membphis
Copy link
Member

@moonming you can make a try with this patch:

diff --git a/bin/apisix b/bin/apisix
index e00e9af6..871de12d 100755
--- a/bin/apisix
+++ b/bin/apisix
@@ -39,9 +39,7 @@ local pkg_path_org = package.path
 local apisix_home = "/usr/local/apisix"
 local pkg_cpath = apisix_home .. "/deps/lib64/lua/5.1/?.so;"
                   .. apisix_home .. "/deps/lib/lua/5.1/?.so;;"
-local pkg_path  = apisix_home .. "/deps/share/lua/5.1/apisix/?.lua;"
-                  .. "/usr/local/share/lua/5.1/apisix/?.lua;"
-                  .. apisix_home .. "/deps/share/lua/5.1/?.lua;;"
+local pkg_path  = apisix_home ..    "/deps/share/lua/5.1/?.lua;;"

 -- only for developer, use current folder as working space
 local is_root_path = false
@@ -58,7 +56,7 @@ if script_path:sub(1, 2) == './' then

     pkg_cpath = apisix_home .. "/deps/lib64/lua/5.1/?.so;"
                 .. apisix_home .. "/deps/lib/lua/5.1/?.so;"
-    pkg_path  = apisix_home .. "/apisix/?.lua;"
+    pkg_path  = apisix_home .. "/?/init.lua;"
                 .. apisix_home .. "/deps/share/lua/5.1/?.lua;;"
 end
 -- print("apisix_home: ", apisix_home)
@@ -114,7 +112,7 @@ env APISIX_PROFILE;
 {% if stream_proxy then %}
 stream {
     lua_package_path  "$prefix/deps/share/lua/5.1/?.lua;]=]
-                      .. [=[{*apisix_lua_home*}/apisix/?.lua;;{*lua_path*};";
+                      .. [=[{*apisix_lua_home*}/?.lua;;{*lua_path*};";
     lua_package_cpath "$prefix/deps/lib64/lua/5.1/?.so;]=]
                       .. [=[$prefix/deps/lib/lua/5.1/?.so;;]=]
                       .. [=[{*lua_cpath*};";
@@ -167,7 +165,7 @@ stream {

 http {
     lua_package_path  "$prefix/deps/share/lua/5.1/?.lua;]=]
-                      .. [=[{*apisix_lua_home*}/apisix/?.lua;;{*lua_path*};";
+                      .. [=[{*apisix_lua_home*}/?.lua;;{*lua_path*};";
     lua_package_cpath "$prefix/deps/lib64/lua/5.1/?.so;]=]
                       .. [=[$prefix/deps/lib/lua/5.1/?.so;;]=]
                       .. [=[{*lua_cpath*};";

@moonming moonming merged commit c76e7e3 into apache:master Mar 31, 2020
SaberMaster pushed a commit to SaberMaster/incubator-apisix that referenced this pull request Jun 30, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants