Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Show a dedicated hint for missing git+ prefixes #9789

Merged
merged 1 commit into from
Dec 10, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
30 changes: 24 additions & 6 deletions crates/uv-distribution/src/metadata/lowering.rs
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,7 @@ use uv_distribution_types::{Index, IndexLocations, IndexName, Origin};
use uv_git::GitReference;
use uv_normalize::{ExtraName, GroupName, PackageName};
use uv_pep440::VersionSpecifiers;
use uv_pep508::{MarkerTree, VerbatimUrl, VersionOrUrl};
use uv_pep508::{looks_like_git_repository, MarkerTree, VerbatimUrl, VersionOrUrl};
use uv_pypi_types::{
ConflictItem, ParsedUrlError, Requirement, RequirementSource, VerbatimParsedUrl,
};
Expand Down Expand Up @@ -200,7 +200,8 @@ impl LoweredRequirement {
marker,
..
} => {
let source = url_source(url, subdirectory.map(PathBuf::from))?;
let source =
url_source(&requirement, url, subdirectory.map(PathBuf::from))?;
(source, marker)
}
Source::Path {
Expand Down Expand Up @@ -436,7 +437,8 @@ impl LoweredRequirement {
marker,
..
} => {
let source = url_source(url, subdirectory.map(PathBuf::from))?;
let source =
url_source(&requirement, url, subdirectory.map(PathBuf::from))?;
(source, marker)
}
Source::Path {
Expand Down Expand Up @@ -531,6 +533,8 @@ pub enum LoweringError {
InvalidVerbatimUrl(#[from] uv_pep508::VerbatimUrlError),
#[error("Fragments are not allowed in URLs: `{0}`")]
ForbiddenFragment(Url),
#[error("`{0}` is associated with a URL source, but references a Git repository. Consider using a Git source instead (e.g., `{0} = {{ git = \"{1}\" }}`)")]
MissingGitSource(PackageName, Url),
#[error("`workspace = false` is not yet supported")]
WorkspaceFalse,
#[error("Editable must refer to a local directory, not a file: `{0}`")]
Expand Down Expand Up @@ -605,7 +609,11 @@ fn git_source(
}

/// Convert a URL source into a [`RequirementSource`].
fn url_source(url: Url, subdirectory: Option<PathBuf>) -> Result<RequirementSource, LoweringError> {
fn url_source(
requirement: &uv_pep508::Requirement<VerbatimParsedUrl>,
url: Url,
subdirectory: Option<PathBuf>,
) -> Result<RequirementSource, LoweringError> {
let mut verbatim_url = url.clone();
if verbatim_url.fragment().is_some() {
return Err(LoweringError::ForbiddenFragment(url));
Expand All @@ -617,8 +625,18 @@ fn url_source(url: Url, subdirectory: Option<PathBuf>) -> Result<RequirementSour
verbatim_url.set_fragment(Some(subdirectory));
}

let ext = DistExtension::from_path(url.path())
.map_err(|err| ParsedUrlError::MissingExtensionUrl(url.to_string(), err))?;
let ext = match DistExtension::from_path(url.path()) {
Ok(ext) => ext,
Err(..) if looks_like_git_repository(&url) => {
return Err(LoweringError::MissingGitSource(
requirement.name.clone(),
url.clone(),
))
}
Err(err) => {
return Err(ParsedUrlError::MissingExtensionUrl(url.to_string(), err).into());
}
};

let verbatim_url = VerbatimUrl::from_url(verbatim_url);
Ok(RequirementSource::Url {
Expand Down
3 changes: 2 additions & 1 deletion crates/uv-pep508/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -42,7 +42,8 @@ pub use uv_normalize::{ExtraName, InvalidNameError, PackageName};
pub use uv_pep440;
use uv_pep440::{VersionSpecifier, VersionSpecifiers};
pub use verbatim_url::{
expand_env_vars, split_scheme, strip_host, Scheme, VerbatimUrl, VerbatimUrlError,
expand_env_vars, looks_like_git_repository, split_scheme, strip_host, Scheme, VerbatimUrl,
VerbatimUrlError,
};

mod cursor;
Expand Down
61 changes: 61 additions & 0 deletions crates/uv-pep508/src/verbatim_url.rs
Original file line number Diff line number Diff line change
Expand Up @@ -396,6 +396,19 @@ pub fn strip_host(path: &str) -> &str {
path
}

/// Returns `true` if a URL looks like a reference to a Git repository (e.g., `https://github.com/user/repo.git`).
pub fn looks_like_git_repository(url: &Url) -> bool {
matches!(
url.host_str(),
Some("github.com" | "gitlab.com" | "bitbucket.org")
) && Path::new(url.path())
.extension()
.map_or(true, |ext| ext.eq_ignore_ascii_case("git"))
&& url
.path_segments()
.map_or(false, |segments| segments.count() == 2)
}

/// Split the fragment from a URL.
///
/// For example, given `file:///home/ferris/project/scripts#hash=somehash`, returns
Expand Down Expand Up @@ -582,4 +595,52 @@ mod tests {
(Cow::Borrowed(Path::new("")), None)
);
}

#[test]
fn git_repository() {
let url = Url::parse("https://github.com/user/repo.git").unwrap();
assert!(looks_like_git_repository(&url));

let url = Url::parse("https://gitlab.com/user/repo.git").unwrap();
assert!(looks_like_git_repository(&url));

let url = Url::parse("https://bitbucket.org/user/repo.git").unwrap();
assert!(looks_like_git_repository(&url));

let url = Url::parse("https://github.com/user/repo").unwrap();
assert!(looks_like_git_repository(&url));

let url = Url::parse("https://example.com/user/repo.git").unwrap();
assert!(!looks_like_git_repository(&url));

let url = Url::parse("https://github.com/user").unwrap();
assert!(!looks_like_git_repository(&url));

let url = Url::parse("https://github.com/user/repo.zip").unwrap();
assert!(!looks_like_git_repository(&url));

let url = Url::parse("https://github.com/").unwrap();
assert!(!looks_like_git_repository(&url));

let url = Url::parse("").unwrap_err();
assert_eq!(url.to_string(), "relative URL without a base");

let url = Url::parse("github.com/user/repo.git").unwrap_err();
assert_eq!(url.to_string(), "relative URL without a base");

let url = Url::parse("https://github.com/user/repo/extra.git").unwrap();
assert!(!looks_like_git_repository(&url));

let url = Url::parse("https://github.com/user/repo.GIT").unwrap();
assert!(looks_like_git_repository(&url));

let url = Url::parse("https://github.com/user/repo.git?foo=bar").unwrap();
assert!(looks_like_git_repository(&url));

let url = Url::parse("https://github.com/user/repo.git#readme").unwrap();
assert!(looks_like_git_repository(&url));

let url = Url::parse("https://github.com/pypa/pip/archive/1.3.1.zip#sha1=da9234ee9982d4bbb3c72346a6de940a148ea686").unwrap();
assert!(!looks_like_git_repository(&url));
}
}
17 changes: 14 additions & 3 deletions crates/uv-pypi-types/src/parsed_url.rs
Original file line number Diff line number Diff line change
@@ -1,10 +1,14 @@
use std::fmt::{Display, Formatter};
use std::path::{Path, PathBuf};

use thiserror::Error;
use url::{ParseError, Url};

use uv_distribution_filename::{DistExtension, ExtensionError};
use uv_git::{GitReference, GitSha, GitUrl, OidParseError};
use uv_pep508::{Pep508Url, UnnamedRequirementUrl, VerbatimUrl, VerbatimUrlError};
use uv_pep508::{
looks_like_git_repository, Pep508Url, UnnamedRequirementUrl, VerbatimUrl, VerbatimUrlError,
};

use crate::{ArchiveInfo, DirInfo, DirectUrl, VcsInfo, VcsKind};

Expand All @@ -24,6 +28,8 @@ pub enum ParsedUrlError {
UrlParse(String, #[source] ParseError),
#[error(transparent)]
VerbatimUrl(#[from] VerbatimUrlError),
#[error("Direct URL (`{0}`) references a Git repository, but is missing the `git+` prefix (e.g., `git+{0}`)")]
MissingGitPrefix(String),
#[error("Expected direct URL (`{0}`) to end in a supported file extension: {1}")]
MissingExtensionUrl(String, ExtensionError),
#[error("Expected path (`{0}`) to end in a supported file extension: {1}")]
Expand Down Expand Up @@ -303,8 +309,13 @@ impl TryFrom<Url> for ParsedArchiveUrl {

fn try_from(url: Url) -> Result<Self, Self::Error> {
let subdirectory = get_subdirectory(&url);
let ext = DistExtension::from_path(url.path())
.map_err(|err| ParsedUrlError::MissingExtensionUrl(url.to_string(), err))?;
let ext = match DistExtension::from_path(url.path()) {
Ok(ext) => ext,
Err(..) if looks_like_git_repository(&url) => {
return Err(ParsedUrlError::MissingGitPrefix(url.to_string()))
}
Err(err) => return Err(ParsedUrlError::MissingExtensionUrl(url.to_string(), err)),
};
Ok(Self {
url,
subdirectory,
Expand Down
36 changes: 36 additions & 0 deletions crates/uv/tests/it/lock.rs
Original file line number Diff line number Diff line change
Expand Up @@ -20087,3 +20087,39 @@ fn lock_self_marker_incompatible() -> Result<()> {

Ok(())
}

#[test]
fn lock_missing_git_prefix() -> Result<()> {
let context = TestContext::new("3.12");

let pyproject_toml = context.temp_dir.child("pyproject.toml");
pyproject_toml.write_str(
r#"
[project]
name = "project"
version = "0.1.0"
requires-python = ">=3.12"
dependencies = ["workspace-in-root-test"]

[tool.uv.sources]
workspace-in-root-test = { url = "https://github.com/astral-sh/workspace-in-root-test" }

[build-system]
requires = ["setuptools>=42"]
build-backend = "setuptools.build_meta"
"#,
)?;

uv_snapshot!(context.filters(), context.lock(), @r###"
success: false
exit_code: 1
----- stdout -----

----- stderr -----
× Failed to build `project @ file://[TEMP_DIR]/`
├─▶ Failed to parse entry: `workspace-in-root-test`
╰─▶ `workspace-in-root-test` is associated with a URL source, but references a Git repository. Consider using a Git source instead (e.g., `workspace-in-root-test = { git = "https://github.com/astral-sh/workspace-in-root-test" }`)
"###);

Ok(())
}
27 changes: 25 additions & 2 deletions crates/uv/tests/it/pip_install.rs
Original file line number Diff line number Diff line change
Expand Up @@ -7590,20 +7590,43 @@ fn build_tag() {
"###);
}

#[test]
fn missing_git_prefix() -> Result<()> {
let context = TestContext::new("3.12");
let requirements_txt = context.temp_dir.child("requirements.txt");
requirements_txt.touch()?;

uv_snapshot!(context.pip_install()
.arg("workspace-in-root-test @ https://github.com/astral-sh/workspace-in-root-test"), @r###"
success: false
exit_code: 2
----- stdout -----

----- stderr -----
error: Failed to parse: `workspace-in-root-test @ https://github.com/astral-sh/workspace-in-root-test`
Caused by: Direct URL (`https://github.com/astral-sh/workspace-in-root-test`) references a Git repository, but is missing the `git+` prefix (e.g., `git+https://github.com/astral-sh/workspace-in-root-test`)
workspace-in-root-test @ https://github.com/astral-sh/workspace-in-root-test
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
"###
);

Ok(())
}

#[test]
fn missing_subdirectory_git() -> Result<()> {
let context = TestContext::new("3.12");
let requirements_txt = context.temp_dir.child("requirements.txt");
requirements_txt.touch()?;

uv_snapshot!(context.pip_install()
.arg("source-distribution @ git+https://github.com/astral-sh/workspace-in-root-test#subdirectory=missing"), @r###"
.arg("workspace-in-root-test @ git+https://github.com/astral-sh/workspace-in-root-test#subdirectory=missing"), @r###"
success: false
exit_code: 1
----- stdout -----

----- stderr -----
× Failed to download and build `source-distribution @ git+https://github.com/astral-sh/workspace-in-root-test#subdirectory=missing`
× Failed to download and build `workspace-in-root-test @ git+https://github.com/astral-sh/workspace-in-root-test#subdirectory=missing`
╰─▶ The source distribution `git+https://github.com/astral-sh/workspace-in-root-test#subdirectory=missing` has no subdirectory `missing`
"###
);
Expand Down
Loading