From 5951fe3efed5af70f2eb0fe9b1d57874bdac48ce Mon Sep 17 00:00:00 2001 From: Marcin Nowak-Liebiediew Date: Tue, 8 Aug 2023 21:36:48 +0200 Subject: [PATCH] apply reviewer's suggestions --- extensions-utils/src/dependencies/call.rs | 14 +++++++-- extensions-utils/src/dependencies/dfx.rs | 16 ++++++++-- .../src/dependencies/download_ic_binaries.rs | 4 +-- extensions/nns/build.rs | 30 +++++++++---------- extensions/nns/src/install_nns.rs | 3 +- extensions/sns/build.rs | 30 +++++++++---------- 6 files changed, 58 insertions(+), 39 deletions(-) diff --git a/extensions-utils/src/dependencies/call.rs b/extensions-utils/src/dependencies/call.rs index 6257422..e68abf6 100644 --- a/extensions-utils/src/dependencies/call.rs +++ b/extensions-utils/src/dependencies/call.rs @@ -1,6 +1,7 @@ use anyhow::Context; use fn_error_context::context; use std::{ + env, ffi::OsStr, path::Path, process::{self, Command}, @@ -11,6 +12,8 @@ use std::{ /// # Returns /// - On success, returns stdout as a string. /// - On error, returns an error message including stdout and stderr. +/// +/// Does not stream stdout/stderr, and instead returns it after the process has exited. #[context("Calling {} CLI failed, or, it returned an error.", binary_name)] pub fn call_extension_bundled_binary( dfx_cache_path: &Path, @@ -30,9 +33,16 @@ where ) })?; let binary_to_call = extension_dir_path.join(binary_name); - let mut command = Command::new(&binary_to_call); + let mut command = Command::new(binary_to_call); // If extension's dependency calls dfx; it should call dfx in this dir. - command.env("PATH", dfx_cache_path.join("dfx")); + if let Some(path) = env::var_os("PATH") { + let mut paths = env::split_paths(&path).collect::>(); + paths.push(dfx_cache_path.to_path_buf()); + let new_path = env::join_paths(paths)?; + command.env("PATH", new_path); + } else { + command.env("PATH", dfx_cache_path); + } command.args(args); let output = command .stdin(process::Stdio::null()) diff --git a/extensions-utils/src/dependencies/dfx.rs b/extensions-utils/src/dependencies/dfx.rs index da42b09..265a4c4 100644 --- a/extensions-utils/src/dependencies/dfx.rs +++ b/extensions-utils/src/dependencies/dfx.rs @@ -3,6 +3,7 @@ use anyhow::Context; use fn_error_context::context; use semver::Version; +use std::env; use std::ffi::OsStr; use std::path::Path; use std::process::{self, Command}; @@ -12,6 +13,8 @@ use std::process::{self, Command}; /// # Returns /// - On success, returns stdout as a string. /// - On error, returns an error message including stdout and stderr. +/// +/// Does not stream stdout/stderr, and instead returns it after the process has exited. #[context("Calling {} CLI, or, it returned an error.", command)] pub fn call_dfx_bundled_binary( dfx_cache_path: &Path, @@ -23,9 +26,16 @@ where S: AsRef, { let binary = dfx_cache_path.join(command); - let mut command = Command::new(&binary); + let mut command = Command::new(binary); // If extension's dependency calls dfx; it should call dfx in this dir. - command.env("PATH", dfx_cache_path.join("dfx")); + if let Some(path) = env::var_os("PATH") { + let mut paths = env::split_paths(&path).collect::>(); + paths.push(dfx_cache_path.to_path_buf()); + let new_path = env::join_paths(paths)?; + command.env("PATH", new_path); + } else { + command.env("PATH", dfx_cache_path); + } command.args(args); let output = command .stdin(process::Stdio::null()) @@ -87,7 +97,7 @@ pub fn dfx_version(dfx_cache_path: &Path) -> Result { .map(|c| *c as char) .collect::(); if let Some(version) = version_cmd_output.split_whitespace().last() { - Version::parse(&version) // make sure the output is really a version + Version::parse(version) // make sure the output is really a version .map_err(DfxError::DfxVersionMalformed) .map(|v| v.to_string()) } else { diff --git a/extensions-utils/src/dependencies/download_ic_binaries.rs b/extensions-utils/src/dependencies/download_ic_binaries.rs index 5347323..287e99c 100644 --- a/extensions-utils/src/dependencies/download_ic_binaries.rs +++ b/extensions-utils/src/dependencies/download_ic_binaries.rs @@ -34,13 +34,13 @@ pub fn download_ic_binary(replica_rev: &str, binary_name: &str, destination_path let mut temp = fs::File::create(&temp_file).expect("Failed to create the file"); copy(&mut d, &mut temp).expect("Failed to copy content"); - fs::rename(temp_file, &destination_path).expect("Failed to move extension"); #[cfg(unix)] { use std::os::unix::fs::PermissionsExt; - dfx_core::fs::set_permissions(&destination_path, std::fs::Permissions::from_mode(0o500)) + dfx_core::fs::set_permissions(destination_path, std::fs::Permissions::from_mode(0o500)) .expect("Failed to set permissions"); } + fs::rename(temp_file, destination_path).expect("Failed to move extension"); } async fn download_bytes(url: &str) -> Vec { diff --git a/extensions/nns/build.rs b/extensions/nns/build.rs index fa87a96..1e9118c 100644 --- a/extensions/nns/build.rs +++ b/extensions/nns/build.rs @@ -1,3 +1,4 @@ +use std::env; use std::path::PathBuf; const REPLICA_REV: &str = "90e2799c255733409d0e61682685afcc2431c928"; @@ -11,26 +12,25 @@ const BINARY_DEPENDENCIES: &[(&str, &str)] = &[ fn main() { // keep copy of the dependency in the root of the project, so that cargo-dist will be able to package it into a tarball - let manifest_dir = PathBuf::from(env!("CARGO_MANIFEST_DIR")); - // and also in `target/debug` or `target/release` for development purposes (e.g. cargo run) - let target_dir = manifest_dir + let manifest_dir = PathBuf::from(env::var("CARGO_MANIFEST_DIR").unwrap()); + // and also in `target/debug` or `target/release` for development purposes (e.g. cargo run), this is a bit hacky: https://github.com/rust-lang/cargo/issues/9661 + let target_dir = PathBuf::from(std::env::var("OUT_DIR").unwrap()) .parent() .unwrap() .parent() .unwrap() - .join("target") - .join(std::env::var("PROFILE").unwrap()); + .parent() + .unwrap() + .to_path_buf(); for (binary_name, renamed_binary_name) in BINARY_DEPENDENCIES { - let destination_paths = ( - manifest_dir.join(renamed_binary_name), - target_dir.join(renamed_binary_name), - ); - dbg!(&destination_paths); - dfx_extensions_utils::download_ic_binary(REPLICA_REV, binary_name, &destination_paths.0); - if destination_paths.1.exists() { - std::fs::remove_file(&destination_paths.1).unwrap(); + let bin_in_manifest_dir = manifest_dir.join(renamed_binary_name); + let bin_in_target_dir = target_dir.join(renamed_binary_name); + dbg!(&bin_in_manifest_dir, &bin_in_target_dir); + dfx_extensions_utils::download_ic_binary(REPLICA_REV, binary_name, &bin_in_manifest_dir); + if bin_in_target_dir.exists() { + std::fs::remove_file(&bin_in_target_dir).unwrap(); } - std::fs::create_dir_all(target_dir).unwrap(); - std::fs::copy(destination_paths.0, destination_paths.1).unwrap(); + std::fs::create_dir_all(bin_in_target_dir.parent().unwrap()).unwrap(); + std::fs::copy(&bin_in_manifest_dir, &bin_in_target_dir).unwrap(); } } diff --git a/extensions/nns/src/install_nns.rs b/extensions/nns/src/install_nns.rs index a4ea1bd..5a29222 100644 --- a/extensions/nns/src/install_nns.rs +++ b/extensions/nns/src/install_nns.rs @@ -430,7 +430,6 @@ pub async fn ic_nns_init(opts: &IcNnsInitOpts, dfx_cache_path: &Path) -> anyhow: args.push(subnets.into()); } call_extension_bundled_binary(dfx_cache_path, "ic-nns-init", &args) - .with_context(|| format!("Error executing `ic-nns-init` with args: {:?}", args)) } /// Sets the exchange rate between ICP and cycles. @@ -506,7 +505,7 @@ pub fn upload_nns_sns_wasms_canister_wasms(dfx_cache_path: &Path) -> anyhow::Res ]; call_extension_bundled_binary(dfx_cache_path,"sns-cli", &args) .map_err(|e| anyhow!( - "Failed to upload {upload_name} from {wasm_path:?} to the nns-sns-wasm canister by calling `sns-cli` with args {args:?}: {e}" + "Failed to upload {upload_name} from {wasm_path:?} to the nns-sns-wasm canister by calling `sns-cli`: {e}" ))?; } Ok(()) diff --git a/extensions/sns/build.rs b/extensions/sns/build.rs index 430f351..baa09d2 100644 --- a/extensions/sns/build.rs +++ b/extensions/sns/build.rs @@ -1,3 +1,4 @@ +use std::env; use std::path::PathBuf; const REPLICA_REV: &str = "90e2799c255733409d0e61682685afcc2431c928"; @@ -9,26 +10,25 @@ const BINARY_DEPENDENCIES: &[(&str, &str)] = &[ fn main() { // keep copy of the dependency in the root of the project, so that cargo-dist will be able to package it into a tarball - let manifest_dir = PathBuf::from(env!("CARGO_MANIFEST_DIR")); - // and also in `target/debug` or `target/release` for development purposes (e.g. cargo run) - let target_dir = manifest_dir + let manifest_dir = PathBuf::from(env::var("CARGO_MANIFEST_DIR").unwrap()); + // and also in `target/debug` or `target/release` for development purposes (e.g. cargo run), this is a bit hacky: https://github.com/rust-lang/cargo/issues/9661 + let target_dir = PathBuf::from(std::env::var("OUT_DIR").unwrap()) .parent() .unwrap() .parent() .unwrap() - .join("target") - .join(std::env::var("PROFILE").unwrap()); + .parent() + .unwrap() + .to_path_buf(); for (binary_name, renamed_binary_name) in BINARY_DEPENDENCIES { - let destination_paths = ( - manifest_dir.join(renamed_binary_name), - target_dir.join(renamed_binary_name), - ); - dbg!(&destination_paths); - dfx_extensions_utils::download_ic_binary(REPLICA_REV, binary_name, &destination_paths.0); - if destination_paths.1.exists() { - std::fs::remove_file(&destination_paths.1).unwrap(); + let bin_in_manifest_dir = manifest_dir.join(renamed_binary_name); + let bin_in_target_dir = target_dir.join(renamed_binary_name); + dbg!(&bin_in_manifest_dir, &bin_in_target_dir); + dfx_extensions_utils::download_ic_binary(REPLICA_REV, binary_name, &bin_in_manifest_dir); + if bin_in_target_dir.exists() { + std::fs::remove_file(&bin_in_target_dir).unwrap(); } - std::fs::create_dir_all(destination_paths.1.parent().unwrap()).unwrap(); - std::fs::copy(destination_paths.0, destination_paths.1).unwrap(); + std::fs::create_dir_all(bin_in_target_dir.parent().unwrap()).unwrap(); + std::fs::copy(&bin_in_manifest_dir, &bin_in_target_dir).unwrap(); } }