Address review point for directories separator

Author: Felixoid

README.md

@@ -287,10 +287,10 @@ By default, sccache requires absolute paths to match for cache hits. To enable c
 export SCCACHE_BASEDIRS=/home/user/project
 ```
 
-You can also specify multiple base directories by separating them with `|` (pipe character). When multiple directories are provided, the longest matching prefix is used:
+You can also specify multiple base directories by separating them by `;` on Windows hosts and by `:` on any other. When multiple directories are provided, the longest matching prefix is used:
 
 ```bash
-export SCCACHE_BASEDIRS="/home/user/project|/home/user/workspace"
+export SCCACHE_BASEDIRS="/home/user/project:/home/user/workspace"
 ```
 
 Path matching is **case-insensitive** on Windows and **case-sensitive** on other operating systems.
@@ -301,7 +301,7 @@ This is similar to ccache's `CCACHE_BASEDIR` and helps when:
 * Multiple developers working with different username paths
 * Working with multiple project checkouts simultaneously
 
-**Note:** Only absolute paths are supported. Relative paths will be ignored with a warning.
+**Note:** Only absolute paths are supported. Relative paths will prevent server from start.
 
 You can also configure this in the sccache config file:
 

docs/Configuration.md

@@ -17,7 +17,7 @@ server_startup_timeout_ms = 10000
 # users in a shared environment.
 # When multiple paths are provided, the longest matching prefix
 # is applied.
-# 
+#
 # Path matching is case-insensitive on Windows and case-sensitive on other OSes.
 #
 # Example:
@@ -154,7 +154,7 @@ Note that some env variables may need sccache server restart to take effect.
 
 * `SCCACHE_ALLOW_CORE_DUMPS` to enable core dumps by the server
 * `SCCACHE_CONF` configuration file path
-* `SCCACHE_BASEDIRS` base directory (or directories) to strip from paths for cache key computation. This is similar to ccache's `CCACHE_BASEDIR` and enables cache hits across different absolute paths when compiling the same source code. Multiple directories can be separated by `|` (pipe character). When multiple directories are specified, the longest matching prefix is used. Path matching is **case-insensitive** on Windows and **case-sensitive** on other operating systems. Environment variable takes precedence over file configuration. Only absolute paths are supported; relative paths will be ignored with a warning.
+* `SCCACHE_BASEDIRS` base directory (or directories) to strip from paths for cache key computation. This is similar to ccache's `CCACHE_BASEDIR` and enables cache hits across different absolute paths when compiling the same source code. Multiple directories can be separated by `;` on Windows hosts and by `:` on any other. When multiple directories are specified, the longest matching prefix is used. Path matching is **case-insensitive** on Windows and **case-sensitive** on other operating systems. Environment variable takes precedence over file configuration. Only absolute paths are supported; relative paths will be ignored with a warning.
 * `SCCACHE_CACHED_CONF`
 * `SCCACHE_IDLE_TIMEOUT` how long the local daemon process waits for more client requests before exiting, in seconds. Set to `0` to run sccache permanently
 * `SCCACHE_STARTUP_NOTIFY` specify a path to a socket which will be used for server completion notification

src/config.rs

@@ -950,11 +950,17 @@ fn config_from_env() -> Result<EnvConfig> {
     };
 
     // ======= Base directory =======
-    // Support multiple paths separated by '|' (a character forbidden in paths)
+    // Support multiple paths separated by ';' on Windows and ':' on other platforms
+    // to match PATH behavior.
+    let split_symbol = if cfg!(target_os = "windows") {
+        ';'
+    } else {
+        ':'
+    };
     let basedirs = env::var_os("SCCACHE_BASEDIRS")
         .map(|s| {
             s.to_string_lossy()
-                .split('|')
+                .split(split_symbol)
                 .map(|p| PathBuf::from(p.trim()))
                 .filter(|p| !p.as_os_str().is_empty())
                 .collect()
@@ -1386,6 +1392,61 @@ fn config_overrides() {
 }
 
 #[test]
+#[cfg(target_os = "windows")]
+fn config_basedirs_overrides() {
+    use std::path::PathBuf;
+
+    // Test that env variable takes precedence over file config
+    let env_conf = EnvConfig {
+        cache: Default::default(),
+        basedirs: vec![PathBuf::from("C:/env/basedir")],
+    };
+
+    let file_conf = FileConfig {
+        cache: Default::default(),
+        dist: Default::default(),
+        server_startup_timeout_ms: None,
+        basedirs: vec![PathBuf::from("C:/file/basedir")],
+    };
+
+    let config = Config::from_env_and_file_configs(env_conf, file_conf).unwrap();
+    assert_eq!(config.basedirs, vec![PathBuf::from("C:\\env\\basedir")]);
+
+    // Test that file config is used when env is empty
+    let env_conf = EnvConfig {
+        cache: Default::default(),
+        basedirs: vec![],
+    };
+
+    let file_conf = FileConfig {
+        cache: Default::default(),
+        dist: Default::default(),
+        server_startup_timeout_ms: None,
+        basedirs: vec![PathBuf::from("C:/file/basedir")],
+    };
+
+    let config = Config::from_env_and_file_configs(env_conf, file_conf).unwrap();
+    assert_eq!(config.basedirs, vec![PathBuf::from("C:\\file\\basedir")]);
+
+    // Test that both empty results in empty
+    let env_conf = EnvConfig {
+        cache: Default::default(),
+        basedirs: vec![],
+    };
+
+    let file_conf = FileConfig {
+        cache: Default::default(),
+        dist: Default::default(),
+        server_startup_timeout_ms: None,
+        basedirs: vec![],
+    };
+
+    let config = Config::from_env_and_file_configs(env_conf, file_conf).unwrap();
+    assert_eq!(config.basedirs, Vec::<PathBuf>::new());
+}
+
+#[test]
+#[cfg(not(target_os = "windows"))]
 fn config_basedirs_overrides() {
     use std::path::PathBuf;
 
@@ -1439,6 +1500,7 @@ fn config_basedirs_overrides() {
 }
 
 #[test]
+#[cfg(not(target_os = "windows"))]
 fn test_deserialize_basedirs() {
     use std::path::PathBuf;
 
@@ -1482,6 +1544,7 @@ fn test_deserialize_basedirs_missing() {
 
 #[test]
 #[serial]
+#[cfg(not(target_os = "windows"))]
 fn test_env_basedirs_single() {
     use std::path::PathBuf;
 
@@ -1497,13 +1560,33 @@ fn test_env_basedirs_single() {
 
 #[test]
 #[serial]
+#[cfg(target_os = "windows")]
+fn test_env_basedirs_single() {
+    use std::path::PathBuf;
+
+    unsafe {
+        std::env::set_var("SCCACHE_BASEDIRS", "C:/home/user/project");
+    }
+    let config = config_from_env().unwrap();
+    assert_eq!(
+        config.basedirs,
+        vec![PathBuf::from("C:\\home\\user\\project")]
+    );
+    unsafe {
+        std::env::remove_var("SCCACHE_BASEDIRS");
+    }
+}
+
+#[test]
+#[serial]
+#[cfg(not(target_os = "windows"))]
 fn test_env_basedirs_multiple() {
     use std::path::PathBuf;
 
     unsafe {
         std::env::set_var(
             "SCCACHE_BASEDIRS",
-            "/home/user/project|/home/user/workspace",
+            "/home/user/project:/home/user/workspace",
         );
     }
     let config = config_from_env().unwrap();
@@ -1521,14 +1604,40 @@ fn test_env_basedirs_multiple() {
 
 #[test]
 #[serial]
+#[cfg(target_os = "windows")]
+fn test_env_basedirs_multiple() {
+    use std::path::PathBuf;
+
+    unsafe {
+        std::env::set_var(
+            "SCCACHE_BASEDIRS",
+            "C:/home/user/project;C:/home/user/workspace",
+        );
+    }
+    let config = config_from_env().unwrap();
+    assert_eq!(
+        config.basedirs,
+        vec![
+            PathBuf::from("C:\\home\\user\\project"),
+            PathBuf::from("C:\\home\\user\\workspace")
+        ]
+    );
+    unsafe {
+        std::env::remove_var("SCCACHE_BASEDIRS");
+    }
+}
+
+#[test]
+#[serial]
+#[cfg(not(target_os = "windows"))]
 fn test_env_basedirs_with_spaces() {
     use std::path::PathBuf;
 
     // Test that spaces around paths are trimmed
     unsafe {
         std::env::set_var(
             "SCCACHE_BASEDIRS",
-            " /home/user/project | /home/user/workspace ",
+            " /home/user/project : /home/user/workspace ",
         );
     }
     let config = config_from_env().unwrap();
@@ -1546,14 +1655,41 @@ fn test_env_basedirs_with_spaces() {
 
 #[test]
 #[serial]
+#[cfg(target_os = "windows")]
+fn test_env_basedirs_with_spaces() {
+    use std::path::PathBuf;
+
+    // Test that spaces around paths are trimmed
+    unsafe {
+        std::env::set_var(
+            "SCCACHE_BASEDIRS",
+            " C:/home/user/project ; C:/home/user/workspace ",
+        );
+    }
+    let config = config_from_env().unwrap();
+    assert_eq!(
+        config.basedirs,
+        vec![
+            PathBuf::from("C:\\home\\user\\project"),
+            PathBuf::from("C:\\home\\user\\workspace")
+        ]
+    );
+    unsafe {
+        std::env::remove_var("SCCACHE_BASEDIRS");
+    }
+}
+
+#[test]
+#[serial]
+#[cfg(not(target_os = "windows"))]
 fn test_env_basedirs_empty_entries() {
     use std::path::PathBuf;
 
     // Test that empty entries are filtered out
     unsafe {
         std::env::set_var(
             "SCCACHE_BASEDIRS",
-            "/home/user/project||/home/user/workspace",
+            "/home/user/project::/home/user/workspace",
         );
     }
     let config = config_from_env().unwrap();
@@ -1569,6 +1705,32 @@ fn test_env_basedirs_empty_entries() {
     }
 }
 
+#[test]
+#[serial]
+#[cfg(target_os = "windows")]
+fn test_env_basedirs_empty_entries() {
+    use std::path::PathBuf;
+
+    // Test that empty entries are filtered out
+    unsafe {
+        std::env::set_var(
+            "SCCACHE_BASEDIRS",
+            "c:/home/user/project;;c:/home/user/workspace",
+        );
+    }
+    let config = config_from_env().unwrap();
+    assert_eq!(
+        config.basedirs,
+        vec![
+            PathBuf::from("c:\\home\\user\\project"),
+            PathBuf::from("c:\\home\\user\\workspace")
+        ]
+    );
+    unsafe {
+        std::env::remove_var("SCCACHE_BASEDIRS");
+    }
+}
+
 #[test]
 #[serial]
 fn test_env_basedirs_not_set() {