Allow relative paths and environment variables in all editable repres…

…entations (#1000)

## Summary

I don't know if this is actually a good change, but it tries to make the
editable install experience more consistent. Specifically, we now
support...

```
# Use a relative path with a `file://` prefix.
# Prior to this PR, we supported `file:../foo`, but not `file://../foo`, which felt inconsistent.
-e file://../foo

# Use environment variables with paths, not just URLs.
# Prior to this PR, we supported `file://${PROJECT_ROOT}/../foo`, but not the below.
-e ${PROJECT_ROOT}/../foo
```

Importantly, `-e file://../foo` is actually not supported by pip... `-e
file:../foo` _is_ supported though. We support both, as of this PR. Open
to feedback.

crates/distribution-types/src/editable.rs

@@ -1,45 +1,40 @@
 use std::borrow::Cow;
-use std::path::{Path, PathBuf};
+use std::path::PathBuf;
 
 use url::Url;
 
 use pep508_rs::VerbatimUrl;
-use requirements_txt::EditableRequirement;
 
 use crate::Verbatim;
 
 #[derive(Debug, Clone)]
 pub struct LocalEditable {
-    pub requirement: EditableRequirement,
+    /// The underlying [`EditableRequirement`] from the `requirements.txt` file.
+    pub url: VerbatimUrl,
     /// Either the path to the editable or its checkout.
     pub path: PathBuf,
 }
 
 impl LocalEditable {
-    /// Return the [`VerbatimUrl`] of the editable.
+    /// Return the editable as a [`Url`].
     pub fn url(&self) -> &VerbatimUrl {
-        self.requirement.url()
-    }
-
-    /// Return the underlying [`Url`] of the editable.
-    pub fn raw(&self) -> &Url {
-        self.requirement.raw()
+        &self.url
     }
 
     /// Return the resolved path to the editable.
-    pub fn path(&self) -> &Path {
-        &self.path
+    pub fn raw(&self) -> &Url {
+        self.url.raw()
     }
 }
 
 impl Verbatim for LocalEditable {
     fn verbatim(&self) -> Cow<'_, str> {
-        self.url().verbatim()
+        self.url.verbatim()
     }
 }
 
 impl std::fmt::Display for LocalEditable {
     fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result {
-        self.requirement.fmt(f)
+        std::fmt::Display::fmt(&self.url, f)
     }
 }

crates/distribution-types/src/lib.rs

@@ -43,7 +43,6 @@ use distribution_filename::{DistFilename, SourceDistFilename, WheelFilename};
 use pep440_rs::Version;
 use pep508_rs::VerbatimUrl;
 use puffin_normalize::PackageName;
-use requirements_txt::EditableRequirement;
 
 pub use crate::any::*;
 pub use crate::cached::*;
@@ -272,24 +271,13 @@ impl Dist {
 
     /// Create a [`Dist`] for a local editable distribution.
     pub fn from_editable(name: PackageName, editable: LocalEditable) -> Result<Self, Error> {
-        match editable.requirement {
-            EditableRequirement::Path { url, path } => {
-                Ok(Self::Source(SourceDist::Path(PathSourceDist {
-                    name,
-                    url,
-                    path,
-                    editable: true,
-                })))
-            }
-            EditableRequirement::Url { url, path } => {
-                Ok(Self::Source(SourceDist::Path(PathSourceDist {
-                    name,
-                    path,
-                    url,
-                    editable: true,
-                })))
-            }
-        }
+        let LocalEditable { url, path } = editable;
+        Ok(Self::Source(SourceDist::Path(PathSourceDist {
+            name,
+            url,
+            path,
+            editable: true,
+        })))
     }
 
     /// Returns the [`File`] instance, if this dist is from a registry with simple json api support
@@ -353,11 +341,7 @@ impl SourceDist {
                 url: VerbatimUrl::unknown(url),
                 ..dist
             }),
-            SourceDist::Path(dist) => SourceDist::Path(PathSourceDist {
-                url: VerbatimUrl::unknown(url),
-                ..dist
-            }),
-            dist @ SourceDist::Registry(_) => dist,
+            dist => dist,
         }
     }
 }

crates/pep508-rs/Cargo.toml

@@ -21,6 +21,7 @@ pep440_rs = { path = "../pep440-rs" }
 puffin-normalize = { path = "../puffin-normalize" }
 
 derivative = { workspace = true }
+fs-err = { workspace = true }
 once_cell = { workspace = true }
 pyo3 = { workspace = true, optional = true, features = ["abi3", "extension-module"] }
 pyo3-log = { workspace = true, optional = true }

crates/pep508-rs/src/lib.rs

@@ -44,13 +44,13 @@ use pep440_rs::{Version, VersionSpecifier, VersionSpecifiers};
 #[cfg(feature = "pyo3")]
 use puffin_normalize::InvalidNameError;
 use puffin_normalize::{ExtraName, PackageName};
-pub use verbatim_url::VerbatimUrl;
+pub use verbatim_url::{VerbatimUrl, VerbatimUrlError};
 
 mod marker;
 mod verbatim_url;
 
 /// Error with a span attached. Not that those aren't `String` but `Vec<char>` indices.
-#[derive(Debug, Clone, Eq, PartialEq)]
+#[derive(Debug)]
 pub struct Pep508Error {
     /// Either we have an error string from our parser or an upstream error from `url`
     pub message: Pep508ErrorSource,
@@ -63,14 +63,14 @@ pub struct Pep508Error {
 }
 
 /// Either we have an error string from our parser or an upstream error from `url`
-#[derive(Debug, Error, Clone, Eq, PartialEq)]
+#[derive(Debug, Error)]
 pub enum Pep508ErrorSource {
     /// An error from our parser.
     #[error("{0}")]
     String(String),
     /// A URL parsing error.
     #[error(transparent)]
-    UrlError(#[from] verbatim_url::Error),
+    UrlError(#[from] verbatim_url::VerbatimUrlError),
 }
 
 impl Display for Pep508Error {

crates/pep508-rs/src/verbatim_url.rs

@@ -1,6 +1,7 @@
 use std::borrow::Cow;
+use std::fmt::Debug;
 use std::ops::Deref;
-use std::path::Path;
+use std::path::{Path, PathBuf};
 
 use once_cell::sync::Lazy;
 use regex::Regex;
@@ -26,19 +27,40 @@ pub struct VerbatimUrl {
 
 impl VerbatimUrl {
     /// Parse a URL from a string, expanding any environment variables.
-    pub fn parse(given: String) -> Result<Self, Error> {
-        let url = Url::parse(&expand_env_vars(&given))?;
+    pub fn parse(given: String) -> Result<Self, VerbatimUrlError> {
+        let url = Url::parse(&expand_env_vars(&given, true))
+            .map_err(|err| VerbatimUrlError::Url(given.clone(), err))?;
         Ok(Self {
             given: Some(given),
             url,
         })
     }
 
     /// Parse a URL from a path.
-    #[allow(clippy::result_unit_err)]
-    pub fn from_path(path: impl AsRef<Path>, given: String) -> Result<Self, ()> {
+    pub fn from_path(
+        path: impl AsRef<str>,
+        working_dir: impl AsRef<Path>,
+        given: String,
+    ) -> Result<Self, VerbatimUrlError> {
+        // Expand any environment variables.
+        let path = PathBuf::from(expand_env_vars(path.as_ref(), false).as_ref());
+
+        // Convert the path to an absolute path, if necessary.
+        let path = if path.is_absolute() {
+            path
+        } else {
+            working_dir.as_ref().join(path)
+        };
+
+        // Canonicalize the path.
+        let path =
+            fs_err::canonicalize(path).map_err(|err| VerbatimUrlError::Path(given.clone(), err))?;
+
+        // Convert to a URL.
+        let url = Url::from_file_path(path).expect("path is absolute");
+
         Ok(Self {
-            url: Url::from_directory_path(path)?,
+            url,
             given: Some(given),
         })
     }
@@ -68,7 +90,7 @@ impl VerbatimUrl {
 }
 
 impl std::str::FromStr for VerbatimUrl {
-    type Err = Error;
+    type Err = VerbatimUrlError;
 
     fn from_str(s: &str) -> Result<Self, Self::Err> {
         Self::parse(s.to_owned())
@@ -77,7 +99,7 @@ impl std::str::FromStr for VerbatimUrl {
 
 impl std::fmt::Display for VerbatimUrl {
     fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result {
-        self.url.fmt(f)
+        std::fmt::Display::fmt(&self.url, f)
     }
 }
 
@@ -89,10 +111,16 @@ impl Deref for VerbatimUrl {
     }
 }
 
-#[derive(thiserror::Error, Debug, Clone, PartialEq, Eq)]
-pub enum Error {
-    #[error(transparent)]
-    Url(#[from] url::ParseError),
+/// An error that can occur when parsing a [`VerbatimUrl`].
+#[derive(thiserror::Error, Debug)]
+pub enum VerbatimUrlError {
+    /// Failed to canonicalize a path.
+    #[error("{0}")]
+    Path(String, #[source] std::io::Error),
+
+    /// Failed to parse a URL.
+    #[error("{0}")]
+    Url(String, #[source] url::ParseError),
 }
 
 /// Expand all available environment variables.
@@ -110,12 +138,12 @@ pub enum Error {
 ///   Valid characters in variable names follow the `POSIX standard
 ///   <http://pubs.opengroup.org/onlinepubs/9699919799/>`_ and are limited
 ///   to uppercase letter, digits and the `_` (underscore).
-fn expand_env_vars(s: &str) -> Cow<'_, str> {
-    // Generate a URL-escaped project root, to be used via the `${PROJECT_ROOT}`
-    // environment variable. Ensure that it's URL-escaped.
+fn expand_env_vars(s: &str, escape: bool) -> Cow<'_, str> {
+    // Generate the project root, to be used via the `${PROJECT_ROOT}`
+    // environment variable.
     static PROJECT_ROOT_FRAGMENT: Lazy<String> = Lazy::new(|| {
         let project_root = std::env::current_dir().unwrap();
-        project_root.to_string_lossy().replace(' ', "%20")
+        project_root.to_string_lossy().to_string()
     });
 
     static RE: Lazy<Regex> =
@@ -124,7 +152,14 @@ fn expand_env_vars(s: &str) -> Cow<'_, str> {
     RE.replace_all(s, |caps: &regex::Captures<'_>| {
         let name = caps.name("name").unwrap().as_str();
         std::env::var(name).unwrap_or_else(|_| match name {
-            "PROJECT_ROOT" => PROJECT_ROOT_FRAGMENT.clone(),
+            // Ensure that the variable is URL-escaped, if necessary.
+            "PROJECT_ROOT" => {
+                if escape {
+                    PROJECT_ROOT_FRAGMENT.replace(' ', "%20")
+                } else {
+                    PROJECT_ROOT_FRAGMENT.to_string()
+                }
+            }
             _ => caps["var"].to_owned(),
         })
     })

