From 71ec568d0f02ed097b3f9160f9639ee206eddc6f Mon Sep 17 00:00:00 2001 From: Zanie Blue Date: Wed, 21 Feb 2024 12:44:32 -0600 Subject: [PATCH] Use `git` command to fetch repositories instead of `libgit2` for robust SSH support (#1781) Closes https://github.com/astral-sh/uv/issues/1775 Closes https://github.com/astral-sh/uv/issues/1452 Closes https://github.com/astral-sh/uv/issues/1514 Follows https://github.com/astral-sh/uv/pull/1717 libgit2 does not support host names with extra identifiers during SSH lookup (e.g. [`github.com-some_identifier`]( https://docs.github.com/en/authentication/connecting-to-github-with-ssh/managing-deploy-keys#using-multiple-repositories-on-one-server)) so we use the `git` command instead for fetching. This is required for `pip` parity. See the [Cargo documentation](https://doc.rust-lang.org/nightly/cargo/reference/config.html#netgit-fetch-with-cli) for more details on using the `git` CLI instead of libgit2. We may want to try to use libgit2 first in the future, as it is more performant (#1786). We now support authentication with: ``` git+ssh://git@/... git+ssh://git@-/... ``` Tested with a deploy key e.g. ``` cargo run -- \ pip install uv-private-pypackage@git+ssh://git@github.com-test-uv-private-pypackage/astral-test/uv-private-pypackage.git \ --reinstall --no-cache -v ``` and ``` cargo run -- \ pip install uv-private-pypackage@git+ssh://git@github.com/astral-test/uv-private-pypackage.git \ --reinstall --no-cache -v ``` with a ssh config like ``` Host github.com Hostname github.com IdentityFile=/Users/mz/.ssh/id_ed25519 Host github.com-test-uv-private-pypackage Hostname github.com IdentityFile=/Users/mz/.ssh/id_ed25519 ``` It seems quite hard to add test coverage for this to the test suite, as we'd need to add the SSH key and I don't know how to isolate that from affecting other developer's machines. --- crates/uv-git/src/git.rs | 56 +++++++++++++++++++++++-- crates/uv-git/src/source.rs | 2 +- crates/uv/tests/common/mod.rs | 76 ++++++++++++++++++++++++++++++---- crates/uv/tests/pip_install.rs | 57 ++++++++++++++++++++++++- 4 files changed, 179 insertions(+), 12 deletions(-) diff --git a/crates/uv-git/src/git.rs b/crates/uv-git/src/git.rs index 84401d542..cc72ca7a4 100644 --- a/crates/uv-git/src/git.rs +++ b/crates/uv-git/src/git.rs @@ -11,7 +11,7 @@ use cargo_util::{paths, ProcessBuilder}; use git2::{self, ErrorClass, ObjectType}; use reqwest::Client; use reqwest::StatusCode; -use tracing::{debug, warn}; +use tracing::{debug, error, warn}; use url::Url; use uv_fs::Normalized; @@ -43,6 +43,14 @@ pub(crate) enum GitReference { DefaultBranch, } +/// Strategy when fetching refspecs for a [`GitReference`] +enum RefspecStrategy { + // All refspecs should be fetched, if any fail then the fetch will fail + All, + // Stop after the first successful fetch, if none suceed then the fetch will fail + First, +} + impl GitReference { pub(crate) fn from_rev(rev: &str) -> Self { if rev.starts_with("refs/") { @@ -908,6 +916,7 @@ pub(crate) fn fetch( // which need to get fetched. Additionally record if we're fetching tags. let mut refspecs = Vec::new(); let mut tags = false; + let mut refspec_strategy = RefspecStrategy::All; // The `+` symbol on the refspec means to allow a forced (fast-forward) // update which is needed if there is ever a force push that requires a // fast-forward. @@ -929,6 +938,7 @@ pub(crate) fn fetch( refspecs.push(format!( "+refs/tags/{branch_or_tag}:refs/remotes/origin/tags/{branch_or_tag}" )); + refspec_strategy = RefspecStrategy::First; } GitReference::DefaultBranch => { @@ -969,13 +979,49 @@ pub(crate) fn fetch( debug!("Performing a Git fetch for: {remote_url}"); match strategy { - FetchStrategy::Cli => fetch_with_cli(repo, remote_url, &refspecs, tags), + FetchStrategy::Cli => { + match refspec_strategy { + RefspecStrategy::All => fetch_with_cli(repo, remote_url, refspecs.as_slice(), tags), + RefspecStrategy::First => { + let num_refspecs = refspecs.len(); + + // Try each refspec + let errors = refspecs + .into_iter() + .map(|refspec| { + ( + refspec.clone(), + fetch_with_cli(repo, remote_url, &[refspec], tags), + ) + }) + // Stop after the first success + .take_while(|(_, result)| result.is_err()) + .collect::>(); + + if errors.len() == num_refspecs { + // If all of the fetches failed, report to the user + for (refspec, err) in errors { + if let Err(err) = err { + error!("failed to fetch refspec `{refspec}`: {err}"); + } + } + Err(anyhow!("failed to fetch all refspecs")) + } else { + Ok(()) + } + } + } + } FetchStrategy::Libgit2 => { + // Libgit2 does not fail if a refspec is missing, so the `refspec_strategy` + // is not handled here + let git_config = git2::Config::open_default()?; with_fetch_options(&git_config, remote_url, &mut |mut opts| { if tags { opts.download_tags(git2::AutotagOption::All); } + // The `fetch` operation here may fail spuriously due to a corrupt // repository. It could also fail, however, for a whole slew of other // reasons (aka network related reasons). We want Cargo to automatically @@ -1057,7 +1103,11 @@ fn fetch_with_cli( .env_remove("GIT_OBJECT_DIRECTORY") .env_remove("GIT_ALTERNATE_OBJECT_DIRECTORIES") .cwd(repo.path()); - cmd.exec()?; + + // We capture the output to avoid streaming it to the user's console during clones. + // The required `on...line` callbacks currently do nothing. + // The output appears to be included in error messages by default. + cmd.exec_with_streaming(&mut |_| Ok(()), &mut |_| Ok(()), true)?; Ok(()) } diff --git a/crates/uv-git/src/source.rs b/crates/uv-git/src/source.rs index d2c4c143a..76ebe4ac6 100644 --- a/crates/uv-git/src/source.rs +++ b/crates/uv-git/src/source.rs @@ -33,7 +33,7 @@ impl GitSource { Self { git, client: Client::new(), - strategy: FetchStrategy::Libgit2, + strategy: FetchStrategy::Cli, cache: cache.into(), reporter: None, } diff --git a/crates/uv/tests/common/mod.rs b/crates/uv/tests/common/mod.rs index e8b68b63f..12ff31b88 100644 --- a/crates/uv/tests/common/mod.rs +++ b/crates/uv/tests/common/mod.rs @@ -1,11 +1,6 @@ // The `unreachable_pub` is to silence false positives in RustRover. #![allow(dead_code, unreachable_pub)] -use std::borrow::BorrowMut; -use std::env; -use std::path::{Path, PathBuf}; -use std::process::Output; - use assert_cmd::assert::{Assert, OutputAssertExt}; use assert_cmd::Command; use assert_fs::assert::PathAssert; @@ -14,7 +9,12 @@ use assert_fs::fixture::PathChild; use fs_err::os::unix::fs::symlink as symlink_file; #[cfg(windows)] use fs_err::os::windows::fs::symlink_file; -use regex::Regex; +use regex::{self, Regex}; +use std::borrow::BorrowMut; +use std::env; +use std::path::{Path, PathBuf}; +use std::process::Output; +use uv_fs::Normalized; use platform_host::Platform; use uv_cache::Cache; @@ -44,17 +44,39 @@ pub struct TestContext { pub temp_dir: assert_fs::TempDir, pub cache_dir: assert_fs::TempDir, pub venv: PathBuf, + + // Standard filters for this test context + filters: Vec<(String, String)>, } impl TestContext { pub fn new(python_version: &str) -> Self { let temp_dir = assert_fs::TempDir::new().expect("Failed to create temp dir"); - let cache_dir = assert_fs::TempDir::new().expect("Failed to create temp dir"); + let cache_dir = assert_fs::TempDir::new().expect("Failed to create cache dir"); let venv = create_venv(&temp_dir, &cache_dir, python_version); + + let mut filters = Vec::new(); + filters.extend( + Self::path_patterns(&cache_dir) + .into_iter() + .map(|pattern| (pattern, "[CACHE_DIR]/".to_string())), + ); + filters.extend( + Self::path_patterns(&temp_dir) + .into_iter() + .map(|pattern| (pattern, "[TEMP_DIR]/".to_string())), + ); + filters.extend( + Self::path_patterns(&venv) + .into_iter() + .map(|pattern| (pattern, "[VENV]/".to_string())), + ); + Self { temp_dir, cache_dir, venv, + filters, } } @@ -96,6 +118,46 @@ impl TestContext { .success() .stdout(version); } + + /// Generate an escaped regex pattern for the given path. + fn path_patterns(path: impl AsRef) -> Vec { + vec![ + format!( + // Trim the trailing separator for cross-platform directories filters + r"{}\\?/?", + regex::escape( + &path + .as_ref() + .canonicalize() + .expect("Failed to create canonical path") + // Normalize the path to match display and remove UNC prefixes on Windows + .normalized() + .display() + .to_string(), + ) + // Make seprators platform agnostic because on Windows we will display + // paths with Unix-style separators sometimes + .replace(r"\\", r"(\\|\/)") + ), + // Include a non-canonicalized version + format!( + r"{}\\?/?", + regex::escape(&path.as_ref().normalized().display().to_string()) + .replace(r"\\", r"(\\|\/)") + ), + ] + } + + /// Canonical snapshot filters for this test context. + pub fn filters(&self) -> Vec<(&str, &str)> { + let mut filters = INSTA_FILTERS.to_vec(); + + for (pattern, replacement) in &self.filters { + filters.push((pattern, replacement)); + } + + filters + } } pub fn venv_to_interpreter(venv: &Path) -> PathBuf { diff --git a/crates/uv/tests/pip_install.rs b/crates/uv/tests/pip_install.rs index 7cb8e1fd3..890699b77 100644 --- a/crates/uv/tests/pip_install.rs +++ b/crates/uv/tests/pip_install.rs @@ -812,6 +812,28 @@ fn install_git_public_https() { context.assert_installed("uv_public_pypackage", "0.1.0"); } +/// Install a package from a public GitHub repository at a ref that does not exist +#[test] +#[cfg(feature = "git")] +fn install_git_public_https_missing_ref() { + let context = TestContext::new("3.8"); + + uv_snapshot!(context.filters(), command(&context) + // 2.0.0 does not exist + .arg("uv-public-pypackage @ git+https://github.com/astral-test/uv-public-pypackage@2.0.0") + , @r###" + success: false + exit_code: 2 + ----- stdout ----- + + ----- stderr ----- + error: Failed to download and build: uv-public-pypackage @ git+https://github.com/astral-test/uv-public-pypackage@2.0.0 + Caused by: Git operation failed + Caused by: failed to clone into: [CACHE_DIR]/git-v0/db/8dab139913c4b566 + Caused by: failed to fetch all refspecs + "###); +} + /// Install a package from a private GitHub repository using a PAT #[test] #[cfg(all(not(windows), feature = "git"))] @@ -886,7 +908,6 @@ fn install_git_private_https_pat_and_username() { let mut filters = INSTA_FILTERS.to_vec(); filters.insert(0, (&token, "***")); - filters.push(("failed to clone into: .*", "failed to clone into: [PATH]")); uv_snapshot!(filters, command(&context) .arg(format!("uv-private-pypackage @ git+https://{user}:{token}@github.com/astral-test/uv-private-pypackage")) @@ -905,6 +926,40 @@ fn install_git_private_https_pat_and_username() { context.assert_installed("uv_private_pypackage", "0.1.0"); } +/// Install a package from a private GitHub repository using a PAT +#[test] +#[cfg(all(not(windows), feature = "git"))] +fn install_git_private_https_pat_not_authorized() { + let context = TestContext::new("3.8"); + + // A revoked token + let token = "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"; + + let mut filters = context.filters(); + filters.insert(0, (&token, "***")); + + // We provide a username otherwise (since the token is invalid), the git cli will prompt for a password + // and hang the test + uv_snapshot!(filters, command(&context) + .arg(format!("uv-private-pypackage @ git+https://git:{token}@github.com/astral-test/uv-private-pypackage")) + , @r###" + success: false + exit_code: 2 + ----- stdout ----- + + ----- stderr ----- + error: Failed to download and build: uv-private-pypackage @ git+https://git:***@github.com/astral-test/uv-private-pypackage + Caused by: Git operation failed + Caused by: failed to clone into: [CACHE_DIR]/git-v0/db/2496970ed6fdf08f + Caused by: process didn't exit successfully: `git fetch --force --update-head-ok 'https://git:***@github.com/astral-test/uv-private-pypackage' '+HEAD:refs/remotes/origin/HEAD'` (exit status: 128) + --- stderr + remote: Support for password authentication was removed on August 13, 2021. + remote: Please see https://docs.github.com/en/get-started/getting-started-with-git/about-remote-repositories#cloning-with-https-urls for information on currently recommended modes of authentication. + fatal: Authentication failed for 'https://github.com/astral-test/uv-private-pypackage/' + + "###); +} + /// Install a package without using pre-built wheels. #[test] fn reinstall_no_binary() {