From e05c888d5652832c8c4bb9fbd7b736d21144443d Mon Sep 17 00:00:00 2001 From: Eric Swanson <64809312+ericswanson-dfinity@users.noreply.github.com> Date: Tue, 26 Nov 2024 04:05:25 -0800 Subject: [PATCH 1/4] feat: extension defined project templates (#4012) --- CHANGELOG.md | 5 + .../extension-defined-project-templates.md | 66 +++++++++ docs/concepts/index.md | 2 + docs/extension-manifest-schema.json | 84 ++++++++++++ e2e/tests-dfx/extension.bash | 127 ++++++++++++++++++ e2e/utils/_.bash | 8 +- .../src/config/model/project_template.rs | 7 +- src/dfx-core/src/config/project_templates.rs | 13 +- src/dfx-core/src/extension/installed.rs | 13 ++ .../src/extension/manifest/extension.rs | 82 ++++++++++- src/dfx/src/commands/new.rs | 60 ++++++++- src/dfx/src/main.rs | 6 +- 12 files changed, 459 insertions(+), 14 deletions(-) create mode 100644 docs/concepts/extension-defined-project-templates.md diff --git a/CHANGELOG.md b/CHANGELOG.md index 5396853a28..c2e6e01994 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -20,6 +20,11 @@ Added a flag `--replica` to `dfx start`. This flag currently has no effect. Once PocketIC becomes the default for `dfx start` this flag will start the replica instead. You can use the `--replica` flag already to write scripts that anticipate that change. +### feat: extensions can define project templates + +An extension can define one or more project templates for `dfx new` to use. +These can be new templates or replace the built-in project templates. + # 0.24.3 ### feat: Bitcoin support in PocketIC diff --git a/docs/concepts/extension-defined-project-templates.md b/docs/concepts/extension-defined-project-templates.md new file mode 100644 index 0000000000..b66789221a --- /dev/null +++ b/docs/concepts/extension-defined-project-templates.md @@ -0,0 +1,66 @@ +# Extension-Defined Project Templates + +## Overview + +An extension can define one or more project templates for `dfx new` to use. + +A project template is a set of files that `dfx new` copies or patches into a new project. + +For examples of project template files, see the [project_templates] directory in the SDK repository. + +# Specification + +The `project_templates` field in an extension's `extension.json` defines the project templates +included in the extension. It is an object field mapping `project template name -> project template properties`. +These are the properties of a project template: + +| Field | Type | Description | +|------------------------------|---------------------------|------------------------------------------------------------------------------------------------------| +| `display` | String | Display name of the project template | +| `category` | String | Category for inclusion in `--backend` and `--frontend` CLI options, as well as interactive selection | +| `requirements` | Array of String | Required project templates | +| `post_create` | String or Array of String | Command(s) to run after adding the canister to the project | +| `post_create_spinner_message` | String | Message to display while running the post_create command | +| `post_create_failure_warning` | String | Warning to display if the post_create command fails | + +Within the files distributed with the extension, the project template files are +located in the `project_templates/{project template name}` directory. + +## The `display` field + +The `display` field is a string that describes the project template. +`dfx new` will use this value for interactive selection of project templates. + +## The `category` field + +The `category` field is an array of strings that categorize the project template. +`dfx new` uses this field to determine whether to include this project template +as an option for the `--backend` and `-frontend` flags, as well as in interactive setup. + +Valid values for the field: +- `frontend` +- `backend` +- `extra` +- `frontend-test` +- `support` + +## The `requirements` field + +The `requirements` field lists any project templates that `dfx new` must apply before this project template. +For example, many of the frontend templates depend on the `dfx_js_base` template, which adds +package.json and tsconfig.json to the project. + +## The `post_create` field + +The `post_create` field specifies a command or commands to run after adding the project template files to the project. +For example, the rust project template runs `cargo update` after adding the files. + +## The `post_create_spinner_message` field + +If this field is set, `dfx new` will display a spinner with this message while running the `post_create` command. + +## The `post_create_failure_warning` field + +If this field is present and the `post_create` command fails, `dfx new` will display this warning but won't stop creating the project. + +[project_templates]: https://github.com/dfinity/sdk/tree/master/src/dfx/assets/project_templates diff --git a/docs/concepts/index.md b/docs/concepts/index.md index 4f45495c5b..cf91724f81 100644 --- a/docs/concepts/index.md +++ b/docs/concepts/index.md @@ -2,3 +2,5 @@ - [Asset Canister Interface](../design/asset-canister-interface.md) - [Canister metadata](./canister-metadata.md) +- [Extension-Defined Canister Types](./extension-defined-canister-types.md) +- [Extension-Defined Project Templates](./extension-defined-project-templates.md) diff --git a/docs/extension-manifest-schema.json b/docs/extension-manifest-schema.json index beaff0a894..544823b5cb 100644 --- a/docs/extension-manifest-schema.json +++ b/docs/extension-manifest-schema.json @@ -70,6 +70,15 @@ "name": { "type": "string" }, + "project_templates": { + "type": [ + "object", + "null" + ], + "additionalProperties": { + "$ref": "#/definitions/ExtensionProjectTemplate" + } + }, "subcommands": { "anyOf": [ { @@ -155,6 +164,58 @@ } ] }, + "ExtensionProjectTemplate": { + "type": "object", + "required": [ + "category", + "display", + "post_create", + "requirements" + ], + "properties": { + "category": { + "description": "Used to determine which CLI group (`--type`, `--backend`, `--frontend`) as well as for interactive selection", + "allOf": [ + { + "$ref": "#/definitions/ProjectTemplateCategory" + } + ] + }, + "display": { + "description": "The name used for display and sorting", + "type": "string" + }, + "post_create": { + "description": "Run a command after adding the canister to dfx.json", + "allOf": [ + { + "$ref": "#/definitions/SerdeVec_for_String" + } + ] + }, + "post_create_failure_warning": { + "description": "If the post-create command fails, display this warning but don't fail", + "type": [ + "string", + "null" + ] + }, + "post_create_spinner_message": { + "description": "If set, display a spinner while this command runs", + "type": [ + "string", + "null" + ] + }, + "requirements": { + "description": "Other project templates to patch in alongside this one", + "type": "array", + "items": { + "type": "string" + } + } + } + }, "ExtensionSubcommandArgOpts": { "type": "object", "properties": { @@ -231,6 +292,16 @@ "$ref": "#/definitions/ExtensionSubcommandOpts" } }, + "ProjectTemplateCategory": { + "type": "string", + "enum": [ + "backend", + "frontend", + "frontend-test", + "extra", + "support" + ] + }, "Range_of_uint": { "type": "object", "required": [ @@ -249,6 +320,19 @@ "minimum": 0.0 } } + }, + "SerdeVec_for_String": { + "anyOf": [ + { + "type": "string" + }, + { + "type": "array", + "items": { + "type": "string" + } + } + ] } } } \ No newline at end of file diff --git a/e2e/tests-dfx/extension.bash b/e2e/tests-dfx/extension.bash index 7aa52b2122..b4059ffab3 100644 --- a/e2e/tests-dfx/extension.bash +++ b/e2e/tests-dfx/extension.bash @@ -14,6 +14,133 @@ teardown() { standard_teardown } +@test "extension-defined project template" { + start_webserver --directory www + EXTENSION_URL="http://localhost:$E2E_WEB_SERVER_PORT/arbitrary/extension.json" + mkdir -p www/arbitrary/downloads www/arbitrary/project_templates/a-template + + cat > www/arbitrary/extension.json < www/arbitrary/dependencies.json <=0.8.0" + } + } +} +EOF + + cp -R "${BATS_TEST_DIRNAME}/../../src/dfx/assets/project_templates/rust" www/arbitrary/project_templates/rust-by-extension + + ARCHIVE_BASENAME="an-extension-v0.1.0" + + mkdir "$ARCHIVE_BASENAME" + cp www/arbitrary/extension.json "$ARCHIVE_BASENAME" + cp -R www/arbitrary/project_templates "$ARCHIVE_BASENAME" + tar -czf "$ARCHIVE_BASENAME".tar.gz "$ARCHIVE_BASENAME" + rm -rf "$ARCHIVE_BASENAME" + + mv "$ARCHIVE_BASENAME".tar.gz www/arbitrary/downloads/ + + assert_command dfx extension install "$EXTENSION_URL" + + setup_rust + + dfx new rbe --type rust-by-extension --no-frontend + cd rbe || exit + + dfx_start + assert_command dfx deploy + assert_command dfx canister call rbe_backend greet '("Rust By Extension")' + assert_contains "Hello, Rust By Extension!" +} + +@test "extension-defined project template replaces built-in type" { + start_webserver --directory www + EXTENSION_URL="http://localhost:$E2E_WEB_SERVER_PORT/arbitrary/extension.json" + mkdir -p www/arbitrary/downloads www/arbitrary/project_templates/a-template + + cat > www/arbitrary/extension.json < www/arbitrary/dependencies.json <=0.8.0" + } + } +} +EOF + + cp -R "${BATS_TEST_DIRNAME}/../../src/dfx/assets/project_templates/rust" www/arbitrary/project_templates/rust + echo "just-proves-it-used-the-project-template" > www/arbitrary/project_templates/rust/proof.txt + + ARCHIVE_BASENAME="an-extension-v0.1.0" + + mkdir "$ARCHIVE_BASENAME" + cp www/arbitrary/extension.json "$ARCHIVE_BASENAME" + cp -R www/arbitrary/project_templates "$ARCHIVE_BASENAME" + tar -czf "$ARCHIVE_BASENAME".tar.gz "$ARCHIVE_BASENAME" + rm -rf "$ARCHIVE_BASENAME" + + mv "$ARCHIVE_BASENAME".tar.gz www/arbitrary/downloads/ + + assert_command dfx extension install "$EXTENSION_URL" + + setup_rust + + dfx new rbe --type rust --no-frontend + assert_command cat rbe/proof.txt + assert_eq "just-proves-it-used-the-project-template" + + cd rbe || exit + + dfx_start + assert_command dfx deploy + assert_command dfx canister call rbe_backend greet '("Rust By Extension")' + assert_contains "Hello, Rust By Extension!" +} + @test "run an extension command with a canister type defined by another extension" { install_shared_asset subnet_type/shared_network_settings/system dfx_start_for_nns_install diff --git a/e2e/utils/_.bash b/e2e/utils/_.bash index 5ec7b8466e..889253e68a 100644 --- a/e2e/utils/_.bash +++ b/e2e/utils/_.bash @@ -82,10 +82,14 @@ dfx_new() { echo PWD: "$(pwd)" >&2 } -dfx_new_rust() { - local project_name=${1:-e2e_project} +setup_rust() { rustup default stable rustup target add wasm32-unknown-unknown +} + +dfx_new_rust() { + local project_name=${1:-e2e_project} + setup_rust dfx new "${project_name}" --type=rust --no-frontend test -d "${project_name}" test -f "${project_name}/dfx.json" diff --git a/src/dfx-core/src/config/model/project_template.rs b/src/dfx-core/src/config/model/project_template.rs index f35ca7e399..3dfd00a7b1 100644 --- a/src/dfx-core/src/config/model/project_template.rs +++ b/src/dfx-core/src/config/model/project_template.rs @@ -1,7 +1,12 @@ -#[derive(Debug, Clone, Eq, PartialEq)] +use schemars::JsonSchema; +use serde::{Deserialize, Serialize}; + +#[derive(Debug, Clone, Eq, PartialEq, Serialize, Deserialize, JsonSchema)] +#[serde(rename_all = "lowercase")] pub enum ProjectTemplateCategory { Backend, Frontend, + #[serde(rename = "frontend-test")] FrontendTest, Extra, Support, diff --git a/src/dfx-core/src/config/project_templates.rs b/src/dfx-core/src/config/project_templates.rs index 9febc449f2..d6194a7a5d 100644 --- a/src/dfx-core/src/config/project_templates.rs +++ b/src/dfx-core/src/config/project_templates.rs @@ -3,6 +3,7 @@ use itertools::Itertools; use std::collections::BTreeMap; use std::fmt::Display; use std::io; +use std::path::PathBuf; use std::sync::OnceLock; type GetArchiveFn = fn() -> Result>, io::Error>; @@ -11,6 +12,9 @@ type GetArchiveFn = fn() -> Result; static PROJECT_TEMPLATES: OnceLock = OnceLock::new(); -pub fn populate(builtin_templates: Vec) { - let templates = builtin_templates - .iter() - .map(|t| (t.name.clone(), t.clone())) +pub fn populate(builtin_templates: Vec, loaded_templates: Vec) { + let templates: ProjectTemplates = builtin_templates + .into_iter() + .map(|t| (t.name.clone(), t)) + .chain(loaded_templates.into_iter().map(|t| (t.name.clone(), t))) .collect(); PROJECT_TEMPLATES.set(templates).unwrap(); diff --git a/src/dfx-core/src/extension/installed.rs b/src/dfx-core/src/extension/installed.rs index 4a85ba598b..c701b3949c 100644 --- a/src/dfx-core/src/extension/installed.rs +++ b/src/dfx-core/src/extension/installed.rs @@ -1,4 +1,6 @@ +use crate::config::project_templates::ProjectTemplate; use crate::error::extension::ConvertExtensionIntoClapCommandError; +use crate::extension::manager::ExtensionManager; use crate::extension::manifest::ExtensionManifest; use crate::extension::ExtensionName; use clap::Command; @@ -28,4 +30,15 @@ impl InstalledExtensionManifests { pub fn contains(&self, extension: &str) -> bool { self.0.contains_key(extension) } + + pub fn loaded_templates( + &self, + em: &ExtensionManager, + builtin_templates: &[ProjectTemplate], + ) -> Vec { + self.0 + .values() + .flat_map(|manifest| manifest.project_templates(em, builtin_templates)) + .collect() + } } diff --git a/src/dfx-core/src/extension/manifest/extension.rs b/src/dfx-core/src/extension/manifest/extension.rs index 7ed0edcf5b..b72b6619de 100644 --- a/src/dfx-core/src/extension/manifest/extension.rs +++ b/src/dfx-core/src/extension/manifest/extension.rs @@ -1,8 +1,11 @@ +use crate::config::model::project_template::ProjectTemplateCategory; +use crate::config::project_templates::{ProjectTemplate, ProjectTemplateName, ResourceLocation}; use crate::error::extension::{ ConvertExtensionSubcommandIntoClapArgError, ConvertExtensionSubcommandIntoClapCommandError, LoadExtensionManifestError, }; -use crate::json::structure::{VersionReqWithJsonSchema, VersionWithJsonSchema}; +use crate::extension::manager::ExtensionManager; +use crate::json::structure::{SerdeVec, VersionReqWithJsonSchema, VersionWithJsonSchema}; use schemars::JsonSchema; use serde::{Deserialize, Deserializer, Serialize, Serializer}; use serde_json::Value; @@ -34,6 +37,8 @@ pub struct ExtensionManifest { pub dependencies: Option>, pub canister_type: Option, + pub project_templates: Option>, + /// Components of the download url template are: /// - `{{tag}}`: the tag of the extension release, which will follow the form "-v" /// - `{{basename}}`: The basename of the release filename, which will follow the form "--", for example "nns-x86_64-unknown-linux-gnu" @@ -56,6 +61,28 @@ pub enum ExtensionDependency { Version(VersionReqWithJsonSchema), } +#[derive(Debug, Serialize, Deserialize, JsonSchema)] +pub struct ExtensionProjectTemplate { + /// The name used for display and sorting + pub display: String, + + /// Used to determine which CLI group (`--type`, `--backend`, `--frontend`) + /// as well as for interactive selection + pub category: ProjectTemplateCategory, + + /// Other project templates to patch in alongside this one + pub requirements: Vec, + + /// Run a command after adding the canister to dfx.json + pub post_create: SerdeVec, + + /// If set, display a spinner while this command runs + pub post_create_spinner_message: Option, + + /// If the post-create command fails, display this warning but don't fail + pub post_create_failure_warning: Option, +} + impl ExtensionManifest { pub fn load( name: &str, @@ -92,6 +119,59 @@ impl ExtensionManifest { Ok(vec![]) } } + + pub fn project_templates( + &self, + em: &ExtensionManager, + builtin_templates: &[ProjectTemplate], + ) -> Vec { + let Some(project_templates) = self.project_templates.as_ref() else { + return vec![]; + }; + + let extension_dir = em.get_extension_directory(&self.name); + + // the default sort order is after everything built-in + let default_sort_order = builtin_templates + .iter() + .map(|t| t.sort_order) + .max() + .unwrap_or(0) + + 1; + + project_templates + .iter() + .map(|(name, template)| { + let resource_dir = extension_dir.join("project_templates").join(name); + let resource_location = ResourceLocation::Directory { path: resource_dir }; + + // keep the sort order as a built-in template of the same name, + // otherwise put it after everything else + let sort_order = builtin_templates + .iter() + .find(|t| t.name == ProjectTemplateName(name.clone())) + .map(|t| t.sort_order) + .unwrap_or(default_sort_order); + + let requirements = template + .requirements + .iter() + .map(|r| ProjectTemplateName(r.clone())) + .collect(); + ProjectTemplate { + name: ProjectTemplateName(name.clone()), + display: template.display.clone(), + resource_location, + category: template.category.clone(), + requirements, + post_create: template.post_create.clone().into_vec(), + post_create_spinner_message: template.post_create_spinner_message.clone(), + post_create_failure_warning: template.post_create_failure_warning.clone(), + sort_order, + } + }) + .collect() + } } #[derive(Debug, Serialize, Deserialize, JsonSchema)] diff --git a/src/dfx/src/commands/new.rs b/src/dfx/src/commands/new.rs index 093fe23805..81b3c28e88 100644 --- a/src/dfx/src/commands/new.rs +++ b/src/dfx/src/commands/new.rs @@ -29,6 +29,7 @@ use std::path::{Path, PathBuf}; use std::process::{Command, ExitStatus, Stdio}; use std::time::Duration; use tar::Archive; +use walkdir::WalkDir; // const DRY_RUN: &str = "dry_run"; // const PROJECT_NAME: &str = "project_name"; @@ -270,6 +271,52 @@ fn write_files_from_entries( Ok(()) } +fn write_files_from_directory( + log: &Logger, + dir: &Path, + root: &Path, + dry_run: bool, + variables: &BTreeMap, +) -> DfxResult { + for entry in WalkDir::new(dir).into_iter().filter_map(Result::ok) { + let path = entry.path(); + + if path.is_dir() { + continue; + } + + // Read file contents into a Vec + let file_content = dfx_core::fs::read(path)?; + + // Process the file content (replace variables) + let processed_content = match String::from_utf8(file_content) { + Err(err) => err.into_bytes(), + Ok(s) => replace_variables(s, variables).into_bytes(), + }; + + // Perform path replacements + let relative_path = path + .strip_prefix(dir)? + .to_str() + .ok_or_else(|| anyhow!("Non-unicode path encountered: {}", path.display()))?; + let relative_path = replace_variables(relative_path.to_string(), variables); + + // Build the final target path + let final_path = root.join(&relative_path); + + // Process files based on their extension + if final_path.extension() == Some("json-patch".as_ref()) { + json_patch_file(log, &final_path, &processed_content, dry_run)?; + } else if final_path.extension() == Some("patch".as_ref()) { + patch_file(log, &final_path, &processed_content, dry_run)?; + } else { + create_file(log, &final_path, &processed_content, dry_run)?; + } + } + + Ok(()) +} + #[context("Failed to scaffold frontend code.")] fn scaffold_frontend_code( env: &dyn Environment, @@ -706,10 +753,15 @@ fn write_project_template_resources( dry_run: bool, variables: &BTreeMap, ) -> DfxResult { - let mut resources = match template.resource_location { - ResourceLocation::Bundled { get_archive_fn } => get_archive_fn()?, - }; - write_files_from_entries(logger, &mut resources, project_name, dry_run, variables) + match &template.resource_location { + ResourceLocation::Bundled { get_archive_fn } => { + let mut resources = get_archive_fn()?; + write_files_from_entries(logger, &mut resources, project_name, dry_run, variables) + } + ResourceLocation::Directory { path } => { + write_files_from_directory(logger, path, project_name, dry_run, variables) + } + } } fn get_opts_interactively(opts: NewOpts) -> DfxResult { diff --git a/src/dfx/src/main.rs b/src/dfx/src/main.rs index ef86de36d3..79a19d3542 100644 --- a/src/dfx/src/main.rs +++ b/src/dfx/src/main.rs @@ -137,7 +137,9 @@ fn get_args_altered_for_extension_run( fn inner_main() -> DfxResult { let em = ExtensionManager::new(dfx_version())?; let installed_extension_manifests = em.load_installed_extension_manifests()?; - project_templates::populate(builtin_templates()); + let builtin_templates = builtin_templates(); + let loaded_templates = installed_extension_manifests.loaded_templates(&em, &builtin_templates); + project_templates::populate(builtin_templates, loaded_templates); let args = get_args_altered_for_extension_run(&installed_extension_manifests)?; @@ -201,7 +203,7 @@ mod tests { #[test] fn validate_cli() { - project_templates::populate(builtin_templates()); + project_templates::populate(builtin_templates(), vec![]); CliOpts::command().debug_assert(); } From 8307f69cbe6fc5dd0703856163e34d0991d76c77 Mon Sep 17 00:00:00 2001 From: Eric Swanson <64809312+ericswanson-dfinity@users.noreply.github.com> Date: Tue, 26 Nov 2024 05:00:52 -0800 Subject: [PATCH 2/4] fix: commands with --all parameter skip remote canisters (#4016) --- CHANGELOG.md | 12 ++++++++ e2e/tests-dfx/cycles-ledger.bash | 7 +++++ e2e/tests-dfx/remote.bash | 28 ++++++++++++++++++- src/dfx/src/commands/canister/create.rs | 12 ++------ src/dfx/src/commands/canister/delete.rs | 5 +++- .../src/commands/canister/deposit_cycles.rs | 5 ++++ src/dfx/src/commands/canister/install.rs | 10 ++----- src/dfx/src/commands/canister/start.rs | 6 ++++ src/dfx/src/commands/canister/status.rs | 6 ++++ src/dfx/src/commands/canister/stop.rs | 6 ++++ .../src/commands/canister/uninstall_code.rs | 4 +++ .../src/commands/canister/update_settings.rs | 8 +++++- .../src/commands/ledger/fabricate_cycles.rs | 6 ++++ src/dfx/src/lib/operations/canister/mod.rs | 2 ++ .../canister/skip_remote_canister.rs | 17 +++++++++++ 15 files changed, 113 insertions(+), 21 deletions(-) create mode 100644 src/dfx/src/lib/operations/canister/skip_remote_canister.rs diff --git a/CHANGELOG.md b/CHANGELOG.md index c2e6e01994..33374dd7dd 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -25,6 +25,18 @@ You can use the `--replica` flag already to write scripts that anticipate that c An extension can define one or more project templates for `dfx new` to use. These can be new templates or replace the built-in project templates. +### fix: all commands with --all parameter skip remote canisters + +This affects the following commands: +- `dfx canister delete` +- `dfx canister deposit-cycles` +- `dfx canister start` +- `dfx canister status` +- `dfx canister stop` +- `dfx canister uninstall-code` +- `dfx canister update-settings` +- `dfx ledger fabricate-cycles` + # 0.24.3 ### feat: Bitcoin support in PocketIC diff --git a/e2e/tests-dfx/cycles-ledger.bash b/e2e/tests-dfx/cycles-ledger.bash index 7c20cbf057..3a279b6c0b 100644 --- a/e2e/tests-dfx/cycles-ledger.bash +++ b/e2e/tests-dfx/cycles-ledger.bash @@ -458,6 +458,13 @@ current_time_nanoseconds() { assert_eq "2399699700000 cycles." assert_command dfx canister status e2e_project_backend assert_contains "Balance: 3_100_002_100_000 Cycles" + + # deposit-cycles --all skips remote canisters + jq '.canisters.remote.remote.id.local="rdmx6-jaaaa-aaaaa-aaadq-cai"' dfx.json | sponge dfx.json + assert_command dfx canister deposit-cycles 10000 --all --identity bob + assert_contains "Skipping canister 'remote' because it is remote for network 'local'" + assert_contains "Depositing 10000 cycles onto e2e_project_backend" + assert_not_contains "Depositing 10000 cycles onto remote" } @test "top-up deduplication" { diff --git a/e2e/tests-dfx/remote.bash b/e2e/tests-dfx/remote.bash index 390070157e..8e8bc5568e 100644 --- a/e2e/tests-dfx/remote.bash +++ b/e2e/tests-dfx/remote.bash @@ -137,7 +137,7 @@ teardown() { assert_match "Canister 'remote' is a remote canister on network 'actuallylocal', and cannot be installed from here." } -@test "canister create --all, canister install --all skip remote canisters" { +@test "all commands with --all skip remote canisters" { install_asset remote/actual dfx_start setup_actuallylocal_shared_network @@ -201,6 +201,32 @@ teardown() { assert_command jq .remote canister_ids.json assert_eq "null" + assert_command dfx ledger fabricate-cycles --all --t 100 --network actuallylocal + assert_contains "Skipping canister 'remote' because it is remote for network 'actuallylocal'" + + assert_command dfx canister status --all --network actuallylocal + assert_contains "Skipping canister 'remote' because it is remote for network 'actuallylocal'" + + assert_command dfx canister update-settings --log-visibility public --all --network actuallylocal + assert_contains "Skipping canister 'remote' because it is remote for network 'actuallylocal'" + + assert_command dfx canister stop --all --network actuallylocal + assert_contains "Skipping canister 'remote' because it is remote for network 'actuallylocal'" + + assert_command dfx canister start --all --network actuallylocal + assert_contains "Skipping canister 'remote' because it is remote for network 'actuallylocal'" + + # have to stop to uninstall + assert_command dfx canister stop --all --network actuallylocal + + assert_command dfx canister uninstall-code --all --network actuallylocal + assert_contains "Skipping canister 'remote' because it is remote for network 'actuallylocal'" + + assert_command dfx build --all --network actuallylocal + + assert_command dfx canister delete --all --network actuallylocal + assert_contains "Skipping canister 'remote' because it is remote for network 'actuallylocal'" + # Assert frontend declarations are actually created dfx generate assert_file_exists "src/declarations/remote/remote.did" diff --git a/src/dfx/src/commands/canister/create.rs b/src/dfx/src/commands/canister/create.rs index a5293f0f1c..368ee604f5 100644 --- a/src/dfx/src/commands/canister/create.rs +++ b/src/dfx/src/commands/canister/create.rs @@ -6,7 +6,7 @@ use crate::lib::ic_attributes::{ get_compute_allocation, get_freezing_threshold, get_log_visibility, get_memory_allocation, get_reserved_cycles_limit, get_wasm_memory_limit, CanisterSettings, }; -use crate::lib::operations::canister::create_canister; +use crate::lib::operations::canister::{create_canister, skip_remote_canister}; use crate::lib::root_key::fetch_root_key_if_needed; use crate::util::clap::parsers::{ compute_allocation_parser, freezing_threshold_parser, log_visibility_parser, @@ -245,15 +245,7 @@ pub async fn exec( if pull_canisters_in_config.contains_key(canister_name) { continue; } - let canister_is_remote = - config_interface.is_remote_canister(canister_name, &network.name)?; - if canister_is_remote { - info!( - env.get_logger(), - "Skipping canister '{canister_name}' because it is remote for network '{}'", - &network.name, - ); - + if skip_remote_canister(env, canister_name)? { continue; } let specified_id = config_interface.get_specified_id(canister_name)?; diff --git a/src/dfx/src/commands/canister/delete.rs b/src/dfx/src/commands/canister/delete.rs index 74a2c66748..02d3dcb30f 100644 --- a/src/dfx/src/commands/canister/delete.rs +++ b/src/dfx/src/commands/canister/delete.rs @@ -3,7 +3,7 @@ use crate::lib::error::DfxResult; use crate::lib::ic_attributes::CanisterSettings; use crate::lib::operations::canister; use crate::lib::operations::canister::{ - deposit_cycles, start_canister, stop_canister, update_settings, + deposit_cycles, skip_remote_canister, start_canister, stop_canister, update_settings, }; use crate::lib::operations::cycles_ledger::wallet_deposit_to_cycles_ledger; use crate::lib::root_key::fetch_root_key_if_needed; @@ -352,6 +352,9 @@ pub async fn exec( } else if opts.all { if let Some(canisters) = &config.get_config().canisters { for canister in canisters.keys() { + if skip_remote_canister(env, canister)? { + continue; + } delete_canister( env, canister, diff --git a/src/dfx/src/commands/canister/deposit_cycles.rs b/src/dfx/src/commands/canister/deposit_cycles.rs index 1db8196e38..3a2bb26e0c 100644 --- a/src/dfx/src/commands/canister/deposit_cycles.rs +++ b/src/dfx/src/commands/canister/deposit_cycles.rs @@ -3,6 +3,7 @@ use std::time::{SystemTime, UNIX_EPOCH}; use crate::lib::error::DfxResult; use crate::lib::identity::wallet::get_or_create_wallet_canister; use crate::lib::operations::canister; +use crate::lib::operations::canister::skip_remote_canister; use crate::lib::root_key::fetch_root_key_if_needed; use crate::lib::{environment::Environment, operations::cycles_ledger}; use crate::util::clap::parsers::{cycle_amount_parser, icrc_subaccount_parser}; @@ -147,6 +148,10 @@ pub async fn exec( if let Some(canisters) = &config.get_config().canisters { for canister in canisters.keys() { + if skip_remote_canister(env, canister)? { + continue; + } + deposit_cycles( env, canister, diff --git a/src/dfx/src/commands/canister/install.rs b/src/dfx/src/commands/canister/install.rs index d992ca2d9e..bb8e1de50b 100644 --- a/src/dfx/src/commands/canister/install.rs +++ b/src/dfx/src/commands/canister/install.rs @@ -10,6 +10,7 @@ use crate::util::clap::install_mode::{InstallModeHint, InstallModeOpt}; use dfx_core::canister::{install_canister_wasm, install_mode_to_prompt}; use dfx_core::identity::CallSender; +use crate::lib::operations::canister::skip_remote_canister; use anyhow::bail; use candid::Principal; use clap::Parser; @@ -187,7 +188,6 @@ pub async fn exec( } else if opts.all { // Install all canisters. let config = env.get_config_or_anyhow()?; - let config_interface = config.get_config(); let env_file = config.get_output_env_file(opts.output_env_file)?; let pull_canisters_in_config = get_pull_canisters_in_config(env)?; if let Some(canisters) = &config.get_config().canisters { @@ -195,13 +195,7 @@ pub async fn exec( if pull_canisters_in_config.contains_key(canister) { continue; } - if config_interface.is_remote_canister(canister, &network.name)? { - info!( - env.get_logger(), - "Skipping canister '{}' because it is remote for network '{}'", - canister, - &network.name, - ); + if skip_remote_canister(env, canister)? { continue; } diff --git a/src/dfx/src/commands/canister/start.rs b/src/dfx/src/commands/canister/start.rs index 529817ca59..e3ff066710 100644 --- a/src/dfx/src/commands/canister/start.rs +++ b/src/dfx/src/commands/canister/start.rs @@ -1,6 +1,7 @@ use crate::lib::environment::Environment; use crate::lib::error::DfxResult; use crate::lib::operations::canister; +use crate::lib::operations::canister::skip_remote_canister; use crate::lib::root_key::fetch_root_key_if_needed; use candid::Principal; use clap::Parser; @@ -51,8 +52,13 @@ pub async fn exec( start_canister(env, canister, call_sender).await } else if opts.all { let config = env.get_config_or_anyhow()?; + if let Some(canisters) = &config.get_config().canisters { for canister in canisters.keys() { + if skip_remote_canister(env, canister)? { + continue; + } + start_canister(env, canister, call_sender).await?; } } diff --git a/src/dfx/src/commands/canister/status.rs b/src/dfx/src/commands/canister/status.rs index d3183759b9..ecd8d7773d 100644 --- a/src/dfx/src/commands/canister/status.rs +++ b/src/dfx/src/commands/canister/status.rs @@ -1,6 +1,7 @@ use crate::lib::environment::Environment; use crate::lib::error::DfxResult; use crate::lib::operations::canister; +use crate::lib::operations::canister::skip_remote_canister; use crate::lib::root_key::fetch_root_key_if_needed; use candid::Principal; use clap::Parser; @@ -95,8 +96,13 @@ pub async fn exec( canister_status(env, canister, call_sender).await } else if opts.all { let config = env.get_config_or_anyhow()?; + if let Some(canisters) = &config.get_config().canisters { for canister in canisters.keys() { + if skip_remote_canister(env, canister)? { + continue; + } + canister_status(env, canister, call_sender).await?; } } diff --git a/src/dfx/src/commands/canister/stop.rs b/src/dfx/src/commands/canister/stop.rs index 7f71472908..4e44a539a8 100644 --- a/src/dfx/src/commands/canister/stop.rs +++ b/src/dfx/src/commands/canister/stop.rs @@ -1,6 +1,7 @@ use crate::lib::environment::Environment; use crate::lib::error::DfxResult; use crate::lib::operations::canister; +use crate::lib::operations::canister::skip_remote_canister; use crate::lib::root_key::fetch_root_key_if_needed; use candid::Principal; use clap::Parser; @@ -52,8 +53,13 @@ pub async fn exec( stop_canister(env, canister, call_sender).await } else if opts.all { let config = env.get_config_or_anyhow()?; + if let Some(canisters) = &config.get_config().canisters { for canister in canisters.keys() { + if skip_remote_canister(env, canister)? { + continue; + } + stop_canister(env, canister, call_sender).await?; } } diff --git a/src/dfx/src/commands/canister/uninstall_code.rs b/src/dfx/src/commands/canister/uninstall_code.rs index 4b386d7a36..602e3182e7 100644 --- a/src/dfx/src/commands/canister/uninstall_code.rs +++ b/src/dfx/src/commands/canister/uninstall_code.rs @@ -1,6 +1,7 @@ use crate::lib::environment::Environment; use crate::lib::error::DfxResult; use crate::lib::operations::canister; +use crate::lib::operations::canister::skip_remote_canister; use crate::lib::root_key::fetch_root_key_if_needed; use candid::Principal; use clap::Parser; @@ -56,6 +57,9 @@ pub async fn exec( if let Some(canisters) = &config.get_config().canisters { for canister in canisters.keys() { + if skip_remote_canister(env, canister)? { + continue; + } uninstall_code(env, canister, call_sender).await?; } } diff --git a/src/dfx/src/commands/canister/update_settings.rs b/src/dfx/src/commands/canister/update_settings.rs index f6867c58af..db3a3184eb 100644 --- a/src/dfx/src/commands/canister/update_settings.rs +++ b/src/dfx/src/commands/canister/update_settings.rs @@ -6,7 +6,9 @@ use crate::lib::ic_attributes::{ get_compute_allocation, get_freezing_threshold, get_log_visibility, get_memory_allocation, get_reserved_cycles_limit, get_wasm_memory_limit, CanisterSettings, }; -use crate::lib::operations::canister::{get_canister_status, update_settings}; +use crate::lib::operations::canister::{ + get_canister_status, skip_remote_canister, update_settings, +}; use crate::lib::root_key::fetch_root_key_if_needed; use crate::util::clap::parsers::{ compute_allocation_parser, freezing_threshold_parser, memory_allocation_parser, @@ -220,8 +222,12 @@ pub async fn exec( // Update all canister settings. let config = env.get_config_or_anyhow()?; let config_interface = config.get_config(); + if let Some(canisters) = &config_interface.canisters { for canister_name in canisters.keys() { + if skip_remote_canister(env, canister_name)? { + continue; + } let mut controllers = controllers.clone(); let canister_id = canister_id_store.get(canister_name)?; let compute_allocation = get_compute_allocation( diff --git a/src/dfx/src/commands/ledger/fabricate_cycles.rs b/src/dfx/src/commands/ledger/fabricate_cycles.rs index 447bc9b405..ef0f239efa 100644 --- a/src/dfx/src/commands/ledger/fabricate_cycles.rs +++ b/src/dfx/src/commands/ledger/fabricate_cycles.rs @@ -3,6 +3,7 @@ use crate::lib::environment::Environment; use crate::lib::error::DfxResult; use crate::lib::nns_types::icpts::ICPTs; use crate::lib::operations::canister; +use crate::lib::operations::canister::skip_remote_canister; use crate::lib::root_key::fetch_root_key_or_anyhow; use crate::util::clap::parsers::{cycle_amount_parser, e8s_parser, trillion_cycle_amount_parser}; use crate::util::currency_conversion::as_cycles_with_current_exchange_rate; @@ -120,8 +121,13 @@ pub async fn exec(env: &dyn Environment, opts: FabricateCyclesOpts) -> DfxResult deposit_minted_cycles(env, canister, &CallSender::SelectedId, cycles).await } else if opts.all { let config = env.get_config_or_anyhow()?; + if let Some(canisters) = &config.get_config().canisters { for canister in canisters.keys() { + if skip_remote_canister(env, canister)? { + continue; + } + deposit_minted_cycles(env, canister, &CallSender::SelectedId, cycles).await?; } } diff --git a/src/dfx/src/lib/operations/canister/mod.rs b/src/dfx/src/lib/operations/canister/mod.rs index 501dc9284d..9551ddc4fb 100644 --- a/src/dfx/src/lib/operations/canister/mod.rs +++ b/src/dfx/src/lib/operations/canister/mod.rs @@ -2,10 +2,12 @@ pub(crate) mod create_canister; pub(crate) mod deploy_canisters; pub(crate) mod install_canister; pub mod motoko_playground; +mod skip_remote_canister; pub use create_canister::create_canister; use ic_utils::interfaces::management_canister::Snapshot; pub use install_canister::install_wallet; +pub use skip_remote_canister::skip_remote_canister; use crate::lib::canister_info::CanisterInfo; use crate::lib::environment::Environment; diff --git a/src/dfx/src/lib/operations/canister/skip_remote_canister.rs b/src/dfx/src/lib/operations/canister/skip_remote_canister.rs new file mode 100644 index 0000000000..6ce603cf1a --- /dev/null +++ b/src/dfx/src/lib/operations/canister/skip_remote_canister.rs @@ -0,0 +1,17 @@ +use crate::lib::environment::Environment; +use crate::lib::error::DfxResult; +use slog::info; + +pub fn skip_remote_canister(env: &dyn Environment, canister: &str) -> DfxResult { + let config = env.get_config_or_anyhow()?; + let config_interface = config.get_config(); + let network = env.get_network_descriptor(); + let canister_is_remote = config_interface.is_remote_canister(canister, &network.name)?; + if canister_is_remote { + info!( + env.get_logger(), + "Skipping canister '{canister}' because it is remote for network '{}'", &network.name, + ); + } + Ok(canister_is_remote) +} From 3200e133bfda0a06088aaede4387b5f1c9141ae5 Mon Sep 17 00:00:00 2001 From: Eric Swanson <64809312+ericswanson-dfinity@users.noreply.github.com> Date: Tue, 26 Nov 2024 09:57:17 -0800 Subject: [PATCH 3/4] refactor: remove dead code (#4017) --- src/dfx-core/src/config/model/local_server_descriptor.rs | 8 -------- src/dfx/src/commands/start.rs | 9 --------- 2 files changed, 17 deletions(-) diff --git a/src/dfx-core/src/config/model/local_server_descriptor.rs b/src/dfx-core/src/config/model/local_server_descriptor.rs index e14afa4727..f9eaf562f9 100644 --- a/src/dfx-core/src/config/model/local_server_descriptor.rs +++ b/src/dfx-core/src/config/model/local_server_descriptor.rs @@ -239,14 +239,6 @@ impl LocalServerDescriptor { } } - pub fn with_replica_port(self, port: u16) -> Self { - let replica = ConfigDefaultsReplica { - port: Some(port), - ..self.replica - }; - Self { replica, ..self } - } - pub fn with_bitcoin_enabled(self) -> LocalServerDescriptor { let bitcoin = ConfigDefaultsBitcoin { enabled: true, diff --git a/src/dfx/src/commands/start.rs b/src/dfx/src/commands/start.rs index 768f6d286b..083ccd3a05 100644 --- a/src/dfx/src/commands/start.rs +++ b/src/dfx/src/commands/start.rs @@ -5,7 +5,6 @@ use crate::actors::{ start_shutdown_controller, }; use crate::config::dfx_version_str; -use crate::error_invalid_argument; use crate::lib::environment::Environment; use crate::lib::error::DfxResult; use crate::lib::info::replica_rev; @@ -188,7 +187,6 @@ pub fn exec( env.get_logger(), network_descriptor, host, - None, enable_bitcoin, bitcoin_node, enable_canister_http, @@ -495,7 +493,6 @@ pub fn apply_command_line_parameters( logger: &Logger, network_descriptor: NetworkDescriptor, host: Option, - replica_port: Option, enable_bitcoin: bool, bitcoin_nodes: Vec, enable_canister_http: bool, @@ -520,12 +517,6 @@ pub fn apply_command_line_parameters( .map_err(|e| anyhow!("Invalid argument: Invalid host: {}", e))?; local_server_descriptor = local_server_descriptor.with_bind_address(host); } - if let Some(replica_port) = replica_port { - let replica_port: u16 = replica_port - .parse() - .map_err(|err| error_invalid_argument!("Invalid port number: {}", err))?; - local_server_descriptor = local_server_descriptor.with_replica_port(replica_port); - } if enable_bitcoin || !bitcoin_nodes.is_empty() { local_server_descriptor = local_server_descriptor.with_bitcoin_enabled(); } From 3405dc693391c144c97ab77d8a67cf70a3606343 Mon Sep 17 00:00:00 2001 From: Vincent Zhang <118719397+vincent-dfinity@users.noreply.github.com> Date: Thu, 28 Nov 2024 07:33:46 +0800 Subject: [PATCH 4/4] chore: improve error message for 'dfx deploy'. (#4018) * Improve error message for 'dfx deploy'. * Fix lint. * Rename the variable. Co-authored-by: Eric Swanson <64809312+ericswanson-dfinity@users.noreply.github.com> * Add test for failing to create canister without enough cycles. * Fixed shellcheck. * Show the correct command according to the network. --------- Co-authored-by: Eric Swanson <64809312+ericswanson-dfinity@users.noreply.github.com> --- CHANGELOG.md | 11 +++++++++++ e2e/tests-dfx/cycles-ledger.bash | 13 +++++++++++++ src/dfx/src/lib/diagnosis.rs | 33 ++++++++++++++++++++++++++++++++ 3 files changed, 57 insertions(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index 33374dd7dd..3145adec2c 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -37,6 +37,17 @@ This affects the following commands: - `dfx canister update-settings` - `dfx ledger fabricate-cycles` +### chore: improve `dfx deploy` messages. + +If users run `dfx deploy` without enough cycles, show additional messages to indicate what to do next. +``` +Error explanation: +Insufficient cycles balance to create the canister. +How to resolve the error: +Please top up your cycles balance by converting ICP to cycles like below: +'dfx cycles convert --amount=0.123'. +``` + # 0.24.3 ### feat: Bitcoin support in PocketIC diff --git a/e2e/tests-dfx/cycles-ledger.bash b/e2e/tests-dfx/cycles-ledger.bash index 3a279b6c0b..83fa0ddd99 100644 --- a/e2e/tests-dfx/cycles-ledger.bash +++ b/e2e/tests-dfx/cycles-ledger.bash @@ -591,9 +591,22 @@ current_time_nanoseconds() { assert_command dfx deploy + # Test canister creation failure before topping up cycles. + cd .. + dfx_new canister_creation_failed + + # shellcheck disable=SC2030,SC2031 + export DFX_DISABLE_AUTO_WALLET=1 + assert_command_fail dfx canister create canister_creation_failed_backend --with-cycles 1T --identity alice + assert_contains "Insufficient cycles balance to create the canister." + + ## Back to top up cycles. + cd ../temporary + assert_command dfx canister call depositor deposit "(record {to = record{owner = principal \"$ALICE\";};cycles = 13_400_000_000_000;})" --identity cycle-giver assert_command dfx canister call depositor deposit "(record {to = record{owner = principal \"$ALICE\"; subaccount = opt blob \"$ALICE_SUBACCT1_CANDID\"};cycles = 2_600_000_000_000;})" --identity cycle-giver + # shellcheck disable=SC2103 cd .. dfx_new # setup done diff --git a/src/dfx/src/lib/diagnosis.rs b/src/dfx/src/lib/diagnosis.rs index 150f1c9b56..b346341cf7 100644 --- a/src/dfx/src/lib/diagnosis.rs +++ b/src/dfx/src/lib/diagnosis.rs @@ -1,6 +1,8 @@ +use crate::lib::cycles_ledger_types::create_canister::CreateCanisterError; use crate::lib::error_code; use anyhow::Error as AnyhowError; use dfx_core::error::root_key::FetchRootKeyError; +use dfx_core::network::provider::get_network_context; use ic_agent::agent::{RejectCode, RejectResponse}; use ic_agent::AgentError; use ic_asset::error::{GatherAssetDescriptorsError, SyncError, UploadContentError}; @@ -72,6 +74,12 @@ pub fn diagnose(err: &AnyhowError) -> Diagnosis { } } + if let Some(create_canister_err) = err.downcast_ref::() { + if insufficient_cycles(create_canister_err) { + return diagnose_insufficient_cycles(); + } + } + NULL_DIAGNOSIS } @@ -262,3 +270,28 @@ fn diagnose_ledger_not_found() -> Diagnosis { (Some(explanation.to_string()), Some(suggestion.to_string())) } + +fn insufficient_cycles(err: &CreateCanisterError) -> bool { + matches!(err, CreateCanisterError::InsufficientFunds { balance: _ }) +} + +fn diagnose_insufficient_cycles() -> Diagnosis { + let network = match get_network_context() { + Ok(value) => { + if value == "local" { + "".to_string() + } else { + format!(" --network {}", value) + } + } + Err(_) => "".to_string(), + }; + + let explanation = "Insufficient cycles balance to create the canister."; + let suggestion = format!( + "Please top up your cycles balance by converting ICP to cycles like below: +'dfx cycles convert --amount=0.123{}'", + network + ); + (Some(explanation.to_string()), Some(suggestion)) +}