crates/puffin/src/commands/pip_compile.rs

@@ -196,15 +196,9 @@ pub(crate) async fn pip_compile(
 
         let editables: Vec<LocalEditable> = editables
             .into_iter()
-            .map(|editable| match &editable {
-                EditableRequirement::Path { path, .. } => Ok(LocalEditable {
-                    path: path.clone(),
-                    requirement: editable,
-                }),
-                EditableRequirement::Url { path, .. } => Ok(LocalEditable {
-                    path: path.clone(),
-                    requirement: editable,
-                }),
+            .map(|editable| {
+                let EditableRequirement { path, url } = editable;
+                Ok(LocalEditable { url, path })
             })
             .collect::<Result<_>>()?;
 

crates/puffin/src/commands/pip_install.rs

@@ -315,15 +315,12 @@ async fn build_editables(
 
     let editables: Vec<LocalEditable> = editables
         .iter()
-        .map(|editable| match editable {
-            EditableRequirement::Path { path, .. } => Ok(LocalEditable {
+        .map(|editable| {
+            let EditableRequirement { path, url } = editable;
+            Ok(LocalEditable {
                 path: path.clone(),
-                requirement: editable.clone(),
-            }),
-            EditableRequirement::Url { path, .. } => Ok(LocalEditable {
-                path: path.clone(),
-                requirement: editable.clone(),
-            }),
+                url: url.clone(),
+            })
         })
         .collect::<Result<_>>()?;
 

crates/puffin/src/commands/pip_sync.rs

@@ -417,15 +417,12 @@ async fn resolve_editables(
 
         let local_editables: Vec<LocalEditable> = uninstalled
             .iter()
-            .map(|editable| match editable {
-                EditableRequirement::Path { path, .. } => Ok(LocalEditable {
+            .map(|editable| {
+                let EditableRequirement { path, url } = editable;
+                Ok(LocalEditable {
                     path: path.clone(),
-                    requirement: editable.clone(),
-                }),
-                EditableRequirement::Url { path, .. } => Ok(LocalEditable {
-                    path: path.clone(),
-                    requirement: editable.clone(),
-                }),
+                    url: url.clone(),
+                })
             })
             .collect::<Result<_>>()?;
 

crates/puffin/tests/pip_compile.rs

@@ -2634,7 +2634,8 @@ fn compile_editable() -> Result<()> {
     let requirements_in = temp_dir.child("requirements.in");
     requirements_in.write_str(indoc! {r"
         -e ../../scripts/editable-installs/poetry_editable
-        -e ../../scripts/editable-installs/maturin_editable
+        -e ${PROJECT_ROOT}/../../scripts/editable-installs/maturin_editable
+        -e file://../../scripts/editable-installs/black_editable
         boltons # normal dependency for comparison
         "
     })?;
@@ -2661,15 +2662,16 @@ fn compile_editable() -> Result<()> {
         ----- stdout -----
         # This file was autogenerated by Puffin v[VERSION] via the following command:
         #    puffin pip compile requirements.in --cache-dir [CACHE_DIR]
+        -e file://../../scripts/editable-installs/black_editable
         boltons==23.1.1
-        -e ../../scripts/editable-installs/maturin_editable
+        -e ${PROJECT_ROOT}/../../scripts/editable-installs/maturin_editable
         numpy==1.26.2
             # via poetry-editable
         -e ../../scripts/editable-installs/poetry_editable
 
         ----- stderr -----
-        Built 2 editables in [TIME]
-        Resolved 4 packages in [TIME]
+        Built 3 editables in [TIME]
+        Resolved 5 packages in [TIME]
         "###);
     });
 
@@ -3491,7 +3493,8 @@ fn missing_editable_requirement() -> Result<()> {
         ----- stdout -----
 
         ----- stderr -----
-        error: failed to canonicalize path `[WORKSPACE_DIR]/../tmp/django-3.2.8.tar.gz`
+        error: Invalid editable path in requirements.in: ../tmp/django-3.2.8.tar.gz
+          Caused by: failed to canonicalize path `[WORKSPACE_DIR]/../tmp/django-3.2.8.tar.gz`
           Caused by: No such file or directory (os error 2)
         "###);
     });