From 6a241abdc9df94312ecb17638d8f22852d628708 Mon Sep 17 00:00:00 2001 From: Steve Lee Date: Sun, 15 Dec 2024 15:58:16 -0800 Subject: [PATCH 1/9] Fix `path()` function where first arg is a drive letter to add separator --- dsc_lib/src/functions/path.rs | 26 +++++++++++++++++++++++++- 1 file changed, 25 insertions(+), 1 deletion(-) diff --git a/dsc_lib/src/functions/path.rs b/dsc_lib/src/functions/path.rs index db439d6cf..79acceb26 100644 --- a/dsc_lib/src/functions/path.rs +++ b/dsc_lib/src/functions/path.rs @@ -31,9 +31,17 @@ impl Function for Path { debug!("Executing path function with args: {:?}", args); let mut path = PathBuf::new(); + let mut first = true; for arg in args { if let Value::String(s) = arg { - path.push(s); + // if first argument is a drive letter, add it with a separator suffix as PathBuf.push() doesn't add it + if first && s.len() == 2 && s.chars().nth(1).unwrap() == ':' { + path.push(s.to_owned() + std::path::MAIN_SEPARATOR.to_string().as_str()); + first = false; + continue; + } else { + path.push(s); + } } else { return Err(DscError::Parser("Arguments must all be strings".to_string())); } @@ -48,6 +56,22 @@ mod tests { use crate::configure::context::Context; use crate::parser::Statement; + #[test] + fn start_with_drive_letter() { + let mut parser = Statement::new().unwrap(); + let separator = std::path::MAIN_SEPARATOR; + let result = parser.parse_and_execute("[path('C:','test')]", &Context::new()).unwrap(); + assert_eq!(result, format!("C:{separator}test")); + } + + #[test] + fn drive_letter_in_middle() { + let mut parser = Statement::new().unwrap(); + let separator = std::path::MAIN_SEPARATOR; + let result = parser.parse_and_execute("[path('a','C:','test')]", &Context::new()).unwrap(); + assert_eq!(result, format!("a{separator}C:{separator}test")); + } + #[test] fn two_args() { let mut parser = Statement::new().unwrap(); From e5f6018a4ed23bd7c504ae188ad6df981379d0b0 Mon Sep 17 00:00:00 2001 From: Steve Lee Date: Sun, 15 Dec 2024 16:39:33 -0800 Subject: [PATCH 2/9] fix clippy and add unix absolute path test --- dsc_lib/src/functions/path.rs | 11 +++++++++-- 1 file changed, 9 insertions(+), 2 deletions(-) diff --git a/dsc_lib/src/functions/path.rs b/dsc_lib/src/functions/path.rs index 79acceb26..cea497dbc 100644 --- a/dsc_lib/src/functions/path.rs +++ b/dsc_lib/src/functions/path.rs @@ -39,9 +39,8 @@ impl Function for Path { path.push(s.to_owned() + std::path::MAIN_SEPARATOR.to_string().as_str()); first = false; continue; - } else { - path.push(s); } + path.push(s); } else { return Err(DscError::Parser("Arguments must all be strings".to_string())); } @@ -72,6 +71,14 @@ mod tests { assert_eq!(result, format!("a{separator}C:{separator}test")); } + #[test] + fn unix_absolute_path() { + let mut parser = Statement::new().unwrap(); + let separator = std::path::MAIN_SEPARATOR; + let result = parser.parse_and_execute("[path('/','a','b')]", &Context::new()).unwrap(); + assert_eq!(result, format!("/a{separator}b")); + } + #[test] fn two_args() { let mut parser = Statement::new().unwrap(); From 2222968b6e003721642bc2f0826856847ffd11f6 Mon Sep 17 00:00:00 2001 From: Steve Lee Date: Sun, 15 Dec 2024 18:27:53 -0800 Subject: [PATCH 3/9] fix test for Windows --- dsc_lib/src/functions/path.rs | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/dsc_lib/src/functions/path.rs b/dsc_lib/src/functions/path.rs index cea497dbc..4240ff5a9 100644 --- a/dsc_lib/src/functions/path.rs +++ b/dsc_lib/src/functions/path.rs @@ -68,6 +68,11 @@ mod tests { let mut parser = Statement::new().unwrap(); let separator = std::path::MAIN_SEPARATOR; let result = parser.parse_and_execute("[path('a','C:','test')]", &Context::new()).unwrap(); + + // if any part of the path is absolute, it replaces it instead of appending + #[cfg(target_os = "windows")] + assert_eq!(result, format!("C:{separator}test")); + #[cfg(not(target_os = "windows"))] assert_eq!(result, format!("a{separator}C:{separator}test")); } From 82210d1d3f9f03e7f04757f9c73b15b7faf17a79 Mon Sep 17 00:00:00 2001 From: Steve Lee Date: Mon, 16 Dec 2024 15:44:07 -0800 Subject: [PATCH 4/9] add more tests --- dsc_lib/src/functions/path.rs | 51 ++++++++++++++++++++++++++--------- 1 file changed, 39 insertions(+), 12 deletions(-) diff --git a/dsc_lib/src/functions/path.rs b/dsc_lib/src/functions/path.rs index 4240ff5a9..ee2428266 100644 --- a/dsc_lib/src/functions/path.rs +++ b/dsc_lib/src/functions/path.rs @@ -55,48 +55,75 @@ mod tests { use crate::configure::context::Context; use crate::parser::Statement; + const SEPARATOR: char = std::path::MAIN_SEPARATOR; + #[test] fn start_with_drive_letter() { let mut parser = Statement::new().unwrap(); - let separator = std::path::MAIN_SEPARATOR; let result = parser.parse_and_execute("[path('C:','test')]", &Context::new()).unwrap(); - assert_eq!(result, format!("C:{separator}test")); + assert_eq!(result, format!("C:{SEPARATOR}test")); } #[test] fn drive_letter_in_middle() { let mut parser = Statement::new().unwrap(); - let separator = std::path::MAIN_SEPARATOR; let result = parser.parse_and_execute("[path('a','C:','test')]", &Context::new()).unwrap(); - // if any part of the path is absolute, it replaces it instead of appending + // if any part of the path is absolute, it replaces it instead of appending on Windows + #[cfg(target_os = "windows")] + assert_eq!(result, format!("C:{SEPARATOR}test")); + + // non-Windows, the colon is a valid character in a path + #[cfg(not(target_os = "windows"))] + assert_eq!(result, format!("a{SEPARATOR}C:{SEPARATOR}test")); + } + + #[test] + fn multiple_drive_letters() { + let mut parser = Statement::new().unwrap(); + let result = parser.parse_and_execute("[path('C:','D:','test')]", &Context::new()).unwrap(); + + // if any part of the path is absolute, it replaces it instead of appending on Windows #[cfg(target_os = "windows")] - assert_eq!(result, format!("C:{separator}test")); + assert_eq!(result, format!("D:{SEPARATOR}test")); + + // non-Windows, the colon is a valid character in a path #[cfg(not(target_os = "windows"))] - assert_eq!(result, format!("a{separator}C:{separator}test")); + assert_eq!(result, format!("C:{SEPARATOR}D:{SEPARATOR}test")); + } + + #[test] + fn relative_path() { + let mut parser = Statement::new().unwrap(); + let result = parser.parse_and_execute("[path('a','..','b')]", &Context::new()).unwrap(); + assert_eq!(result, format!("a{SEPARATOR}..{SEPARATOR}b")); + } + + #[test] + fn path_segement_with_separator() { + let mut parser = Statement::new().unwrap(); + let result = parser.parse_and_execute(format!("[path('a','b{SEPARATOR}c')]").as_str(), &Context::new()).unwrap(); + assert_eq!(result, format!("a{SEPARATOR}b{SEPARATOR}c")); } #[test] fn unix_absolute_path() { let mut parser = Statement::new().unwrap(); - let separator = std::path::MAIN_SEPARATOR; let result = parser.parse_and_execute("[path('/','a','b')]", &Context::new()).unwrap(); - assert_eq!(result, format!("/a{separator}b")); + assert_eq!(result, format!("/a{SEPARATOR}b")); } #[test] fn two_args() { let mut parser = Statement::new().unwrap(); - let separator = std::path::MAIN_SEPARATOR; let result = parser.parse_and_execute("[path('a','b')]", &Context::new()).unwrap(); - assert_eq!(result, format!("a{separator}b")); + assert_eq!(result, format!("a{SEPARATOR}b")); } #[test] fn three_args() { let mut parser = Statement::new().unwrap(); - let separator = std::path::MAIN_SEPARATOR; let result = parser.parse_and_execute("[path('a','b','c')]", &Context::new()).unwrap(); - assert_eq!(result, format!("a{separator}b{separator}c")); + assert_eq!(result, format!("a{SEPARATOR}b{SEPARATOR}c")); } } From c49ef0458d11970ef9dbdce5b832b343e1a2a3f3 Mon Sep 17 00:00:00 2001 From: Steve Lee Date: Mon, 16 Dec 2024 16:09:21 -0800 Subject: [PATCH 5/9] fix windows test --- dsc_lib/src/functions/path.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/dsc_lib/src/functions/path.rs b/dsc_lib/src/functions/path.rs index ee2428266..40a2b8722 100644 --- a/dsc_lib/src/functions/path.rs +++ b/dsc_lib/src/functions/path.rs @@ -85,7 +85,7 @@ mod tests { // if any part of the path is absolute, it replaces it instead of appending on Windows #[cfg(target_os = "windows")] - assert_eq!(result, format!("D:{SEPARATOR}test")); + assert_eq!(result, format!("D:test")); // non-Windows, the colon is a valid character in a path #[cfg(not(target_os = "windows"))] From 8d63099082ab5098f239897b6e51716f1e1a8513 Mon Sep 17 00:00:00 2001 From: Steve Lee Date: Mon, 16 Dec 2024 17:01:12 -0800 Subject: [PATCH 6/9] remove handling in `path()` and ensure `systemRoot()` has trailing separator --- dsc/src/subcommand.rs | 7 ++++++- dsc_lib/src/configure/context.rs | 4 ++-- dsc_lib/src/functions/path.rs | 15 ++++----------- 3 files changed, 12 insertions(+), 14 deletions(-) diff --git a/dsc/src/subcommand.rs b/dsc/src/subcommand.rs index 25046a8cc..e9851424c 100644 --- a/dsc/src/subcommand.rs +++ b/dsc/src/subcommand.rs @@ -341,7 +341,12 @@ pub fn config(subcommand: &ConfigSubCommand, parameters: &Option, mounte exit(EXIT_INVALID_ARGS); } - configurator.set_system_root(path); + // make sure path has a trailing separator if it's a drive letter + if path.len() == 2 && path.chars().nth(1).unwrap_or(' ') == ':' { + configurator.set_system_root(&format!("{path}\\")); + } else { + configurator.set_system_root(path); + } } if let Err(err) = configurator.set_context(parameters.as_ref()) { diff --git a/dsc_lib/src/configure/context.rs b/dsc_lib/src/configure/context.rs index 49e90b65f..a2a1e79f0 100644 --- a/dsc_lib/src/configure/context.rs +++ b/dsc_lib/src/configure/context.rs @@ -45,9 +45,9 @@ impl Default for Context { #[cfg(target_os = "windows")] fn get_default_os_system_root() -> PathBuf { - // use SYSTEMDRIVE env var to get the default target path + // use SYSTEMDRIVE env var to get the default target path, append trailing separator let system_drive = std::env::var("SYSTEMDRIVE").unwrap_or_else(|_| "C:".to_string()); - PathBuf::from(system_drive) + PathBuf::from(system_drive + "\\") } #[cfg(not(target_os = "windows"))] diff --git a/dsc_lib/src/functions/path.rs b/dsc_lib/src/functions/path.rs index 40a2b8722..09d2dcf2c 100644 --- a/dsc_lib/src/functions/path.rs +++ b/dsc_lib/src/functions/path.rs @@ -31,15 +31,8 @@ impl Function for Path { debug!("Executing path function with args: {:?}", args); let mut path = PathBuf::new(); - let mut first = true; for arg in args { if let Value::String(s) = arg { - // if first argument is a drive letter, add it with a separator suffix as PathBuf.push() doesn't add it - if first && s.len() == 2 && s.chars().nth(1).unwrap() == ':' { - path.push(s.to_owned() + std::path::MAIN_SEPARATOR.to_string().as_str()); - first = false; - continue; - } path.push(s); } else { return Err(DscError::Parser("Arguments must all be strings".to_string())); @@ -60,14 +53,14 @@ mod tests { #[test] fn start_with_drive_letter() { let mut parser = Statement::new().unwrap(); - let result = parser.parse_and_execute("[path('C:','test')]", &Context::new()).unwrap(); + let result = parser.parse_and_execute("[path('C:\\','test')]", &Context::new()).unwrap(); assert_eq!(result, format!("C:{SEPARATOR}test")); } #[test] fn drive_letter_in_middle() { let mut parser = Statement::new().unwrap(); - let result = parser.parse_and_execute("[path('a','C:','test')]", &Context::new()).unwrap(); + let result = parser.parse_and_execute("[path('a','C:\\','test')]", &Context::new()).unwrap(); // if any part of the path is absolute, it replaces it instead of appending on Windows #[cfg(target_os = "windows")] @@ -81,11 +74,11 @@ mod tests { #[test] fn multiple_drive_letters() { let mut parser = Statement::new().unwrap(); - let result = parser.parse_and_execute("[path('C:','D:','test')]", &Context::new()).unwrap(); + let result = parser.parse_and_execute("[path('C:\\','D:\\','test')]", &Context::new()).unwrap(); // if any part of the path is absolute, it replaces it instead of appending on Windows #[cfg(target_os = "windows")] - assert_eq!(result, format!("D:test")); + assert_eq!(result, format!("D:\\test")); // non-Windows, the colon is a valid character in a path #[cfg(not(target_os = "windows"))] From 97dd6a9901d10b8aea89ef35b3809bd23a2dda5e Mon Sep 17 00:00:00 2001 From: Steve Lee Date: Mon, 16 Dec 2024 17:11:52 -0800 Subject: [PATCH 7/9] fix unit tests --- dsc_lib/src/functions/path.rs | 9 +++++++-- 1 file changed, 7 insertions(+), 2 deletions(-) diff --git a/dsc_lib/src/functions/path.rs b/dsc_lib/src/functions/path.rs index 09d2dcf2c..37ab14f28 100644 --- a/dsc_lib/src/functions/path.rs +++ b/dsc_lib/src/functions/path.rs @@ -54,7 +54,12 @@ mod tests { fn start_with_drive_letter() { let mut parser = Statement::new().unwrap(); let result = parser.parse_and_execute("[path('C:\\','test')]", &Context::new()).unwrap(); + + #[cfg(target_os = "windows")] assert_eq!(result, format!("C:{SEPARATOR}test")); + + #[cfg(not(target_os = "windows"))] + assert_eq!(result, format!("C:\\{SEPARATOR}test")); } #[test] @@ -68,7 +73,7 @@ mod tests { // non-Windows, the colon is a valid character in a path #[cfg(not(target_os = "windows"))] - assert_eq!(result, format!("a{SEPARATOR}C:{SEPARATOR}test")); + assert_eq!(result, format!("a{SEPARATOR}C:\\{SEPARATOR}test")); } #[test] @@ -82,7 +87,7 @@ mod tests { // non-Windows, the colon is a valid character in a path #[cfg(not(target_os = "windows"))] - assert_eq!(result, format!("C:{SEPARATOR}D:{SEPARATOR}test")); + assert_eq!(result, format!("C:\\{SEPARATOR}D:\\{SEPARATOR}test")); } #[test] From e7d708db51a9a571498d4b70faaca2415864311f Mon Sep 17 00:00:00 2001 From: Steve Lee Date: Mon, 16 Dec 2024 17:30:45 -0800 Subject: [PATCH 8/9] fix windows test --- dsc_lib/src/functions/system_root.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/dsc_lib/src/functions/system_root.rs b/dsc_lib/src/functions/system_root.rs index cf885c674..8e2ce017a 100644 --- a/dsc_lib/src/functions/system_root.rs +++ b/dsc_lib/src/functions/system_root.rs @@ -44,7 +44,7 @@ mod tests { let result = parser.parse_and_execute("[systemRoot()]", &Context::new()).unwrap(); // on windows, the default is SYSTEMDRIVE env var #[cfg(target_os = "windows")] - assert_eq!(result, std::env::var("SYSTEMDRIVE").unwrap()); + assert_eq!(result, format!("{}\\", std::env::var("SYSTEMDRIVE").unwrap())); // on linux/macOS, the default is / #[cfg(not(target_os = "windows"))] assert_eq!(result, "/"); From f17e266df1444632653d40061f80f930158e59b0 Mon Sep 17 00:00:00 2001 From: Steve Lee Date: Mon, 16 Dec 2024 18:02:35 -0800 Subject: [PATCH 9/9] fix pester test --- dsc/tests/dsc_functions.tests.ps1 | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/dsc/tests/dsc_functions.tests.ps1 b/dsc/tests/dsc_functions.tests.ps1 index 5754e3e76..e4f5bdb15 100644 --- a/dsc/tests/dsc_functions.tests.ps1 +++ b/dsc/tests/dsc_functions.tests.ps1 @@ -54,7 +54,7 @@ Describe 'tests for function expressions' { '@ $expected = if ($IsWindows) { - $env:SYSTEMDRIVE + $env:SYSTEMDRIVE + '\' } else { '/' }