From 5c81fee558d59379683219e78c1e77870dd9232b Mon Sep 17 00:00:00 2001 From: Michael Mueller Date: Tue, 20 Apr 2021 09:51:13 +0200 Subject: [PATCH 1/6] Add fix --- src/crate_metadata.rs | 21 ++++++++++++++++++++- 1 file changed, 20 insertions(+), 1 deletion(-) diff --git a/src/crate_metadata.rs b/src/crate_metadata.rs index 26bf7f5f..86dd7b6c 100644 --- a/src/crate_metadata.rs +++ b/src/crate_metadata.rs @@ -43,7 +43,26 @@ impl CrateMetadata { /// Parses the contract manifest and returns relevant metadata. pub fn collect(manifest_path: &ManifestPath) -> Result { let (metadata, root_package) = get_cargo_metadata(manifest_path)?; - let mut target_directory = metadata.target_directory.as_path().join("ink"); + let mut target_directory = metadata.target_directory.to_path_buf(); + + #[cfg(test)] + { + // Our CI uses `CARGO_TARGET_DIR` to overwrite the `target_directory` returned + // here (for caching purposes). We still want to ensure that each test builds + // to its own, unique folder, without interfering with other tests. + // Hence we append a hash of the `manifest_path`, which for tests typically + // contains a `tmp_dir`. + use impl_serde::serialize as serde_hex; + let hex = serde_hex::to_hex( + &codec::Encode::encode(&manifest_path.as_ref().display().to_string()), + false, + ); + target_directory = target_directory.join(hex); + } + + // We have some tests which check that the target director path always ends with `ink`, + // hence we can only append it after (possibly) having appended the hash. + target_directory = target_directory.join("ink"); // Normalize the package name. let package_name = root_package.name.replace("-", "_"); -- GitLab From 80cc4246c4f59af17368e1c6c1259b3eaeb94c07 Mon Sep 17 00:00:00 2001 From: Michael Mueller Date: Tue, 20 Apr 2021 12:11:05 +0200 Subject: [PATCH 2/6] Revert "Add fix" This reverts commit 5c81fee558d59379683219e78c1e77870dd9232b. --- src/crate_metadata.rs | 21 +-------------------- 1 file changed, 1 insertion(+), 20 deletions(-) diff --git a/src/crate_metadata.rs b/src/crate_metadata.rs index 86dd7b6c..26bf7f5f 100644 --- a/src/crate_metadata.rs +++ b/src/crate_metadata.rs @@ -43,26 +43,7 @@ impl CrateMetadata { /// Parses the contract manifest and returns relevant metadata. pub fn collect(manifest_path: &ManifestPath) -> Result { let (metadata, root_package) = get_cargo_metadata(manifest_path)?; - let mut target_directory = metadata.target_directory.to_path_buf(); - - #[cfg(test)] - { - // Our CI uses `CARGO_TARGET_DIR` to overwrite the `target_directory` returned - // here (for caching purposes). We still want to ensure that each test builds - // to its own, unique folder, without interfering with other tests. - // Hence we append a hash of the `manifest_path`, which for tests typically - // contains a `tmp_dir`. - use impl_serde::serialize as serde_hex; - let hex = serde_hex::to_hex( - &codec::Encode::encode(&manifest_path.as_ref().display().to_string()), - false, - ); - target_directory = target_directory.join(hex); - } - - // We have some tests which check that the target director path always ends with `ink`, - // hence we can only append it after (possibly) having appended the hash. - target_directory = target_directory.join("ink"); + let mut target_directory = metadata.target_directory.as_path().join("ink"); // Normalize the package name. let package_name = root_package.name.replace("-", "_"); -- GitLab From a322ae17260903a197693f5d070050d8ca304354 Mon Sep 17 00:00:00 2001 From: Michael Mueller Date: Tue, 20 Apr 2021 12:23:29 +0200 Subject: [PATCH 3/6] Generate unique contract names for tests --- src/cmd/build.rs | 51 +++++++++++++++++++-------------------------- src/cmd/metadata.rs | 8 ++----- src/cmd/new.rs | 20 +++++++++--------- src/util.rs | 26 +++++++++++++++++++++++ 4 files changed, 59 insertions(+), 46 deletions(-) diff --git a/src/cmd/build.rs b/src/cmd/build.rs index a0a19a76..c29f590a 100644 --- a/src/cmd/build.rs +++ b/src/cmd/build.rs @@ -590,8 +590,8 @@ pub(crate) fn execute( mod tests_ci_only { use super::{assert_compatible_ink_dependencies, check_wasm_opt_version_compatibility}; use crate::{ - cmd::{self, BuildCommand}, - util::tests::with_tmp_dir, + cmd::BuildCommand, + util::tests::{with_new_contract_project, with_tmp_dir}, workspace::Manifest, BuildArtifacts, ManifestPath, OptimizationPasses, UnstableFlags, UnstableOptions, Verbosity, VerbosityFlags, @@ -638,10 +638,7 @@ mod tests_ci_only { #[test] fn build_code_only() { - with_tmp_dir(|path| { - cmd::new::execute("new_project", Some(path)).expect("new project creation failed"); - let manifest_path = - ManifestPath::new(&path.join("new_project").join("Cargo.toml")).unwrap(); + with_new_contract_project(|manifest_path| { let res = super::execute( &manifest_path, Verbosity::Default, @@ -676,11 +673,9 @@ mod tests_ci_only { #[test] fn check_must_not_output_contract_artifacts_in_project_dir() { - with_tmp_dir(|path| { + with_new_contract_project(|manifest_path| { // given - cmd::new::execute("new_project", Some(path)).expect("new project creation failed"); - let project_dir = path.join("new_project"); - let manifest_path = ManifestPath::new(&project_dir.join("Cargo.toml")).unwrap(); + let project_dir = manifest_path.directory().expect("directory must exist"); // when super::execute( @@ -707,13 +702,14 @@ mod tests_ci_only { #[test] fn optimization_passes_from_cli_must_take_precedence_over_profile() { - with_tmp_dir(|path| { + with_new_contract_project(|manifest_path| { // given - cmd::new::execute("new_project", Some(path)).expect("new project creation failed"); - let cargo_toml_path = path.join("new_project").join("Cargo.toml"); - write_optimization_passes_into_manifest(&cargo_toml_path, OptimizationPasses::Three); + write_optimization_passes_into_manifest( + &manifest_path.clone().into(), + OptimizationPasses::Three, + ); let cmd = BuildCommand { - manifest_path: Some(cargo_toml_path), + manifest_path: Some(manifest_path.into()), build_artifact: BuildArtifacts::All, verbosity: VerbosityFlags::default(), unstable_options: UnstableOptions::default(), @@ -746,13 +742,14 @@ mod tests_ci_only { #[test] fn optimization_passes_from_profile_must_be_used() { - with_tmp_dir(|path| { + with_new_contract_project(|manifest_path| { // given - cmd::new::execute("new_project", Some(path)).expect("new project creation failed"); - let cargo_toml_path = path.join("new_project").join("Cargo.toml"); - write_optimization_passes_into_manifest(&cargo_toml_path, OptimizationPasses::Three); + write_optimization_passes_into_manifest( + &manifest_path.clone().into(), + OptimizationPasses::Three, + ); let cmd = BuildCommand { - manifest_path: Some(cargo_toml_path), + manifest_path: Some(manifest_path.into()), build_artifact: BuildArtifacts::All, verbosity: VerbosityFlags::default(), unstable_options: UnstableOptions::default(), @@ -785,12 +782,9 @@ mod tests_ci_only { #[test] fn project_template_dependencies_must_be_ink_compatible() { - with_tmp_dir(|path| { + with_new_contract_project(|manifest_path| { // given - cmd::new::execute("new_project", Some(path)).expect("new project creation failed"); - let cargo_toml_path = path.join("new_project").join("Cargo.toml"); - let manifest_path = - ManifestPath::new(&cargo_toml_path).expect("manifest path creation failed"); + // the manifest path // when let res = assert_compatible_ink_dependencies(&manifest_path, Verbosity::Default); @@ -803,12 +797,9 @@ mod tests_ci_only { #[test] fn detect_mismatching_parity_scale_codec_dependencies() { - with_tmp_dir(|path| { + with_new_contract_project(|manifest_path| { // given - cmd::new::execute("new_project", Some(path)).expect("new project creation failed"); - let cargo_toml_path = path.join("new_project").join("Cargo.toml"); - let manifest_path = - ManifestPath::new(&cargo_toml_path).expect("manifest path creation failed"); + // the manifest path // at the time of writing this test ink! already uses `parity-scale-codec` // in a version > 2, hence 1 is an incompatible version. diff --git a/src/cmd/metadata.rs b/src/cmd/metadata.rs index 4030e446..94cb4d10 100644 --- a/src/cmd/metadata.rs +++ b/src/cmd/metadata.rs @@ -225,7 +225,7 @@ fn blake2_hash(code: &[u8]) -> CodeHash { mod tests { use crate::cmd::metadata::blake2_hash; use crate::{ - cmd, crate_metadata::CrateMetadata, util::tests::with_tmp_dir, BuildArtifacts, + cmd, crate_metadata::CrateMetadata, util::tests::with_new_contract_project, BuildArtifacts, ManifestPath, OptimizationPasses, UnstableFlags, Verbosity, }; use contract_metadata::*; @@ -299,11 +299,7 @@ mod tests { #[test] fn generate_metadata() { env_logger::try_init().ok(); - with_tmp_dir(|path| { - cmd::new::execute("new_project", Some(path)).expect("new project creation failed"); - let working_dir = path.join("new_project"); - let manifest_path = ManifestPath::new(working_dir.join("Cargo.toml"))?; - + with_new_contract_project(|manifest_path| { // add optional metadata fields let mut test_manifest = TestContractManifest::new(manifest_path)?; test_manifest.add_package_value("description", "contract description".into())?; diff --git a/src/cmd/new.rs b/src/cmd/new.rs index 3c88c5b2..52d9caa4 100644 --- a/src/cmd/new.rs +++ b/src/cmd/new.rs @@ -108,12 +108,12 @@ where #[cfg(test)] mod tests { use super::*; - use crate::util::tests::with_tmp_dir; + use crate::util::tests::{with_new_contract_project, with_tmp_dir}; #[test] fn rejects_hyphenated_name() { - with_tmp_dir(|path| { - let result = execute("rejects-hyphenated-name", Some(path)); + with_new_contract_project(|manifest_path| { + let result = execute("rejects-hyphenated-name", Some(manifest_path)); assert!(result.is_err(), "Should fail"); assert_eq!( result.err().unwrap().to_string(), @@ -125,8 +125,8 @@ mod tests { #[test] fn rejects_name_with_period() { - with_tmp_dir(|path| { - let result = execute("../xxx", Some(path)); + with_new_contract_project(|manifest_path| { + let result = execute("../xxx", Some(manifest_path)); assert!(result.is_err(), "Should fail"); assert_eq!( result.err().unwrap().to_string(), @@ -138,8 +138,8 @@ mod tests { #[test] fn rejects_name_beginning_with_number() { - with_tmp_dir(|path| { - let result = execute("1xxx", Some(path)); + with_new_contract_project(|manifest_path| { + let result = execute("1xxx", Some(manifest_path)); assert!(result.is_err(), "Should fail"); assert_eq!( result.err().unwrap().to_string(), @@ -151,10 +151,10 @@ mod tests { #[test] fn contract_cargo_project_already_exists() { - with_tmp_dir(|path| { + with_new_contract_project(|manifest_path| { let name = "test_contract_cargo_project_already_exists"; - let _ = execute(name, Some(path)); - let result = execute(name, Some(path)); + let _ = execute(name, Some(manifest_path.clone())); + let result = execute(name, Some(manifest_path)); assert!(result.is_err(), "Should fail"); assert_eq!( diff --git a/src/util.rs b/src/util.rs index c701f0ab..789c43bb 100644 --- a/src/util.rs +++ b/src/util.rs @@ -104,8 +104,12 @@ macro_rules! maybe_println { #[cfg(test)] pub mod tests { + use crate::ManifestPath; use std::path::Path; + use std::sync::atomic::{AtomicU32, Ordering}; + /// Creates a temporary directory and passes the `tmp_dir` path to `f`. + /// Panics if `f` returns an `Err`. pub fn with_tmp_dir(f: F) where F: FnOnce(&Path) -> anyhow::Result<()>, @@ -118,4 +122,26 @@ pub mod tests { // catch test panics in order to clean up temp dir which will be very large f(tmp_dir.path()).expect("Error executing test with tmp dir") } + + /// Counter to generate unique project names in `with_new_contract_project`. + static COUNTER: AtomicU32 = AtomicU32::new(0); + + /// Creates a new contract into a temporary directory. The contract's + /// `ManifestPath` is passed into `f`. + pub fn with_new_contract_project(f: F) + where + F: FnOnce(ManifestPath) -> anyhow::Result<()>, + { + with_tmp_dir(|tmp_dir| { + let unique_name = format!("new_project_{}", COUNTER.load(Ordering::SeqCst)); + COUNTER.fetch_add(1, Ordering::SeqCst); + + crate::cmd::new::execute(&unique_name, Some(tmp_dir)) + .expect("new project creation failed"); + let working_dir = tmp_dir.join(unique_name); + let manifest_path = ManifestPath::new(working_dir.join("Cargo.toml"))?; + + f(manifest_path) + }) + } } -- GitLab From e56db789a8f08ef174b4884f28d4f5c10272a880 Mon Sep 17 00:00:00 2001 From: Michael Mueller Date: Tue, 20 Apr 2021 13:22:10 +0200 Subject: [PATCH 4/6] Fix test --- src/cmd/new.rs | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/src/cmd/new.rs b/src/cmd/new.rs index 52d9caa4..23eccd40 100644 --- a/src/cmd/new.rs +++ b/src/cmd/new.rs @@ -151,10 +151,10 @@ mod tests { #[test] fn contract_cargo_project_already_exists() { - with_new_contract_project(|manifest_path| { + with_tmp_dir(|path| { let name = "test_contract_cargo_project_already_exists"; - let _ = execute(name, Some(manifest_path.clone())); - let result = execute(name, Some(manifest_path)); + let _ = execute(name, Some(path)); + let result = execute(name, Some(path)); assert!(result.is_err(), "Should fail"); assert_eq!( -- GitLab From dd48efe60491c4fd58b174591a46c3e6f55fa572 Mon Sep 17 00:00:00 2001 From: Michael Mueller Date: Tue, 20 Apr 2021 14:59:02 +0200 Subject: [PATCH 5/6] Implement comments --- src/util.rs | 18 +++++++++++++++--- 1 file changed, 15 insertions(+), 3 deletions(-) diff --git a/src/util.rs b/src/util.rs index 789c43bb..9411f214 100644 --- a/src/util.rs +++ b/src/util.rs @@ -123,7 +123,20 @@ pub mod tests { f(tmp_dir.path()).expect("Error executing test with tmp dir") } - /// Counter to generate unique project names in `with_new_contract_project`. + /// Global counter to generate unique contract names in `with_new_contract_project`. + /// + /// We typically use `with_tmp_dir` to generate temporary folders to build contracts + /// in. But for caching purposes our CI uses `CARGO_TARGET_DIR` to overwrite the + /// target directory of any contract build. + /// This poses a problem since we still want to ensure that each test builds to its + /// own, unique target directory -- without interfering with the target directory of + /// other tests. In the past this has been a problem when a test tried to create a + /// contract with the same contract name as another test -- both were then build + /// into the same target directory, sometimes causing test failures for strange reasons. + /// + /// The fix we decided on is to append a unique number to each contract name which + /// is created. This `COUNTER` provides a global counter which is accessed by each test + /// (in each thread) to get the current `COUNTER` number and increase it afterwards. static COUNTER: AtomicU32 = AtomicU32::new(0); /// Creates a new contract into a temporary directory. The contract's @@ -133,8 +146,7 @@ pub mod tests { F: FnOnce(ManifestPath) -> anyhow::Result<()>, { with_tmp_dir(|tmp_dir| { - let unique_name = format!("new_project_{}", COUNTER.load(Ordering::SeqCst)); - COUNTER.fetch_add(1, Ordering::SeqCst); + let unique_name = format!("new_project_{}", COUNTER.fetch_add(1, Ordering::SeqCst)); crate::cmd::new::execute(&unique_name, Some(tmp_dir)) .expect("new project creation failed"); -- GitLab From 71d1d939bd7bb6bb375b5b140882ff1503ba98ba Mon Sep 17 00:00:00 2001 From: Michael Mueller Date: Tue, 20 Apr 2021 15:12:03 +0200 Subject: [PATCH 6/6] Implement comments --- src/util.rs | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/src/util.rs b/src/util.rs index 9411f214..71fcaa45 100644 --- a/src/util.rs +++ b/src/util.rs @@ -127,7 +127,8 @@ pub mod tests { /// /// We typically use `with_tmp_dir` to generate temporary folders to build contracts /// in. But for caching purposes our CI uses `CARGO_TARGET_DIR` to overwrite the - /// target directory of any contract build. + /// target directory of any contract build -- it is set to a fixed cache directory + /// instead. /// This poses a problem since we still want to ensure that each test builds to its /// own, unique target directory -- without interfering with the target directory of /// other tests. In the past this has been a problem when a test tried to create a @@ -137,6 +138,9 @@ pub mod tests { /// The fix we decided on is to append a unique number to each contract name which /// is created. This `COUNTER` provides a global counter which is accessed by each test /// (in each thread) to get the current `COUNTER` number and increase it afterwards. + /// + /// We decided to go for this counter instead of hashing (with e.g. the temp dir) to + /// prevent an infinite number of contract artifacts being created in the cache directory. static COUNTER: AtomicU32 = AtomicU32::new(0); /// Creates a new contract into a temporary directory. The contract's -- GitLab