apply reviewer's suggestions

Cargo.toml

@@ -64,7 +64,6 @@ tempfile = "3.3.0"
 thiserror = "1.0.24"
 time = "0.3.9"
 tokio = "1.24.2"
-toml = "0.7.3"
 url = "2.1.0"
 walkdir = "2.3.2"
 

src/dfx/Cargo.toml

@@ -98,15 +98,13 @@ sysinfo = "0.28.4"
 tar.workspace = true
 tempfile.workspace = true
 term = "0.7.0"
-textwrap = { version = "0.16", features = ["terminal_size"] }
 thiserror.workspace = true
 time = { workspace = true, features = [
  "serde",
  "macros",
  "serde-human-readable",
 ] }
 tokio = { workspace = true, features = ["fs"] }
-toml.workspace = true
 url.workspace = true
 walkdir.workspace = true
 wasmparser = "0.87.0"

src/dfx/src/lib/error/extension.rs

@@ -18,6 +18,9 @@ pub enum ExtensionError {
  #[error("Extension '{0}' is already installed.")]
  ExtensionAlreadyInstalled(String),
 
+ #[error("Extension '{0}' cannot be installed because it conflicts with an existing command.")]
+ CommandAlreadyExists(String),
+
  #[error("Cannot fetch compatibility.json from '{0}': {1}")]
  CompatibilityMatrixFetchError(String, reqwest::Error),
 
@@ -65,8 +68,8 @@ pub enum ExtensionError {
  // TODO: this error should be `#[from dfx_core::...::StructureFileError]`, however,
  // adding new error type to enum StructureFileError causes a cascade of problems,
  // and they should be addressed separately: https://dfinity.atlassian.net/browse/SDK-1096
- #[error("Malformed extension manifest: Failed to parse contents of {0} as toml: {1}")]
- ExtensionManifestIsNotValid(Box<std::path::PathBuf>, toml::de::Error),
+ #[error("Malformed extension manifest: Failed to parse contents of {0} as toml: ")]
+ ExtensionManifestIsNotValid(dfx_core::error::structured_file::StructuredFileError),
 
  #[error("Missing 'extension.toml' file for extension '{0}'.")]
  ExtensionManifestMissing(String),

src/dfx/src/lib/extension/manager/install.rs

@@ -1,6 +1,8 @@
+use crate::commands::Command;
 use crate::lib::error::extension::ExtensionError;
 use crate::lib::extension::{manager::ExtensionManager, manifest::ExtensionCompatibilityMatrix};
 
+use clap::Subcommand;
 use flate2::read::GzDecoder;
 use reqwest::Url;
 use semver::{BuildMetadata, Prerelease, Version};
@@ -21,6 +23,11 @@ impl ExtensionManager {
  extension_name.to_string(),
  ));
  }
+ if Command::has_subcommand(extension_name) {
+ return Err(ExtensionError::CommandAlreadyExists(
+ extension_name.to_string(),
+ ));
+ }
 
  let extension_version = self.get_extension_compatible_version(extension_name)?;
  let github_release_tag = get_git_release_tag(extension_name, &extension_version);

src/dfx/src/lib/extension/manager/list.rs

@@ -1,11 +1,5 @@
 use super::ExtensionManager;
-use crate::lib::{
- error::ExtensionError,
- extension::{manifest::ExtensionManifest, Extension},
-};
-
-use console::style;
-use textwrap::{termwidth, wrap, Options};
+use crate::lib::{error::ExtensionError, extension::Extension};
 
 impl ExtensionManager {
  pub fn list_installed_extensions(&self) -> Result<Vec<Extension>, ExtensionError> {
@@ -25,82 +19,4 @@ impl ExtensionManager {
  })
  .collect())
  }
