Require URL dependencies to be declared upfront (#319)

In the resolver, our current model for solving URL dependencies requires
that we visit the URL dependency _before_ the registry-based dependency.
This PR encodes a strict requirement that all URL dependencies be
declared upfront, either as requirements or constraints.

I wrote more about how it works and why it's necessary in documentation
[here](https://github.com/astral-sh/puffin/pull/319/files#diff-2b1c4f36af0c62a2b7bebeae9473ae083588f2a6b18a3ec52393a24266adecbbR20).
I think we could relax this constraint over time, but it requires a more
sophisticated model -- and for now, I just want something that's (1)
correct, (2) easy for us to reason about, and (3) easy for users to
reason about.

As additional motivation... allowing arbitrary URL dependencies anywhere
in the tree creates some really confusing situations in which I'm not
even sure what the right answers are. For example, assume you declare a
direct dependency on `Werkzeug==2.0.0`. You then depend on a version of
Flask that depends on a version of `Werkzeug` from some arbitrary URL.
You build the source distribution at that arbitrary URL, and it turns
out it _does_ build to a declared version of 2.0.0. What should happen?
(And if it resolves to a version that _isn't_ 2.0.0, what should happen
_then_?) I suspect different tools handle this differently, but it must
lead to a lot of "silent" failures. In my testing of Poetry, it seems
like Poetry just ignores the URL dependency, which seems wrong, but is
also a behavior we could implement in the future.

Closes #303.
Closes #284.

crates/puffin-cache/src/canonical_url.rs

@@ -1,3 +1,4 @@
+use std::hash::{Hash, Hasher};
 use url::Url;
 
 use crate::cache_key::{CacheKey, CacheKeyHasher};
@@ -74,6 +75,14 @@ impl CacheKey for CanonicalUrl {
     }
 }
 
+impl Hash for CanonicalUrl {
+    fn hash<H: Hasher>(&self, state: &mut H) {
+        // `as_str` gives the serialisation of a url (which has a spec) and so insulates against
+        // possible changes in how the URL crate does hashing.
+        self.0.as_str().hash(state);
+    }
+}
+
 /// Like [`CanonicalUrl`], but attempts to represent an underlying source repository, abstracting
 /// away details like the specific commit or branch, or the subdirectory to build within the
 /// repository.
@@ -116,6 +125,14 @@ impl CacheKey for RepositoryUrl {
     }
 }
 
