apply reviewer's suggestions

dfinity · Aug 8, 2023 · 5951fe3 · 5951fe3
1 parent 515c2f2
commit 5951fe3
Show file tree

Hide file tree

Showing 6 changed files with 58 additions and 39 deletions.
diff --git 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<S, I>(
     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::<Vec<_>>();
+        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
@@ -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<S, I>(
     dfx_cache_path: &Path,
@@ -23,9 +26,16 @@ where
     S: AsRef<OsStr>,
 {
     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::<Vec<_>>();
+        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<String, DfxError> {
         .map(|c| *c as char)
         .collect::<String>();
     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
@@ -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<u8> {

diff --git 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
@@ -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
@@ -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();
     }
 }