Allow more-precise Git URLs to override less-precise Git URLs (#2285)

## Summary

This PR removes the URL conflict errors when the output of a `uv pip
compile` is used as a constraint to a subsequent `uv pip compile`.

If you run `uv pip compile`, the output file will contain your Git
dependencies, but pinned to a specific commit, like:

```
git+https://github.com/pallets/werkzeug@32e69512134c2f8183c6438b2b2e13fd24e9d19f
```

If you then use the output as a constraint to a subsequent resolution
(e.g., perhaps you require
`git+https://github.com/pallets/werkzeug@main`), we currently fail. I
think this is a reasonable workflow to support when all of these
requirements are coming from _your own_ dependencies. So we now assume
when resolving that the former is a more precise variant of the latter.

Closes https://github.com/astral-sh/uv/issues/1903.

Closes https://github.com/astral-sh/uv/issues/2266.

## Test Plan

`cargo test`
This commit is contained in:
Charlie Marsh 2024-03-07 15:47:31 -08:00 committed by GitHub
parent 9f1452cb72
commit f15af6771a
No known key found for this signature in database
GPG key ID: B5690EEEBB952194
3 changed files with 300 additions and 36 deletions

View file

@ -152,23 +152,23 @@ fn to_pubgrub(
// The requirement has a URL (e.g., `flask @ file:///path/to/flask`).
Some(VersionOrUrl::Url(url)) => {
let Some(allowed) = urls.get(&requirement.name) else {
let Some(expected) = urls.get(&requirement.name) else {
return Err(ResolveError::DisallowedUrl(
requirement.name.clone(),
url.verbatim().to_string(),
));
};
if cache_key::CanonicalUrl::new(allowed) != cache_key::CanonicalUrl::new(url) {
if !urls.is_allowed(expected, url) {
return Err(ResolveError::ConflictingUrlsTransitive(
requirement.name.clone(),
allowed.verbatim().to_string(),
expected.verbatim().to_string(),
url.verbatim().to_string(),
));
}
Ok((
PubGrubPackage::Package(requirement.name.clone(), extra, Some(allowed.clone())),
PubGrubPackage::Package(requirement.name.clone(), extra, Some(expected.clone())),
Range::full(),
))
}

View file

@ -1,4 +1,5 @@
use rustc_hash::FxHashMap;
use tracing::debug;
use distribution_types::Verbatim;
use pep508_rs::{MarkerEnvironment, VerbatimUrl};
@ -7,14 +8,20 @@ use uv_normalize::PackageName;
use crate::{Manifest, ResolveError};
#[derive(Debug, Default)]
pub(crate) struct Urls(FxHashMap<PackageName, VerbatimUrl>);
pub(crate) struct Urls {
/// A map of package names to their associated, required URLs.
required: FxHashMap<PackageName, VerbatimUrl>,
/// A map from required URL to URL that is assumed to be a less precise variant.
allowed: FxHashMap<VerbatimUrl, VerbatimUrl>,
}
impl Urls {
pub(crate) fn from_manifest(
manifest: &Manifest,
markers: &MarkerEnvironment,
) -> Result<Self, ResolveError> {
let mut urls = FxHashMap::default();
let mut required: FxHashMap<PackageName, VerbatimUrl> = FxHashMap::default();
let mut allowed: FxHashMap<VerbatimUrl, VerbatimUrl> = FxHashMap::default();
// Add all direct requirements and constraints. If there are any conflicts, return an error.
for requirement in manifest
@ -27,31 +34,43 @@ impl Urls {
}
if let Some(pep508_rs::VersionOrUrl::Url(url)) = &requirement.version_or_url {
if let Some(previous) = urls.insert(requirement.name.clone(), url.clone()) {
if cache_key::CanonicalUrl::new(previous.raw())
!= cache_key::CanonicalUrl::new(url.raw())
{
return Err(ResolveError::ConflictingUrlsDirect(
requirement.name.clone(),
previous.verbatim().to_string(),
url.verbatim().to_string(),
));
if let Some(previous) = required.insert(requirement.name.clone(), url.clone()) {
if is_equal(&previous, url) {
continue;
}
if is_precise(&previous, url) {
debug!("Assuming {url} is a precise variant of {previous}");
allowed.insert(url.clone(), previous);
continue;
}
return Err(ResolveError::ConflictingUrlsDirect(
requirement.name.clone(),
previous.verbatim().to_string(),
url.verbatim().to_string(),
));
}
}
}
// Add any editable requirements. If there are any conflicts, return an error.
for (editable, metadata) in &manifest.editables {
if let Some(previous) = urls.insert(metadata.name.clone(), editable.url.clone()) {
if cache_key::CanonicalUrl::new(previous.raw())
!= cache_key::CanonicalUrl::new(editable.raw())
{
return Err(ResolveError::ConflictingUrlsDirect(
metadata.name.clone(),
previous.verbatim().to_string(),
editable.verbatim().to_string(),
));
if let Some(previous) = required.insert(metadata.name.clone(), editable.url.clone()) {
if !is_equal(&previous, &editable.url) {
if is_precise(&previous, &editable.url) {
debug!(
"Assuming {} is a precise variant of {previous}",
editable.url
);
allowed.insert(editable.url.clone(), previous);
} else {
return Err(ResolveError::ConflictingUrlsDirect(
metadata.name.clone(),
previous.verbatim().to_string(),
editable.verbatim().to_string(),
));
}
}
}
@ -61,15 +80,18 @@ impl Urls {
}
if let Some(pep508_rs::VersionOrUrl::Url(url)) = &requirement.version_or_url {
if let Some(previous) = urls.insert(requirement.name.clone(), url.clone()) {
if cache_key::CanonicalUrl::new(previous.raw())
!= cache_key::CanonicalUrl::new(url.raw())
{
return Err(ResolveError::ConflictingUrlsDirect(
requirement.name.clone(),
previous.verbatim().to_string(),
url.verbatim().to_string(),
));
if let Some(previous) = required.insert(requirement.name.clone(), url.clone()) {
if !is_equal(&previous, url) {
if is_precise(&previous, url) {
debug!("Assuming {url} is a precise variant of {previous}");
allowed.insert(url.clone(), previous);
} else {
return Err(ResolveError::ConflictingUrlsDirect(
requirement.name.clone(),
previous.verbatim().to_string(),
url.verbatim().to_string(),
));
}
}
}
}
@ -84,14 +106,157 @@ impl Urls {
}
if let Some(pep508_rs::VersionOrUrl::Url(url)) = &requirement.version_or_url {
urls.insert(requirement.name.clone(), url.clone());
required.insert(requirement.name.clone(), url.clone());
}
}
Ok(Self(urls))
Ok(Self { required, allowed })
}
/// Return the [`VerbatimUrl`] associated with the given package name, if any.
pub(crate) fn get(&self, package: &PackageName) -> Option<&VerbatimUrl> {
self.0.get(package)
self.required.get(package)
}
/// Returns `true` if the provided URL is compatible with the given "allowed" URL.
pub(crate) fn is_allowed(&self, expected: &VerbatimUrl, provided: &VerbatimUrl) -> bool {
#[allow(clippy::if_same_then_else)]
if is_equal(expected, provided) {
// If the URLs are canonically equivalent, they're compatible.
true
} else if self
.allowed
.get(expected)
.is_some_and(|allowed| is_equal(allowed, provided))
{
// If the URL is canonically equivalent to the imprecise variant of the URL, they're
// compatible.
true
} else {
// Otherwise, they're incompatible.
false
}
}
}
/// Returns `true` if the [`VerbatimUrl`] is compatible with the previous [`VerbatimUrl`].
///
/// Accepts URLs that map to the same [`CanonicalUrl`].
fn is_equal(previous: &VerbatimUrl, url: &VerbatimUrl) -> bool {
cache_key::CanonicalUrl::new(previous.raw()) == cache_key::CanonicalUrl::new(url.raw())
}
/// Returns `true` if the [`VerbatimUrl`] appears to be a more precise variant of the previous
/// [`VerbatimUrl`].
///
/// Primarily, this method intends to accept URLs that map to the same repository, but with a
/// precise Git commit hash overriding a looser tag or branch. For example, if the previous URL
/// is `git+https://github.com/pallets/werkzeug.git@main`, this method would accept
/// `git+https://github.com/pallets/werkzeug@32e69512134c2f8183c6438b2b2e13fd24e9d19f`, and
/// assume that the latter is a more precise variant of the former. This is particularly useful
/// for workflows in which the output of `uv pip compile` is used as an input constraint on a
/// subsequent resolution, since `uv` will pin the exact commit hash of the package.
fn is_precise(previous: &VerbatimUrl, url: &VerbatimUrl) -> bool {
if cache_key::RepositoryUrl::new(previous.raw()) != cache_key::RepositoryUrl::new(url.raw()) {
return false;
}
// If there's no tag in the overriding URL, consider it incompatible.
let Some(url_tag) = url
.raw()
.path()
.rsplit_once('@')
.map(|(_prefix, suffix)| suffix)
else {
return false;
};
// Accept the overriding URL, as long as it's a full commit hash...
let url_is_commit = url_tag.len() == 40 && url_tag.chars().all(|ch| ch.is_ascii_hexdigit());
if !url_is_commit {
return false;
}
// If there's no tag in the previous URL, consider it compatible.
let Some(previous_tag) = previous
.raw()
.path()
.rsplit_once('@')
.map(|(_prefix, suffix)| suffix)
else {
return true;
};
// If the previous URL is a full commit hash, consider it incompatible.
let previous_is_commit =
previous_tag.len() == 40 && previous_tag.chars().all(|ch| ch.is_ascii_hexdigit());
!previous_is_commit
}
#[cfg(test)]
mod tests {
use super::*;
#[test]
fn url_compatibility() -> Result<(), url::ParseError> {
// Same repository, same tag.
let previous = VerbatimUrl::parse("git+https://example.com/MyProject.git@v1.0")?;
let url = VerbatimUrl::parse("git+https://example.com/MyProject.git@v1.0")?;
assert!(is_equal(&previous, &url));
// Same repository, different tags.
let previous = VerbatimUrl::parse("git+https://example.com/MyProject.git@v1.0")?;
let url = VerbatimUrl::parse("git+https://example.com/MyProject.git@v1.1")?;
assert!(!is_equal(&previous, &url));
// Same repository (with and without `.git`), same tag.
let previous = VerbatimUrl::parse("git+https://example.com/MyProject@v1.0")?;
let url = VerbatimUrl::parse("git+https://example.com/MyProject.git@v1.0")?;
assert!(is_equal(&previous, &url));
// Same repository, no tag on the previous URL.
let previous = VerbatimUrl::parse("git+https://example.com/MyProject.git")?;
let url = VerbatimUrl::parse("git+https://example.com/MyProject.git@v1.0")?;
assert!(!is_equal(&previous, &url));
// Same repository, tag on the previous URL, no tag on the overriding URL.
let previous = VerbatimUrl::parse("git+https://example.com/MyProject.git@v1.0")?;
let url = VerbatimUrl::parse("git+https://example.com/MyProject.git")?;
assert!(!is_equal(&previous, &url));
Ok(())
}
#[test]
fn url_precision() -> Result<(), url::ParseError> {
// Same repository, no tag on the previous URL, non-SHA on the overriding URL.
let previous = VerbatimUrl::parse("git+https://example.com/MyProject.git")?;
let url = VerbatimUrl::parse("git+https://example.com/MyProject.git@v1.0")?;
assert!(!is_precise(&previous, &url));
// Same repository, no tag on the previous URL, SHA on the overriding URL.
let previous = VerbatimUrl::parse("git+https://example.com/MyProject.git")?;
let url = VerbatimUrl::parse(
"git+https://example.com/MyProject.git@c3cd550a7a7c41b2c286ca52fbb6dec5fea195ef",
)?;
assert!(is_precise(&previous, &url));
// Same repository, tag on the previous URL, SHA on the overriding URL.
let previous = VerbatimUrl::parse("git+https://example.com/MyProject.git@v1.0")?;
let url = VerbatimUrl::parse(
"git+https://example.com/MyProject.git@c3cd550a7a7c41b2c286ca52fbb6dec5fea195ef",
)?;
assert!(is_precise(&previous, &url));
// Same repository, SHA on the previous URL, different SHA on the overriding URL.
let previous = VerbatimUrl::parse(
"git+https://example.com/MyProject.git@5ae5980c885e350a34ca019a84ba14a2a228d262",
)?;
let url = VerbatimUrl::parse(
"git+https://example.com/MyProject.git@c3cd550a7a7c41b2c286ca52fbb6dec5fea195ef",
)?;
assert!(!is_precise(&previous, &url));
Ok(())
}
}

View file

@ -1386,6 +1386,105 @@ fn compatible_repeated_url_dependency() -> Result<()> {
Ok(())
}
/// Request Werkzeug via two different URLs which resolve to the same repository, but different
/// commits.
#[test]
fn conflicting_repeated_url_dependency() -> Result<()> {
let context = TestContext::new("3.12");
let requirements_in = context.temp_dir.child("requirements.in");
requirements_in.write_str("werkzeug @ git+https://github.com/pallets/werkzeug.git@2.0.0\nwerkzeug @ git+https://github.com/pallets/werkzeug@3.0.0")?;
uv_snapshot!(context.compile()
.arg("requirements.in"), @r###"
success: false
exit_code: 2
----- stdout -----
----- stderr -----
error: Requirements contain conflicting URLs for package `werkzeug`:
- git+https://github.com/pallets/werkzeug.git@2.0.0
- git+https://github.com/pallets/werkzeug@3.0.0
"###
);
Ok(())
}
/// Request Werkzeug via two different URLs: `main`, and a precise SHA. Allow the precise SHA
/// to override the `main` branch.
#[test]
fn compatible_narrowed_url_dependency() -> Result<()> {
let context = TestContext::new("3.12");
let requirements_in = context.temp_dir.child("requirements.in");
requirements_in.write_str("werkzeug @ git+https://github.com/pallets/werkzeug.git@main\nwerkzeug @ git+https://github.com/pallets/werkzeug@32e69512134c2f8183c6438b2b2e13fd24e9d19f")?;
uv_snapshot!(context.compile()
.arg("requirements.in"), @r###"
success: true
exit_code: 0
----- stdout -----
# This file was autogenerated by uv via the following command:
# uv pip compile --cache-dir [CACHE_DIR] --exclude-newer 2023-11-18T12:00:00Z requirements.in
markupsafe==2.1.3
# via werkzeug
werkzeug @ git+https://github.com/pallets/werkzeug@32e69512134c2f8183c6438b2b2e13fd24e9d19f
----- stderr -----
Resolved 2 packages in [TIME]
"###
);
Ok(())
}
/// Request Werkzeug via two different URLs: `main`, and a precise SHA, followed by `main` again.
/// We _may_ want to allow this, but we don't right now.
#[test]
fn compatible_repeated_narrowed_url_dependency() -> Result<()> {
let context = TestContext::new("3.12");
let requirements_in = context.temp_dir.child("requirements.in");
requirements_in.write_str("werkzeug @ git+https://github.com/pallets/werkzeug@main\nwerkzeug @ git+https://github.com/pallets/werkzeug.git@main\nwerkzeug @ git+https://github.com/pallets/werkzeug@32e69512134c2f8183c6438b2b2e13fd24e9d19f\nwerkzeug @ git+https://github.com/pallets/werkzeug.git@main")?;
uv_snapshot!(context.compile()
.arg("requirements.in"), @r###"
success: false
exit_code: 2
----- stdout -----
----- stderr -----
error: Requirements contain conflicting URLs for package `werkzeug`:
- git+https://github.com/pallets/werkzeug@32e69512134c2f8183c6438b2b2e13fd24e9d19f
- git+https://github.com/pallets/werkzeug.git@main
"###
);
Ok(())
}
/// Request Werkzeug via two different URLs: `main`, and a precise SHA. Allow the precise SHA
/// to override the `main` branch, but error when we see yet another URL for the same package.
#[test]
fn incompatible_narrowed_url_dependency() -> Result<()> {
let context = TestContext::new("3.12");
let requirements_in = context.temp_dir.child("requirements.in");
requirements_in.write_str("werkzeug @ git+https://github.com/pallets/werkzeug.git@main\nwerkzeug @ git+https://github.com/pallets/werkzeug@32e69512134c2f8183c6438b2b2e13fd24e9d19f\nwerkzeug @ git+https://github.com/pallets/werkzeug.git@v1.0.0")?;
uv_snapshot!(context.compile()
.arg("requirements.in"), @r###"
success: false
exit_code: 2
----- stdout -----
----- stderr -----
error: Requirements contain conflicting URLs for package `werkzeug`:
- git+https://github.com/pallets/werkzeug@32e69512134c2f8183c6438b2b2e13fd24e9d19f
- git+https://github.com/pallets/werkzeug.git@v1.0.0
"###
);
Ok(())
}
/// Request `transitive_url_dependency`, which depends on `git+https://github.com/pallets/werkzeug@2.0.0`.
/// Since this URL isn't declared upfront, we should reject it.
#[test]