Merge pull request #7602 from Shiffted/memcache-fix

fix load package memcache

6 files changed, 95 insertions, 64 deletions

diff --git a/tests/modules/package/load_from_repository/foo/xmake.lua b/tests/modules/package/load_from_repository/foo/xmake.lua
new file mode 100644
index 000000000..a96023b7f
--- /dev/null
+++ b/tests/modules/package/load_from_repository/foo/xmake.lua
@@ -0,0 +1,3 @@
+package("foo")
+    add_defines("FOO_BASE")
+package_end()
diff --git a/tests/modules/package/load_from_repository/test.lua b/tests/modules/package/load_from_repository/test.lua
new file mode 100644
index 000000000..7899175d8
--- /dev/null
+++ b/tests/modules/package/load_from_repository/test.lua
@@ -0,0 +1,20 @@
+import("core.package.package")
+
+function test_load_from_repository(t)
+    local packagedir = path.absolute("foo")
+    local foo1 = package.load_from_repository("foo", packagedir, {})
+    local foo2 = package.load_from_repository("foo", packagedir, {})
+    assert(foo1 and foo2, "load_from_repository(foo) failed!")
+
+    t:require(table.contains(table.wrap(foo1:get("defines")), "FOO_BASE"))
+    t:require(table.contains(table.wrap(foo2:get("defines")), "FOO_BASE"))
+
+    -- mutating one instance should not leak into the other
+    foo1:add("defines", "FOO_LEAK")
+    t:require(table.contains(table.wrap(foo1:get("defines")), "FOO_LEAK"))
+    t:require_not(table.contains(table.wrap(foo2:get("defines")), "FOO_LEAK"))
+
+    foo1:set("foo_leak", false)
+    t:require(foo1:get("foo_leak") == false)
+    t:require_not(foo2:get("foo_leak") == false)
+end
diff --git a/tests/projects/package/load_from_project/check.lua b/tests/projects/package/load_from_project/check.lua
new file mode 100644
index 000000000..d9a4edd49
--- /dev/null
+++ b/tests/projects/package/load_from_project/check.lua
@@ -0,0 +1,19 @@
+import("core.package.package")
+
+function main()
+    local foo1 = package.load_from_project("foo")
+    local foo2 = package.load_from_project("foo")
+    assert(foo1 and foo2, "load_from_project(foo) failed!")
+
+    assert(table.contains(table.wrap(foo1:get("defines")), "FOO_BASE"), "foo1 missing FOO_BASE")
+    assert(table.contains(table.wrap(foo2:get("defines")), "FOO_BASE"), "foo2 missing FOO_BASE")
+
+    -- mutating one instance should not leak into the other
+    foo1:add("defines", "FOO_LEAK")
+    assert(table.contains(table.wrap(foo1:get("defines")), "FOO_LEAK"), "foo1 missing FOO_LEAK")
+    assert(not table.contains(table.wrap(foo2:get("defines")), "FOO_LEAK"), "FOO_LEAK leaked into foo2")
+
+    foo1:set("foo_leak", false)
+    assert(foo1:get("foo_leak") == false, "foo1 foo_leak not set")
+    assert(foo2:get("foo_leak") ~= false, "foo_leak leaked into foo2")
+end
diff --git a/tests/projects/package/load_from_project/test.lua b/tests/projects/package/load_from_project/test.lua
new file mode 100644
index 000000000..9ec42530e
--- /dev/null
+++ b/tests/projects/package/load_from_project/test.lua
@@ -0,0 +1,3 @@
+function main(t)
+    os.exec("xmake l -vD check.lua")
+end
diff --git a/tests/projects/package/load_from_project/xmake.lua b/tests/projects/package/load_from_project/xmake.lua
new file mode 100644
index 000000000..a96023b7f
--- /dev/null
+++ b/tests/projects/package/load_from_project/xmake.lua
@@ -0,0 +1,3 @@
+package("foo")
+    add_defines("FOO_BASE")
+package_end()
diff --git a/xmake/core/package/package.lua b/xmake/core/package/package.lua
index 2021452d7..674e17e28 100644
--- a/xmake/core/package/package.lua
+++ b/xmake/core/package/package.lua
@@ -3006,13 +3006,8 @@ end
 -- load the package from the system directories
 function package.load_from_system(packagename)
 
-    -- get it directly from cache first
-    local instance = package._memcache():get2("packages", packagename)
-    if instance then
-        return instance
-    end
-
     -- get package info
+    local instance
     local packageinfo = {}
     local is_thirdparty = false
     if packagename:find("::", 1, true) then
@@ -3073,20 +3068,12 @@ function package.load_from_system(packagename)
         instance:set("parallelize", false)
     end
 
