Skip to content

Commit

Permalink
Flatten requirements eagerly in get_dependencies (#4430)
Browse files Browse the repository at this point in the history 
Downstack PR: #4515 Upstack PR: #4481

Consider these two cases:

A:
```
werkzeug==2.0.0
werkzeug @ https://files.pythonhosted.org/packages/ff/1d/960bb4017c68674a1cb099534840f18d3def3ce44aed12b5ed8b78e0153e/Werkzeug-2.0.0-py3-none-any.whl
```

B:
```toml
dependencies = [
  "iniconfig == 1.1.1 ; python_version < '3.12'",
  "iniconfig @ git+https://github.com/pytest-dev/iniconfig@93f5930e668c0d1ddf4597e38dd0dea4e2665e7a ; python_version >= '3.12'",
]
```

In the first case, `werkzeug==2.0.0` should be overridden by the url. In
the second case `iniconfig == 1.1.1` is in a different fork and must
remain a registry distribution.

That means the conversion from `Requirement` to `PubGrubPackage` is
dependent on the other requirements of the package. We can either look
into the other packages immediately, or we can move the forking before
the conversion to `PubGrubDependencies` instead of after. Either version
requires a flat list of `Requirement`s to use. This refactoring gives us
this list.

I'll add support for both of the above cases in the forking urls branch
before merging this PR. I also have to move constraints over to this.
  • Loading branch information
@konstin
konstin committed Jun 25, 2024
1 parent e242cdf commit f2f48d3
Show file tree
Hide file tree
Showing 3 changed files with 231 additions and 284 deletions.
6 changes: 5 additions & 1 deletion crates/uv-configuration/src/constraints.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,11 @@ impl Constraints {
constraints
.entry(requirement.name.clone())
.or_default()
.push(requirement);
.push(Requirement {
// We add and apply constraints independent of their extras.
extras: vec![],
..requirement
});
}
Self(constraints)
}
Expand Down
293 changes: 42 additions & 251 deletions crates/uv-resolver/src/pubgrub/dependencies.rs
View file
Original file line number Diff line number Diff line change
@@ -1,26 +1,19 @@
use std::collections::BTreeMap;

use either::Either;
use itertools::Itertools;
use pubgrub::range::Range;
use rustc_hash::FxHashSet;
use tracing::{trace, warn};
use tracing::warn;

use distribution_types::Verbatim;
use pep440_rs::Version;
use pep508_rs::{MarkerEnvironment, MarkerTree};
use pypi_types::{
ParsedArchiveUrl, ParsedDirectoryUrl, ParsedGitUrl, ParsedPathUrl, ParsedUrl, Requirement,
RequirementSource,
};
use uv_configuration::{Constraints, Overrides};
use uv_git::GitResolver;
use uv_normalize::{ExtraName, GroupName, PackageName};
use uv_normalize::{ExtraName, PackageName};

use crate::pubgrub::specifier::PubGrubSpecifier;
use crate::pubgrub::{PubGrubPackage, PubGrubPackageInner};
use crate::resolver::{Locals, Urls};
use crate::ResolveError;
use crate::{PubGrubSpecifier, ResolveError};

