Always include package names for Git and HTTPS dependencies (#3821)

## Summary Related to #3818. We should _always_ include the package name if we know it's not a file path, even if it starts with an environment variable.
astral-sh · May 24, 2024 · 999d072 · 999d072
1 parent 73f6708
commit 999d072
Show file tree

Hide file tree

Showing 5 changed files with 73 additions and 35 deletions.
diff --git a/crates/distribution-types/src/installed.rs b/crates/distribution-types/src/installed.rs
@@ -284,6 +284,16 @@ impl InstalledDist {
             Self::LegacyEditable(dist) => Some(&dist.target_url),
         }
     }
+
+    /// Return true if the distribution refers to a local file or directory.
+    pub fn is_local(&self) -> bool {
+        match self {
+            Self::Registry(_) => false,
+            Self::Url(dist) => matches!(&*dist.direct_url, DirectUrl::LocalDirectory { .. }),
+            Self::EggInfo(_) => false,
+            Self::LegacyEditable(_) => true,
+        }
+    }
 }
 
 impl DistributionMetadata for InstalledDist {

diff --git a/crates/distribution-types/src/lib.rs b/crates/distribution-types/src/lib.rs
@@ -427,6 +427,14 @@ impl Dist {
         }
     }
 
+    /// Return true if the distribution refers to a local file or directory.
+    pub fn is_local(&self) -> bool {
+        match self {
+            Self::Source(dist) => dist.is_local(),
+            Self::Built(dist) => dist.is_local(),
+        }
+    }
+
     /// Returns the [`IndexUrl`], if the distribution is from a registry.
     pub fn index(&self) -> Option<&IndexUrl> {
         match self {
@@ -452,6 +460,11 @@ impl Dist {
 }
 
 impl BuiltDist {
+    /// Return true if the distribution refers to a local file or directory.
+    pub fn is_local(&self) -> bool {
+        matches!(self, Self::Path(_))
+    }
+
     /// Returns the [`IndexUrl`], if the distribution is from a registry.
     pub fn index(&self) -> Option<&IndexUrl> {
         match self {
@@ -510,6 +523,11 @@ impl SourceDist {
         }
     }
 
+    /// Return true if the distribution refers to a local file or directory.
+    pub fn is_local(&self) -> bool {
+        matches!(self, Self::Directory(_) | Self::Path(_))
+    }
+
     /// Returns the path to the source distribution, if it's a local distribution.
     pub fn as_path(&self) -> Option<&Path> {
         match self {

diff --git a/crates/distribution-types/src/resolved.rs b/crates/distribution-types/src/resolved.rs
@@ -45,6 +45,14 @@ impl ResolvedDist {
         }
     }
 
+    /// Return true if the distribution refers to a local file or directory.
+    pub fn is_local(&self) -> bool {
+        match self {
+            Self::Installable(dist) => dist.is_local(),
+            Self::Installed(dist) => dist.is_local(),
+        }
+    }
+
     /// Returns the [`IndexUrl`], if the distribution is from a registry.
     pub fn index(&self) -> Option<&IndexUrl> {
         match self {

diff --git a/crates/uv-resolver/src/resolution/mod.rs b/crates/uv-resolver/src/resolution/mod.rs
@@ -35,45 +35,47 @@ impl AnnotatedDist {
     /// supported in `requirements.txt`).
     pub(crate) fn to_requirements_txt(&self, include_extras: bool) -> Cow<str> {
         // If the URL is not _definitively_ an absolute `file://` URL, write it as a relative path.
-        if let VersionOrUrlRef::Url(url) = self.dist.version_or_url() {
-            let given = url.verbatim();
-            match split_scheme(&given) {
-                Some((scheme, path)) => {
-                    match Scheme::parse(scheme) {
-                        Some(Scheme::File) => {
-                            if path
-                                .strip_prefix("//localhost")
-                                .filter(|path| path.starts_with('/'))
-                                .is_some()
-                            {
-                                // Always absolute; nothing to do.
-                            } else if let Some(path) = path.strip_prefix("//") {
-                                // Strip the prefix, to convert, e.g., `file://flask-3.0.3-py3-none-any.whl` to `flask-3.0.3-py3-none-any.whl`.
-                                //
-                                // However, we should allow any of the following:
-                                // - `file://flask-3.0.3-py3-none-any.whl`
-                                // - `file://C:\Users\user\flask-3.0.3-py3-none-any.whl`
-                                // - `file:///C:\Users\user\flask-3.0.3-py3-none-any.whl`
-                                if !path.starts_with("${PROJECT_ROOT}")
-                                    && !Path::new(path).has_root()
+        if self.dist.is_local() {
+            if let VersionOrUrlRef::Url(url) = self.dist.version_or_url() {
+                let given = url.verbatim();
+                match split_scheme(&given) {
+                    Some((scheme, path)) => {
+                        match Scheme::parse(scheme) {
+                            Some(Scheme::File) => {
+                                if path
+                                    .strip_prefix("//localhost")
+                                    .filter(|path| path.starts_with('/'))
+                                    .is_some()
                                 {
-                                    return Cow::Owned(path.to_string());
+                                    // Always absolute; nothing to do.
+                                } else if let Some(path) = path.strip_prefix("//") {
+                                    // Strip the prefix, to convert, e.g., `file://flask-3.0.3-py3-none-any.whl` to `flask-3.0.3-py3-none-any.whl`.
+                                    //
+                                    // However, we should allow any of the following:
+                                    // - `file://flask-3.0.3-py3-none-any.whl`
+                                    // - `file://C:\Users\user\flask-3.0.3-py3-none-any.whl`
+                                    // - `file:///C:\Users\user\flask-3.0.3-py3-none-any.whl`
+                                    if !path.starts_with("${PROJECT_ROOT}")
+                                        && !Path::new(path).has_root()
+                                    {
+                                        return Cow::Owned(path.to_string());
+                                    }
+                                } else {
+                                    // Ex) `file:./flask-3.0.3-py3-none-any.whl`
+                                    return given;
                                 }
-                            } else {
-                                // Ex) `file:./flask-3.0.3-py3-none-any.whl`
+                            }
+                            Some(_) => {}
+                            None => {
+                                // Ex) `flask @ C:\Users\user\flask-3.0.3-py3-none-any.whl`
                                 return given;
                             }
                         }
-                        Some(_) => {}
-                        None => {
-                            // Ex) `flask @ C:\Users\user\flask-3.0.3-py3-none-any.whl`
-                            return given;
-                        }
                     }
-                }
-                None => {
-                    // Ex) `flask @ flask-3.0.3-py3-none-any.whl`
-                    return given;
+                    None => {
+                        // Ex) `flask @ flask-3.0.3-py3-none-any.whl`
+                        return given;
+                    }
                 }
             }
         }

diff --git a/crates/uv/tests/pip_compile.rs b/crates/uv/tests/pip_compile.rs
@@ -3190,7 +3190,7 @@ fn respect_http_env_var() -> Result<()> {
         # via flask
     click==8.1.7
         # via flask
-    ${URL}
+    flask @ ${URL}
         # via -r requirements.in
     itsdangerous==2.1.2
         # via flask
@@ -3231,7 +3231,7 @@ fn respect_unnamed_env_var() -> Result<()> {
         # via flask
     click==8.1.7
         # via flask
-    ${URL}
+    flask @ ${URL}
         # via -r requirements.in
     itsdangerous==2.1.2
         # via flask