-
- pub fn dfx_help_print_installed_extensions(&self) -> String {
- // explenation for const:
- // SUBCOMMANDS:
- // quickstart Use the command to
- // ^^#^ ^^$^ perform action
- // ^^%^
- // <-------- CLAP_HELP_WIDTH --------->
- // <-------- TRUE TERMINAL WINDOW WIDTH ------->
- const INDENT_BEFORE_COMMAND: usize = 4; // ^^#^
- const LONGEST_DFX_COMMAND: usize = 10; // "quickstart".len()
- const PADDING_BETWEEN_COMMAND_AND_SUMMARY: usize = 4; // ^^$^
- const INDENT_COMMAND_SUMMARY: usize = 4; // ^^%^
- const CLAP_HELP_WIDTH: usize = 100; // clap sets a default width for printing
- fn wrap_text(text: &str, len_longest_ext: usize, len_current_ext: usize) -> String {
- let termwidth = std::cmp::min(termwidth(), CLAP_HELP_WIDTH);
- let ext_name_offset = std::cmp::max(len_longest_ext, LONGEST_DFX_COMMAND);
- let padding = ext_name_offset - len_current_ext + PADDING_BETWEEN_COMMAND_AND_SUMMARY;
- let command_name_with_margins = INDENT_BEFORE_COMMAND + len_current_ext + padding;
-
- // the width for the column of text for the extension summary
- let wraptext_width = termwidth.saturating_sub(command_name_with_margins);
- // when summary for the command doesn't fit one line, each next line should be indented by X spaces
- let subsequent_indent = " ".repeat(INDENT_COMMAND_SUMMARY);
- let options = Options::new(wraptext_width).subsequent_indent(&subsequent_indent);
-
- // subsequent_indent only pushes each new line by X spaces from them left, however,
- // this is insufficient because the new line should start directly under the previous one.
- // This would not be possible to achive only with Options.subsequent_indent,
- // because it modifies the width of the column of the text which is undesirable
- // (think of it as if increasing the value of right margin).
- let newline_indent = " ".repeat(command_name_with_margins);
- let wrapped_text = wrap(text, &options).join(&format!("\n{newline_indent}"));
- // The whitespace between extension name and and extension summary.
- // This would not be possible to achive with Options.initial_indent,
- // because it modifies the width of the column of the text which is undesirable
- // (think of it as if increasing the value of right margin).
- let initial_indent = " ".repeat(padding);
- format!("{initial_indent}{wrapped_text}",)
- }
-
- let mut extensions = self.list_installed_extensions().unwrap_or_default();
- if extensions.is_empty() {
- return "No extensions installed.".to_string();
- }
-
- let mut output = style(String::from("EXTENSIONS:\n")).yellow().to_string();
-
- // name of extension should not be longer than 30 chars
- // ok to unwrap, we already checked that extensions is not empty
- let len_longest_ext_name =
- std::cmp::min(30, extensions.iter().map(|v| v.name.len()).max().unwrap());
-
- extensions.sort_by(|a, b| a.name.cmp(&b.name));
- for extension in extensions {
- let extension_name = if extension.name.len() > 30 {
- // Name of ext will get cropped if its longer than 30 chars.
- // In such case, if the user wants to see full name of extension
- // they should issue `dfx extension list`
- format!("{}...", &extension.name[..27])
- } else {
- extension.name.to_string()
- };
- let desc = {
- let text = match ExtensionManifest::from_extension_directory(
- self.get_extension_directory(&extension.name),
- ) {
- Ok(extension_manifest) => extension_manifest.summary,
- Err(err) => format!("Error while loading extension manifest: {err}"),
- };
- wrap_text(&text, len_longest_ext_name, extension_name.len())
- };
- let name = style(&extension_name).green();
- let initial_indent = " ".repeat(INDENT_BEFORE_COMMAND);
- output.push_str(&format!("{initial_indent}{name}{desc}\n",));
- }
- output
- }
 }

src/dfx/src/lib/extension/manager/mod.rs