#[derive(Debug)]
pub struct PubGrubDependencies(Vec<(PubGrubPackage, Range<Version>)>);
Expand All @@ -29,41 +22,51 @@ impl PubGrubDependencies {
/// Generate a set of PubGrub dependencies from a set of requirements.
#[allow(clippy::too_many_arguments)]
pub(crate) fn from_requirements(
requirements: &[Requirement],
dev_dependencies: &BTreeMap<GroupName, Vec<Requirement>>,
constraints: &Constraints,
overrides: &Overrides,
flattened_requirements: &[&Requirement],
source_name: Option<&PackageName>,
source_extra: Option<&ExtraName>,
source_dev: Option<&GroupName>,
urls: &Urls,
locals: &Locals,
git: &GitResolver,
env: Option<&MarkerEnvironment>,
requires_python: Option<&MarkerTree>,
fork_markers: &MarkerTree,
) -> Result<Self, ResolveError> {
let mut dependencies = Vec::default();
let mut seen = FxHashSet::default();

add_requirements(
requirements,
dev_dependencies,
constraints,
overrides,
source_name,
source_extra,
source_dev,
urls,
locals,
git,
env,
requires_python,
fork_markers,
&mut dependencies,
&mut seen,
)?;
let mut dependencies = Vec::new();
for requirement in flattened_requirements {
// Add the package, plus any extra variants.
for result in std::iter::once(PubGrubRequirement::from_requirement(
requirement,
None,
urls,
locals,
git,
))
.chain(requirement.extras.clone().into_iter().map(|extra| {
PubGrubRequirement::from_requirement(requirement, Some(extra), urls, locals, git)
})) {
let PubGrubRequirement { package, version } = result?;

match &*package {
PubGrubPackageInner::Package { name, .. } => {
// Detect self-dependencies.
if source_name.is_some_and(|source_name| source_name == name) {
warn!("{name} has a dependency on itself");
continue;
}

dependencies.push((package.clone(), version.clone()));
}
PubGrubPackageInner::Marker { .. } => {
dependencies.push((package.clone(), version.clone()));
}
PubGrubPackageInner::Extra { name, .. } => {
debug_assert!(
!source_name.is_some_and(|source_name| source_name == name),
"extras not flattened for {name}"
);
dependencies.push((package.clone(), version.clone()));
}
_ => {}
}
}
}
Ok(Self(dependencies))
}

Expand All @@ -78,183 +81,6 @@ impl PubGrubDependencies {
}
}

/// Add a set of requirements to a list of dependencies.
#[allow(clippy::too_many_arguments)]
fn add_requirements(
requirements: &[Requirement],
dev_dependencies: &BTreeMap<GroupName, Vec<Requirement>>,
constraints: &Constraints,
overrides: &Overrides,
source_name: Option<&PackageName>,
source_extra: Option<&ExtraName>,
source_dev: Option<&GroupName>,
urls: &Urls,
locals: &Locals,
git: &GitResolver,
env: Option<&MarkerEnvironment>,
requires_python: Option<&MarkerTree>,
fork_markers: &MarkerTree,
dependencies: &mut Vec<(PubGrubPackage, Range<Version>)>,
seen: &mut FxHashSet<ExtraName>,
) -> Result<(), ResolveError> {
// Iterate over all declared requirements.
for requirement in overrides.apply(if let Some(source_dev) = source_dev {
Either::Left(dev_dependencies.get(source_dev).into_iter().flatten())
} else {
Either::Right(requirements.iter())
}) {
// If the requirement would not be selected with any Python version
// supported by the root, skip it.
if !satisfies_requires_python(requires_python, requirement) {
trace!(
"skipping {requirement} because of Requires-Python {requires_python}",
// OK because this filter only applies when there is a present
// Requires-Python specifier.
requires_python = requires_python.unwrap()
);
continue;
}
// If we're in universal mode, `fork_markers` might correspond to a
// non-trivial marker expression that provoked the resolver to fork.
// In that case, we should ignore any dependency that cannot possibly
// satisfy the markers that provoked the fork.
if !possible_to_satisfy_markers(fork_markers, requirement) {
trace!("skipping {requirement} because of context resolver markers {fork_markers}");
continue;
}
// If the requirement isn't relevant for the current platform, skip it.
match source_extra {
Some(source_extra) => {
// Only include requirements that are relevant for the current extra.
if requirement.evaluate_markers(env, &[]) {
continue;
}
if !requirement.evaluate_markers(env, std::slice::from_ref(source_extra)) {
continue;
}
}
None => {
if !requirement.evaluate_markers(env, &[]) {
continue;
}
}
}

// Add the package, plus any extra variants.
for result in std::iter::once(PubGrubRequirement::from_requirement(
requirement,
None,
urls,
locals,
git,
))
.chain(requirement.extras.clone().into_iter().map(|extra| {
PubGrubRequirement::from_requirement(requirement, Some(extra), urls, locals, git)
})) {
let PubGrubRequirement { package, version } = result?;

match &*package {
PubGrubPackageInner::Package { name, .. } => {
// Detect self-dependencies.
if source_name.is_some_and(|source_name| source_name == name) {
warn!("{name} has a dependency on itself");
continue;
}

dependencies.push((package.clone(), version.clone()));
}
PubGrubPackageInner::Marker { .. } => {
dependencies.push((package.clone(), version.clone()));
}
PubGrubPackageInner::Extra { name, extra, .. } => {
// Recursively add the dependencies of the current package (e.g., `black` depending on
// `black[colorama]`).
if source_name.is_some_and(|source_name| source_name == name) {
if seen.insert(extra.clone()) {
add_requirements(
requirements,
dev_dependencies,
constraints,
overrides,
source_name,
Some(extra),
None,
urls,
locals,
git,
env,
requires_python,
fork_markers,
dependencies,
seen,
)?;
}
} else {
dependencies.push((package.clone(), version.clone()));
}
}
_ => {}
}

// If the requirement was constrained, add those constraints.
for constraint in constraints.get(&requirement.name).into_iter().flatten() {
// If the requirement would not be selected with any Python
// version supported by the root, skip it.
if !satisfies_requires_python(requires_python, constraint) {
trace!(
"skipping {requirement} because of Requires-Python {requires_python}",
// OK because this filter only applies when there is a present
// Requires-Python specifier.
requires_python = requires_python.unwrap()
);
continue;
}
// If we're in universal mode, `fork_markers` might correspond
// to a non-trivial marker expression that provoked the
// resolver to fork. In that case, we should ignore any
// dependency that cannot possibly satisfy the markers that
// provoked the fork.
if !possible_to_satisfy_markers(fork_markers, constraint) {
trace!(
"skipping {requirement} because of context resolver markers {fork_markers}"
);
continue;
}
// If the requirement isn't relevant for the current platform, skip it.
match source_extra {
Some(source_extra) => {
if !constraint.evaluate_markers(env, std::slice::from_ref(source_extra)) {
continue;
}
}
None => {
if !constraint.evaluate_markers(env, &[]) {
continue;
}
}
}

// Add the package.
let PubGrubRequirement { package, version } =
PubGrubRequirement::from_constraint(constraint, urls, locals, git)?;

// Ignore self-dependencies.
if let PubGrubPackageInner::Package { name, .. } = &*package {
// Detect self-dependencies.
if source_name.is_some_and(|source_name| source_name == name) {
warn!("{name} has a dependency on itself");
continue;
}
}

dependencies.push((package.clone(), version.clone()));
}
}
}

Ok(())
}

