From 79c6786d643bee3177543f9409278d28a6b972a6 Mon Sep 17 00:00:00 2001 From: Eric Swanson <64809312+ericswanson-dfinity@users.noreply.github.com> Date: Tue, 7 Nov 2023 12:15:05 -0800 Subject: [PATCH 1/2] fix: output .env file is now relative to project root, not cwd (#3435) dfx.json can specify `output_env_file` (the default for new projects is `".env"`), and some commands accept `--output-env-file .env` on the command line. If `dfx deploy`, `dfx build`, or `dfx canister install` were executed in a subdirectory of a project, they would create/read this file in that subdirectory, rather than the same directory as dfx.json (the project root). With this change, the location of the env file is taken to be relative to the project root, and furthermore must be contained within the project directory. Also fixed three places that could output "No such file or directory (OS error 2)" without telling which path wasn't found. Fixes: https://dfinity.atlassian.net/browse/SDK-1028 --- CHANGELOG.md | 6 ++++ e2e/tests-dfx/dotenv.bash | 37 ++++++++++++++++++++++++ src/dfx-core/src/config/model/dfinity.rs | 30 +++++++++++++++++++ src/dfx-core/src/error/config.rs | 16 ++++++++++ src/dfx/src/commands/build.rs | 4 +-- src/dfx/src/commands/canister/install.rs | 8 ++--- src/dfx/src/commands/deploy.rs | 4 +-- src/dfx/src/lib/builders/mod.rs | 8 ++--- 8 files changed, 97 insertions(+), 16 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 55e6e868d5..03fe44d2ed 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -2,6 +2,12 @@ # UNRELEASED +=== fix: output_env_file is now considered relative to project root + +The .env file location, whether specified as `output_env_file` in dfx.json +or `--output-env-file ` on the commandline, is now considered relative +to the project root, rather than relative to the current working directory. + === feat: Read dfx canister install argument from a file Enables passing large arguments that cannot be passed directly in the command line using the `--argument-file` flag. For example `dfx canister install --argument-file ./my/argument/file.txt my_canister_name`. diff --git a/e2e/tests-dfx/dotenv.bash b/e2e/tests-dfx/dotenv.bash index 1f6f2ab755..2f92e07d0e 100644 --- a/e2e/tests-dfx/dotenv.bash +++ b/e2e/tests-dfx/dotenv.bash @@ -14,6 +14,43 @@ teardown() { standard_teardown } + +@test "puts .env in project root" { + dfx_start + jq '.canisters["e2e_project_backend"].post_install="echo post install backend"' dfx.json | sponge dfx.json + jq '.canisters["e2e_project_frontend"].post_install="echo post install frontend"' dfx.json | sponge dfx.json + + mkdir subdir + mkdir subdir/canister-install-all subdir/canister-install-single + mkdir subdir/build-all subdir/build-single + mkdir subdir/deploy-single subdir/deploy-all + dfx canister create --all + ( cd subdir/build-single && dfx build e2e_project_frontend ) + ( cd subdir/build-all && dfx build --all ) + ( cd subdir/canister-install-single && dfx canister install e2e_project_backend ) + dfx canister uninstall-code e2e_project_backend + ( cd subdir/canister-install-all && dfx canister install --all ) + rm -rf .dfx + ( cd subdir/deploy-single && dfx deploy e2e_project_backend) + ( cd subdir/deploy-all && dfx deploy ) + + assert_command find . -name .env + assert_eq "./.env" +} + +@test "the output_env_file must be contained within project" { + dfx_start + mkdir ../outside + + assert_command_fail dfx deploy --output-env-file nonexistent/.env + assert_contains "failed to canonicalize output_env_file" + assert_contains "working-dir/e2e_project/nonexistent: No such file or directory" + assert_command_fail dfx deploy --output-env-file /etc/passwd + assert_contains "The output_env_file must be a relative path, but is /etc/passwd" + assert_command_fail dfx deploy --output-env-file ../outside/.env + assert_match "The output_env_file must be within the project root, but is .*/working-dir/e2e_project/../outside/.env" +} + @test "writes environment variables to .env" { dfx_start dfx canister create --all diff --git a/src/dfx-core/src/config/model/dfinity.rs b/src/dfx-core/src/config/model/dfinity.rs index e67d9a7c38..17c8b1d8a7 100644 --- a/src/dfx-core/src/config/model/dfinity.rs +++ b/src/dfx-core/src/config/model/dfinity.rs @@ -3,6 +3,7 @@ use crate::config::directories::get_user_dfx_config_dir; use crate::config::model::bitcoin_adapter::BitcoinAdapterLogLevel; use crate::config::model::canister_http_adapter::HttpAdapterLogLevel; +use crate::error::config::GetOutputEnvFileError; use crate::error::dfx_config::AddDependenciesError::CanisterCircularDependency; use crate::error::dfx_config::GetCanisterNamesWithDependenciesError::AddDependenciesFailed; use crate::error::dfx_config::GetComputeAllocationError::GetComputeAllocationFailed; @@ -1013,6 +1014,35 @@ impl Config { ) } + // returns the path to the output env file if any, guaranteed to be + // a child relative to the project root + pub fn get_output_env_file( + &self, + from_cmdline: Option, + ) -> Result, GetOutputEnvFileError> { + from_cmdline + .or(self.config.output_env_file.clone()) + .map(|p| { + if p.is_relative() { + let p = self.get_project_root().join(p); + + // cannot canonicalize a path that doesn't exist, but the parent should exist + let env_parent = + crate::fs::parent(&p).map_err(GetOutputEnvFileError::Parent)?; + let env_parent = crate::fs::canonicalize(&env_parent) + .map_err(GetOutputEnvFileError::Canonicalize)?; + if !env_parent.starts_with(self.get_project_root()) { + Err(GetOutputEnvFileError::OutputEnvFileMustBeInProjectRoot(p)) + } else { + Ok(self.get_project_root().join(p)) + } + } else { + Err(GetOutputEnvFileError::OutputEnvFileMustBeRelative(p)) + } + }) + .transpose() + } + pub fn save(&self) -> Result<(), StructuredFileError> { save_json_file(&self.path, &self.json) } diff --git a/src/dfx-core/src/error/config.rs b/src/dfx-core/src/error/config.rs index 1121332e42..1658ba575e 100644 --- a/src/dfx-core/src/error/config.rs +++ b/src/dfx-core/src/error/config.rs @@ -1,5 +1,6 @@ use crate::error::fs::FsError; use crate::error::get_user_home::GetUserHomeError; +use std::path::PathBuf; use thiserror::Error; #[derive(Error, Debug)] @@ -13,3 +14,18 @@ pub enum ConfigError { #[error("Failed to determine shared network data directory: {0}")] DetermineSharedNetworkDirectoryFailed(GetUserHomeError), } + +#[derive(Error, Debug)] +pub enum GetOutputEnvFileError { + #[error("failed to canonicalize output_env_file")] + Canonicalize(#[source] FsError), + + #[error("The output_env_file must be within the project root, but is {}", .0.display())] + OutputEnvFileMustBeInProjectRoot(PathBuf), + + #[error("The output_env_file must be a relative path, but is {}", .0.display())] + OutputEnvFileMustBeRelative(PathBuf), + + #[error(transparent)] + Parent(FsError), +} diff --git a/src/dfx/src/commands/build.rs b/src/dfx/src/commands/build.rs index d9abf3412b..22d58f3dcf 100644 --- a/src/dfx/src/commands/build.rs +++ b/src/dfx/src/commands/build.rs @@ -40,9 +40,7 @@ pub fn exec(env: &dyn Environment, opts: CanisterBuildOpts) -> DfxResult { // Read the config. let config = env.get_config_or_anyhow()?; - let env_file = opts - .output_env_file - .or_else(|| config.get_config().output_env_file.clone()); + let env_file = config.get_output_env_file(opts.output_env_file)?; // Check the cache. This will only install the cache if there isn't one installed // already. diff --git a/src/dfx/src/commands/canister/install.rs b/src/dfx/src/commands/canister/install.rs index 304c5eab69..0b01dd243d 100644 --- a/src/dfx/src/commands/canister/install.rs +++ b/src/dfx/src/commands/canister/install.rs @@ -155,9 +155,7 @@ pub async fn exec( } else { let canister_info = canister_info?; let config = config.unwrap(); - let env_file = opts - .output_env_file - .or_else(|| config.get_config().output_env_file.clone()); + let env_file = config.get_output_env_file(opts.output_env_file)?; let idl_path = canister_info.get_constructor_idl_path(); let init_type = get_candid_init_type(&idl_path); let install_args = || blob_from_arguments(arguments, None, arg_type, &init_type); @@ -182,9 +180,7 @@ pub async fn exec( } else if opts.all { // Install all canisters. let config = env.get_config_or_anyhow()?; - let env_file = opts - .output_env_file - .or_else(|| config.get_config().output_env_file.clone()); + let env_file = config.get_output_env_file(opts.output_env_file)?; if let Some(canisters) = &config.get_config().canisters { for canister in canisters.keys() { if pull_canisters_in_config.contains_key(canister) { diff --git a/src/dfx/src/commands/deploy.rs b/src/dfx/src/commands/deploy.rs index 7a429dcd85..5058ecb88f 100644 --- a/src/dfx/src/commands/deploy.rs +++ b/src/dfx/src/commands/deploy.rs @@ -115,9 +115,7 @@ pub fn exec(env: &dyn Environment, opts: DeployOpts) -> DfxResult { .map_err(|err| anyhow!(err)) .context("Failed to parse InstallMode.")?; let config = env.get_config_or_anyhow()?; - let env_file = opts - .output_env_file - .or_else(|| config.get_config().output_env_file.clone()); + let env_file = config.get_output_env_file(opts.output_env_file)?; let with_cycles = opts.with_cycles; diff --git a/src/dfx/src/lib/builders/mod.rs b/src/dfx/src/lib/builders/mod.rs index cfa2934da2..0a553ef1ff 100644 --- a/src/dfx/src/lib/builders/mod.rs +++ b/src/dfx/src/lib/builders/mod.rs @@ -448,7 +448,7 @@ fn write_environment_variables(vars: &[Env<'_>], write_path: &Path) -> DfxResult // the section is correctly formed let end_pos = end_pos + END_TAG.len() + start_pos + START_TAG.len(); existing_file.replace_range(start_pos..end_pos, &write_string); - fs::write(write_path, existing_file)?; + dfx_core::fs::write(write_path, existing_file)?; return Ok(()); } else { // the file has been edited, so we don't know how much to delete, so we append instead @@ -456,10 +456,10 @@ fn write_environment_variables(vars: &[Env<'_>], write_path: &Path) -> DfxResult } // append to the existing file existing_file.push_str(&write_string); - fs::write(write_path, existing_file)?; + dfx_core::fs::write(write_path, existing_file)?; } else { // no existing file, okay to clobber - fs::write(write_path, write_string)?; + dfx_core::fs::write(write_path, write_string)?; } Ok(()) } @@ -501,7 +501,7 @@ impl BuildConfig { idl_root: canister_root.join("idl/"), // TODO: possibly move to `network_root.join("idl/")` lsp_root: network_root.join("lsp/"), canisters_to_build: None, - env_file: config_intf.output_env_file.clone(), + env_file: config.get_output_env_file(None)?, }) } From 485426826886b8280dc4fab015d3084d04e6c234 Mon Sep 17 00:00:00 2001 From: Eric Swanson <64809312+ericswanson-dfinity@users.noreply.github.com> Date: Tue, 7 Nov 2023 12:50:22 -0800 Subject: [PATCH 2/2] fix: dfx extension install will no longer create a corrupt cache directory (#3436) Running `dfx extension install ` immediately after installing a new dfx version, or after `dfx cache delete`, would result in a cache directory that contained only an `extensions` subdirectory. Later, dfx would see that the cache directory exists and therefore not install it. Then, commands like `dfx start` or `dfx build` would fail due to missing files. Fixes: https://dfinity.atlassian.net/browse/SDK-1240 --- CHANGELOG.md | 8 ++++++++ e2e/tests-dfx/extension.bash | 6 ++++++ src/dfx/src/commands/extension/install.rs | 4 ++++ 3 files changed, 18 insertions(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index 03fe44d2ed..e459166cb6 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -2,6 +2,14 @@ # UNRELEASED +=== fix: dfx extension install can no longer create a corrupt cache directory + +Running `dfx cache delete && dfx extension install nns` would previously +create a cache directory containing only an `extensions` subdirectory. +dfx only looks for the existence of a cache version subdirectory to +determine whether it has been installed. The end result was that later +commands would fail when the cache did not contain expected files. + === fix: output_env_file is now considered relative to project root The .env file location, whether specified as `output_env_file` in dfx.json diff --git a/e2e/tests-dfx/extension.bash b/e2e/tests-dfx/extension.bash index f5688d51ef..bf2c627940 100644 --- a/e2e/tests-dfx/extension.bash +++ b/e2e/tests-dfx/extension.bash @@ -13,6 +13,12 @@ teardown() { standard_teardown } +@test "extension install with an empty cache does not create a corrupt cache" { + dfx cache delete + dfx extension install nns --version 0.2.1 + dfx_start +} + @test "install extension from official registry" { assert_command_fail dfx snsx diff --git a/src/dfx/src/commands/extension/install.rs b/src/dfx/src/commands/extension/install.rs index 6435915951..58e6b472d7 100644 --- a/src/dfx/src/commands/extension/install.rs +++ b/src/dfx/src/commands/extension/install.rs @@ -1,4 +1,5 @@ use crate::commands::DfxCommand; +use crate::config::cache::DiskBasedCache; use crate::lib::environment::Environment; use crate::lib::error::DfxResult; use clap::Parser; @@ -19,6 +20,9 @@ pub struct InstallOpts { } pub fn exec(env: &dyn Environment, opts: InstallOpts) -> DfxResult<()> { + // creating an `extensions` directory in an otherwise empty cache directory would + // cause the cache to be considered "installed" and later commands would fail + DiskBasedCache::install(&env.get_cache().version_str())?; let spinner = env.new_spinner(format!("Installing extension: {}", opts.name).into()); let mgr = env.new_extension_manager()?; let effective_extension_name = opts.install_as.clone().unwrap_or_else(|| opts.name.clone());