fs: always check for failure of fs.which_tool (#1157)

5 files changed, 48 insertions, 23 deletions

diff --git a/spec/install_spec.lua b/spec/install_spec.lua
index 5ee913f0..78b4b28d 100644
--- a/spec/install_spec.lua
+++ b/spec/install_spec.lua
@@ -56,6 +56,18 @@ describe("luarocks install #integration", function()
          assert.is_false(run.luarocks_bool("install --local luasocket ", { USER = "root" } ))
       end)
 
+      it("fails with no downloader", function()
+         if test_env.TYPE_TEST_ENV ~= "full" then
+            local output = assert(run.luarocks("install https://example.com/rock-1.0.src.rock", { LUAROCKS_CONFIG = testing_paths.testrun_dir .. "/testing_config_no_downloader.lua" } ))
+            assert.match("no downloader tool", output)
+
+            -- can do http but not https
+            assert(run.luarocks("install luasocket"))
+            output = assert(run.luarocks("install https://example.com/rock-1.0.src.rock", { LUAROCKS_CONFIG = testing_paths.testrun_dir .. "/testing_config_no_downloader.lua" } ))
+            assert.match("no downloader tool", output)
+         end
+      end)
+
       it("fails not a zip file", function()
          test_env.run_in_tmp(function(tmpdir)
             write_file("not_a_zipfile-1.0-1.src.rock", [[
diff --git a/spec/util/test_env.lua b/spec/util/test_env.lua
index d4591c91..8aebd74e 100644
--- a/spec/util/test_env.lua
+++ b/spec/util/test_env.lua
@@ -709,7 +709,9 @@ end
 --- Create configs for luacov and several versions of Luarocks
 -- configs needed for some tests.
 local function create_configs()
-   -- testing_config.lua and testing_config_show_downloads.lua
+   -- testing_config.lua
+   -- testing_config_show_downloads.lua
+   -- testing_config_no_downloader.lua
    local config_content = substitute([[
       rocks_trees = {
          "%{testing_tree}",
@@ -737,6 +739,8 @@ local function create_configs()
    test_env.write_file(test_env.testing_paths.testrun_dir .. "/testing_config.lua", config_content .. " \nweb_browser = \"true\"")
    test_env.write_file(test_env.testing_paths.testrun_dir .. "/testing_config_show_downloads.lua", config_content
                   .. "show_downloads = true \n rocks_servers={\"http://luarocks.org/repositories/rocks\"}")
+   test_env.write_file(test_env.testing_paths.testrun_dir .. "/testing_config_no_downloader.lua", config_content
+                  .. "variables = { WGET = 'invalid', CURL = 'invalid' }")
 
    -- testing_config_sftp.lua
    config_content = substitute([[
diff --git a/src/luarocks/fetch.lua b/src/luarocks/fetch.lua
index 464061d1..da912d3e 100644
--- a/src/luarocks/fetch.lua
+++ b/src/luarocks/fetch.lua
@@ -31,7 +31,7 @@ function fetch.fetch_caching(url)
 
    local file, err, errcode, from_cache = fetch.fetch_url(url, cachefile, true)
    if not file then
-      return nil, "Failed downloading "..repo_url..(err and " - "..err or ""), errcode
+      return nil, err or "Failed downloading "..url, errcode
    end
    return file, nil, nil, from_cache
 end
@@ -67,7 +67,7 @@ function fetch.fetch_url(url, filename, cache)
    elseif dir.is_basic_protocol(protocol) then
       local ok, name, from_cache = fs.download(url, filename, cache)
       if not ok then
-         return nil, "Failed downloading "..url..(filename and " - "..filename or ""), "network"
+         return nil, "Failed downloading "..url..(name and " - "..name or ""), "network"
       end
       return name, nil, nil, from_cache
    else
diff --git a/src/luarocks/fs/lua.lua b/src/luarocks/fs/lua.lua
index c5cebd2f..b6314a44 100644
--- a/src/luarocks/fs/lua.lua
+++ b/src/luarocks/fs/lua.lua
@@ -905,8 +905,11 @@ function fs_lua.download(url, filename, cache)
       err = "Unsupported protocol"
    end
    if https_err then
+      local downloader, err = fs.which_tool("downloader")
+      if not downloader then
+         return nil, err
+      end
       if not downloader_warning then
-         local downloader = fs.which_tool("downloader")
          util.warning("falling back to "..downloader.." - install luasec to get native HTTPS support")
          downloader_warning = true
       end
diff --git a/src/luarocks/fs/tools.lua b/src/luarocks/fs/tools.lua
index 541352c2..fdb93dba 100644
--- a/src/luarocks/fs/tools.lua
+++ b/src/luarocks/fs/tools.lua
@@ -15,10 +15,12 @@ do
    
    local tool_options = {
       downloader = {
+         desc = "downloader",
          { var = "WGET", name = "wget" },
          { var = "CURL", name = "curl" },
       },
       md5checker = {
+         desc = "MD5 checker",
          { var = "MD5SUM", name = "md5sum" },
          { var = "OPENSSL", name = "openssl", cmdarg = "md5", checkarg = "version" },
          { var = "MD5", name = "md5", checkarg = "-shello" },
@@ -27,8 +29,10 @@ do
 
    function tools.which_tool(tooltype)
       local tool = tool_cache[tooltype]
+      local names = {}
       if not tool then
          for _, opt in ipairs(tool_options[tooltype]) do
+            table.insert(names, opt.name)
             if fs.is_tool_available(vars[opt.var], opt.name, opt.checkarg) then
                tool = opt
                tool_cache[tooltype] = opt
@@ -37,7 +41,8 @@ do
          end
       end
       if not tool then
-         return nil
+         local tool_names = table.concat(names, ", ", 1, #names - 1) .. " or " .. names[#names]
+         return nil, "no " .. tool_options[tooltype].desc .. " tool available," .. " please install " .. tool_names .. " in your system"
       end
       return tool.name, vars[tool.var] .. (tool.cmdarg and " "..tool.cmdarg or "")
    end
@@ -143,9 +148,12 @@ function tools.use_downloader(url, filename, cache)
 
    filename = fs.absolute_name(filename or dir.base_name(url))
    
-   local downloader = fs.which_tool("downloader")
+   local downloader, err = fs.which_tool("downloader")
+   if not downloader then
+      return nil, err
+   end
 
-   local ok
+   local ok = false
    if downloader == "wget" then
       local wget_cmd = vars.WGET.." "..vars.WGETNOCERTFLAG.." --no-cache --user-agent=\""..cfg.user_agent.." via wget\" --quiet "
       if cfg.connection_timeout and cfg.connection_timeout > 0 then
@@ -172,14 +180,12 @@ function tools.use_downloader(url, filename, cache)
          curl_cmd = curl_cmd .. " -R -z \"" .. filename .. "\" "
       end
       ok = fs.execute_string(fs.quiet_stderr(curl_cmd..fs.Q(url).." --output "..fs.Q(filename)))
-   else
-      return false, "No downloader tool available -- please install 'wget' or 'curl' in your system"
    end
    if ok then
       return true, filename
    else
       os.remove(filename)
-      return false, "Failed downloading " .. url
+      return false, "failed downloading " .. url
    end
 end
 
@@ -188,20 +194,20 @@ end
 -- @return string: The MD5 checksum or nil + message
 function tools.get_md5(file)
    local ok, md5checker = fs.which_tool("md5checker")
-   if ok then
-      local pipe = io.popen(md5checker.." "..fs.Q(fs.absolute_name(file)))
-      local computed = pipe:read("*l")
-      pipe:close()
-      if computed then
-         computed = computed:match("("..("%x"):rep(32)..")")
-      end
-      if computed then
-         return computed
-      else
-         return nil, "Failed to compute MD5 hash for file "..tostring(fs.absolute_name(file))
-      end
+   if not ok then
+      return false, md5checker
+   end
+
+   local pipe = io.popen(md5checker.." "..fs.Q(fs.absolute_name(file)))
+   local computed = pipe:read("*l")
+   pipe:close()
+   if computed then
+      computed = computed:match("("..("%x"):rep(32)..")")
+   end
+   if computed then
+      return computed
    else
-      return false, "No MD5 checking tool available -- please install 'md5', 'md5sum' or 'openssl' in your system"
+      return nil, "Failed to compute MD5 hash for file "..tostring(fs.absolute_name(file))
    end
 end