Review feedback

astral-sh · Apr 3, 2024 · 1c6c606 · 1c6c606
1 parent f703b2a
commit 1c6c606
Show file tree

Hide file tree

Showing 8 changed files with 40 additions and 44 deletions.
diff --git a/Cargo.lock b/Cargo.lock
diff --git a/PIP_COMPATIBILITY.md b/PIP_COMPATIBILITY.md
@@ -128,6 +128,12 @@ internal package, thus causing the malicious package to be installed instead of
 package. See, for example, [the `torchtriton` attack](https://pytorch.org/blog/compromised-nightly-dependency/)
 from December 2022.
 
+As of v0.1.29, users can opt in to `pip`-style behavior for multiple indexes via the
+`--index-strategy unsafe-any-match` command-line option, or the `UV_INDEX_STRATEGY` environment
+variable. When enabled, uv will search for each package across all indexes, and consider all
+available versions when resolving dependencies, prioritizing the `--extra-index-url` indexes over
+the default index URL. (Versions that are duplicated _across_ indexes will be ignored.)
+
 In the future, uv will support pinning packages to dedicated indexes (see: [#171](https://github.com/astral-sh/uv/issues/171)).
 Additionally, [PEP 708](https://peps.python.org/pep-0708/) is a provisional standard that aims to
 address the "dependency confusion" issue across package registries and installers.

diff --git a/README.md b/README.md
@@ -449,6 +449,9 @@ uv accepts the following command-line arguments as environment variables:
   should be used with caution, as it can modify the system Python installation.
 - `UV_NATIVE_TLS`: Equivalent to the `--native-tls` command-line argument. If set to `true`, uv
   will use the system's trust store instead of the bundled `webpki-roots` crate.
+- `UV_INDEX_STRATEGY`: Equivalent to the `--index-strategy` command-line argument. For example, if
+  set to `unsafe-any-match`, uv will consider versions of a given package available across all
+  index URLs, rather than limiting its search to the first index URL that contains the package.
 
 In each case, the corresponding command-line argument takes precedence over an environment variable.
 

diff --git a/crates/uv-types/Cargo.toml b/crates/uv-types/Cargo.toml
@@ -21,6 +21,7 @@ uv-interpreter = { workspace = true }
 uv-normalize = { workspace = true }
 
 anyhow = { workspace = true }
+clap = { workspace = true, features = ["derive"], optional = true }
 itertools = { workspace = true }
 rustc-hash = { workspace = true }
 serde = { workspace = true, optional = true }

diff --git a/crates/uv-types/src/build_options.rs b/crates/uv-types/src/build_options.rs
@@ -212,14 +212,16 @@ impl NoBuild {
 }
 
 #[derive(Debug, Default, Clone, Hash, Eq, PartialEq)]
+#[cfg_attr(feature = "clap", derive(clap::ValueEnum))]
 pub enum IndexStrategy {
-    /// Only use results from the first index that returns a match for a given package.
+    /// Only use results from the first index that returns a match for a given package name.
     ///
     /// While this differs from pip's behavior, it's the default index strategy as it's the most
     /// secure.
     #[default]
     FirstMatch,
-    /// Search for every package across all indexes.
+    /// Search for every package name across all indexes, exhausting the versions from the first
+    /// index before moving on to the next.
     ///
     /// In this strategy, we look for every package across all indexes. When resolving, we attempt
     /// to use versions from the indexes in order, such that we exhaust all available versions from
@@ -229,17 +231,7 @@ pub enum IndexStrategy {
     /// versions with different ABI tags or Python version constraints).
     ///
     /// See: https://peps.python.org/pep-0708/
-    Flatten,
-}
-
-impl IndexStrategy {
-    pub fn from_args(unsafe_index_merge: bool) -> Self {
-        if unsafe_index_merge {
-            Self::Flatten
-        } else {
-            Self::FirstMatch
-        }
-    }
+    UnsafeAnyMatch,
 }
 
 #[cfg(test)]

diff --git a/crates/uv/Cargo.toml b/crates/uv/Cargo.toml
@@ -31,7 +31,7 @@ uv-interpreter = { workspace = true }
 uv-normalize = { workspace = true }
 uv-requirements = { workspace = true }
 uv-resolver = { workspace = true, features = ["clap"] }
-uv-types = { workspace = true }
+uv-types = { workspace = true, features = ["clap"] }
 uv-virtualenv = { workspace = true }
 uv-warnings = { workspace = true }
 

diff --git a/crates/uv/src/main.rs b/crates/uv/src/main.rs
@@ -384,15 +384,14 @@ struct PipCompileArgs {
     #[clap(long, conflicts_with = "index_url", conflicts_with = "extra_index_url")]
     no_index: bool,
 
-    /// When resolving against multiple index URLs, search for every package on every index, and
-    /// merge the results.
+    /// The strategy to use when resolving against multiple index URLs.
     ///
     /// By default, `uv` will stop at the first index on which a given package is available, and
     /// limit resolutions to those present on that first index. This prevents "dependency confusion"
     /// attacks, whereby an attack can upload a malicious package under the same name to a secondary
     /// index.
-    #[clap(long)]
-    unsafe_index_merge: bool,
+    #[clap(long, default_value_t, value_enum, env = "UV_INDEX_STRATEGY")]
+    index_strategy: IndexStrategy,
 
     /// Attempt to use `keyring` for authentication for index urls
     ///
@@ -580,15 +579,14 @@ struct PipSyncArgs {
     #[clap(long, conflicts_with = "index_url", conflicts_with = "extra_index_url")]
     no_index: bool,
 
-    /// When resolving against multiple index URLs, search for every package on every index, and
-    /// merge the results.
+    /// The strategy to use when resolving against multiple index URLs.
     ///
     /// By default, `uv` will stop at the first index on which a given package is available, and
     /// limit resolutions to those present on that first index. This prevents "dependency confusion"
     /// attacks, whereby an attack can upload a malicious package under the same name to a secondary
     /// index.
-    #[clap(long)]
-    unsafe_index_merge: bool,
+    #[clap(long, default_value_t, value_enum, env = "UV_INDEX_STRATEGY")]
+    index_strategy: IndexStrategy,
 
     /// Attempt to use `keyring` for authentication for index urls
     ///
@@ -855,15 +853,14 @@ struct PipInstallArgs {
     #[clap(long, conflicts_with = "index_url", conflicts_with = "extra_index_url")]
     no_index: bool,
 
-    /// When resolving against multiple index URLs, search for every package on every index, and
-    /// merge the results.
+    /// The strategy to use when resolving against multiple index URLs.
     ///
     /// By default, `uv` will stop at the first index on which a given package is available, and
     /// limit resolutions to those present on that first index. This prevents "dependency confusion"
     /// attacks, whereby an attack can upload a malicious package under the same name to a secondary
     /// index.
-    #[clap(long)]
-    unsafe_index_merge: bool,
+    #[clap(long, default_value_t, value_enum, env = "UV_INDEX_STRATEGY")]
+    index_strategy: IndexStrategy,
 
     /// Attempt to use `keyring` for authentication for index urls
     ///
@@ -1366,15 +1363,14 @@ struct VenvArgs {
     #[clap(long, conflicts_with = "index_url", conflicts_with = "extra_index_url")]
     no_index: bool,
 
-    /// When resolving against multiple index URLs, search for every package on every index, and
-    /// merge the results.
+    /// The strategy to use when resolving against multiple index URLs.
     ///
     /// By default, `uv` will stop at the first index on which a given package is available, and
     /// limit resolutions to those present on that first index. This prevents "dependency confusion"
     /// attacks, whereby an attack can upload a malicious package under the same name to a secondary
     /// index.
-    #[clap(long)]
-    unsafe_index_merge: bool,
+    #[clap(long, default_value_t, value_enum, env = "UV_INDEX_STRATEGY")]
+    index_strategy: IndexStrategy,
 
     /// Attempt to use `keyring` for authentication for index urls
     ///
@@ -1556,7 +1552,6 @@ async fn run() -> Result<ExitStatus> {
             };
             let upgrade = Upgrade::from_args(args.upgrade, args.upgrade_package);
             let no_build = NoBuild::from_args(args.only_binary, args.no_build);
-            let index_strategy = IndexStrategy::from_args(args.unsafe_index_merge);
             let dependency_mode = if args.no_deps {
                 DependencyMode::Direct
             } else {
@@ -1593,7 +1588,7 @@ async fn run() -> Result<ExitStatus> {
                 args.emit_find_links,
                 args.emit_marker_expression,
                 index_urls,
-                index_strategy,
+                args.index_strategy,
                 args.keyring_provider,
                 setup_py,
                 config_settings,
@@ -1637,7 +1632,6 @@ async fn run() -> Result<ExitStatus> {
             let reinstall = Reinstall::from_args(args.reinstall, args.reinstall_package);
             let no_binary = NoBinary::from_args(args.no_binary);
             let no_build = NoBuild::from_args(args.only_binary, args.no_build);
-            let index_strategy = IndexStrategy::from_args(args.unsafe_index_merge);
             let setup_py = if args.legacy_setup_py {
                 SetupPyStrategy::Setuptools
             } else {
@@ -1651,7 +1645,7 @@ async fn run() -> Result<ExitStatus> {
                 args.link_mode,
                 args.compile,
                 index_urls,
-                index_strategy,
+                args.index_strategy,
                 args.keyring_provider,
                 setup_py,
                 if args.offline {
@@ -1718,7 +1712,6 @@ async fn run() -> Result<ExitStatus> {
             let upgrade = Upgrade::from_args(args.upgrade, args.upgrade_package);
             let no_binary = NoBinary::from_args(args.no_binary);
             let no_build = NoBuild::from_args(args.only_binary, args.no_build);
-            let index_strategy = IndexStrategy::from_args(args.unsafe_index_merge);
             let dependency_mode = if args.no_deps {
                 DependencyMode::Direct
             } else {
@@ -1746,7 +1739,7 @@ async fn run() -> Result<ExitStatus> {
                 dependency_mode,
                 upgrade,
                 index_urls,
-                index_strategy,
+                args.index_strategy,
                 args.keyring_provider,
                 reinstall,
                 args.link_mode,
@@ -1866,8 +1859,6 @@ async fn run() -> Result<ExitStatus> {
                 args.no_index,
             );
 
-            let index_strategy = IndexStrategy::from_args(args.unsafe_index_merge);
-
             // Since we use ".venv" as the default name, we use "." as the default prompt.
             let prompt = args.prompt.or_else(|| {
                 if args.name == PathBuf::from(DEFAULT_VENV_NAME) {
@@ -1881,7 +1872,7 @@ async fn run() -> Result<ExitStatus> {
                 &args.name,
                 args.python.as_deref(),
                 &index_locations,
-                index_strategy,
+                args.index_strategy,
                 args.keyring_provider,
                 uv_virtualenv::Prompt::from_args(prompt),
                 args.system_site_packages,

diff --git a/crates/uv/tests/pip_compile.rs b/crates/uv/tests/pip_compile.rs
@@ -7077,7 +7077,7 @@ fn compile_index_url_first_match() -> Result<()> {
 /// Install a package via `--extra-index-url`.
 ///
 /// If the package exists exist on the "extra" index, but at an incompatible version, the
-/// resolution should fallback to the "primary" index when `--unsafe-index-merge`
+/// resolution should fallback to the "primary" index when `--index-strategy unsafe-any-match`
 /// is provided.
 #[test]
 fn compile_index_url_fallback() -> Result<()> {
@@ -7087,7 +7087,8 @@ fn compile_index_url_fallback() -> Result<()> {
     requirements_in.write_str("jinja2==3.1.0")?;
 
     uv_snapshot!(context.compile()
-        .arg("--unsafe-index-merge")
+        .arg("--index-strategy")
+        .arg("unsafe-any-match")
         .arg("--index-url")
         .arg("https://pypi.org/simple")
         .arg("--extra-index-url")
@@ -7098,7 +7099,7 @@ fn compile_index_url_fallback() -> Result<()> {
     exit_code: 0
     ----- stdout -----
     # This file was autogenerated by uv via the following command:
-    #    uv pip compile --cache-dir [CACHE_DIR] --exclude-newer 2024-03-25T00:00:00Z --unsafe-index-merge requirements.in --no-deps
+    #    uv pip compile --cache-dir [CACHE_DIR] --exclude-newer 2024-03-25T00:00:00Z --index-strategy unsafe-any-match requirements.in --no-deps
     jinja2==3.1.0
 
     ----- stderr -----
@@ -7124,7 +7125,8 @@ fn compile_index_url_fallback_prefer_primary() -> Result<()> {
     requirements_in.write_str("jinja2")?;
 
     uv_snapshot!(context.compile_without_exclude_newer()
-        .arg("--unsafe-index-merge")
+        .arg("--index-strategy")
+        .arg("unsafe-any-match")
         .arg("--index-url")
         .arg("https://pypi.org/simple")
         .arg("--extra-index-url")
@@ -7135,7 +7137,7 @@ fn compile_index_url_fallback_prefer_primary() -> Result<()> {
     exit_code: 0
     ----- stdout -----
     # This file was autogenerated by uv via the following command:
-    #    uv pip compile --cache-dir [CACHE_DIR] --unsafe-index-merge requirements.in --no-deps
+    #    uv pip compile --cache-dir [CACHE_DIR] --index-strategy unsafe-any-match requirements.in --no-deps
     jinja2==3.1.2
 
     ----- stderr -----