diff --git a/Cargo.lock b/Cargo.lock index 83eddeee..156473ad 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -470,6 +470,7 @@ dependencies = [ "regex", "serde", "serde_json", + "tempfile", "tracing", "tracing-subscriber", "winresource", diff --git a/crates/pet/Cargo.toml b/crates/pet/Cargo.toml index 375a5f1d..0afc7077 100644 --- a/crates/pet/Cargo.toml +++ b/crates/pet/Cargo.toml @@ -50,6 +50,7 @@ lazy_static = "1.4.0" [dev-dependencies] regex = "1.10.4" +tempfile = "3.10" [features] ci = [] diff --git a/crates/pet/src/find.rs b/crates/pet/src/find.rs index 16ae7377..30d9a631 100644 --- a/crates/pet/src/find.rs +++ b/crates/pet/src/find.rs @@ -162,7 +162,9 @@ pub fn find_and_report_envs( possible_environments.append( &mut reader .filter_map(Result::ok) - .filter(|d| d.file_type().is_ok_and(|f| f.is_dir())) + // Use path().is_dir() instead of file_type().is_dir() to follow symlinks + // See: https://github.com/microsoft/python-environment-tools/issues/196 + .filter(|d| d.path().is_dir()) .map(|p| p.path()) .collect(), ); @@ -285,7 +287,8 @@ pub fn find_python_environments_in_workspace_folder_recursive( if let Ok(reader) = fs::read_dir(workspace_folder.join(".pixi").join("envs")) { reader .filter_map(Result::ok) - .filter(|d| d.file_type().is_ok_and(|f| f.is_dir())) + // Use path().is_dir() instead of file_type().is_dir() to follow symlinks + .filter(|d| d.path().is_dir()) .map(|p| p.path()) .for_each(|p| paths_to_search_first.push(p)); } @@ -310,7 +313,8 @@ pub fn find_python_environments_in_workspace_folder_recursive( if let Ok(reader) = fs::read_dir(workspace_folder) { for folder in reader .filter_map(Result::ok) - .filter(|d| d.file_type().is_ok_and(|f| f.is_dir())) + // Use path().is_dir() instead of file_type().is_dir() to follow symlinks + .filter(|d| d.path().is_dir()) .map(|p| p.path()) .filter(|p| { // If this directory is a sub directory or is in the environment_directories, then do not search in this directory. @@ -431,3 +435,222 @@ pub fn identify_python_executables_using_locators( } } } + +#[cfg(test)] +mod tests { + use std::fs; + #[cfg(unix)] + use std::path::PathBuf; + use tempfile::TempDir; + + /// Test that `path().is_dir()` properly follows symlinks to directories. + /// This is the fix for https://github.com/microsoft/python-environment-tools/issues/196 + /// + /// The issue was that `DirEntry::file_type().is_dir()` returns false for symlinks + /// to directories on Unix, causing symlinked virtual environments to be missed. + #[test] + #[cfg(unix)] + fn test_symlinked_directory_is_detected() { + use std::os::unix::fs::symlink; + + // Create temporary directories + let tmp = TempDir::new().expect("Failed to create temp dir"); + let target_dir = tmp.path().join("actual_venv"); + let container_dir = tmp.path().join("envs"); + let symlink_dir = container_dir.join("linked_venv"); + + // Create the target directory (simulating a venv) + fs::create_dir_all(&target_dir).expect("Failed to create target dir"); + fs::create_dir_all(&container_dir).expect("Failed to create container dir"); + + // Create a symlink from envs/linked_venv -> actual_venv + symlink(&target_dir, &symlink_dir).expect("Failed to create symlink"); + + // Verify the symlink was created + assert!(symlink_dir.exists(), "Symlink should exist"); + + // Test that path().is_dir() follows the symlink + let entries: Vec<_> = fs::read_dir(&container_dir) + .expect("Failed to read dir") + .filter_map(Result::ok) + .collect(); + + assert_eq!(entries.len(), 1, "Should have one entry"); + + let entry = &entries[0]; + + // This is the OLD behavior that caused the bug: + // file_type().is_dir() does NOT follow symlinks + let file_type_is_dir = entry.file_type().is_ok_and(|ft| ft.is_dir()); + assert!( + !file_type_is_dir, + "file_type().is_dir() should return false for symlinks (this is the bug)" + ); + + // This is the NEW behavior that fixes the bug: + // path().is_dir() DOES follow symlinks + let path_is_dir = entry.path().is_dir(); + assert!( + path_is_dir, + "path().is_dir() should return true for symlinks to directories" + ); + } + + /// Test that regular directories still work with the fix + #[test] + fn test_regular_directory_is_detected() { + let tmp = TempDir::new().expect("Failed to create temp dir"); + let container_dir = tmp.path().join("envs"); + let sub_dir = container_dir.join("my_venv"); + + fs::create_dir_all(&sub_dir).expect("Failed to create dirs"); + + let entries: Vec<_> = fs::read_dir(&container_dir) + .expect("Failed to read dir") + .filter_map(Result::ok) + .filter(|d| d.path().is_dir()) + .collect(); + + assert_eq!(entries.len(), 1, "Should detect the regular directory"); + assert!( + entries[0].path().ends_with("my_venv"), + "Should be the my_venv directory" + ); + } + + /// Test that files are not incorrectly detected as directories + #[test] + fn test_file_is_not_detected_as_directory() { + let tmp = TempDir::new().expect("Failed to create temp dir"); + let container_dir = tmp.path().join("envs"); + let file_path = container_dir.join("some_file.txt"); + + fs::create_dir_all(&container_dir).expect("Failed to create dirs"); + fs::write(&file_path, "test content").expect("Failed to write file"); + + let dirs: Vec<_> = fs::read_dir(&container_dir) + .expect("Failed to read dir") + .filter_map(Result::ok) + .filter(|d| d.path().is_dir()) + .collect(); + + assert!(dirs.is_empty(), "Should not detect files as directories"); + } + + /// Test symlinked directory scenario matching the original issue: + /// User has ~/envs with symlinks to venvs in other locations + #[test] + #[cfg(unix)] + fn test_symlinked_venv_in_envs_directory() { + use std::os::unix::fs::symlink; + + let tmp = TempDir::new().expect("Failed to create temp dir"); + + // Simulate user's actual venv location + let project_dir = tmp.path().join("projects").join("myproject"); + let actual_venv = project_dir.join(".venv"); + + // Simulate ~/envs directory with symlink + let envs_dir = tmp.path().join("envs"); + let symlinked_venv = envs_dir.join("myproject_venv"); + + // Create the actual venv structure + fs::create_dir_all(actual_venv.join("bin")).expect("Failed to create venv"); + fs::write(actual_venv.join("bin").join("python"), "").expect("Failed to create python"); + fs::write(actual_venv.join("pyvenv.cfg"), "home = /usr/bin") + .expect("Failed to create pyvenv.cfg"); + + // Create envs directory with symlink + fs::create_dir_all(&envs_dir).expect("Failed to create envs dir"); + symlink(&actual_venv, &symlinked_venv).expect("Failed to create symlink"); + + // The fix ensures this symlinked directory is discovered + let discovered: Vec<_> = fs::read_dir(&envs_dir) + .expect("Failed to read envs dir") + .filter_map(Result::ok) + .filter(|d| d.path().is_dir()) // The fix: using path().is_dir() + .map(|d| d.path()) + .collect(); + + assert_eq!(discovered.len(), 1, "Should discover the symlinked venv"); + assert_eq!( + discovered[0], symlinked_venv, + "Should be the symlinked venv path" + ); + + // Verify it's actually a venv by checking for pyvenv.cfg + assert!( + discovered[0].join("pyvenv.cfg").exists(), + "Symlink should point to a valid venv" + ); + } + + /// CRITICAL TEST: Verify that path().is_dir() does NOT resolve symlinks to their target paths. + /// This ensures we use the symlink path (e.g., ~/envs/myenv) not the deep target path + /// (e.g., /some/deep/path/to/actual/venv). + /// + /// This is important because: + /// 1. Users expect to see the symlink path in their environment list + /// 2. We don't want to accidentally traverse into deep filesystem locations + /// 3. The symlink path is the "user-facing" path they configured + #[test] + #[cfg(unix)] + fn test_symlink_path_is_preserved_not_resolved() { + use std::os::unix::fs::symlink; + + let tmp = TempDir::new().expect("Failed to create temp dir"); + + // Create a "deep" target directory structure + let deep_target = tmp + .path() + .join("deep") + .join("nested") + .join("path") + .join("venv"); + fs::create_dir_all(&deep_target).expect("Failed to create deep target"); + + // Create a container with a symlink pointing to the deep target + let container_dir = tmp.path().join("envs"); + let symlink_path = container_dir.join("my_venv"); + fs::create_dir_all(&container_dir).expect("Failed to create container"); + symlink(&deep_target, &symlink_path).expect("Failed to create symlink"); + + // Get the discovered paths using the same pattern as our fix + let discovered: Vec = fs::read_dir(&container_dir) + .expect("Failed to read dir") + .filter_map(Result::ok) + .filter(|d| d.path().is_dir()) // This follows symlink to CHECK if it's a dir + .map(|d| d.path()) // But this returns the SYMLINK path, not the target + .collect(); + + assert_eq!(discovered.len(), 1); + + // CRITICAL: The path should be the symlink, NOT the resolved target + assert_eq!( + discovered[0], symlink_path, + "Should return the symlink path, not the deep target" + ); + + // Verify we did NOT get the deep target path + assert_ne!( + discovered[0], deep_target, + "Should NOT resolve to the deep target path" + ); + + // The path should NOT contain the deep nested structure + assert!( + !discovered[0].to_string_lossy().contains("deep/nested"), + "Path should not contain the deep nested target structure" + ); + + // Extra verification: fs::canonicalize WOULD resolve it (showing the difference) + // Note: We canonicalize both paths for comparison because on macOS /var is a + // symlink to /private/var, so canonicalize resolves that too. + let resolved = fs::canonicalize(&discovered[0]).expect("Should resolve"); + let canonical_target = fs::canonicalize(&deep_target).expect("Should resolve target"); + assert_eq!( + resolved, canonical_target, + "canonicalize() would resolve to target, but path() does not" + ); + } +}