diff --git a/crates/nu-command/src/core_commands/register.rs b/crates/nu-command/src/core_commands/register.rs index e0750735c1..d175480b56 100644 --- a/crates/nu-command/src/core_commands/register.rs +++ b/crates/nu-command/src/core_commands/register.rs @@ -21,12 +21,6 @@ impl Command for Register { SyntaxShape::Filepath, "path of executable for plugin", ) - .required_named( - "encoding", - SyntaxShape::String, - "Encoding used to communicate with plugin. Options: [json, msgpack]", - Some('e'), - ) .optional( "signature", SyntaxShape::Any, @@ -64,12 +58,12 @@ impl Command for Register { vec![ Example { description: "Register `nu_plugin_query` plugin from ~/.cargo/bin/ dir", - example: r#"register -e json ~/.cargo/bin/nu_plugin_query"#, + example: r#"register ~/.cargo/bin/nu_plugin_query"#, result: None, }, Example { description: "Register `nu_plugin_query` plugin from `nu -c`(plugin will be available in that nu session only)", - example: r#"let plugin = ((which nu).path.0 | path dirname | path join 'nu_plugin_query'); nu -c $'register -e json ($plugin); version'"#, + example: r#"let plugin = ((which nu).path.0 | path dirname | path join 'nu_plugin_query'); nu -c $'register ($plugin); version'"#, result: None, }, ] diff --git a/crates/nu-parser/src/parse_keywords.rs b/crates/nu-parser/src/parse_keywords.rs index 4768dacac1..8ec660988e 100644 --- a/crates/nu-parser/src/parse_keywords.rs +++ b/crates/nu-parser/src/parse_keywords.rs @@ -3027,7 +3027,7 @@ pub fn parse_register( spans: &[Span], expand_aliases_denylist: &[usize], ) -> (Pipeline, Option) { - use nu_plugin::{get_signature, EncodingType, PluginDeclaration}; + use nu_plugin::{get_signature, PluginDeclaration}; use nu_protocol::{engine::Stack, Signature}; let cwd = working_set.get_cwd(); @@ -3120,22 +3120,7 @@ pub fn parse_register( } } }) - .expect("required positional has being checked") - .and_then(|path| { - call.get_flag_expr("encoding") - .map(|expr| { - EncodingType::try_from_bytes(working_set.get_span_contents(expr.span)) - .ok_or_else(|| { - ParseError::IncorrectValue( - "wrong encoding".into(), - expr.span, - "Encodings available: json, and msgpack".into(), - ) - }) - }) - .expect("required named has being checked") - .map(|encoding| (path, encoding)) - }); + .expect("required positional has being checked"); // Signature is an optional value from the call and will be used to decide if // the plugin is called to get the signatures or to use the given signature @@ -3196,7 +3181,7 @@ pub fn parse_register( let current_envs = nu_engine::env::env_to_strings(working_set.permanent_state, &stack).unwrap_or_default(); let error = match signature { - Some(signature) => arguments.and_then(|(path, encoding)| { + Some(signature) => arguments.and_then(|path| { // restrict plugin file name starts with `nu_plugin_` let f_name = path .file_name() @@ -3204,7 +3189,7 @@ pub fn parse_register( if let Some(true) = f_name { signature.map(|signature| { - let plugin_decl = PluginDeclaration::new(path, signature, encoding, shell); + let plugin_decl = PluginDeclaration::new(path, signature, shell); working_set.add_decl(Box::new(plugin_decl)); working_set.mark_plugins_file_dirty(); }) @@ -3212,14 +3197,14 @@ pub fn parse_register( Ok(()) } }), - None => arguments.and_then(|(path, encoding)| { + None => arguments.and_then(|path| { // restrict plugin file name starts with `nu_plugin_` let f_name = path .file_name() .map(|s| s.to_string_lossy().starts_with("nu_plugin_")); if let Some(true) = f_name { - get_signature(path.as_path(), &encoding, &shell, ¤t_envs) + get_signature(path.as_path(), &shell, ¤t_envs) .map_err(|err| { ParseError::LabeledError( "Error getting signatures".into(), @@ -3231,12 +3216,8 @@ pub fn parse_register( for signature in signatures { // create plugin command declaration (need struct impl Command) // store declaration in working set - let plugin_decl = PluginDeclaration::new( - path.clone(), - signature, - encoding.clone(), - shell.clone(), - ); + let plugin_decl = + PluginDeclaration::new(path.clone(), signature, shell.clone()); working_set.add_decl(Box::new(plugin_decl)); } diff --git a/crates/nu-plugin/src/plugin/declaration.rs b/crates/nu-plugin/src/plugin/declaration.rs index c47b9003f2..9657f4959d 100644 --- a/crates/nu-plugin/src/plugin/declaration.rs +++ b/crates/nu-plugin/src/plugin/declaration.rs @@ -1,6 +1,6 @@ -use crate::{EncodingType, EvaluatedCall}; +use crate::EvaluatedCall; -use super::{call_plugin, create_command}; +use super::{call_plugin, create_command, get_plugin_encoding}; use crate::protocol::{ CallInfo, CallInput, PluginCall, PluginCustomValue, PluginData, PluginResponse, }; @@ -16,21 +16,14 @@ pub struct PluginDeclaration { signature: Signature, filename: PathBuf, shell: Option, - encoding: EncodingType, } impl PluginDeclaration { - pub fn new( - filename: PathBuf, - signature: Signature, - encoding: EncodingType, - shell: Option, - ) -> Self { + pub fn new(filename: PathBuf, signature: Signature, shell: Option) -> Self { Self { name: signature.name.clone(), signature, filename, - encoding, shell, } } @@ -111,17 +104,27 @@ impl Command for PluginDeclaration { input, }); - let response = - call_plugin(&mut child, plugin_call, &self.encoding, call.head).map_err(|err| { - let decl = engine_state.get_decl(call.decl_id); - ShellError::GenericError( - format!("Unable to decode call for {}", decl.name()), - err.to_string(), - Some(call.head), - None, - Vec::new(), - ) - }); + let encoding = { + let stdout_reader = match &mut child.stdout { + Some(out) => out, + None => { + return Err(ShellError::PluginFailedToLoad( + "Plugin missing stdout reader".into(), + )) + } + }; + get_plugin_encoding(stdout_reader)? + }; + let response = call_plugin(&mut child, plugin_call, &encoding, call.head).map_err(|err| { + let decl = engine_state.get_decl(call.decl_id); + ShellError::GenericError( + format!("Unable to decode call for {}", decl.name()), + err.to_string(), + Some(call.head), + None, + Vec::new(), + ) + }); let pipeline_data = match response { Ok(PluginResponse::Value(value)) => { @@ -134,7 +137,6 @@ impl Command for PluginDeclaration { data: plugin_data.data, filename: self.filename.clone(), shell: self.shell.clone(), - encoding: self.encoding.clone(), source: engine_state.get_decl(call.decl_id).name().to_owned(), }), span: plugin_data.span, @@ -158,7 +160,7 @@ impl Command for PluginDeclaration { pipeline_data } - fn is_plugin(&self) -> Option<(&PathBuf, &str, &Option)> { - Some((&self.filename, self.encoding.to_str(), &self.shell)) + fn is_plugin(&self) -> Option<(&PathBuf, &Option)> { + Some((&self.filename, &self.shell)) } } diff --git a/crates/nu-plugin/src/plugin/mod.rs b/crates/nu-plugin/src/plugin/mod.rs index 194617c291..aa82633d62 100644 --- a/crates/nu-plugin/src/plugin/mod.rs +++ b/crates/nu-plugin/src/plugin/mod.rs @@ -7,9 +7,9 @@ use crate::protocol::{CallInput, LabeledError, PluginCall, PluginData, PluginRes use crate::EncodingType; use std::env; use std::fmt::Write; -use std::io::BufReader; +use std::io::{BufReader, Read, Write as WriteTrait}; use std::path::{Path, PathBuf}; -use std::process::{Child, Command as CommandSys, Stdio}; +use std::process::{Child, ChildStdout, Command as CommandSys, Stdio}; use nu_protocol::{CustomValue, ShellError, Span}; use nu_protocol::{Signature, Value}; @@ -116,7 +116,6 @@ pub(crate) fn call_plugin( pub fn get_signature( path: &Path, - encoding: &EncodingType, shell: &Option, current_envs: &HashMap, ) -> Result, ShellError> { @@ -127,32 +126,34 @@ pub fn get_signature( ShellError::PluginFailedToLoad(format!("Error spawning child process: {}", err)) })?; + let mut stdin_writer = child + .stdin + .take() + .ok_or_else(|| ShellError::PluginFailedToLoad("plugin missing stdin writer".into()))?; + let mut stdout_reader = child + .stdout + .take() + .ok_or_else(|| ShellError::PluginFailedToLoad("Plugin missing stdout reader".into()))?; + let encoding = get_plugin_encoding(&mut stdout_reader)?; + // Create message to plugin to indicate that signature is required and // send call to plugin asking for signature - if let Some(mut stdin_writer) = child.stdin.take() { - let encoding_clone = encoding.clone(); - std::thread::spawn(move || { - encoding_clone.encode_call(&PluginCall::Signature, &mut stdin_writer) - }); - } + let encoding_clone = encoding.clone(); + std::thread::spawn(move || { + encoding_clone.encode_call(&PluginCall::Signature, &mut stdin_writer) + }); // deserialize response from plugin to extract the signature - let signatures = if let Some(stdout_reader) = &mut child.stdout { - let reader = stdout_reader; - let mut buf_read = BufReader::with_capacity(OUTPUT_BUFFER_SIZE, reader); - let response = encoding.decode_response(&mut buf_read)?; + let reader = stdout_reader; + let mut buf_read = BufReader::with_capacity(OUTPUT_BUFFER_SIZE, reader); + let response = encoding.decode_response(&mut buf_read)?; - match response { - PluginResponse::Signature(sign) => Ok(sign), - PluginResponse::Error(err) => Err(err.into()), - _ => Err(ShellError::PluginFailedToLoad( - "Plugin missing signature".into(), - )), - } - } else { - Err(ShellError::PluginFailedToLoad( - "Plugin missing stdout reader".into(), - )) + let signatures = match response { + PluginResponse::Signature(sign) => Ok(sign), + PluginResponse::Error(err) => Err(err.into()), + _ => Err(ShellError::PluginFailedToLoad( + "Plugin missing signature".into(), + )), }?; match child.wait() { @@ -196,6 +197,24 @@ pub fn serve_plugin(plugin: &mut impl Plugin, encoder: impl PluginEncoder) { std::process::exit(0) } + // tell nushell encoding. + // + // 1 byte + // encoding format: | content-length | content | + { + let mut stdout = std::io::stdout(); + let encoding = encoder.name(); + let length = encoding.len() as u8; + let mut encoding_content: Vec = encoding.as_bytes().to_vec(); + encoding_content.insert(0, length); + stdout + .write_all(&encoding_content) + .expect("Failed to tell nushell my encoding"); + stdout + .flush() + .expect("Failed to tell nushell my encoding when flushing stdout"); + } + let mut stdin_buf = BufReader::with_capacity(OUTPUT_BUFFER_SIZE, std::io::stdin()); let plugin_call = encoder.decode_call(&mut stdin_buf); @@ -332,3 +351,22 @@ fn print_help(plugin: &mut impl Plugin, encoder: impl PluginEncoder) { println!("{}", help) } + +pub fn get_plugin_encoding(child_stdout: &mut ChildStdout) -> Result { + let mut length_buf = [0u8; 1]; + child_stdout.read_exact(&mut length_buf).map_err(|e| { + ShellError::PluginFailedToLoad(format!("unable to get encoding from plugin: {e}")) + })?; + + let mut buf = vec![0u8; length_buf[0] as usize]; + child_stdout.read_exact(&mut buf).map_err(|e| { + ShellError::PluginFailedToLoad(format!("unable to get encoding from plugin: {e}")) + })?; + + EncodingType::try_from_bytes(&buf).ok_or_else(|| { + let encoding_for_debug = String::from_utf8_lossy(&buf); + ShellError::PluginFailedToLoad(format!( + "get unsupported plugin encoding: {encoding_for_debug}" + )) + }) +} diff --git a/crates/nu-plugin/src/protocol/plugin_custom_value.rs b/crates/nu-plugin/src/protocol/plugin_custom_value.rs index a85be449e7..dbde579909 100644 --- a/crates/nu-plugin/src/protocol/plugin_custom_value.rs +++ b/crates/nu-plugin/src/protocol/plugin_custom_value.rs @@ -3,10 +3,7 @@ use std::path::PathBuf; use nu_protocol::{CustomValue, ShellError, Value}; use serde::Serialize; -use crate::{ - plugin::{call_plugin, create_command}, - EncodingType, -}; +use crate::plugin::{call_plugin, create_command, get_plugin_encoding}; use super::{PluginCall, PluginData, PluginResponse}; @@ -32,8 +29,6 @@ pub struct PluginCustomValue { #[serde(skip)] pub shell: Option, #[serde(skip)] - pub encoding: EncodingType, - #[serde(skip)] pub source: String, } @@ -72,8 +67,19 @@ impl CustomValue for PluginCustomValue { data: self.data.clone(), span, }); + let encoding = { + let stdout_reader = match &mut child.stdout { + Some(out) => out, + None => { + return Err(ShellError::PluginFailedToLoad( + "Plugin missing stdout reader".into(), + )) + } + }; + get_plugin_encoding(stdout_reader)? + }; - let response = call_plugin(&mut child, plugin_call, &self.encoding, span).map_err(|err| { + let response = call_plugin(&mut child, plugin_call, &encoding, span).map_err(|err| { ShellError::GenericError( format!( "Unable to decode call for {} to get base value", diff --git a/crates/nu-plugin/src/serializers/json.rs b/crates/nu-plugin/src/serializers/json.rs index be90100f15..6d77e11c19 100644 --- a/crates/nu-plugin/src/serializers/json.rs +++ b/crates/nu-plugin/src/serializers/json.rs @@ -7,7 +7,7 @@ pub struct JsonSerializer; impl PluginEncoder for JsonSerializer { fn name(&self) -> &str { - "Json Serializer" + "json" } fn encode_call( diff --git a/crates/nu-plugin/src/serializers/msgpack.rs b/crates/nu-plugin/src/serializers/msgpack.rs index 733561877a..20e6df5920 100644 --- a/crates/nu-plugin/src/serializers/msgpack.rs +++ b/crates/nu-plugin/src/serializers/msgpack.rs @@ -6,7 +6,7 @@ pub struct MsgPackSerializer; impl PluginEncoder for MsgPackSerializer { fn name(&self) -> &str { - "MsgPack Serializer" + "msgpack" } fn encode_call( @@ -23,7 +23,7 @@ impl PluginEncoder for MsgPackSerializer { reader: &mut impl std::io::BufRead, ) -> Result { rmp_serde::from_read(reader) - .map_err(|err| ShellError::PluginFailedToEncode(err.to_string())) + .map_err(|err| ShellError::PluginFailedToDecode(err.to_string())) } fn encode_response( @@ -40,7 +40,7 @@ impl PluginEncoder for MsgPackSerializer { reader: &mut impl std::io::BufRead, ) -> Result { rmp_serde::from_read(reader) - .map_err(|err| ShellError::PluginFailedToEncode(err.to_string())) + .map_err(|err| ShellError::PluginFailedToDecode(err.to_string())) } } diff --git a/crates/nu-protocol/src/engine/command.rs b/crates/nu-protocol/src/engine/command.rs index 7d9d0242e6..0fa1e22799 100644 --- a/crates/nu-protocol/src/engine/command.rs +++ b/crates/nu-protocol/src/engine/command.rs @@ -57,9 +57,8 @@ pub trait Command: Send + Sync + CommandClone { false } - // Is a plugin command (returns plugin's path, encoding and type of shell - // if the declaration is a plugin) - fn is_plugin(&self) -> Option<(&PathBuf, &str, &Option)> { + // Is a plugin command (returns plugin's path, type of shell if the declaration is a plugin) + fn is_plugin(&self) -> Option<(&PathBuf, &Option)> { None } diff --git a/crates/nu-protocol/src/engine/engine_state.rs b/crates/nu-protocol/src/engine/engine_state.rs index 240d06ef4a..ff5e73ceeb 100644 --- a/crates/nu-protocol/src/engine/engine_state.rs +++ b/crates/nu-protocol/src/engine/engine_state.rs @@ -366,8 +366,7 @@ impl EngineState { self.plugin_decls().try_for_each(|decl| { // A successful plugin registration already includes the plugin filename // No need to check the None option - let (path, encoding, shell) = - decl.is_plugin().expect("plugin should have file name"); + let (path, shell) = decl.is_plugin().expect("plugin should have file name"); let mut file_name = path .to_str() .expect("path was checked during registration as a str") @@ -394,14 +393,10 @@ impl EngineState { None => "".into(), }; - // Each signature is stored in the plugin file with the required - // encoding, shell and signature + // Each signature is stored in the plugin file with the shell and signature // This information will be used when loading the plugin // information when nushell starts - format!( - "register {} -e {} {} {}\n\n", - file_name, encoding, shell_str, signature - ) + format!("register {} {} {}\n\n", file_name, shell_str, signature) }) .map_err(|err| ShellError::PluginFailedToLoad(err.to_string())) .and_then(|line| { diff --git a/crates/nu-test-support/src/macros.rs b/crates/nu-test-support/src/macros.rs index 4851baf6a7..593202c998 100644 --- a/crates/nu-test-support/src/macros.rs +++ b/crates/nu-test-support/src/macros.rs @@ -225,11 +225,11 @@ macro_rules! with_exe { #[macro_export] macro_rules! nu_with_plugins { - (cwd: $cwd:expr, plugins: [$(($format:expr, $plugin_name:expr)),+$(,)?], $command:expr) => {{ - nu_with_plugins!($cwd, [$(($format, $plugin_name)),+], $command) + (cwd: $cwd:expr, plugins: [$(($plugin_name:expr)),+$(,)?], $command:expr) => {{ + nu_with_plugins!($cwd, [$(("", $plugin_name)),+], $command) }}; - (cwd: $cwd:expr, plugin: ($format:expr, $plugin_name:expr), $command:expr) => {{ - nu_with_plugins!($cwd, [($format, $plugin_name)], $command) + (cwd: $cwd:expr, plugin: ($plugin_name:expr), $command:expr) => {{ + nu_with_plugins!($cwd, [("", $plugin_name)], $command) }}; ($cwd:expr, [$(($format:expr, $plugin_name:expr)),+$(,)?], $command:expr) => {{ @@ -254,8 +254,10 @@ macro_rules! nu_with_plugins { $($crate::commands::ensure_binary_present($plugin_name);)+ + // TODO: the `$format` is a dummy empty string, but `plugin_name` is repeatable + // just keep it here for now. Need to find a way to remove it. let registrations = format!( - concat!($(concat!("register -e ", $format, " {};")),+), + concat!($(concat!("register ", $format, " {};")),+), $( nu_path::canonicalize_with(with_exe!($plugin_name), &test_bins) .unwrap_or_else(|e| { diff --git a/tests/plugins/core_inc.rs b/tests/plugins/core_inc.rs index 5811f24865..61f105fd57 100644 --- a/tests/plugins/core_inc.rs +++ b/tests/plugins/core_inc.rs @@ -6,7 +6,7 @@ use nu_test_support::playground::Playground; fn chooses_highest_increment_if_given_more_than_one() { let actual = nu_with_plugins!( cwd: "tests/fixtures/formats", - plugin: ("json", "nu_plugin_inc"), + plugin: ("nu_plugin_inc"), "open cargo_sample.toml | first 1 | inc package.version --major --minor | get package.version" ); @@ -14,7 +14,7 @@ fn chooses_highest_increment_if_given_more_than_one() { let actual = nu_with_plugins!( cwd: "tests/fixtures/formats", - plugin: ("json", "nu_plugin_inc"), + plugin: ("nu_plugin_inc"), // Regardless of order of arguments "open cargo_sample.toml | first 1 | inc package.version --minor --major | get package.version" ); @@ -35,7 +35,7 @@ fn by_one_with_field_passed() { let actual = nu_with_plugins!( cwd: dirs.test(), - plugin: ("json", "nu_plugin_inc"), + plugin: ("nu_plugin_inc"), "open sample.toml | inc package.edition | get package.edition" ); @@ -56,7 +56,7 @@ fn by_one_with_no_field_passed() { let actual = nu_with_plugins!( cwd: dirs.test(), - plugin: ("json", "nu_plugin_inc"), + plugin: ("nu_plugin_inc"), "open sample.toml | get package.contributors | inc" ); @@ -77,7 +77,7 @@ fn semversion_major_inc() { let actual = nu_with_plugins!( cwd: dirs.test(), - plugin: ("json", "nu_plugin_inc"), + plugin: ("nu_plugin_inc"), "open sample.toml | inc package.version -M | get package.version" ); @@ -98,7 +98,7 @@ fn semversion_minor_inc() { let actual = nu_with_plugins!( cwd: dirs.test(), - plugin: ("json", "nu_plugin_inc"), + plugin: ("nu_plugin_inc"), "open sample.toml | inc package.version --minor | get package.version" ); @@ -119,7 +119,7 @@ fn semversion_patch_inc() { let actual = nu_with_plugins!( cwd: dirs.test(), - plugin: ("json", "nu_plugin_inc"), + plugin: ("nu_plugin_inc"), "open sample.toml | inc package.version --patch | get package.version" ); @@ -140,7 +140,7 @@ fn semversion_without_passing_field() { let actual = nu_with_plugins!( cwd: dirs.test(), - plugin: ("json", "nu_plugin_inc"), + plugin: ("nu_plugin_inc"), "open sample.toml | get package.version | inc --patch" ); diff --git a/tests/plugins/custom_values.rs b/tests/plugins/custom_values.rs index 946df2e6b8..6493d4f7e9 100644 --- a/tests/plugins/custom_values.rs +++ b/tests/plugins/custom_values.rs @@ -4,7 +4,7 @@ use nu_test_support::nu_with_plugins; fn can_get_custom_value_from_plugin_and_instantly_collapse_it() { let actual = nu_with_plugins!( cwd: "tests", - plugin: ("msgpack", "nu_plugin_custom_values"), + plugin: ("nu_plugin_custom_values"), "custom-value generate" ); @@ -15,7 +15,7 @@ fn can_get_custom_value_from_plugin_and_instantly_collapse_it() { fn can_get_custom_value_from_plugin_and_pass_it_over() { let actual = nu_with_plugins!( cwd: "tests", - plugin: ("msgpack", "nu_plugin_custom_values"), + plugin: ("nu_plugin_custom_values"), "custom-value generate | custom-value update" ); @@ -29,7 +29,7 @@ fn can_get_custom_value_from_plugin_and_pass_it_over() { fn can_generate_and_updated_multiple_types_of_custom_values() { let actual = nu_with_plugins!( cwd: "tests", - plugin: ("msgpack", "nu_plugin_custom_values"), + plugin: ("nu_plugin_custom_values"), "custom-value generate2 | custom-value update" ); @@ -43,7 +43,7 @@ fn can_generate_and_updated_multiple_types_of_custom_values() { fn can_get_describe_plugin_custom_values() { let actual = nu_with_plugins!( cwd: "tests", - plugin: ("msgpack", "nu_plugin_custom_values"), + plugin: ("nu_plugin_custom_values"), "custom-value generate | describe" ); @@ -58,7 +58,7 @@ fn can_get_describe_plugin_custom_values() { fn fails_if_passing_engine_custom_values_to_plugins() { let actual = nu_with_plugins!( cwd: "tests/fixtures/formats", - plugin: ("msgpack", "nu_plugin_custom_values"), + plugin: ("nu_plugin_custom_values"), "open-db sample.db | custom-value update" ); @@ -72,8 +72,8 @@ fn fails_if_passing_custom_values_across_plugins() { let actual = nu_with_plugins!( cwd: "tests", plugins: [ - ("msgpack", "nu_plugin_custom_values"), - ("json", "nu_plugin_inc") + ("nu_plugin_custom_values"), + ("nu_plugin_inc") ], "custom-value generate | inc --major" );