Merge pull request #631 from mpeterv/fix-conflict-resolution

Fix conflict resolution

4 files changed, 188 insertions, 96 deletions

diff --git a/spec/install_spec.lua b/spec/install_spec.lua
index 2bbf4221..a00396fa 100644
--- a/spec/install_spec.lua
+++ b/spec/install_spec.lua
@@ -21,6 +21,8 @@ local extra_rocks = {
    "/wsapi-1.6-1.src.rock",
    "/luafilesystem-1.6.3-2.src.rock",
    "/luafilesystem-1.6.3-1.src.rock",
+   "/luacheck-0.7.3-1.src.rock",
+   "/luacheck-0.8.0-1.src.rock",
 }
 
 describe("LuaRocks install tests #blackbox #b_install", function()
@@ -87,13 +89,35 @@ describe("LuaRocks install tests #blackbox #b_install", function()
       it('LuaRocks install - handle versioned modules when installing another version with --keep #268', function()
          assert.is_true(run.luarocks_bool("install luafilesystem"))
          assert.is.truthy(lfs.attributes(testing_paths.testing_sys_tree .. "/lib/lua/"..env_variables.LUA_VERSION.."/lfs."..test_env.lib_extension))
+
          assert.is_true(run.luarocks_bool("install luafilesystem 1.6.3-1 --keep"))
          assert.is.truthy(lfs.attributes(testing_paths.testing_sys_tree .. "/lib/lua/"..env_variables.LUA_VERSION.."/lfs."..test_env.lib_extension))
          assert.is.truthy(lfs.attributes(testing_paths.testing_sys_tree .. "/lib/lua/"..env_variables.LUA_VERSION.."/luafilesystem_1_6_3_1-lfs."..test_env.lib_extension))
+
          assert.is_true(run.luarocks_bool("install luafilesystem"))
          assert.is.truthy(lfs.attributes(testing_paths.testing_sys_tree .. "/lib/lua/"..env_variables.LUA_VERSION.."/lfs."..test_env.lib_extension))
          assert.is.falsy(lfs.attributes(testing_paths.testing_sys_tree .. "/lib/lua/"..env_variables.LUA_VERSION.."/luafilesystem_1_6_3_1-lfs."..test_env.lib_extension))
       end)
+
+      it('LuaRocks install - handle versioned modules and commands from different files when upgrading #302', function()
+         io.open(testing_paths.testing_sys_tree .. "/bin/luacheck"..test_env.wrapper_extension, "w"):close()
+         assert.is_true(run.luarocks_bool("install luacheck 0.7.3 --deps-mode=none"))
+         assert.is.truthy(lfs.attributes(testing_paths.testing_sys_tree .. "/share/lua/"..env_variables.LUA_VERSION.."/luacheck.lua"))
+         assert.is.truthy(lfs.attributes(testing_paths.testing_sys_tree .. "/bin/luacheck"..test_env.wrapper_extension))
+         assert.is.truthy(lfs.attributes(testing_paths.testing_sys_tree .. "/bin/luacheck"..test_env.wrapper_extension .. "~"))
+
+         assert.is_true(run.luarocks_bool("install luacheck 0.8.0 --deps-mode=none"))
+         assert.is.truthy(lfs.attributes(testing_paths.testing_sys_tree .. "/share/lua/"..env_variables.LUA_VERSION.."/luacheck/init.lua"))
+         assert.is.truthy(lfs.attributes(testing_paths.testing_sys_tree .. "/bin/luacheck"..test_env.wrapper_extension))
+         assert.is.falsy(lfs.attributes(testing_paths.testing_sys_tree .. "/share/lua/"..env_variables.LUA_VERSION.."/luacheck_0_7_3_1-luacheck.lua"))
+         assert.is.falsy(lfs.attributes(testing_paths.testing_sys_tree .. "/bin/luacheck_0_7_3_1-luacheck"..test_env.wrapper_extension))
+
+         assert.is_true(run.luarocks_bool("install luacheck 0.7.3 --keep --deps-mode=none"))
+         assert.is.truthy(lfs.attributes(testing_paths.testing_sys_tree .. "/share/lua/"..env_variables.LUA_VERSION.."/luacheck/init.lua"))
+         assert.is.truthy(lfs.attributes(testing_paths.testing_sys_tree .. "/bin/luacheck"..test_env.wrapper_extension))
+         assert.is.truthy(lfs.attributes(testing_paths.testing_sys_tree .. "/share/lua/"..env_variables.LUA_VERSION.."/luacheck_0_7_3_1-luacheck.lua"))
+         assert.is.truthy(lfs.attributes(testing_paths.testing_sys_tree .. "/bin/luacheck_0_7_3_1-luacheck"..test_env.wrapper_extension))
+      end)
       
       it("LuaRocks install only-deps of luasocket packed rock", function()
          assert.is_true(run.luarocks_bool("build --pack-binary-rock luasocket 3.0rc1-2"))
diff --git a/src/luarocks/manif.lua b/src/luarocks/manif.lua
index a8bbf279..1b75452b 100644
--- a/src/luarocks/manif.lua
+++ b/src/luarocks/manif.lua
@@ -469,33 +469,34 @@ local function relative_path(from_dir, to_file)
    return (to_file:sub(#from_dir + 1):gsub("^[\\/]*", ""))
 end
 
-local function find_providers(file, root)
-   assert(type(file) == "string")
-   root = root or cfg.root_dir
-
-   local manifest, err = manif_core.load_local_manifest(path.rocks_dir(root))
-   if not manifest then
-      return nil, "untracked"
-   end
+local function file_manifest_coordinates(manifest, file, root)
    local deploy_bin = path.deploy_bin_dir(root)
    local deploy_lua = path.deploy_lua_dir(root)
    local deploy_lib = path.deploy_lib_dir(root)
-   local key, manifest_tbl
 
    if util.starts_with(file, deploy_lua) then
-      manifest_tbl = manifest.modules
-      key = path.path_to_module(relative_path(deploy_lua, file):gsub("\\", "/"))
+      return "modules", path.path_to_module(relative_path(deploy_lua, file):gsub("\\", "/")), deploy_lua
    elseif util.starts_with(file, deploy_lib) then
-      manifest_tbl = manifest.modules
-      key = path.path_to_module(relative_path(deploy_lib, file):gsub("\\", "/"))
+      return "modules", path.path_to_module(relative_path(deploy_lib, file):gsub("\\", "/")), deploy_lib
    elseif util.starts_with(file, deploy_bin) then
-      manifest_tbl = manifest.commands
-      key = relative_path(deploy_bin, file)
+      return "commands", relative_path(deploy_bin, file), deploy_bin
    else
       assert(false, "Assertion failed: '"..file.."' is not a deployed file.")
    end
+end
+
+local function find_providers(file, root)
+   assert(type(file) == "string")
+   root = root or cfg.root_dir
+
+   local manifest, err = manif_core.load_local_manifest(path.rocks_dir(root))
+   if not manifest then
+      return nil, "untracked"
+   end
+
+   local type_key, key = file_manifest_coordinates(manifest, file, root)
 
-   local providers = manifest_tbl[key]
+   local providers = manifest[type_key][key]
    if not providers then
       return nil, "untracked"
    end
@@ -523,4 +524,25 @@ function manif.find_next_provider(file, root)
    end
 end
 
+--- Given a file conflicting with a module or command
+-- provided by a version of a package, return which file
+-- in that version corresponds to the conflicting item.
+-- @param name string: name of the package with conflicting module or command.
+-- @param version string: version of the package with conflicting module or command.
+-- @param file string: full, unversioned path to a deployed file.
+-- @return string: full, unversioned path to a deployed file in
+-- given package that conflicts with given file.
+function manif.find_conflicting_file(name, version, file, root)
+   root = root or cfg.root_dir
+
+   local manifest = manif_core.load_local_manifest(path.rocks_dir(root))
+   if not manifest then
+      return
+   end
+
+   local entry_table = manifest.repository[name][version][1]
+   local type_key, key, deploy_dir = file_manifest_coordinates(manifest, file, root)
+   return dir.path(deploy_dir, entry_table[type_key][key])
+end
+
 return manif
diff --git a/src/luarocks/repos.lua b/src/luarocks/repos.lua
index a1cf2bb4..161cdd8a 100644
--- a/src/luarocks/repos.lua
+++ b/src/luarocks/repos.lua
@@ -151,50 +151,83 @@ function repos.run_hook(rockspec, hook_name)
    return true
 end
 
-local function install_binary(source, target, name, version)
-   assert(type(source) == "string")
-   assert(type(target) == "string")
-   
-   if fs.is_lua(source) then
-      return fs.wrap_script(source, target, name, version)
-   else
-      return fs.copy_binary(source, target)
+function repos.should_wrap_bin_scripts(rockspec)
+   assert(type(rockspec) == "table")
+
+   if cfg.wrap_bin_scripts ~= nil then
+      return cfg.wrap_bin_scripts
+   end
+   if rockspec.deploy and rockspec.deploy.wrap_bin_scripts == false then
+      return false
    end
+   return true
 end
 
-local function resolve_conflict(target, deploy_dir, name, version)
-   local cname, cversion = manif.find_current_provider(target)
-   if not cname then
-      return nil, cversion
+local function find_suffixed(file, suffix)
+   local filenames = {file}
+   if suffix and suffix ~= "" then
+      table.insert(filenames, 1, file .. suffix)
    end
-   if name ~= cname or deps.compare_versions(version, cversion) then
-      local versioned = path.versioned_name(target, deploy_dir, cname, cversion)
-      local ok, err = fs.make_dir(dir.dir_name(versioned))
-      if not ok then return nil, err end
-      fs.move(target, versioned)
-      return target
-   else
-      return path.versioned_name(target, deploy_dir, name, version)
+
+   for _, filename in ipairs(filenames) do
+      if fs.exists(filename) then
+         return filename
+      end
    end
 end
 
-function repos.should_wrap_bin_scripts(rockspec)
-   assert(type(rockspec) == "table")
+local function move_suffixed(from_file, to_file, suffix)
+   local suffixed_from_file = find_suffixed(from_file, suffix)
+   if not suffixed_from_file then
+      return nil, "File not found"
+   end
 
-   if cfg.wrap_bin_scripts ~= nil then
-      return cfg.wrap_bin_scripts
+   suffix = suffixed_from_file:sub(#from_file + 1)
+   local suffixed_to_file = to_file .. suffix
+   return fs.move(suffixed_from_file, suffixed_to_file)
+end
+
+local function delete_suffixed(file, suffix)
+   local suffixed_file = find_suffixed(file, suffix)
+   if not suffixed_file then
+      return nil, "File not found", "not found"
    end
-   if rockspec.deploy and rockspec.deploy.wrap_bin_scripts == false then
-      return false
+
+   fs.delete(suffixed_file)
+   if fs.exists(suffixed_file) then
+      return nil, "Failed deleting " .. suffixed_file, "fail"
    end
+
    return true
 end
 
+local function resolve_conflict(target, deploy_dir, name, version, cur_name, cur_version, suffix)
+   if name < cur_name or (name == cur_name and deps.compare_versions(version, cur_version)) then
+      -- New version has priority. Move currently provided version back using versioned name.
+      local cur_target = manif.find_conflicting_file(cur_name, cur_version, target)
+      local versioned = path.versioned_name(cur_target, deploy_dir, cur_name, cur_version)
+
+      local ok, err = fs.make_dir(dir.dir_name(versioned))
+      if not ok then
+         return nil, err
+      end
+
+      ok, err = move_suffixed(cur_target, versioned, suffix)
+      if not ok then
+         return nil, err
+      end
+
+      return target
+   else
+      -- Current version has priority, deploy new version using versioned name.
+      return path.versioned_name(target, deploy_dir, name, version)
+   end
+end
+
 --- Deploy a package from the rocks subdirectory.
--- It is maintained that for each file the one that is provided
+-- It is maintained that for each module and command the one that is provided
 -- by the newest version of the lexicographically smallest package
--- is installed using unversioned name, and other versions of the file
--- use versioned names.
+-- is installed using unversioned name, and other versions use versioned names.
 -- @param name string: name of package
 -- @param version string: exact package version in string format
 -- @param wrap_bin_scripts bool: whether commands written in Lua should be wrapped.
@@ -206,50 +239,66 @@ function repos.deploy_files(name, version, wrap_bin_scripts, deps_mode)
    assert(type(version) == "string")
    assert(type(wrap_bin_scripts) == "boolean")
 
-   local function deploy_file_tree(file_tree, path_fn, deploy_dir, move_fn)
+   local function deploy_file_tree(file_tree, path_fn, deploy_dir, move_fn, suffix)
       local source_dir = path_fn(name, version)
       return recurse_rock_manifest_tree(file_tree, 
          function(parent_path, parent_module, file)
             local source = dir.path(source_dir, parent_path, file)
             local target = dir.path(deploy_dir, parent_path, file)
-            local ok, err
-            if fs.exists(target) then
-               local new_target, err = resolve_conflict(target, deploy_dir, name, version)
-               if err == "untracked" then
-                  local backup = target
-                  repeat
-                     backup = backup.."~"
-                  until not fs.exists(backup) -- slight race condition here, but shouldn't be a problem.
-                  util.printerr("Warning: "..target.." is not tracked by this installation of LuaRocks. Moving it to "..backup)
-                  fs.move(target, backup)
-               elseif err then
-                  return nil, err.." Cannot install new version."
-               else
-                  target = new_target
+
+            local cur_name, cur_version = manif.find_current_provider(target)
+            if cur_name then
+               local resolve_err
+               target, resolve_err = resolve_conflict(target, deploy_dir, name, version, cur_name, cur_version, suffix)
+               if not target then
+                  return nil, resolve_err
                end
             end
-            ok, err = fs.make_dir(dir.dir_name(target))
+
+            local ok, err = fs.make_dir(dir.dir_name(target))
             if not ok then return nil, err end
-            ok, err = move_fn(source, target, name, version)
+
+            local suffixed_target, mover = move_fn(source, target, name, version)
+            if fs.exists(suffixed_target) then
+               local backup = suffixed_target
+               repeat
+                  backup = backup.."~"
+               until not fs.exists(backup) -- Slight race condition here, but shouldn't be a problem.
+
+               util.printerr("Warning: "..suffixed_target.." is not tracked by this installation of LuaRocks. Moving it to "..backup)
+               local ok, err = fs.move(suffixed_target, backup)
+               if not ok then
+                  return nil, err
+               end
+            end
+
+            ok, err = mover()
             fs.remove_dir_tree_if_empty(dir.dir_name(source))
-            if not ok then return nil, err end
-            return true
+            return ok, err
          end
       )
    end
 
    local rock_manifest = manif.load_rock_manifest(name, version)
-   
-   local ok, err = true
-   if rock_manifest.bin then
-      local move_bin_fn = wrap_bin_scripts and install_binary or fs.copy_binary
-      ok, err = deploy_file_tree(rock_manifest.bin, path.bin_dir, cfg.deploy_bin_dir, move_bin_fn)
+
+   local function install_binary(source, target, name, version)
+      if wrap_bin_scripts and fs.is_lua(source) then
+         return target .. (cfg.wrapper_suffix or ""), function() return fs.wrap_script(source, target, name, version) end
+      else
+         return target, function() return fs.copy_binary(source, target) end
+      end
    end
+
    local function make_mover(perms)
-      return function (src, dest) 
-         return fs.move(src, dest, perms)
+      return function(source, target)
+         return target, function() return fs.move(source, target, perms) end
       end
    end
+
+   local ok, err = true
+   if rock_manifest.bin then
+      ok, err = deploy_file_tree(rock_manifest.bin, path.bin_dir, cfg.deploy_bin_dir, install_binary, cfg.wrapper_suffix)
+   end
    if ok and rock_manifest.lua then
       ok, err = deploy_file_tree(rock_manifest.lua, path.lua_dir, cfg.deploy_lua_dir, make_mover(cfg.perm_read))
    end
@@ -264,26 +313,10 @@ function repos.deploy_files(name, version, wrap_bin_scripts, deps_mode)
    return manif.update_manifest(name, version, nil, deps_mode)
 end
 
-local function delete_suffixed(filename, suffix)
-   local filenames = { filename }
-   if suffix and suffix ~= "" then filenames = { filename..suffix, filename } end
-   for _, name in ipairs(filenames) do
-      if fs.exists(name) then
-         fs.delete(name)
-         if fs.exists(name) then
-            return nil, "Failed deleting "..name, "fail"
-         end
-         return true, name
-      end
-   end
-   return false, "File not found", "not found"
-end
-
 --- Delete a package from the local repository.
--- It is maintained that for each file the one that is provided
+-- It is maintained that for each module and command the one that is provided
 -- by the newest version of the lexicographically smallest package
--- is installed using unversioned name, and other versions of the file
--- use versioned names.
+-- is installed using unversioned name, and other versions use versioned names.
 -- @param name string: name of package
 -- @param version string: exact package version in string format
 -- @param deps_mode: string: Which trees to check dependencies for:
@@ -303,19 +336,31 @@ function repos.delete_version(name, version, deps_mode, quick)
          function(parent_path, parent_module, file)
             local target = dir.path(deploy_dir, parent_path, file)
             local versioned = path.versioned_name(target, deploy_dir, name, version)
-            local ok, name, err = delete_suffixed(versioned, suffix)
+
+            local ok, err, err_type = delete_suffixed(versioned, suffix)
             if ok then
                fs.remove_dir_tree_if_empty(dir.dir_name(versioned))
                return true
+            elseif err_type == "fail" then
+               return nil, err
+            end
+
+            ok, err = delete_suffixed(target, suffix)
+            if not ok then
+               return nil, err
             end
-            if err == "fail" then return nil, name end
-            ok, name, err = delete_suffixed(target, suffix)
-            if err == "fail" then return nil, name end
+
             if not quick then
                local next_name, next_version = manif.find_next_provider(target)
                if next_name then
-                  local versioned = path.versioned_name(name, deploy_dir, next_name, next_version)
-                  fs.move(versioned, name)
+                  local next_target = manif.find_conflicting_file(next_name, next_version, target)
+                  local next_versioned = path.versioned_name(next_target, deploy_dir, next_name, next_version)
+
+                  ok, err = move_suffixed(next_versioned, next_target, suffix)
+                  if not ok then
+                     return nil, err
+                  end
+
                   fs.remove_dir_tree_if_empty(dir.dir_name(versioned))
                end
             end
@@ -340,7 +385,7 @@ function repos.delete_version(name, version, deps_mode, quick)
    if ok and rock_manifest.lib then
       ok, err = delete_deployed_file_tree(rock_manifest.lib, cfg.deploy_lib_dir)
    end
-   if err then return nil, err end
+   if not ok then return nil, err end
 
    fs.delete(path.install_dir(name, version))
    if not get_installed_versions(name) then
diff --git a/test/test_environment.lua b/test/test_environment.lua
index 1d0b0c32..196202df 100644
--- a/test/test_environment.lua
+++ b/test/test_environment.lua
@@ -515,6 +515,7 @@ function test_env.setup_specs(extra_rocks)
 
       test_env.platform = execute_output(test_env.testing_paths.lua .. " -e \"print(require('luarocks.cfg').arch)\"", false, test_env.env_variables)
       test_env.lib_extension = execute_output(test_env.testing_paths.lua .. " -e \"print(require('luarocks.cfg').lib_extension)\"", false, test_env.env_variables)
+      test_env.wrapper_extension = test_env.TEST_TARGET_OS == "windows" and ".bat" or ""
       test_env.md5sums = create_md5sums(test_env.testing_paths)
       test_env.setup_done = true
       title("RUNNING TESTS")