From 2d87c0c2fcc47088feecc72721c46d8e07e3c220 Mon Sep 17 00:00:00 2001 From: evalir Date: Thu, 3 Aug 2023 16:35:44 -0400 Subject: [PATCH] fix(`forge`): Dedup remappings on build correctly & do not set context by default (#5532) * chore: dedup remappings properly on build * chore: tests * chore: do not use cwd * chore: use cwd again, but do not set context * chore: sort exactly * chore: fmt * chore: add docs for eventual context re-add * fmt * chore: update tests * chore: fix last test * chore: fix bounds --- cli/src/cmd/forge/build/core.rs | 4 ++-- cli/tests/it/config.rs | 32 +++++++++--------------------- config/src/providers/remappings.rs | 16 ++++++--------- 3 files changed, 17 insertions(+), 35 deletions(-) diff --git a/cli/src/cmd/forge/build/core.rs b/cli/src/cmd/forge/build/core.rs index 7e226c86f..d1db3fa44 100644 --- a/cli/src/cmd/forge/build/core.rs +++ b/cli/src/cmd/forge/build/core.rs @@ -152,8 +152,8 @@ impl<'a> From<&'a CoreBuildArgs> for Figment { let mut remappings = args.project_paths.get_remappings(); remappings .extend(figment.extract_inner::>("remappings").unwrap_or_default()); - remappings.sort_by(|a, b| a.name.cmp(&b.name)); - remappings.dedup_by(|a, b| a.name.eq(&b.name)); + remappings.sort_by(|a, b| (&a.context, &a.name).cmp(&(&b.context, &b.name))); + remappings.dedup_by(|a, b| (&a.context, &a.name).eq(&(&b.context, &b.name))); figment.merge(("remappings", remappings)).merge(args) } } diff --git a/cli/tests/it/config.rs b/cli/tests/it/config.rs index 106203ff3..442b10f98 100644 --- a/cli/tests/it/config.rs +++ b/cli/tests/it/config.rs @@ -144,18 +144,10 @@ forgetest_init!( let foundry_toml = prj.root().join(Config::FILE_NAME); assert!(foundry_toml.exists()); - let mut profile = Config::load_with_root(prj.root()); + let profile = Config::load_with_root(prj.root()); // ensure that the auto-generated internal remapping for forge-std's ds-test exists - assert_eq!(profile.remappings.len(), 3); - pretty_eq!( - "lib/forge-std:ds-test/=lib/forge-std/lib/ds-test/src/", - profile.remappings[2].to_string() - ); - - // remove the auto-generated remapping to compare with `forge config` since `forge config` - // does not include the auto-generated remappings - profile.remappings.remove(2); assert_eq!(profile.remappings.len(), 2); + pretty_eq!("ds-test/=lib/forge-std/lib/ds-test/src/", profile.remappings[0].to_string()); // ensure remappings contain test pretty_eq!("ds-test/=lib/forge-std/lib/ds-test/src/", profile.remappings[0].to_string()); @@ -433,7 +425,7 @@ forgetest_init!(can_detect_lib_foundry_toml, |prj: TestProject, mut cmd: TestCom "forge-std/=lib/forge-std/src/".parse().unwrap(), "nested-lib/=lib/nested-lib/src/".parse().unwrap(), // remapping is local to the lib - "lib/nested-lib:nested/=lib/nested-lib/lib/nested/".parse().unwrap(), + "nested/=lib/nested-lib/lib/nested/".parse().unwrap(), ] ); @@ -452,16 +444,14 @@ forgetest_init!(can_detect_lib_foundry_toml, |prj: TestProject, mut cmd: TestCom remappings, vec![ // local to the lib - "lib/nested-lib:another-lib/=lib/nested-lib/lib/another-lib/src/".parse().unwrap(), + "another-lib/=lib/nested-lib/lib/another-lib/src/".parse().unwrap(), // global "ds-test/=lib/forge-std/lib/ds-test/src/".parse().unwrap(), "forge-std/=lib/forge-std/src/".parse().unwrap(), "nested-lib/=lib/nested-lib/src/".parse().unwrap(), // remappings local to the lib - "lib/nested-lib:nested-twice/=lib/nested-lib/lib/another-lib/lib/nested-twice/" - .parse() - .unwrap(), - "lib/nested-lib:nested/=lib/nested-lib/lib/nested/".parse().unwrap(), + "nested-twice/=lib/nested-lib/lib/another-lib/lib/nested-twice/".parse().unwrap(), + "nested/=lib/nested-lib/lib/nested/".parse().unwrap(), ] ); @@ -473,18 +463,14 @@ forgetest_init!(can_detect_lib_foundry_toml, |prj: TestProject, mut cmd: TestCom remappings, vec![ // local to the lib - "lib/nested-lib:another-lib/=lib/nested-lib/lib/another-lib/custom-source-dir/" - .parse() - .unwrap(), + "another-lib/=lib/nested-lib/lib/another-lib/custom-source-dir/".parse().unwrap(), // global "ds-test/=lib/forge-std/lib/ds-test/src/".parse().unwrap(), "forge-std/=lib/forge-std/src/".parse().unwrap(), "nested-lib/=lib/nested-lib/src/".parse().unwrap(), // remappings local to the lib - "lib/nested-lib:nested-twice/=lib/nested-lib/lib/another-lib/lib/nested-twice/" - .parse() - .unwrap(), - "lib/nested-lib:nested/=lib/nested-lib/lib/nested/".parse().unwrap(), + "nested-twice/=lib/nested-lib/lib/another-lib/lib/nested-twice/".parse().unwrap(), + "nested/=lib/nested-lib/lib/nested/".parse().unwrap(), ] ); }); diff --git a/config/src/providers/remappings.rs b/config/src/providers/remappings.rs index 4601922cd..15666875b 100644 --- a/config/src/providers/remappings.rs +++ b/config/src/providers/remappings.rs @@ -165,16 +165,12 @@ impl<'a> RemappingsProvider<'a> { } } - let mut remappings = config - .remappings - .into_iter() - .map(Remapping::from) - .map(|mut r| { - // todo: windows/mac/linux - r.context = Some(lib.to_string_lossy().to_string()); - r - }) - .collect::>(); + // Eventually, we could set context for remappings at this location, + // taking into account the OS platform. We'll need to be able to handle nested + // contexts depending on dependencies for this to work. + // For now, we just leave the default context (none). + let mut remappings = + config.remappings.into_iter().map(Remapping::from).collect::>(); if let Some(r) = src_remapping { remappings.push(r);