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.

Sign up for GitHub

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

Jump to bottom

Handle errors in finding binary path #82

Open
wants to merge 4 commits into
base: master
Choose a base branch
from
Open

Handle errors in finding binary path #82

Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
f89f5d7
Handle errors in finding binary path
aarondill Jun 18, 2023
184bf75
clear self.std* when closing
aarondill Jul 7, 2023
a9bf829
add warnings in case the binary can't be found (not installed)
aarondill Jul 12, 2023
c132306
move *up* arch check to ensure that unsupported arches get aborted ea…
aarondill Jul 12, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
29 changes: 27 additions & 2 deletions lua/tabnine/binary.lua
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,8 @@ local function arch_and_platform()
return "x86_64-pc-windows-gnu"
elseif os_uname.sysname == "Windows_NT" then
return "i686-pc-windows-gnu"
else
return nil
end
end

Expand All @@ -36,6 +38,11 @@ local function binary_name()
end

local function binary_path()
local machine = arch_and_platform()
Copy link
Contributor

Choose a reason for hiding this comment

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

What do you say about rewriting the build script to lua? and have it run on initialization

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I could do it, but we would end up forking to a shell for curl, unzip and chmod (and their windows equivalents) because there’s no way to do those in lua without external libraries. If thats what you want, I’ll start another PR and do that, but that seems out of scope of this PR

Copy link
Contributor

Choose a reason for hiding this comment

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

I get it. I believe it would make this a bit easier to manage... I didn't do it to begin with since it requires extra effort which I didn't have at the time.

As for this PR. I would expect binary_path() to throw an error in case it fails to find the binary for any case. The current implementation is too scattered in my opinion

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I can make binary_path to throw an error, but I would just end up calling pcall on it so we can give a nice error. It makes sense that nil represents a value that couldn't be found, which we can then interpret to give the user a warning.

Additionally, binary_path throwing an error was the original issue of #77, in which his hardware isn't supported, so it never finds the binary_path and repeatedly throws errors.

Copy link
Contributor

Choose a reason for hiding this comment

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

yes, i was aiming to use pcall & yield the error in one place. does not make sense to you?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sorry, I never saw this reply.

It does not make sense to me to use pcall higher in the call stack when we can use nil and check for it instead.

In my past experience using pcall to detect errors is unwieldy. Error() is kinda like JavaScript's throw, in where you can technically pass any data type, but most people use X (strings and Errors, respectively). This makes it hard to properly handle errors, and even if strings are properly passed and determined, they are hard to parse due to the possibility that any operation or function call errored and it climbed the call stack. Additionally the inclusion of a call stack in the error string means that we may not even be able to reliably parse the error without a complicated pattern, which would be very prone to typos and bugs.

I would much prefer to keep this limited to the one area, where we can easily check for nil (and an lsp can warn you if you forget).

Furthermore, throwing an error is exactly the problem that caused this PR. @vendion is using unsupported hardware and since this function is called (and errors) frequently, he receives constant error messages (on cursorhold). This pr intends to fix that by detecting misconfigurations and unsupported hardware and warning once for configuration, and being silent for unsupported hardware (per @vendion's request)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@amirbilu do you have feedback on this response?
If you think it's a better approach, I can change to using pcall, but this will likely make debugging and detecting issues harder in the future. I personally think it's a better idea to return an explicit not-supported value (nil in this case), and handle it directly at the call site.

if not machine then
return nil -- Is this machine supported?
end

local paths = vim.tbl_map(function(path)
return fn.fnamemodify(path, ":t")
end, fn.glob(binaries_path .. "/*", true, true))
Expand All @@ -46,7 +53,19 @@ local function binary_path()

table.sort(paths)

return binaries_path .. "/" .. tostring(paths[#paths]) .. "/" .. arch_and_platform() .. "/" .. binary_name()
local version = paths[#paths]
if not version then
vim.notify("Did you remember to run the build script for tabnine-nvim?", vim.log.levels.WARN)
return nil -- Is it installed?
end
local binary = binaries_path .. "/" .. tostring(version) .. "/" .. machine .. "/" .. binary_name()

if vim.fn.filereadable(binary) then -- Double check that it's installed
return binary
else
vim.notify("Did you remember to run the build script for tabnine-nvim?", vim.log.levels.WARN)
return nil -- File doesn't exist or isn't readable. Is it installed?
end
end

local function optional_args()
Expand All @@ -61,7 +80,7 @@ function TabnineBinary:start()
self.stdin = uv.new_pipe()
self.stdout = uv.new_pipe()
self.stderr = uv.new_pipe()
self.handle, self.pid = uv.spawn(binary_path(), {
self.handle, self.pid = uv.spawn(self.binary_path, {
args = vim.list_extend({
"--client",
"nvim",
Expand All @@ -73,6 +92,8 @@ function TabnineBinary:start()
}, function()
self.handle, self.pid = nil, nil
uv.read_stop(self.stdout)
uv.read_stop(self.stderr)
self.stdout, self.stderr, self.stdin = nil, nil, nil
end)

uv.read_start(
Expand Down Expand Up @@ -103,12 +124,16 @@ function TabnineBinary:new(o)
self.restart_counter = 0
self.handle = nil
self.pid = nil
self.binary_path = binary_path()
self.callbacks = {}

return o
end

function TabnineBinary:request(request, on_response)
if not self.binary_path then
return function() end -- To avoid breaking user configurations
end
if not self.pid then
self.restart_counter = self.restart_counter + 1
self:start()
Expand Down
Loading