+impl Hash for RepositoryUrl {
+    fn hash<H: Hasher>(&self, state: &mut H) {
+        // `as_str` gives the serialisation of a url (which has a spec) and so insulates against
+        // possible changes in how the URL crate does hashing.
+        self.0.as_str().hash(state);
+    }
+}
+
 #[cfg(test)]
 mod tests {
     use super::*;

crates/puffin-cli/tests/pip_compile.rs

@@ -996,12 +996,9 @@ fn mixed_url_dependency() -> Result<()> {
     Ok(())
 }
 
-/// Request Flask, but include a URL dependency for a conflicting version of Werkzeug.
-///
-/// TODO(charlie): This test _should_ fail, but sometimes passes due to inadequacies in our
-/// URL dependency model.
+/// Request Werkzeug via both a version and a URL dependency at a _different_ version, which
+/// should result in a conflict.
 #[test]
-#[ignore]
 fn conflicting_direct_url_dependency() -> Result<()> {
     let temp_dir = assert_fs::TempDir::new()?;
     let cache_dir = assert_fs::TempDir::new()?;
@@ -1036,6 +1033,116 @@ fn conflicting_direct_url_dependency() -> Result<()> {
     Ok(())
 }
 
+/// Request Werkzeug via both a version and a URL dependency at _the same_ version, which
+/// should prefer the direct URL dependency.
+#[test]
+fn compatible_direct_url_dependency() -> Result<()> {
+    let temp_dir = assert_fs::TempDir::new()?;
+    let cache_dir = assert_fs::TempDir::new()?;
+    let venv = temp_dir.child(".venv");
+
+    Command::new(get_cargo_bin(BIN_NAME))
+        .arg("venv")
+        .arg(venv.as_os_str())
+        .arg("--cache-dir")
+        .arg(cache_dir.path())
+        .current_dir(&temp_dir)
+        .assert()
+        .success();
+    venv.assert(predicates::path::is_dir());
+
+    let requirements_in = temp_dir.child("requirements.in");
+    requirements_in.touch()?;
+    requirements_in.write_str("werkzeug==2.0.0\nwerkzeug @ https://files.pythonhosted.org/packages/ff/1d/960bb4017c68674a1cb099534840f18d3def3ce44aed12b5ed8b78e0153e/Werkzeug-2.0.0-py3-none-any.whl")?;
+
+    insta::with_settings!({
+        filters => INSTA_FILTERS.to_vec()
+    }, {
+        assert_cmd_snapshot!(Command::new(get_cargo_bin(BIN_NAME))
+            .arg("pip-compile")
+            .arg("requirements.in")
+            .arg("--cache-dir")
+            .arg(cache_dir.path())
+            .env("VIRTUAL_ENV", venv.as_os_str())
+            .current_dir(&temp_dir));
+    });
+
+    Ok(())
+}
+
+/// Request Werkzeug via two different URLs at different versions, which should result in a conflict.
+#[test]
+fn conflicting_repeated_url_dependency_version_mismatch() -> Result<()> {
+    let temp_dir = assert_fs::TempDir::new()?;
+    let cache_dir = assert_fs::TempDir::new()?;
+    let venv = temp_dir.child(".venv");
+
+    Command::new(get_cargo_bin(BIN_NAME))
+        .arg("venv")
+        .arg(venv.as_os_str())
+        .arg("--cache-dir")
+        .arg(cache_dir.path())
+        .current_dir(&temp_dir)
+        .assert()
+        .success();
+    venv.assert(predicates::path::is_dir());
+
+    let requirements_in = temp_dir.child("requirements.in");
+    requirements_in.touch()?;
+    requirements_in.write_str("werkzeug @ https://files.pythonhosted.org/packages/bd/24/11c3ea5a7e866bf2d97f0501d0b4b1c9bbeade102bb4b588f0d2919a5212/Werkzeug-2.0.1-py3-none-any.whl\nwerkzeug @ https://files.pythonhosted.org/packages/ff/1d/960bb4017c68674a1cb099534840f18d3def3ce44aed12b5ed8b78e0153e/Werkzeug-2.0.0-py3-none-any.whl")?;
+
+    insta::with_settings!({
+        filters => INSTA_FILTERS.to_vec()
+    }, {
+        assert_cmd_snapshot!(Command::new(get_cargo_bin(BIN_NAME))
+            .arg("pip-compile")
+            .arg("requirements.in")
+            .arg("--cache-dir")
+            .arg(cache_dir.path())
+            .env("VIRTUAL_ENV", venv.as_os_str())
+            .current_dir(&temp_dir));
+    });
+
+    Ok(())
+}
+
+/// Request Werkzeug via two different URLs at the same version. Despite mapping to the same
+/// version, it should still result in a conflict.
+#[test]
+fn conflicting_repeated_url_dependency_version_match() -> Result<()> {
+    let temp_dir = assert_fs::TempDir::new()?;
+    let cache_dir = assert_fs::TempDir::new()?;
+    let venv = temp_dir.child(".venv");
+
+    Command::new(get_cargo_bin(BIN_NAME))
+        .arg("venv")
+        .arg(venv.as_os_str())
+        .arg("--cache-dir")
+        .arg(cache_dir.path())
+        .current_dir(&temp_dir)
+        .assert()
+        .success();
+    venv.assert(predicates::path::is_dir());
+
+    let requirements_in = temp_dir.child("requirements.in");
+    requirements_in.touch()?;
+    requirements_in.write_str("werkzeug @ git+git+https://github.com/pallets/[email protected] \nwerkzeug @ https://files.pythonhosted.org/packages/ff/1d/960bb4017c68674a1cb099534840f18d3def3ce44aed12b5ed8b78e0153e/Werkzeug-2.0.0-py3-none-any.whl")?;
+
+    insta::with_settings!({
+        filters => INSTA_FILTERS.to_vec()
+    }, {
+        assert_cmd_snapshot!(Command::new(get_cargo_bin(BIN_NAME))
+            .arg("pip-compile")
+            .arg("requirements.in")
+            .arg("--cache-dir")
+            .arg(cache_dir.path())
+            .env("VIRTUAL_ENV", venv.as_os_str())
+            .current_dir(&temp_dir));
+    });
+
+    Ok(())
+}
+
 /// Request Flask, but include a URL dependency for a conflicting version of Werkzeug.
 #[test]
 fn conflicting_transitive_url_dependency() -> Result<()> {
@@ -1072,6 +1179,130 @@ fn conflicting_transitive_url_dependency() -> Result<()> {
     Ok(())
 }
 
+/// Request `transitive_url_dependency`, which depends on `git+https://github.com/pallets/[email protected]`.
+/// Since this URL isn't declared upfront, we should reject it.
+#[test]
+fn disallowed_transitive_url_dependency() -> Result<()> {
+    let temp_dir = assert_fs::TempDir::new()?;
+    let cache_dir = assert_fs::TempDir::new()?;
+    let venv = temp_dir.child(".venv");
+
+    Command::new(get_cargo_bin(BIN_NAME))
+        .arg("venv")
+        .arg(venv.as_os_str())
+        .arg("--cache-dir")
+        .arg(cache_dir.path())
+        .current_dir(&temp_dir)
+        .assert()
+        .success();
+    venv.assert(predicates::path::is_dir());
+
+    let requirements_in = temp_dir.child("requirements.in");
+    requirements_in.touch()?;
+    requirements_in.write_str("transitive_url_dependency @ https://github.com/astral-sh/ruff/files/13257454/transitive_url_dependency.zip")?;
+
+    insta::with_settings!({
+        filters => INSTA_FILTERS.to_vec()
+    }, {
+        assert_cmd_snapshot!(Command::new(get_cargo_bin(BIN_NAME))
+            .arg("pip-compile")
+            .arg("requirements.in")
+            .arg("--cache-dir")
+            .arg(cache_dir.path())
+            .env("VIRTUAL_ENV", venv.as_os_str())
+            .current_dir(&temp_dir));
+    });
+
+    Ok(())
+}
+
+/// Request `transitive_url_dependency`, which depends on `git+https://github.com/pallets/[email protected]`.
+/// Since this URL is declared as a constraint, we should accept it.
+#[test]
+fn allowed_transitive_url_dependency() -> Result<()> {
+    let temp_dir = assert_fs::TempDir::new()?;
+    let cache_dir = assert_fs::TempDir::new()?;
+    let venv = temp_dir.child(".venv");
+
+    Command::new(get_cargo_bin(BIN_NAME))
+        .arg("venv")
+        .arg(venv.as_os_str())
+        .arg("--cache-dir")
+        .arg(cache_dir.path())
+        .current_dir(&temp_dir)
+        .assert()
+        .success();
+    venv.assert(predicates::path::is_dir());
+
+    let requirements_in = temp_dir.child("requirements.in");
+    requirements_in.touch()?;
+    requirements_in.write_str("transitive_url_dependency @ https://github.com/astral-sh/ruff/files/13257454/transitive_url_dependency.zip")?;
+
+    let constraints_txt = temp_dir.child("constraints.txt");
+    constraints_txt.touch()?;
+    constraints_txt.write_str("werkzeug @ git+https://github.com/pallets/[email protected]")?;
+
+    insta::with_settings!({
+        filters => INSTA_FILTERS.to_vec()
+    }, {
+        assert_cmd_snapshot!(Command::new(get_cargo_bin(BIN_NAME))
+            .arg("pip-compile")
+            .arg("requirements.in")
+            .arg("--constraint")
+            .arg("constraints.txt")
+            .arg("--cache-dir")
+            .arg(cache_dir.path())
+            .env("VIRTUAL_ENV", venv.as_os_str())
+            .current_dir(&temp_dir));
+    });
+
+    Ok(())
+}
+
+/// Request `transitive_url_dependency`, which depends on `git+https://github.com/pallets/[email protected]`.
+/// Since this `git+https://github.com/pallets/[email protected]` is declared as a constraint, and
+/// those map to the same canonical URL, we should accept it.
+#[test]
+fn allowed_transitive_canonical_url_dependency() -> Result<()> {
+    let temp_dir = assert_fs::TempDir::new()?;
+    let cache_dir = assert_fs::TempDir::new()?;
+    let venv = temp_dir.child(".venv");
+
+    Command::new(get_cargo_bin(BIN_NAME))
+        .arg("venv")
+        .arg(venv.as_os_str())
+        .arg("--cache-dir")
+        .arg(cache_dir.path())
+        .current_dir(&temp_dir)
+        .assert()
+        .success();
+    venv.assert(predicates::path::is_dir());
+
+    let requirements_in = temp_dir.child("requirements.in");
+    requirements_in.touch()?;
+    requirements_in.write_str("transitive_url_dependency @ https://github.com/astral-sh/ruff/files/13257454/transitive_url_dependency.zip")?;
+
+    let constraints_txt = temp_dir.child("constraints.txt");
+    constraints_txt.touch()?;
+    constraints_txt.write_str("werkzeug @ git+https://github.com/pallets/[email protected]")?;
+
+    insta::with_settings!({
+        filters => INSTA_FILTERS.to_vec()
+    }, {
+        assert_cmd_snapshot!(Command::new(get_cargo_bin(BIN_NAME))
+            .arg("pip-compile")
+            .arg("requirements.in")
+            .arg("--constraint")
+            .arg("constraints.txt")
+            .arg("--cache-dir")
+            .arg(cache_dir.path())
+            .env("VIRTUAL_ENV", venv.as_os_str())
+            .current_dir(&temp_dir));
+    });
+
+    Ok(())
+}
+
 /// Resolve packages from all optional dependency groups in a `pyproject.toml` file.
 #[test]
 fn compile_pyproject_toml_all_extras() -> Result<()> {

crates/puffin-cli/tests/snapshots/pip_compile__allowed_transitive_canonical_url_dependency.snap

@@ -0,0 +1,26 @@
+---
+source: crates/puffin-cli/tests/pip_compile.rs
+info:
+  program: puffin
+  args:
+    - pip-compile
+    - requirements.in
+    - "--constraint"
+    - constraints.txt
+    - "--cache-dir"
+    - /var/folders/nt/6gf2v7_s3k13zq_t3944rwz40000gn/T/.tmpb2n1p9
+  env:
+    VIRTUAL_ENV: /var/folders/nt/6gf2v7_s3k13zq_t3944rwz40000gn/T/.tmpAnFqUK/.venv
+---
+success: true
+exit_code: 0
+----- stdout -----
+# This file was autogenerated by Puffin v0.0.1 via the following command:
+#    [BIN_PATH] pip-compile requirements.in --constraint constraints.txt --cache-dir [CACHE_DIR]
+transitive-url-dependency @ https://github.com/astral-sh/ruff/files/13257454/transitive_url_dependency.zip
+werkzeug @ git+https://github.com/pallets/werkzeug@af160e0b6b7ddd81c22f1652c728ff5ac72d5c74
+    # via transitive-url-dependency
+
+----- stderr -----
+Resolved 2 packages in [TIME]
+

crates/puffin-cli/tests/snapshots/pip_compile__allowed_transitive_url_dependency.snap

@@ -0,0 +1,26 @@
+---
+source: crates/puffin-cli/tests/pip_compile.rs
+info:
+  program: puffin
+  args:
+    - pip-compile
+    - requirements.in
+    - "--constraint"
+    - constraints.txt
+    - "--cache-dir"
+    - /var/folders/nt/6gf2v7_s3k13zq_t3944rwz40000gn/T/.tmpUeMnec
+  env:
+    VIRTUAL_ENV: /var/folders/nt/6gf2v7_s3k13zq_t3944rwz40000gn/T/.tmpaClkf0/.venv
+---
+success: true
+exit_code: 0
+----- stdout -----
+# This file was autogenerated by Puffin v0.0.1 via the following command:
+#    [BIN_PATH] pip-compile requirements.in --constraint constraints.txt --cache-dir [CACHE_DIR]
+transitive-url-dependency @ https://github.com/astral-sh/ruff/files/13257454/transitive_url_dependency.zip
+werkzeug @ git+https://github.com/pallets/werkzeug@af160e0b6b7ddd81c22f1652c728ff5ac72d5c74
+    # via transitive-url-dependency
+
+----- stderr -----
+Resolved 2 packages in [TIME]
+

crates/puffin-cli/tests/snapshots/pip_compile__compatible_direct_url_dependency.snap

@@ -0,0 +1,22 @@
+---
+source: crates/puffin-cli/tests/pip_compile.rs
+info:
+  program: puffin
+  args:
+    - pip-compile
+    - requirements.in
+    - "--cache-dir"
+    - /var/folders/nt/6gf2v7_s3k13zq_t3944rwz40000gn/T/.tmp5pOmCe
+  env:
+    VIRTUAL_ENV: /var/folders/nt/6gf2v7_s3k13zq_t3944rwz40000gn/T/.tmpMF1dXI/.venv
+---
+success: true
+exit_code: 0
+----- stdout -----
+# This file was autogenerated by Puffin v0.0.1 via the following command:
+#    [BIN_PATH] pip-compile requirements.in --cache-dir [CACHE_DIR]
+werkzeug @ https://files.pythonhosted.org/packages/ff/1d/960bb4017c68674a1cb099534840f18d3def3ce44aed12b5ed8b78e0153e/Werkzeug-2.0.0-py3-none-any.whl
+
+----- stderr -----
+Resolved 1 package in [TIME]
+

crates/puffin-cli/tests/snapshots/pip_compile__conflicting_direct_url_dependency.snap

@@ -0,0 +1,20 @@
+---
+source: crates/puffin-cli/tests/pip_compile.rs
+info:
+  program: puffin
+  args:
+    - pip-compile
+    - requirements.in
+    - "--cache-dir"
+    - /var/folders/nt/6gf2v7_s3k13zq_t3944rwz40000gn/T/.tmpVyrxMG
+  env:
+    VIRTUAL_ENV: /var/folders/nt/6gf2v7_s3k13zq_t3944rwz40000gn/T/.tmpF8a4y3/.venv
+---
+success: false
+exit_code: 1
+----- stdout -----
+
+----- stderr -----
+  × No solution found when resolving dependencies:
+  ╰─▶ root depends on werkzeug 3.0.0
+