-    -- save instance to the cache
-    package._memcache():set2("packages", instance)
     return instance
 end
 
 -- load the package from the project file
 function package.load_from_project(packagename, project)
 
-    -- get it directly from cache first
-    local instance = package._memcache():get2("packages", packagename)
-    if instance then
-        return instance
-    end
-
     -- load packages (with cache)
     local packages, errors = project.packages()
     if not packages then
@@ -3109,21 +3096,13 @@ function package.load_from_project(packagename, project)
     end
 
     -- new an instance
-    instance = _instance.new(packagename, packageinfo, {scriptdir = os.projectdir()})
-    package._memcache():set2("packages", instance)
-    return instance
+    return _instance.new(packagename, packageinfo:clone(), {scriptdir = os.projectdir()})
 end
 
 -- load the package from the package directory or package description file
 function package.load_from_repository(packagename, packagedir, opt)
 
-    -- get it directly from cache first
     opt = opt or {}
-    local instance = package._memcache():get2("packages", packagename)
-    if instance then
-        return instance
-    end
-
 
     -- find the package script path
     local scriptpath = opt.packagefile
@@ -3134,55 +3113,59 @@ function package.load_from_repository(packagename, packagedir, opt)
         return nil, string.format("package %s not found!", packagename)
     end
 
-    -- get interpreter
-    local interp = package._interpreter()
+    -- we can only cache the description scope info, but not the package instance,
+    -- because the caller will modify the instance for each required package
+    local cachekey = scriptpath .. "/" .. packagename .. "/" .. (opt.plat or "") .. "/" .. (opt.arch or "")
+    local packageinfo = package._memcache():get2("packageinfos.repository", cachekey)
+    if not packageinfo then
 
-    -- we need to modify plat/arch in description scope at same time
-    -- if plat/arch are passed to add_requires.
-    --
-    -- @see https://github.com/orgs/xmake-io/discussions/3439
-    --
-    -- e.g. add_requires("zlib~mingw", {plat = "mingw", arch = "x86_64"})
-    --
-    if opt.plat then
-        package._memcache():set("target_plat", opt.plat)
-    end
-    if opt.arch then
-        package._memcache():set("target_arch", opt.arch)
-    end
+        -- get interpreter
+        local interp = package._interpreter()
 
-    -- load script
-    local ok, errors = interp:load(scriptpath)
-    if not ok then
-        return nil, errors
-    end
+        -- we need to modify plat/arch in description scope at same time
+        -- if plat/arch are passed to add_requires.
+        --
+        -- @see https://github.com/orgs/xmake-io/discussions/3439
+        --
+        -- e.g. add_requires("zlib~mingw", {plat = "mingw", arch = "x86_64"})
+        --
+        if opt.plat then
+            package._memcache():set("target_plat", opt.plat)
+        end
+        if opt.arch then
+            package._memcache():set("target_arch", opt.arch)
+        end
 
-    -- load package and disable filter, we will process filter after a while
-    local results, errors = interp:make("package", true, false)
-    if not results then
-        return nil, errors
-    end
+        -- load script
+        local ok, errors = interp:load(scriptpath)
+        if not ok then
+            return nil, errors
+        end
 
-    -- get package info
-    local packageinfo = results[packagename]
-    if not packageinfo then
-        return nil, string.format("%s: package(%s) not found!", scriptpath, packagename)
-    end
+        -- load package and disable filter, we will process filter after a while
+        local results, errors = interp:make("package", true, false)
+        if not results then
+            return nil, errors
+        end
 
-    -- new an instance
-    instance = _instance.new(packagename, packageinfo, {scriptdir = path.directory(scriptpath), repo = opt.repo})
+        -- reset plat/arch
+        if opt.plat then
+            package._memcache():set("target_plat", nil)
+        end
+        if opt.arch then
+            package._memcache():set("target_arch", nil)
+        end
 
-    -- reset plat/arch
-    if opt.plat then
-        package._memcache():set("target_plat", nil)
-    end
-    if opt.arch then
-        package._memcache():set("target_arch", nil)
+        -- get package info
+        packageinfo = results[packagename]
+        if not packageinfo then
+            return nil, string.format("%s: package(%s) not found!", scriptpath, packagename)
+        end
+
+        package._memcache():set2("packageinfos.repository", cachekey, packageinfo)
     end
 
-    -- save instance to the cache
-    package._memcache():set2("packages", instance)
-    return instance
+    return _instance.new(packagename, packageinfo:clone(), {scriptdir = path.directory(scriptpath), repo = opt.repo})
 end
 
 -- new a package instance