From 795dacca793fbfbac7098761610230081197b19f Mon Sep 17 00:00:00 2001 From: Severin Siffert Date: Wed, 12 Oct 2022 14:08:57 +0200 Subject: [PATCH 01/13] initial working version, still needs cleanup and tests --- e2e/tests-dfx/remote.bash | 2 +- src/dfx/src/commands/build.rs | 6 +- src/dfx/src/commands/generate.rs | 18 ++- src/dfx/src/lib/builders/motoko.rs | 1 + src/dfx/src/lib/models/canister.rs | 104 ++++++++++++++++-- .../operations/canister/deploy_canisters.rs | 54 +++++---- 6 files changed, 149 insertions(+), 36 deletions(-) diff --git a/e2e/tests-dfx/remote.bash b/e2e/tests-dfx/remote.bash index 0f7691c7fd..f0214c1ab3 100644 --- a/e2e/tests-dfx/remote.bash +++ b/e2e/tests-dfx/remote.bash @@ -37,7 +37,7 @@ teardown() { # set up: remote method is update, local is query # call remote method as update to make a change - assert_command dfx deploy --network actuallylocal + assert_command dfx deploy --network actuallylocal -vv assert_command dfx canister call remote which_am_i --network actuallylocal assert_eq '("actual")' diff --git a/src/dfx/src/commands/build.rs b/src/dfx/src/commands/build.rs index f7cb66a4ef..de104c6a81 100644 --- a/src/dfx/src/commands/build.rs +++ b/src/dfx/src/commands/build.rs @@ -71,7 +71,11 @@ pub fn exec(env: &dyn Environment, opts: CanisterBuildOpts) -> DfxResult { let runtime = Runtime::new().expect("Unable to create a runtime"); let build_config = BuildConfig::from_config(&config)?.with_build_mode_check(build_mode_check); - runtime.block_on(canister_pool.build_or_fail(&build_config))?; + runtime.block_on(canister_pool.build_or_fail( + logger, + &build_config, + &canister_names.as_slice(), + ))?; Ok(()) } diff --git a/src/dfx/src/commands/generate.rs b/src/dfx/src/commands/generate.rs index 942a346620..df6e66f010 100644 --- a/src/dfx/src/commands/generate.rs +++ b/src/dfx/src/commands/generate.rs @@ -7,6 +7,7 @@ use crate::lib::provider::create_agent_environment; use crate::NetworkOpt; use clap::Parser; +use slog::trace; use tokio::runtime::Runtime; /// Generate type declarations for canisters from the code in your project @@ -41,26 +42,35 @@ pub fn exec(env: &dyn Environment, opts: GenerateOpts) -> DfxResult { // This is just to display an error if trying to generate before creating the canister. let store = CanisterIdStore::for_env(&env)?; // If generate for motoko canister, build first - let mut build_before_generate = false; + let mut build_before_generate = Vec::new(); for canister in canister_pool.get_canister_list() { let canister_name = canister.get_name(); let canister_id = store.get(canister_name)?; if let Some(info) = canister_pool.get_canister_info(&canister_id) { if info.is_motoko() { - build_before_generate = true; + build_before_generate.push(canister_name.to_string()); + trace!( + env.get_logger(), + "Found Motoko canister '{}' - will have to build before generating IDL.", + canister_name + ); } } } let build_config = BuildConfig::from_config(&config)?; - if build_before_generate { + if !build_before_generate.is_empty() { slog::info!( env.get_logger(), "Building canisters before generate for Motoko" ); let runtime = Runtime::new().expect("Unable to create a runtime"); - runtime.block_on(canister_pool.build_or_fail(&build_config))?; + runtime.block_on(canister_pool.build_or_fail( + env.get_logger(), + &build_config, + &build_before_generate, + ))?; } for canister in canister_pool.get_canister_list() { diff --git a/src/dfx/src/lib/builders/motoko.rs b/src/dfx/src/lib/builders/motoko.rs index 2b250c02f0..4f5745e842 100644 --- a/src/dfx/src/lib/builders/motoko.rs +++ b/src/dfx/src/lib/builders/motoko.rs @@ -146,6 +146,7 @@ impl CanisterBuilder for MotokoBuilder { }; // Generate wasm + // todo!("add dependencies properly"); let params = MotokoParams { build_target: match profile { Profile::Release => BuildTarget::Release, diff --git a/src/dfx/src/lib/models/canister.rs b/src/dfx/src/lib/models/canister.rs index 80a1f9bfb5..070ef950d4 100644 --- a/src/dfx/src/lib/models/canister.rs +++ b/src/dfx/src/lib/models/canister.rs @@ -122,6 +122,12 @@ impl CanisterPool { _ => None, }; let info = CanisterInfo::load(pool_helper.config, canister_name, canister_id)?; + println!( + "Inserting into pool: canister {}, cid {:?}, cid-info {}", + canister_name, + canister_id.map(|a| a.to_text()), + info.get_canister_id().map(|c| c.to_text())? + ); let builder = pool_helper.builder_pool.get(&info); pool_helper .canisters_map @@ -200,8 +206,17 @@ impl CanisterPool { let mut id_set: BTreeMap> = BTreeMap::new(); // Add all the canisters as nodes. + println!( + "All canisters in dependencies graph: {} canisters: {:?}", + self.canisters.len(), + self.canisters + .iter() + .map(|c| c.get_name()) + .collect::>() + ); for canister in &self.canisters { let canister_id = canister.info.get_canister_id()?; + println!("Inserted id: {}", canister_id.to_text()); id_set.insert(canister_id, graph.add_node(canister_id)); } @@ -238,8 +253,18 @@ impl CanisterPool { } #[context("Failed step_prebuild_all.")] - fn step_prebuild_all(&self, _build_config: &BuildConfig) -> DfxResult<()> { - if self.canisters.iter().any(|can| can.info.is_rust()) { + fn step_prebuild_all( + &self, + build_config: &BuildConfig, + canisters_to_build: &[String], + ) -> DfxResult<()> { + // cargo audit + if self + .canisters + .iter() + .filter(|can| canisters_to_build.contains(&can.info.get_name().to_string())) + .any(|can| can.info.is_rust()) + { self.run_cargo_audit()?; } else { trace!( @@ -247,6 +272,22 @@ impl CanisterPool { "No canister of type 'rust' found. Not trying to run 'cargo audit'." ) } + + // make .did files available for canisters that are not getting built + for canister in &self.canisters { + if !canisters_to_build.contains(&canister.info.get_name().to_string()) { + if canister.info.is_remote() { + println!("Copying did for canister {}.", canister.info.get_name()); + let to = build_config.idl_root.join(format!( + "{}.did", + canister.info.get_canister_id()?.to_text() + )); + std::fs::copy(canister.info.get_output_idl_path().unwrap(), to); + // .with_context(|| format!("Failed to copy remote candid from {} to {}.",)) + } + } + } + Ok(()) } @@ -365,10 +406,16 @@ impl CanisterPool { &self, build_config: &BuildConfig, _order: &[CanisterId], + canisters_to_build: &[String], ) -> DfxResult<()> { // We don't want to simply remove the whole directory, as in the future, // we may want to keep the IDL files downloaded from network. - for canister in &self.canisters { + for canister in self + .canisters + .iter() + .filter(|can| canisters_to_build.contains(&can.info.get_name().to_string())) + .collect::>>() + { let idl_root = &build_config.idl_root; let canister_id = canister.canister_id(); let idl_file_path = idl_root.join(canister_id.to_text()).with_extension("did"); @@ -384,12 +431,23 @@ impl CanisterPool { #[context("Failed while trying to build all canisters in the canister pool.")] pub fn build( &self, + log: &Logger, build_config: &BuildConfig, + canisters_to_build: &[String], ) -> DfxResult>> { - self.step_prebuild_all(build_config) + self.step_prebuild_all(build_config, canisters_to_build) .map_err(|e| DfxError::new(BuildError::PreBuildAllStepFailed(Box::new(e))))?; let graph = self.build_dependencies_graph()?; + println!( + "Graph node count: {}, nodes: {:?}", + graph.node_count(), + graph + .raw_nodes() + .iter() + .map(|n| n.weight.to_text()) + .collect::>() + ); let nodes = petgraph::algo::toposort(&graph, None).map_err(|cycle| { let message = match graph.node_weight(cycle.node_id()) { Some(canister_id) => match self.get_canister_info(canister_id) { @@ -400,15 +458,28 @@ impl CanisterPool { }; BuildError::DependencyError(format!("Found circular dependency: {}", message)) })?; + println!("Nodes: {:?}", nodes); let order: Vec = nodes .iter() .rev() // Reverse the order, as we have a dependency graph, we want to reverse indices. .map(|idx| *graph.node_weight(*idx).unwrap()) .collect(); + println!( + "Canisters in order: {:?}", + order.iter().map(|cid| cid.to_text()).collect::>() + ); let mut result = Vec::new(); + println!("CAnisters to build: {:?}", canisters_to_build); for canister_id in &order { + println!("Looking at cid {}", canister_id); if let Some(canister) = self.get_canister(canister_id) { + if canisters_to_build.contains(&canister.info.get_name().to_string()) { + trace!(log, "Building canister '{}'.", canister.info.get_name()); + } else { + println!("Not building {}", canister.info.get_name()); + continue; + } result.push( self.step_prebuild(build_config, canister) .map_err(|e| { @@ -442,7 +513,7 @@ impl CanisterPool { } } - self.step_postbuild_all(build_config, &order) + self.step_postbuild_all(build_config, &order, canisters_to_build) .map_err(|e| DfxError::new(BuildError::PostBuildAllStepFailed(Box::new(e))))?; Ok(result) @@ -451,9 +522,14 @@ impl CanisterPool { /// Build all canisters, failing with the first that failed the build. Will return /// nothing if all succeeded. #[context("Failed while trying to build all canisters.")] - pub async fn build_or_fail(&self, build_config: &BuildConfig) -> DfxResult<()> { - self.download(build_config).await?; - let outputs = self.build(build_config)?; + pub async fn build_or_fail( + &self, + log: &Logger, + build_config: &BuildConfig, + canisters_to_build: &[String], + ) -> DfxResult<()> { + self.download(build_config, canisters_to_build).await?; + let outputs = self.build(log, build_config, canisters_to_build)?; for output in outputs { output.map_err(DfxError::new)?; @@ -462,8 +538,16 @@ impl CanisterPool { Ok(()) } - async fn download(&self, _build_config: &BuildConfig) -> DfxResult { - for canister in &self.canisters { + async fn download( + &self, + _build_config: &BuildConfig, + canisters_to_build: &[String], + ) -> DfxResult { + for canister in self + .canisters + .iter() + .filter(|can| canisters_to_build.contains(&can.info.get_name().to_string())) + { let info = canister.get_info(); if info.is_custom() { diff --git a/src/dfx/src/lib/operations/canister/deploy_canisters.rs b/src/dfx/src/lib/operations/canister/deploy_canisters.rs index aa175a14be..47c01c76c9 100644 --- a/src/dfx/src/lib/operations/canister/deploy_canisters.rs +++ b/src/dfx/src/lib/operations/canister/deploy_canisters.rs @@ -16,7 +16,7 @@ use ic_utils::interfaces::management_canister::attributes::{ ComputeAllocation, FreezingThreshold, MemoryAllocation, }; use ic_utils::interfaces::management_canister::builders::InstallMode; -use slog::info; +use slog::{info, trace}; use std::convert::TryFrom; use std::time::Duration; @@ -43,7 +43,13 @@ pub async fn deploy_canisters( let network = env.get_network_descriptor(); - let canisters_to_build = canister_with_dependencies(&config, some_canister)?; + let canisters_to_load = canister_with_dependencies(&config, some_canister)?; + trace!( + log, + "Found {} canisters to load: {:?}", + canisters_to_load.len(), + &canisters_to_load + ); let canisters_to_deploy = if force_reinstall { // don't force-reinstall the dependencies too. @@ -58,19 +64,24 @@ pub async fn deploy_canisters( None => bail!("The --mode=reinstall is only valid when deploying a single canister, because reinstallation destroys all data in the canister."), } } else { - canisters_to_build.clone() + canisters_to_load + .clone() + .into_iter() + .filter(|canister_name| { + !matches!( + config + .get_config() + .get_remote_canister_id(canister_name, &network.name), + Ok(Some(_)) + ) + }) + .collect() }; - let canisters_to_deploy: Vec = canisters_to_deploy - .into_iter() - .filter(|canister_name| { - !matches!( - config - .get_config() - .get_remote_canister_id(canister_name, &network.name), - Ok(Some(_)) - ) - }) - .collect(); + trace!( + log, + "Found {} canisters to deploy.", + canisters_to_deploy.len() + ); if some_canister.is_some() { info!(log, "Deploying: {}", canisters_to_deploy.join(" ")); @@ -80,7 +91,7 @@ pub async fn deploy_canisters( register_canisters( env, - &canisters_to_build, + &canisters_to_deploy, &initial_canister_id_store, timeout, with_cycles, @@ -89,7 +100,7 @@ pub async fn deploy_canisters( ) .await?; - let pool = build_canisters(env, &canisters_to_build, &config).await?; + let pool = build_canisters(env, &canisters_to_load, &canisters_to_deploy, &config).await?; install_canisters( env, @@ -124,6 +135,7 @@ fn canister_with_dependencies( Ok(canister_names) } +/// Creates canisters that have not been created yet. #[context("Failed while trying to register all canisters.")] async fn register_canisters( env: &dyn Environment, @@ -193,15 +205,17 @@ async fn register_canisters( #[context("Failed to build call canisters.")] async fn build_canisters( env: &dyn Environment, - canister_names: &[String], + required_canisters: &[String], + canisters_to_build: &[String], config: &Config, ) -> DfxResult { - info!(env.get_logger(), "Building canisters..."); + let log = env.get_logger(); + info!(log, "Building canisters..."); let build_mode_check = false; - let canister_pool = CanisterPool::load(env, build_mode_check, canister_names)?; + let canister_pool = CanisterPool::load(env, build_mode_check, required_canisters)?; canister_pool - .build_or_fail(&BuildConfig::from_config(config)?) + .build_or_fail(log, &BuildConfig::from_config(config)?, canisters_to_build) .await?; Ok(canister_pool) } From b756a2d28a35e425077f7aa104348a5285c9cb1a Mon Sep 17 00:00:00 2001 From: Severin Siffert Date: Thu, 13 Oct 2022 11:54:22 +0200 Subject: [PATCH 02/13] clean up debug prints, .did copy logic --- e2e/tests-dfx/remote.bash | 1 + src/dfx/src/commands/build.rs | 2 +- src/dfx/src/lib/builders/motoko.rs | 1 - src/dfx/src/lib/models/canister.rs | 81 ++++++++----------- .../operations/canister/deploy_canisters.rs | 13 +-- 5 files changed, 37 insertions(+), 61 deletions(-) diff --git a/e2e/tests-dfx/remote.bash b/e2e/tests-dfx/remote.bash index f0214c1ab3..b4d1028c8b 100644 --- a/e2e/tests-dfx/remote.bash +++ b/e2e/tests-dfx/remote.bash @@ -38,6 +38,7 @@ teardown() { # set up: remote method is update, local is query # call remote method as update to make a change assert_command dfx deploy --network actuallylocal -vv + # todo!("add test case") assert_command dfx canister call remote which_am_i --network actuallylocal assert_eq '("actual")' diff --git a/src/dfx/src/commands/build.rs b/src/dfx/src/commands/build.rs index de104c6a81..5a42e41520 100644 --- a/src/dfx/src/commands/build.rs +++ b/src/dfx/src/commands/build.rs @@ -54,7 +54,7 @@ pub fn exec(env: &dyn Environment, opts: CanisterBuildOpts) -> DfxResult { // Create canisters on the replica and associate canister ids locally. if build_mode_check { slog::warn!( - env.get_logger(), + logger, "Building canisters to check they build ok. Canister IDs might be hard coded." ); } else { diff --git a/src/dfx/src/lib/builders/motoko.rs b/src/dfx/src/lib/builders/motoko.rs index 4f5745e842..2b250c02f0 100644 --- a/src/dfx/src/lib/builders/motoko.rs +++ b/src/dfx/src/lib/builders/motoko.rs @@ -146,7 +146,6 @@ impl CanisterBuilder for MotokoBuilder { }; // Generate wasm - // todo!("add dependencies properly"); let params = MotokoParams { build_target: match profile { Profile::Release => BuildTarget::Release, diff --git a/src/dfx/src/lib/models/canister.rs b/src/dfx/src/lib/models/canister.rs index 070ef950d4..c501aea24f 100644 --- a/src/dfx/src/lib/models/canister.rs +++ b/src/dfx/src/lib/models/canister.rs @@ -122,12 +122,6 @@ impl CanisterPool { _ => None, }; let info = CanisterInfo::load(pool_helper.config, canister_name, canister_id)?; - println!( - "Inserting into pool: canister {}, cid {:?}, cid-info {}", - canister_name, - canister_id.map(|a| a.to_text()), - info.get_canister_id().map(|c| c.to_text())? - ); let builder = pool_helper.builder_pool.get(&info); pool_helper .canisters_map @@ -206,14 +200,6 @@ impl CanisterPool { let mut id_set: BTreeMap> = BTreeMap::new(); // Add all the canisters as nodes. - println!( - "All canisters in dependencies graph: {} canisters: {:?}", - self.canisters.len(), - self.canisters - .iter() - .map(|c| c.get_name()) - .collect::>() - ); for canister in &self.canisters { let canister_id = canister.info.get_canister_id()?; println!("Inserted id: {}", canister_id.to_text()); @@ -255,9 +241,40 @@ impl CanisterPool { #[context("Failed step_prebuild_all.")] fn step_prebuild_all( &self, + log: &Logger, build_config: &BuildConfig, canisters_to_build: &[String], ) -> DfxResult<()> { + // moc expects all .did files of dependencies to be in with name .did. + // Because remote canisters don't get built (and the did file not output in the right place) the .did files have to be copied over manually. + for canister in &self.canisters { + if !canisters_to_build.contains(&canister.info.get_name().to_string()) { + let maybe_from = + if let Some(remote_candid) = canister.info.get_remote_candid_if_remote() { + Some(remote_candid) + } else { + canister.info.get_output_idl_path() + }; + if maybe_from.is_some() && maybe_from.as_ref().unwrap().exists() { + let from = maybe_from.unwrap(); + let to = build_config.idl_root.join(format!( + "{}.did", + canister.info.get_canister_id()?.to_text() + )); + if std::fs::copy(&from, &to).is_err() { + warn!( + log, + "Failed to copy remote canister candid from {} to {}. This may produce errors during the build.", + from.to_string_lossy(), + to.to_string_lossy() + ); + } + } else { + warn!(log, "Failed to find a .did file for canister '{}'. Not providing that file may produce errors during the build.", canister.info.get_name()); + } + } + } + // cargo audit if self .canisters @@ -273,21 +290,6 @@ impl CanisterPool { ) } - // make .did files available for canisters that are not getting built - for canister in &self.canisters { - if !canisters_to_build.contains(&canister.info.get_name().to_string()) { - if canister.info.is_remote() { - println!("Copying did for canister {}.", canister.info.get_name()); - let to = build_config.idl_root.join(format!( - "{}.did", - canister.info.get_canister_id()?.to_text() - )); - std::fs::copy(canister.info.get_output_idl_path().unwrap(), to); - // .with_context(|| format!("Failed to copy remote candid from {} to {}.",)) - } - } - } - Ok(()) } @@ -435,19 +437,11 @@ impl CanisterPool { build_config: &BuildConfig, canisters_to_build: &[String], ) -> DfxResult>> { - self.step_prebuild_all(build_config, canisters_to_build) + // todo!("is canisters_to_build really necessary? can it be put into buildconfig?"); + self.step_prebuild_all(log, build_config, canisters_to_build) .map_err(|e| DfxError::new(BuildError::PreBuildAllStepFailed(Box::new(e))))?; let graph = self.build_dependencies_graph()?; - println!( - "Graph node count: {}, nodes: {:?}", - graph.node_count(), - graph - .raw_nodes() - .iter() - .map(|n| n.weight.to_text()) - .collect::>() - ); let nodes = petgraph::algo::toposort(&graph, None).map_err(|cycle| { let message = match graph.node_weight(cycle.node_id()) { Some(canister_id) => match self.get_canister_info(canister_id) { @@ -458,26 +452,19 @@ impl CanisterPool { }; BuildError::DependencyError(format!("Found circular dependency: {}", message)) })?; - println!("Nodes: {:?}", nodes); let order: Vec = nodes .iter() .rev() // Reverse the order, as we have a dependency graph, we want to reverse indices. .map(|idx| *graph.node_weight(*idx).unwrap()) .collect(); - println!( - "Canisters in order: {:?}", - order.iter().map(|cid| cid.to_text()).collect::>() - ); let mut result = Vec::new(); - println!("CAnisters to build: {:?}", canisters_to_build); for canister_id in &order { - println!("Looking at cid {}", canister_id); if let Some(canister) = self.get_canister(canister_id) { if canisters_to_build.contains(&canister.info.get_name().to_string()) { trace!(log, "Building canister '{}'.", canister.info.get_name()); } else { - println!("Not building {}", canister.info.get_name()); + trace!(log, "Not building canister '{}'.", canister.info.get_name()); continue; } result.push( diff --git a/src/dfx/src/lib/operations/canister/deploy_canisters.rs b/src/dfx/src/lib/operations/canister/deploy_canisters.rs index 47c01c76c9..cf6bf5b228 100644 --- a/src/dfx/src/lib/operations/canister/deploy_canisters.rs +++ b/src/dfx/src/lib/operations/canister/deploy_canisters.rs @@ -16,7 +16,7 @@ use ic_utils::interfaces::management_canister::attributes::{ ComputeAllocation, FreezingThreshold, MemoryAllocation, }; use ic_utils::interfaces::management_canister::builders::InstallMode; -use slog::{info, trace}; +use slog::info; use std::convert::TryFrom; use std::time::Duration; @@ -44,12 +44,6 @@ pub async fn deploy_canisters( let network = env.get_network_descriptor(); let canisters_to_load = canister_with_dependencies(&config, some_canister)?; - trace!( - log, - "Found {} canisters to load: {:?}", - canisters_to_load.len(), - &canisters_to_load - ); let canisters_to_deploy = if force_reinstall { // don't force-reinstall the dependencies too. @@ -77,11 +71,6 @@ pub async fn deploy_canisters( }) .collect() }; - trace!( - log, - "Found {} canisters to deploy.", - canisters_to_deploy.len() - ); if some_canister.is_some() { info!(log, "Deploying: {}", canisters_to_deploy.join(" ")); From 5316bd126d6209303c700b589a2993cba90bfdb9 Mon Sep 17 00:00:00 2001 From: Severin Siffert Date: Fri, 14 Oct 2022 11:07:59 +0200 Subject: [PATCH 03/13] move build_before_generate into BuildConfig --- e2e/tests-dfx/remote.bash | 16 ++- src/dfx/src/commands/build.rs | 26 +++-- src/dfx/src/commands/generate.rs | 38 +++++-- src/dfx/src/lib/builders/mod.rs | 10 ++ src/dfx/src/lib/models/canister.rs | 100 ++++++++---------- .../operations/canister/deploy_canisters.rs | 10 +- 6 files changed, 114 insertions(+), 86 deletions(-) diff --git a/e2e/tests-dfx/remote.bash b/e2e/tests-dfx/remote.bash index b4d1028c8b..69fd8d3eac 100644 --- a/e2e/tests-dfx/remote.bash +++ b/e2e/tests-dfx/remote.bash @@ -37,8 +37,7 @@ teardown() { # set up: remote method is update, local is query # call remote method as update to make a change - assert_command dfx deploy --network actuallylocal -vv - # todo!("add test case") + assert_command dfx deploy --network actuallylocal assert_command dfx canister call remote which_am_i --network actuallylocal assert_eq '("actual")' @@ -138,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 and canister install --all skip remote canisters" { +@test "canister create --all, canister install --all, dfx generate skip remote canisters" { install_asset remote/actual dfx_start setup_actuallylocal_shared_network @@ -175,8 +174,12 @@ teardown() { assert_eq '("mock")' assert_command dfx canister create --all --network actuallylocal - assert_command dfx build --network actuallylocal + assert_command dfx build --network actuallylocal -vv + assert_match "Not building canister 'remote'" assert_command dfx canister install --all --network actuallylocal + assert_command dfx generate --network actuallylocal + assert_match "Generating type declarations for canister basic" + assert_not_match "Generating type declarations for canister remote" assert_command dfx canister call basic read_remote --network actuallylocal assert_eq '("this is data in the remote canister")' @@ -267,6 +270,7 @@ teardown() { setup_actuallylocal_shared_network setup_local_shared_network jq ".canisters.remote.remote.id.actuallylocal=\"$REMOTE_CANISTER_ID\"" dfx.json | sponge dfx.json + cat dfx.json assert_command dfx deploy assert_command dfx canister call basic read_remote @@ -274,7 +278,9 @@ teardown() { assert_command dfx canister call remote which_am_i assert_eq '("mock")' - assert_command dfx deploy --network actuallylocal + echo "DEPLOY STARTS HERE" + assert_command dfx deploy --network actuallylocal -vv + assert_match "Not building canister 'remote'" assert_command dfx canister call basic read_remote --network actuallylocal assert_eq '("this is data in the remote canister")' diff --git a/src/dfx/src/commands/build.rs b/src/dfx/src/commands/build.rs index 5a42e41520..41cfe6a5c2 100644 --- a/src/dfx/src/commands/build.rs +++ b/src/dfx/src/commands/build.rs @@ -44,12 +44,24 @@ pub fn exec(env: &dyn Environment, opts: CanisterBuildOpts) -> DfxResult { let _all = opts.all; // Option can be None in which case --all was specified - let canister_names = config + let canisters_to_load = config .get_config() .get_canister_names_with_dependencies(opts.canister_name.as_deref())?; + let canisters_to_build = canisters_to_load + .clone() + .into_iter() + .filter(|canister_name| { + !matches!( + config + .get_config() + .get_remote_canister_id(canister_name, &env.get_network_descriptor().name), + Ok(Some(_)) + ) + }) + .collect(); // Get pool of canisters to build - let canister_pool = CanisterPool::load(&env, build_mode_check, &canister_names)?; + let canister_pool = CanisterPool::load(&env, build_mode_check, &canisters_to_load)?; // Create canisters on the replica and associate canister ids locally. if build_mode_check { @@ -70,12 +82,10 @@ pub fn exec(env: &dyn Environment, opts: CanisterBuildOpts) -> DfxResult { slog::info!(logger, "Building canisters..."); let runtime = Runtime::new().expect("Unable to create a runtime"); - let build_config = BuildConfig::from_config(&config)?.with_build_mode_check(build_mode_check); - runtime.block_on(canister_pool.build_or_fail( - logger, - &build_config, - &canister_names.as_slice(), - ))?; + let build_config = BuildConfig::from_config(&config)? + .with_build_mode_check(build_mode_check) + .with_canisters_to_build(canisters_to_build); + runtime.block_on(canister_pool.build_or_fail(logger, &build_config))?; Ok(()) } diff --git a/src/dfx/src/commands/generate.rs b/src/dfx/src/commands/generate.rs index df6e66f010..83d7b07345 100644 --- a/src/dfx/src/commands/generate.rs +++ b/src/dfx/src/commands/generate.rs @@ -32,12 +32,24 @@ pub fn exec(env: &dyn Environment, opts: GenerateOpts) -> DfxResult { env.get_cache().install()?; // Option can be None which means generate types for all canisters - let canister_names = config + let canisters_to_load = config .get_config() .get_canister_names_with_dependencies(opts.canister_name.as_deref())?; + let canisters_to_generate = canisters_to_load + .clone() + .into_iter() + .filter(|canister_name| { + !matches!( + config + .get_config() + .get_remote_canister_id(canister_name, &env.get_network_descriptor().name), + Ok(Some(_)) + ) + }) + .collect(); // Get pool of canisters to build - let canister_pool = CanisterPool::load(&env, false, &canister_names)?; + let canister_pool = CanisterPool::load(&env, false, &canisters_to_load)?; // This is just to display an error if trying to generate before creating the canister. let store = CanisterIdStore::for_env(&env)?; @@ -58,23 +70,27 @@ pub fn exec(env: &dyn Environment, opts: GenerateOpts) -> DfxResult { } } - let build_config = BuildConfig::from_config(&config)?; + let build_config = + BuildConfig::from_config(&config)?.with_canisters_to_build(build_before_generate); + let generate_config = + BuildConfig::from_config(&config)?.with_canisters_to_build(canisters_to_generate); - if !build_before_generate.is_empty() { + if !build_config + .canisters_to_build + .as_ref() + .map(|v| v.is_empty()) + .unwrap_or(true) + { slog::info!( env.get_logger(), "Building canisters before generate for Motoko" ); let runtime = Runtime::new().expect("Unable to create a runtime"); - runtime.block_on(canister_pool.build_or_fail( - env.get_logger(), - &build_config, - &build_before_generate, - ))?; + runtime.block_on(canister_pool.build_or_fail(env.get_logger(), &build_config))?; } - for canister in canister_pool.get_canister_list() { - canister.generate(&canister_pool, &build_config)?; + for canister in canister_pool.with_canisters_to_build(&generate_config) { + canister.generate(&canister_pool, &generate_config)?; } Ok(()) diff --git a/src/dfx/src/lib/builders/mod.rs b/src/dfx/src/lib/builders/mod.rs index bba486ff4f..589a71bb1c 100644 --- a/src/dfx/src/lib/builders/mod.rs +++ b/src/dfx/src/lib/builders/mod.rs @@ -365,6 +365,8 @@ pub struct BuildConfig { pub lsp_root: PathBuf, /// The root for all build files. pub build_root: PathBuf, + /// If only a subset of canisters should be built, then canisters_to_build contains these canisters's names. + pub canisters_to_build: Option>, } impl BuildConfig { @@ -382,6 +384,7 @@ impl BuildConfig { build_root: canister_root.clone(), idl_root: canister_root.join("idl/"), // TODO: possibly move to `network_root.join("idl/")` lsp_root: network_root.join("lsp/"), + canisters_to_build: None, }) } @@ -391,6 +394,13 @@ impl BuildConfig { ..self } } + + pub fn with_canisters_to_build(self, canisters: Vec) -> Self { + Self { + canisters_to_build: Some(canisters), + ..self + } + } } #[context("Failed to shrink wasm at {}.", &wasm_path.as_ref().display())] diff --git a/src/dfx/src/lib/models/canister.rs b/src/dfx/src/lib/models/canister.rs index c501aea24f..6c1dadecdb 100644 --- a/src/dfx/src/lib/models/canister.rs +++ b/src/dfx/src/lib/models/canister.rs @@ -202,7 +202,6 @@ impl CanisterPool { // Add all the canisters as nodes. for canister in &self.canisters { let canister_id = canister.info.get_canister_id()?; - println!("Inserted id: {}", canister_id.to_text()); id_set.insert(canister_id, graph.add_node(canister_id)); } @@ -239,47 +238,39 @@ impl CanisterPool { } #[context("Failed step_prebuild_all.")] - fn step_prebuild_all( - &self, - log: &Logger, - build_config: &BuildConfig, - canisters_to_build: &[String], - ) -> DfxResult<()> { + fn step_prebuild_all(&self, log: &Logger, build_config: &BuildConfig) -> DfxResult<()> { // moc expects all .did files of dependencies to be in with name .did. // Because remote canisters don't get built (and the did file not output in the right place) the .did files have to be copied over manually. for canister in &self.canisters { - if !canisters_to_build.contains(&canister.info.get_name().to_string()) { - let maybe_from = - if let Some(remote_candid) = canister.info.get_remote_candid_if_remote() { - Some(remote_candid) - } else { - canister.info.get_output_idl_path() - }; - if maybe_from.is_some() && maybe_from.as_ref().unwrap().exists() { - let from = maybe_from.unwrap(); - let to = build_config.idl_root.join(format!( - "{}.did", - canister.info.get_canister_id()?.to_text() - )); - if std::fs::copy(&from, &to).is_err() { - warn!( + let maybe_from = + if let Some(remote_candid) = canister.info.get_remote_candid_if_remote() { + Some(remote_candid) + } else { + canister.info.get_output_idl_path() + }; + if maybe_from.is_some() && maybe_from.as_ref().unwrap().exists() { + let from = maybe_from.unwrap(); + let to = build_config.idl_root.join(format!( + "{}.did", + canister.info.get_canister_id()?.to_text() + )); + if std::fs::copy(&from, &to).is_err() { + warn!( log, "Failed to copy remote canister candid from {} to {}. This may produce errors during the build.", from.to_string_lossy(), to.to_string_lossy() ); - } - } else { - warn!(log, "Failed to find a .did file for canister '{}'. Not providing that file may produce errors during the build.", canister.info.get_name()); } + } else { + warn!(log, "Failed to find a .did file for canister '{}'. Not providing that file may produce errors during the build.", canister.info.get_name()); } } // cargo audit if self - .canisters + .with_canisters_to_build(build_config) .iter() - .filter(|can| canisters_to_build.contains(&can.info.get_name().to_string())) .any(|can| can.info.is_rust()) { self.run_cargo_audit()?; @@ -408,16 +399,10 @@ impl CanisterPool { &self, build_config: &BuildConfig, _order: &[CanisterId], - canisters_to_build: &[String], ) -> DfxResult<()> { // We don't want to simply remove the whole directory, as in the future, // we may want to keep the IDL files downloaded from network. - for canister in self - .canisters - .iter() - .filter(|can| canisters_to_build.contains(&can.info.get_name().to_string())) - .collect::>>() - { + for canister in self.with_canisters_to_build(build_config) { let idl_root = &build_config.idl_root; let canister_id = canister.canister_id(); let idl_file_path = idl_root.join(canister_id.to_text()).with_extension("did"); @@ -435,10 +420,8 @@ impl CanisterPool { &self, log: &Logger, build_config: &BuildConfig, - canisters_to_build: &[String], ) -> DfxResult>> { - // todo!("is canisters_to_build really necessary? can it be put into buildconfig?"); - self.step_prebuild_all(log, build_config, canisters_to_build) + self.step_prebuild_all(log, build_config) .map_err(|e| DfxError::new(BuildError::PreBuildAllStepFailed(Box::new(e))))?; let graph = self.build_dependencies_graph()?; @@ -458,10 +441,15 @@ impl CanisterPool { .map(|idx| *graph.node_weight(*idx).unwrap()) .collect(); + let canister_names_to_build = self + .with_canisters_to_build(build_config) + .iter() + .map(|c| c.get_name()) + .collect::>(); let mut result = Vec::new(); for canister_id in &order { if let Some(canister) = self.get_canister(canister_id) { - if canisters_to_build.contains(&canister.info.get_name().to_string()) { + if canister_names_to_build.contains(&canister.get_name()) { trace!(log, "Building canister '{}'.", canister.info.get_name()); } else { trace!(log, "Not building canister '{}'.", canister.info.get_name()); @@ -500,7 +488,7 @@ impl CanisterPool { } } - self.step_postbuild_all(build_config, &order, canisters_to_build) + self.step_postbuild_all(build_config, &order) .map_err(|e| DfxError::new(BuildError::PostBuildAllStepFailed(Box::new(e))))?; Ok(result) @@ -509,14 +497,9 @@ impl CanisterPool { /// Build all canisters, failing with the first that failed the build. Will return /// nothing if all succeeded. #[context("Failed while trying to build all canisters.")] - pub async fn build_or_fail( - &self, - log: &Logger, - build_config: &BuildConfig, - canisters_to_build: &[String], - ) -> DfxResult<()> { - self.download(build_config, canisters_to_build).await?; - let outputs = self.build(log, build_config, canisters_to_build)?; + pub async fn build_or_fail(&self, log: &Logger, build_config: &BuildConfig) -> DfxResult<()> { + self.download(build_config).await?; + let outputs = self.build(log, build_config)?; for output in outputs { output.map_err(DfxError::new)?; @@ -525,16 +508,8 @@ impl CanisterPool { Ok(()) } - async fn download( - &self, - _build_config: &BuildConfig, - canisters_to_build: &[String], - ) -> DfxResult { - for canister in self - .canisters - .iter() - .filter(|can| canisters_to_build.contains(&can.info.get_name().to_string())) - { + async fn download(&self, build_config: &BuildConfig) -> DfxResult { + for canister in self.with_canisters_to_build(build_config) { let info = canister.get_info(); if info.is_custom() { @@ -596,6 +571,17 @@ impl CanisterPool { } Ok(()) } + + pub fn with_canisters_to_build(&self, build_config: &BuildConfig) -> Vec<&Arc> { + if let Some(canister_names) = &build_config.canisters_to_build { + self.canisters + .iter() + .filter(|can| canister_names.contains(&can.info.get_name().to_string())) + .collect() + } else { + self.canisters.iter().collect() + } + } } #[context("Failed to decode path to str.")] diff --git a/src/dfx/src/lib/operations/canister/deploy_canisters.rs b/src/dfx/src/lib/operations/canister/deploy_canisters.rs index cf6bf5b228..df0a1aeb7d 100644 --- a/src/dfx/src/lib/operations/canister/deploy_canisters.rs +++ b/src/dfx/src/lib/operations/canister/deploy_canisters.rs @@ -194,18 +194,18 @@ async fn register_canisters( #[context("Failed to build call canisters.")] async fn build_canisters( env: &dyn Environment, - required_canisters: &[String], + referenced_canisters: &[String], canisters_to_build: &[String], config: &Config, ) -> DfxResult { let log = env.get_logger(); info!(log, "Building canisters..."); let build_mode_check = false; - let canister_pool = CanisterPool::load(env, build_mode_check, required_canisters)?; + let canister_pool = CanisterPool::load(env, build_mode_check, referenced_canisters)?; - canister_pool - .build_or_fail(log, &BuildConfig::from_config(config)?, canisters_to_build) - .await?; + let build_config = + BuildConfig::from_config(config)?.with_canisters_to_build(canisters_to_build.into()); + canister_pool.build_or_fail(log, &build_config).await?; Ok(canister_pool) } From 434c9afa49e41cd5ae17ca0f74676b6a256574ee Mon Sep 17 00:00:00 2001 From: Severin Siffert Date: Fri, 14 Oct 2022 11:55:38 +0200 Subject: [PATCH 04/13] changelog --- CHANGELOG.md | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index 4aa10c2d5c..1100c4d26b 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -180,6 +180,12 @@ The Replica returned an error: code 1, message: "Canister requested a compute al ### fix: For `dfx start --clean --background`, the background process no longer cleans a second time. +### fix: do not build or generate remote canisters + +Canisters that specify a remote id for the network that's getting built falsely had their build steps run, blocking some normal use patterns of `dfx deploy`. +Canisters with a remote id specified no longer get built. +The same applies to `dfx generate`. + ### refactor: Move replica URL functions into a module for reuse The running replica port and url are generally useful information. Previously the code to get the URL was embedded in the network proxy code. This moves it out into a library for reuse. From 848b096f08b90e28415cfb4f36f69e9b7e6acc60 Mon Sep 17 00:00:00 2001 From: Severin Siffert Date: Fri, 14 Oct 2022 12:14:16 +0200 Subject: [PATCH 05/13] cleanup --- e2e/tests-dfx/remote.bash | 2 -- src/dfx/src/commands/build.rs | 1 - src/dfx/src/commands/generate.rs | 16 +++++++--------- src/dfx/src/lib/builders/mod.rs | 2 +- src/dfx/src/lib/models/canister.rs | 23 +++++++++++------------ 5 files changed, 19 insertions(+), 25 deletions(-) diff --git a/e2e/tests-dfx/remote.bash b/e2e/tests-dfx/remote.bash index 69fd8d3eac..4e9eafcd3d 100644 --- a/e2e/tests-dfx/remote.bash +++ b/e2e/tests-dfx/remote.bash @@ -270,7 +270,6 @@ teardown() { setup_actuallylocal_shared_network setup_local_shared_network jq ".canisters.remote.remote.id.actuallylocal=\"$REMOTE_CANISTER_ID\"" dfx.json | sponge dfx.json - cat dfx.json assert_command dfx deploy assert_command dfx canister call basic read_remote @@ -278,7 +277,6 @@ teardown() { assert_command dfx canister call remote which_am_i assert_eq '("mock")' - echo "DEPLOY STARTS HERE" assert_command dfx deploy --network actuallylocal -vv assert_match "Not building canister 'remote'" assert_command dfx canister call basic read_remote --network actuallylocal diff --git a/src/dfx/src/commands/build.rs b/src/dfx/src/commands/build.rs index 41cfe6a5c2..b8f7a34407 100644 --- a/src/dfx/src/commands/build.rs +++ b/src/dfx/src/commands/build.rs @@ -60,7 +60,6 @@ pub fn exec(env: &dyn Environment, opts: CanisterBuildOpts) -> DfxResult { }) .collect(); - // Get pool of canisters to build let canister_pool = CanisterPool::load(&env, build_mode_check, &canisters_to_load)?; // Create canisters on the replica and associate canister ids locally. diff --git a/src/dfx/src/commands/generate.rs b/src/dfx/src/commands/generate.rs index 83d7b07345..dc0743a590 100644 --- a/src/dfx/src/commands/generate.rs +++ b/src/dfx/src/commands/generate.rs @@ -23,6 +23,7 @@ pub struct GenerateOpts { pub fn exec(env: &dyn Environment, opts: GenerateOpts) -> DfxResult { let env = create_agent_environment(env, opts.network.network)?; + let log = env.get_logger(); // Read the config. let config = env.get_config_or_anyhow()?; @@ -48,11 +49,11 @@ pub fn exec(env: &dyn Environment, opts: GenerateOpts) -> DfxResult { }) .collect(); - // Get pool of canisters to build let canister_pool = CanisterPool::load(&env, false, &canisters_to_load)?; - // This is just to display an error if trying to generate before creating the canister. + // This is just to display an error if trying to generate before creating the canister(s). let store = CanisterIdStore::for_env(&env)?; + // If generate for motoko canister, build first let mut build_before_generate = Vec::new(); for canister in canister_pool.get_canister_list() { @@ -62,7 +63,7 @@ pub fn exec(env: &dyn Environment, opts: GenerateOpts) -> DfxResult { if info.is_motoko() { build_before_generate.push(canister_name.to_string()); trace!( - env.get_logger(), + log, "Found Motoko canister '{}' - will have to build before generating IDL.", canister_name ); @@ -81,15 +82,12 @@ pub fn exec(env: &dyn Environment, opts: GenerateOpts) -> DfxResult { .map(|v| v.is_empty()) .unwrap_or(true) { - slog::info!( - env.get_logger(), - "Building canisters before generate for Motoko" - ); + slog::info!(log, "Building canisters before generate for Motoko"); let runtime = Runtime::new().expect("Unable to create a runtime"); - runtime.block_on(canister_pool.build_or_fail(env.get_logger(), &build_config))?; + runtime.block_on(canister_pool.build_or_fail(log, &build_config))?; } - for canister in canister_pool.with_canisters_to_build(&generate_config) { + for canister in canister_pool.canisters_to_build(&generate_config) { canister.generate(&canister_pool, &generate_config)?; } diff --git a/src/dfx/src/lib/builders/mod.rs b/src/dfx/src/lib/builders/mod.rs index 589a71bb1c..067ff7bb63 100644 --- a/src/dfx/src/lib/builders/mod.rs +++ b/src/dfx/src/lib/builders/mod.rs @@ -365,7 +365,7 @@ pub struct BuildConfig { pub lsp_root: PathBuf, /// The root for all build files. pub build_root: PathBuf, - /// If only a subset of canisters should be built, then canisters_to_build contains these canisters's names. + /// If only a subset of canisters should be built, then canisters_to_build contains these canisters' names. pub canisters_to_build: Option>, } diff --git a/src/dfx/src/lib/models/canister.rs b/src/dfx/src/lib/models/canister.rs index 6c1dadecdb..7b96becf0a 100644 --- a/src/dfx/src/lib/models/canister.rs +++ b/src/dfx/src/lib/models/canister.rs @@ -241,13 +241,12 @@ impl CanisterPool { fn step_prebuild_all(&self, log: &Logger, build_config: &BuildConfig) -> DfxResult<()> { // moc expects all .did files of dependencies to be in with name .did. // Because remote canisters don't get built (and the did file not output in the right place) the .did files have to be copied over manually. - for canister in &self.canisters { - let maybe_from = - if let Some(remote_candid) = canister.info.get_remote_candid_if_remote() { - Some(remote_candid) - } else { - canister.info.get_output_idl_path() - }; + for canister in self.canisters.iter().filter(|c| c.info.is_remote()) { + let maybe_from = if let Some(remote_candid) = canister.info.get_remote_candid() { + Some(remote_candid) + } else { + canister.info.get_output_idl_path() + }; if maybe_from.is_some() && maybe_from.as_ref().unwrap().exists() { let from = maybe_from.unwrap(); let to = build_config.idl_root.join(format!( @@ -269,7 +268,7 @@ impl CanisterPool { // cargo audit if self - .with_canisters_to_build(build_config) + .canisters_to_build(build_config) .iter() .any(|can| can.info.is_rust()) { @@ -402,7 +401,7 @@ impl CanisterPool { ) -> DfxResult<()> { // We don't want to simply remove the whole directory, as in the future, // we may want to keep the IDL files downloaded from network. - for canister in self.with_canisters_to_build(build_config) { + for canister in self.canisters_to_build(build_config) { let idl_root = &build_config.idl_root; let canister_id = canister.canister_id(); let idl_file_path = idl_root.join(canister_id.to_text()).with_extension("did"); @@ -442,7 +441,7 @@ impl CanisterPool { .collect(); let canister_names_to_build = self - .with_canisters_to_build(build_config) + .canisters_to_build(build_config) .iter() .map(|c| c.get_name()) .collect::>(); @@ -509,7 +508,7 @@ impl CanisterPool { } async fn download(&self, build_config: &BuildConfig) -> DfxResult { - for canister in self.with_canisters_to_build(build_config) { + for canister in self.canisters_to_build(build_config) { let info = canister.get_info(); if info.is_custom() { @@ -572,7 +571,7 @@ impl CanisterPool { Ok(()) } - pub fn with_canisters_to_build(&self, build_config: &BuildConfig) -> Vec<&Arc> { + pub fn canisters_to_build(&self, build_config: &BuildConfig) -> Vec<&Arc> { if let Some(canister_names) = &build_config.canisters_to_build { self.canisters .iter() From 450f48c83ff23be48c5c60c402b22491a8abc19d Mon Sep 17 00:00:00 2001 From: Severin Siffert Date: Fri, 14 Oct 2022 13:39:05 +0200 Subject: [PATCH 06/13] appease clippy --- src/dfx/src/lib/models/canister.rs | 13 +++++++------ 1 file changed, 7 insertions(+), 6 deletions(-) diff --git a/src/dfx/src/lib/models/canister.rs b/src/dfx/src/lib/models/canister.rs index 7b96becf0a..d99257d370 100644 --- a/src/dfx/src/lib/models/canister.rs +++ b/src/dfx/src/lib/models/canister.rs @@ -13,6 +13,7 @@ use crate::lib::wasm::metadata::add_candid_service_metadata; use anyhow::{anyhow, bail, Context}; use candid::Principal as CanisterId; use fn_error_context::context; +use itertools::Itertools; use petgraph::graph::{DiGraph, NodeIndex}; use rand::{thread_rng, RngCore}; use slog::{error, info, trace, warn, Logger}; @@ -440,15 +441,15 @@ impl CanisterPool { .map(|idx| *graph.node_weight(*idx).unwrap()) .collect(); - let canister_names_to_build = self - .canisters_to_build(build_config) - .iter() - .map(|c| c.get_name()) - .collect::>(); + let canisters_to_build = self.canisters_to_build(build_config); let mut result = Vec::new(); for canister_id in &order { if let Some(canister) = self.get_canister(canister_id) { - if canister_names_to_build.contains(&canister.get_name()) { + if canisters_to_build + .iter() + .map(|c| c.get_name()) + .contains(&canister.get_name()) + { trace!(log, "Building canister '{}'.", canister.info.get_name()); } else { trace!(log, "Not building canister '{}'.", canister.info.get_name()); From 354bcbdb49a604ba729679fb6315f1004ec472ea Mon Sep 17 00:00:00 2001 From: Severin Siffert Date: Mon, 17 Oct 2022 11:13:57 +0200 Subject: [PATCH 07/13] Clarify meaning of BuildConfig::canisters_to_build --- src/dfx/src/lib/builders/mod.rs | 1 + 1 file changed, 1 insertion(+) diff --git a/src/dfx/src/lib/builders/mod.rs b/src/dfx/src/lib/builders/mod.rs index 067ff7bb63..f955d28c9b 100644 --- a/src/dfx/src/lib/builders/mod.rs +++ b/src/dfx/src/lib/builders/mod.rs @@ -366,6 +366,7 @@ pub struct BuildConfig { /// The root for all build files. pub build_root: PathBuf, /// If only a subset of canisters should be built, then canisters_to_build contains these canisters' names. + /// If all canisters should be built, then this is None. pub canisters_to_build: Option>, } From fa116ab94b7a095b6fd087dc905395e61e0538b0 Mon Sep 17 00:00:00 2001 From: Severin Siffert Date: Mon, 17 Oct 2022 11:16:49 +0200 Subject: [PATCH 08/13] Clarify logic to determine if mo canisters need to be built --- src/dfx/src/commands/generate.rs | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/src/dfx/src/commands/generate.rs b/src/dfx/src/commands/generate.rs index dc0743a590..ee93329fad 100644 --- a/src/dfx/src/commands/generate.rs +++ b/src/dfx/src/commands/generate.rs @@ -76,11 +76,11 @@ pub fn exec(env: &dyn Environment, opts: GenerateOpts) -> DfxResult { let generate_config = BuildConfig::from_config(&config)?.with_canisters_to_build(canisters_to_generate); - if !build_config + if build_config .canisters_to_build .as_ref() - .map(|v| v.is_empty()) - .unwrap_or(true) + .map(|v| !v.is_empty()) + .unwrap_or(false) { slog::info!(log, "Building canisters before generate for Motoko"); let runtime = Runtime::new().expect("Unable to create a runtime"); From ce5577d04447e7550036fb80d4a1087e86a09ed0 Mon Sep 17 00:00:00 2001 From: Severin Siffert Date: Mon, 17 Oct 2022 11:23:12 +0200 Subject: [PATCH 09/13] Print distinct warnings when remote .did file cannot be found vs not specified --- src/dfx/src/lib/models/canister.rs | 35 ++++++++++++++++-------------- 1 file changed, 19 insertions(+), 16 deletions(-) diff --git a/src/dfx/src/lib/models/canister.rs b/src/dfx/src/lib/models/canister.rs index d99257d370..7d483aedbc 100644 --- a/src/dfx/src/lib/models/canister.rs +++ b/src/dfx/src/lib/models/canister.rs @@ -248,22 +248,25 @@ impl CanisterPool { } else { canister.info.get_output_idl_path() }; - if maybe_from.is_some() && maybe_from.as_ref().unwrap().exists() { - let from = maybe_from.unwrap(); - let to = build_config.idl_root.join(format!( - "{}.did", - canister.info.get_canister_id()?.to_text() - )); - if std::fs::copy(&from, &to).is_err() { - warn!( - log, - "Failed to copy remote canister candid from {} to {}. This may produce errors during the build.", - from.to_string_lossy(), - to.to_string_lossy() - ); + if let Some(from) = maybe_from.as_ref() { + if from.exists() { + let to = build_config.idl_root.join(format!( + "{}.did", + canister.info.get_canister_id()?.to_text() + )); + if std::fs::copy(&from, &to).is_err() { + warn!( + log, + "Failed to copy remote canister candid from {} to {}. This may produce errors during the build.", + from.to_string_lossy(), + to.to_string_lossy() + ); + } + } else { + warn!(log, ".did file for canister '{}' does not exist at {}. This may result in errors during the build.", canister.get_name(), from.to_string_lossy()); } } else { - warn!(log, "Failed to find a .did file for canister '{}'. Not providing that file may produce errors during the build.", canister.info.get_name()); + warn!(log, "Failed to find a configured .did file for canister '{}'. Not specifying that field may result in errors during the build.", canister.get_name()); } } @@ -450,9 +453,9 @@ impl CanisterPool { .map(|c| c.get_name()) .contains(&canister.get_name()) { - trace!(log, "Building canister '{}'.", canister.info.get_name()); + trace!(log, "Building canister '{}'.", canister.get_name()); } else { - trace!(log, "Not building canister '{}'.", canister.info.get_name()); + trace!(log, "Not building canister '{}'.", canister.get_name()); continue; } result.push( From 368b295fefaf4f0f139dea9569c7c19a4e448e08 Mon Sep 17 00:00:00 2001 From: Severin Siffert Date: Mon, 17 Oct 2022 11:24:45 +0200 Subject: [PATCH 10/13] Fix warning message --- src/dfx/src/lib/models/canister.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/dfx/src/lib/models/canister.rs b/src/dfx/src/lib/models/canister.rs index 7d483aedbc..946a9382a2 100644 --- a/src/dfx/src/lib/models/canister.rs +++ b/src/dfx/src/lib/models/canister.rs @@ -257,7 +257,7 @@ impl CanisterPool { if std::fs::copy(&from, &to).is_err() { warn!( log, - "Failed to copy remote canister candid from {} to {}. This may produce errors during the build.", + "Failed to copy canister candid from {} to {}. This may produce errors during the build.", from.to_string_lossy(), to.to_string_lossy() ); From 6ebd5db7a50c4a78ded603a1c80de7aaca9b6a7c Mon Sep 17 00:00:00 2001 From: Severin Siffert Date: Mon, 17 Oct 2022 13:46:23 +0200 Subject: [PATCH 11/13] Add e2e for remote cid envvar --- e2e/assets/remote/envvar/dfx.json | 21 +++++++++++++++++++++ e2e/tests-dfx/remote.bash | 10 ++++++++++ 2 files changed, 31 insertions(+) create mode 100644 e2e/assets/remote/envvar/dfx.json diff --git a/e2e/assets/remote/envvar/dfx.json b/e2e/assets/remote/envvar/dfx.json new file mode 100644 index 0000000000..beb81e522a --- /dev/null +++ b/e2e/assets/remote/envvar/dfx.json @@ -0,0 +1,21 @@ +{ + "canisters": { + "basic": { + "type": "custom", + "candid": "main.did", + "wasm": "main.wasm", + "build": "bash -c 'echo \"CANISTER_ID_remote: $CANISTER_ID_remote\"'", + "dependencies": [ + "remote" + ] + }, + "remote": { + "main": "remote.mo", + "remote": { + "id": { + "actuallylocal": "qoctq-giaaa-aaaaa-aaaea-cai" + } + } + } + } +} \ No newline at end of file diff --git a/e2e/tests-dfx/remote.bash b/e2e/tests-dfx/remote.bash index 4e9eafcd3d..1f64dad483 100644 --- a/e2e/tests-dfx/remote.bash +++ b/e2e/tests-dfx/remote.bash @@ -302,3 +302,13 @@ teardown() { assert_command jq .remote canister_ids.json assert_eq "null" } + +@test "build step sets remote cid env var correctly" { + install_asset remote/envvar + install_asset wasm/identity # need to have some .did and .wasm files to point our custom canister to + dfx_start + setup_actuallylocal_shared_network + + assert_command dfx deploy --network actuallylocal -vv + assert_match "CANISTER_ID_remote: qoctq-giaaa-aaaaa-aaaea-cai" +} From 40732f9a478f6d813658e01afc1e125f9ef64262 Mon Sep 17 00:00:00 2001 From: Severin Siffert Date: Mon, 17 Oct 2022 13:47:15 +0200 Subject: [PATCH 12/13] wording --- e2e/tests-dfx/remote.bash | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/e2e/tests-dfx/remote.bash b/e2e/tests-dfx/remote.bash index 1f64dad483..03be8cd197 100644 --- a/e2e/tests-dfx/remote.bash +++ b/e2e/tests-dfx/remote.bash @@ -303,7 +303,7 @@ teardown() { assert_eq "null" } -@test "build step sets remote cid env var correctly" { +@test "build step sets remote cid environment variable correctly" { install_asset remote/envvar install_asset wasm/identity # need to have some .did and .wasm files to point our custom canister to dfx_start From 30112ef27449a654bf80182d841f6410dbb704ec Mon Sep 17 00:00:00 2001 From: Severin Siffert Date: Mon, 17 Oct 2022 14:33:05 +0200 Subject: [PATCH 13/13] last cleanup --- src/dfx/src/commands/build.rs | 10 ++++------ src/dfx/src/commands/generate.rs | 10 ++++------ .../src/lib/operations/canister/deploy_canisters.rs | 10 ++++------ 3 files changed, 12 insertions(+), 18 deletions(-) diff --git a/src/dfx/src/commands/build.rs b/src/dfx/src/commands/build.rs index 72bcaf8e56..5e12ef2f62 100644 --- a/src/dfx/src/commands/build.rs +++ b/src/dfx/src/commands/build.rs @@ -50,12 +50,10 @@ pub fn exec(env: &dyn Environment, opts: CanisterBuildOpts) -> DfxResult { .clone() .into_iter() .filter(|canister_name| { - !matches!( - config - .get_config() - .get_remote_canister_id(canister_name, &env.get_network_descriptor().name), - Ok(Some(_)) - ) + !config + .get_config() + .is_remote_canister(canister_name, &env.get_network_descriptor().name) + .unwrap_or(false) }) .collect(); diff --git a/src/dfx/src/commands/generate.rs b/src/dfx/src/commands/generate.rs index ee93329fad..891643ddcd 100644 --- a/src/dfx/src/commands/generate.rs +++ b/src/dfx/src/commands/generate.rs @@ -40,12 +40,10 @@ pub fn exec(env: &dyn Environment, opts: GenerateOpts) -> DfxResult { .clone() .into_iter() .filter(|canister_name| { - !matches!( - config - .get_config() - .get_remote_canister_id(canister_name, &env.get_network_descriptor().name), - Ok(Some(_)) - ) + !config + .get_config() + .is_remote_canister(canister_name, &env.get_network_descriptor().name) + .unwrap_or(false) }) .collect(); diff --git a/src/dfx/src/lib/operations/canister/deploy_canisters.rs b/src/dfx/src/lib/operations/canister/deploy_canisters.rs index df0a1aeb7d..d8337860dc 100644 --- a/src/dfx/src/lib/operations/canister/deploy_canisters.rs +++ b/src/dfx/src/lib/operations/canister/deploy_canisters.rs @@ -62,12 +62,10 @@ pub async fn deploy_canisters( .clone() .into_iter() .filter(|canister_name| { - !matches!( - config - .get_config() - .get_remote_canister_id(canister_name, &network.name), - Ok(Some(_)) - ) + !config + .get_config() + .is_remote_canister(canister_name, &env.get_network_descriptor().name) + .unwrap_or(false) }) .collect() };