@@ -19,7 +19,10 @@ impl ExtensionManager {
  pub fn new(version: &Version) -> Result<Self, ExtensionError> {
  let versioned_cache_dir = get_bin_cache(version.to_string().as_str()).map_err(|e| {
  ExtensionError::FindCacheDirectoryFailed(
- get_cache_root().unwrap_or_default().join("versions"),
+ get_cache_root()
+ .unwrap_or_default()
+ .join("versions")
+ .join(version.to_string()),
  e,
  )
  })?;

src/dfx/src/lib/extension/manifest/extension.rs

@@ -1,17 +1,12 @@
 use serde::{Deserialize, Serialize};
 use serde_json::Value as JsonValue;
-use std::{collections::HashMap, fmt::Display, path::PathBuf};
+use std::{collections::HashMap, fmt::Display, path::Path};
 
 use crate::lib::error::ExtensionError;
 
-pub static MANIFEST_FILE_NAME: &str = "extension.toml";
+pub static MANIFEST_FILE_NAME: &str = "extension.json";
 
-#[derive(Debug, Deserialize)]
-struct ExtensionManifestWrapper {
- extension: ExtensionManifest,
-}
-
-#[derive(Debug, Serialize, Deserialize)]
+#[derive(Debug, Serialize, Deserialize, Default)]
 pub struct ExtensionManifest {
  pub name: String,
  pub version: String,
@@ -25,32 +20,29 @@ pub struct ExtensionManifest {
  pub dependencies: Option<HashMap<String, String>>,
 }
 
+#[derive(Debug, Serialize, Deserialize)]
+pub struct ExtensionSubcommand {
+ pub subcommands: Vec<ExtensionSubcommand>,
+ pub key: String,
+ pub summary: String,
+}
+
 impl Display for ExtensionManifest {
  fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result {
- let Ok(s) = toml::to_string_pretty(self) else {
+ let Ok(s) = serde_json::to_string_pretty(self) else {
  return Err(std::fmt::Error)
  };
  write!(f, "{}", s)
  }
 }
 
 impl ExtensionManifest {
- pub fn from_extension_directory(path: PathBuf) -> Result<Self, ExtensionError> {
- let manifest_path = path.join(MANIFEST_FILE_NAME);
+ pub fn new(name: &str, extensions_root_dir: &Path) -> Result<Self, ExtensionError> {
+ let manifest_path = extensions_root_dir.join(name).join(MANIFEST_FILE_NAME);
  if !manifest_path.exists() {
- return Err(ExtensionError::ExtensionManifestMissing(
- path.components()
- .last()
- .unwrap() // safe to unwrap - the `path` parameter is guaranteed not to be root (`/`)
- .as_os_str()
- .to_string_lossy()
- .to_string(),
- ));
+ return Err(ExtensionError::ExtensionManifestMissing(name.to_owned()));
  }
- let ext: ExtensionManifestWrapper =
- toml::from_str(&dfx_core::fs::read_to_string(&manifest_path)?).map_err(|e| {
- ExtensionError::ExtensionManifestIsNotValid(Box::new(manifest_path), e)
- })?;
- Ok(ext.extension)
+ dfx_core::json::load_json_file(&manifest_path)
+ .map_err(ExtensionError::ExtensionManifestIsNotValid)
  }
 }

src/dfx/src/lib/extension/mod.rs

@@ -8,6 +8,10 @@ use std::{
  fs::DirEntry,
 };
 
+use clap::Command;
+
+use self::{manager::ExtensionManager, manifest::ExtensionManifest};
+
 #[derive(Debug, Default)]
 pub struct Extension {
  pub name: String,
@@ -25,3 +29,11 @@ impl Display for Extension {
  write!(f, "{}", self.name)
  }
 }
+
+impl Extension {
+ pub fn into_clap_command(self, manager: &ExtensionManager) -> Command {
+ let summary = ExtensionManifest::new(&self.name, &manager.dir)
+ .map_or_else(|e| e.to_string(), |v| v.summary);
+ Command::new(&self.name).about(summary)
+ }
+}