Tokenize command arguments in $NEOVIM_BIN (fix #2060)

[rhysd]

What kind of change does this PR introduce?

• Fix ERROR: NEOVIM_BIN was not found. #2060

Did this PR introduce a breaking change?

A breaking change includes anything that breaks backwards compatibility either at compile or run time.

• No

I confirmed this fix on Ubuntu 22.04 and macOS 13.6.

[rhysd]

@fredizzimo Thanks for your review. I simplified my changes and removed 4 redundant clones. Would you review again?

[rhysd]

@fredizzimo BTW I got interested in why it wraps the command execution with a shell on macOS though it doesn't do it on Linux. Does executing the command directly on macOS cause any issue?

neovide/src/bridge/command.rs

Lines 149 to 164 in a8cec7e

	#[cfg(target_os = "macos")]
	fn nvim_cmd_impl(bin: &str, args: &[String]) -> TokioCommand {
	let shell = env::var("SHELL").unwrap_or_else(|_| "/bin/sh".to_string());
	let mut cmd = TokioCommand::new(shell);
	let args_str = args
	.iter()
	.map(|arg| shlex::quote(arg))
	.collect::<Vec<_>>()
	.join(" ");
	if env::var_os("TERM").is_none() {
	cmd.arg("-l");
	}
	cmd.arg("-c");
	cmd.arg(&format!("{} {}", bin, args_str));
	cmd
	}

[rhysd]

@fredizzimo I have another idea to split off the logic to lex Neovim command and args into a new function. Thanks to early returns with ?, it can avoid nested if blocks and the repeat of neovim_ok calls. Let me know if you prefer this.

diff --git a/src/bridge/command.rs b/src/bridge/command.rs
index 0ec0cff..38996a8 100644
--- a/src/bridge/command.rs
+++ b/src/bridge/command.rs
@@ -32,21 +32,8 @@ fn set_windows_creation_flags(cmd: &mut TokioCommand) {
 
 fn build_nvim_cmd() -> TokioCommand {
     if let Some(cmdline) = SETTINGS.get::<CmdLineSettings>().neovim_bin {
-        if let Some((bin, args)) = shlex::split(&cmdline)
-            .filter(|tokens| !tokens.is_empty())
-            .map(|mut tokens| (tokens.remove(0), tokens))
-        {
-            // if neovim_bin contains a path separator, then try to launch it directly
-            // otherwise use which to find the fully path
-            if bin.contains('/') || bin.contains('\\') {
-                if neovim_ok(&bin, &args) {
-                    return build_nvim_cmd_with_args(bin, args);
-                }
-            } else if let Some(bin) = platform_which(&bin) {
-                if neovim_ok(&bin, &args) {
-                    return build_nvim_cmd_with_args(bin, args);
-                }
-            }
+        if let Some((bin, args)) = lex_nvim_cmdline(&cmdline) {
+            return build_nvim_cmd_with_args(bin, args);
         }
 
         eprintln!("ERROR: NEOVIM_BIN='{}' was not found.", cmdline);
@@ -127,6 +114,19 @@ fn neovim_ok(bin: &str, args: &[String]) -> bool {
     false
 }
 
+fn lex_nvim_cmdline(cmdline: &str) -> Option<(String, Vec<String>)> {
+    let mut tokens = shlex::split(cmdline).filter(|t| !t.is_empty())?;
+    let (mut bin, args) = (tokens.remove(0), tokens);
+
+    // if neovim_bin contains a path separator, then try to launch it directly
+    // otherwise use which to find the fully path
+    if !bin.contains('/') && !bin.contains('\\') {
+        bin = platform_which(&bin)?;
+    }
+
+    neovim_ok(&bin, &args).then_some((bin, args))
+}
+
 fn platform_which(bin: &str) -> Option<String> {
     let is_wsl = SETTINGS.get::<CmdLineSettings>().wsl;
 

[fredizzimo]

I'm happy with the code now, but I have not tested it yet, so I'm not merging until that's done.

[fredizzimo]

@rhysd

@fredizzimo BTW I got interested in why it wraps the command execution with a shell on macOS though it doesn't do it on Linux. Does executing the command directly on macOS cause any issue?

neovide/src/bridge/command.rs

Lines 149 to 164 in a8cec7e

	#[cfg(target_os = "macos")]
	fn nvim_cmd_impl(bin: &str, args: &[String]) -> TokioCommand {
	let shell = env::var("SHELL").unwrap_or_else(|_| "/bin/sh".to_string());
	let mut cmd = TokioCommand::new(shell);
	let args_str = args
	.iter()
	.map(|arg| shlex::quote(arg))
	.collect::<Vec<_>>()
	.join(" ");
	if env::var_os("TERM").is_none() {
	cmd.arg("-l");
	}
	cmd.arg("-c");
	cmd.arg(&format!("{} {}", bin, args_str));
	cmd
	}

macOS does some strange things when not launched from a shell. and that were multiple attempts to fix it, but I don't think it's enough, because people still report problems like #1859. We need a better solution for this, but unfortunately, I don't own a mac, so I have no idea how it's supposed to work, and what's the correct way to fix it is.

Here are some of those attempts:

• Fix opening from Finder in macOS #1832
• Preserve PATH when launching from terminal on MacOS #1758
• Solved the pathing issue with a bit less hacking [OSX] #309

[rhysd]

@fredizzimo Thank you for the answer. I understood the reason.

When starting an application by clicking Foo.app in Finder or clicking an application icon in LaunchPad, it is intentional that no environment variable is set. This is the same as MacVim.app (by opening MacVim from LaunchPad and checking :echo $PATH).

Users need to set $PATH in their config files (e.g. .gvimrc). However this workaround would be reasonable as well for usability.

[rhysd]

@fredizzimo I applied the patch at c4b8eea and confirmed the behavior manually.

• OS
  • macOS 13.6
  • Ubuntu 22.04

For each above OS, I confirmed the following cases:

• NEOVIM_BIN
  • Not set → Start with no error
  • Command "nvim" → Start with no error
  • Full path "/usr/local/bin/nvim" → Start with no error
  • Command with argument "env nvim" → Start with no error
  • Full path with argument "/usr/bin/env nvim" → Start with no error
  • Empty "" → ERROR: NEOVIM_BIN='' was not found.
  • Invalid command "echo" → ERROR: Unexpected output from neovim binary:
  • Invalid quote "'foo" → ERROR: NEOVIM_BIN='"foo' was not found.
  • Empty quoted command "''" → ERROR: NEOVIM_BIN='''' was not found.

[fredizzimo]

Thank you. I did some additional tests with WSL on Windows and everything seems to work fine.

[rhysd]

@fredizzimo Thank you. It's very helpful since I don't have those environments.