debugger: Add program and module path fallbacks for debugpy toolchain (#40975)

Anthony Eid and Remco Smits created 2 days ago

Fixes the Debugpy toolchain detection bug in #40324 

When detecting what toolchain (venv) to use in the Debugpy configuration
stage, we used to only base it off of the current working directory
argument passed to the config. This is wrong behavior for cases like
mono repos, where the correct virtual environment to use is nested in
another folder.

This PR fixes this issue by adding the program and module fields as
fallbacks to check for virtual environments. We also added support for
program/module relative paths as well when cwd is not None.

Release Notes:

- debugger: Improve mono repo virtual environment detection with Debugpy

---------

Co-authored-by: Remco Smits <[e-mail]>

Change summary

crates/dap_adapters/src/dap_adapters.rs | 86 +++++++++++++-------------
crates/dap_adapters/src/python.rs       | 75 ++++++++++++++++-------
2 files changed, 96 insertions(+), 65 deletions(-)

Detailed changes

crates/dap_adapters/src/dap_adapters.rs

@@ -42,61 +42,63 @@ pub fn init(cx: &mut App) {
 }
 
 #[cfg(test)]
-struct MockDelegate {
-    worktree_root: PathBuf,
-}
+mod test_mocks {
+    use super::*;
 
-#[cfg(test)]
-impl MockDelegate {
-    fn new() -> Arc<dyn adapters::DapDelegate> {
-        Arc::new(Self {
-            worktree_root: PathBuf::from("/tmp/test"),
-        })
+    pub(crate) struct MockDelegate {
+        worktree_root: PathBuf,
     }
-}
 
-#[cfg(test)]
-#[async_trait::async_trait]
-impl adapters::DapDelegate for MockDelegate {
-    fn worktree_id(&self) -> settings::WorktreeId {
-        settings::WorktreeId::from_usize(0)
+    impl MockDelegate {
+        pub(crate) fn new() -> Arc<dyn adapters::DapDelegate> {
+            Arc::new(Self {
+                worktree_root: PathBuf::from("/tmp/test"),
+            })
+        }
     }
 
-    fn worktree_root_path(&self) -> &std::path::Path {
-        &self.worktree_root
-    }
+    #[async_trait::async_trait]
+    impl adapters::DapDelegate for MockDelegate {
+        fn worktree_id(&self) -> settings::WorktreeId {
+            settings::WorktreeId::from_usize(0)
+        }
 
-    fn http_client(&self) -> Arc<dyn http_client::HttpClient> {
-        unimplemented!("Not needed for tests")
-    }
+        fn worktree_root_path(&self) -> &std::path::Path {
+            &self.worktree_root
+        }
 
-    fn node_runtime(&self) -> node_runtime::NodeRuntime {
-        unimplemented!("Not needed for tests")
-    }
+        fn http_client(&self) -> Arc<dyn http_client::HttpClient> {
+            unimplemented!("Not needed for tests")
+        }
 
-    fn toolchain_store(&self) -> Arc<dyn language::LanguageToolchainStore> {
-        unimplemented!("Not needed for tests")
-    }
+        fn node_runtime(&self) -> node_runtime::NodeRuntime {
+            unimplemented!("Not needed for tests")
+        }
 
-    fn fs(&self) -> Arc<dyn fs::Fs> {
-        unimplemented!("Not needed for tests")
-    }
+        fn toolchain_store(&self) -> Arc<dyn language::LanguageToolchainStore> {
+            unimplemented!("Not needed for tests")
+        }
 
-    fn output_to_console(&self, _msg: String) {}
+        fn fs(&self) -> Arc<dyn fs::Fs> {
+            unimplemented!("Not needed for tests")
+        }
 
-    async fn which(&self, _command: &std::ffi::OsStr) -> Option<PathBuf> {
-        None
-    }
+        fn output_to_console(&self, _msg: String) {}
 
-    async fn read_text_file(&self, _path: &util::rel_path::RelPath) -> Result<String> {
-        Ok(String::new())
-    }
+        async fn which(&self, _command: &std::ffi::OsStr) -> Option<PathBuf> {
+            None
+        }
 
-    async fn shell_env(&self) -> collections::HashMap<String, String> {
-        collections::HashMap::default()
-    }
+        async fn read_text_file(&self, _path: &util::rel_path::RelPath) -> Result<String> {
+            Ok(String::new())
+        }
 
-    fn is_headless(&self) -> bool {
-        false
+        async fn shell_env(&self) -> collections::HashMap<String, String> {
+            collections::HashMap::default()
+        }
+
+        fn is_headless(&self) -> bool {
+            false
+        }
     }
 }

crates/dap_adapters/src/python.rs

@@ -824,29 +824,58 @@ impl DebugAdapter for PythonDebugAdapter {
                 .await;
         }
 
-        let base_path = config
-            .config
-            .get("cwd")
-            .and_then(|cwd| {
-                RelPath::new(
-                    cwd.as_str()
-                        .map(Path::new)?
-                        .strip_prefix(delegate.worktree_root_path())
-                        .ok()?,
-                    PathStyle::local(),
-                )
-                .ok()
+        let base_paths = ["cwd", "program", "module"]
+            .into_iter()
+            .filter_map(|key| {
+                config.config.get(key).and_then(|cwd| {
+                    RelPath::new(
+                        cwd.as_str()
+                            .map(Path::new)?
+                            .strip_prefix(delegate.worktree_root_path())
+                            .ok()?,
+                        PathStyle::local(),
+                    )
+                    .ok()
+                })
             })
-            .unwrap_or_else(|| RelPath::empty().into());
-        let toolchain = delegate
-            .toolchain_store()
-            .active_toolchain(
-                delegate.worktree_id(),
-                base_path.into_arc(),
-                language::LanguageName::new(Self::LANGUAGE_NAME),
-                cx,
+            .chain(
+                // While Debugpy's wiki saids absolute paths are required, but it actually supports relative paths when cwd is passed in.
+                // (Which should always be the case because Zed defaults to the cwd worktree root)
+                // So we want to check that these relative paths find toolchains as well. Otherwise, they won't be checked
+                // because the strip prefix in the iteration above will return an error
+                config
+                    .config
+                    .get("cwd")
+                    .map(|_| {
+                        ["program", "module"].into_iter().filter_map(|key| {
+                            config.config.get(key).and_then(|value| {
+                                let path = Path::new(value.as_str()?);
+                                RelPath::new(path, PathStyle::local()).ok()
+                            })
+                        })
+                    })
+                    .into_iter()
+                    .flatten(),
             )
-            .await;
+            .chain([RelPath::empty().into()]);
+
+        let mut toolchain = None;
+
+        for base_path in base_paths {
+            if let Some(found_toolchain) = delegate
+                .toolchain_store()
+                .active_toolchain(
+                    delegate.worktree_id(),
+                    base_path.into_arc(),
+                    language::LanguageName::new(Self::LANGUAGE_NAME),
+                    cx,
+                )
+                .await
+            {
+                toolchain = Some(found_toolchain);
+                break;
+            }
+        }
 
         self.fetch_debugpy_whl(toolchain.clone(), delegate)
             .await
@@ -914,7 +943,7 @@ mod tests {
 
         let result = adapter
             .get_installed_binary(
-                &MockDelegate::new(),
+                &test_mocks::MockDelegate::new(),
                 &task_def,
                 None,
                 None,
@@ -955,7 +984,7 @@ mod tests {
 
         let result_host = adapter
             .get_installed_binary(
-                &MockDelegate::new(),
+                &test_mocks::MockDelegate::new(),
                 &task_def_host,
                 None,
                 None,