/// Convert a [`PubGrubDependencies`] to a [`DependencyConstraints`].
impl From<PubGrubDependencies> for Vec<(PubGrubPackage, Range<Version>)> {
fn from(dependencies: PubGrubDependencies) -> Self {
Expand Down Expand Up @@ -455,39 +281,4 @@ impl PubGrubRequirement {
}
}
}

/// Convert a constraint to a PubGrub-compatible package and range.
pub(crate) fn from_constraint(
constraint: &Requirement,
urls: &Urls,
locals: &Locals,
git: &GitResolver,
) -> Result<Self, ResolveError> {
Self::from_requirement(constraint, None, urls, locals, git)
}
}

/// Returns true if and only if the given requirement's marker expression has a
/// possible true value given the `requires_python` specifier given.
///
/// While this is always called, a `requires_python` is only non-None when in
/// universal resolution mode. In non-universal mode, `requires_python` is
/// `None` and this always returns `true`.
fn satisfies_requires_python(
requires_python: Option<&MarkerTree>,
requirement: &Requirement,
) -> bool {
let Some(requires_python) = requires_python else {
return true;
};
possible_to_satisfy_markers(requires_python, requirement)
}

/// Returns true if and only if the given requirement's marker expression has a
/// possible true value given the `markers` expression given.
fn possible_to_satisfy_markers(markers: &MarkerTree, requirement: &Requirement) -> bool {
let Some(marker) = requirement.marker.as_ref() else {
return true;
};
!crate::marker::is_disjoint(markers, marker)
}
Loading

0 comments on commit f2f48d3

Please sign in to comment.