Skip to content

Commit

Permalink
Enable install audits without resolving named requirements (#2575)
Browse files Browse the repository at this point in the history 
## Summary

This PR ensures that if a package is already satisfied by the current
environment, we don't bother resolving the named requirement.

Part of: #313.

## Test Plan

- `cargo run pip install ./scripts/editable-installs/black_editable`
- `cargo run pip install black --verbose`
  • Loading branch information
@charliermarsh
charliermarsh authored Mar 21, 2024
1 parent e5b0cf7 commit 34bef37
Show file tree
Hide file tree
Showing 4 changed files with 148 additions and 41 deletions.
74 changes: 73 additions & 1 deletion crates/pep508-rs/src/lib.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -473,6 +473,69 @@ impl Requirement {
}
}

impl UnnamedRequirement {
/// Returns whether the markers apply for the given environment
pub fn evaluate_markers(&self, env: &MarkerEnvironment, extras: &[ExtraName]) -> bool {
if let Some(marker) = &self.marker {
marker.evaluate(env, extras)
} else {
true
}
}
}

impl RequirementsTxtRequirement {
/// Returns whether the markers apply for the given environment
pub fn evaluate_markers(&self, env: &MarkerEnvironment, extras: &[ExtraName]) -> bool {
match self {
Self::Pep508(requirement) => requirement.evaluate_markers(env, extras),
Self::Unnamed(requirement) => requirement.evaluate_markers(env, extras),
}
}

/// Returns the extras for the requirement.
pub fn extras(&self) -> &[ExtraName] {
match self {
Self::Pep508(requirement) => requirement.extras.as_slice(),
Self::Unnamed(requirement) => requirement.extras.as_slice(),
}
}

/// Returns the markers for the requirement.
pub fn markers(&self) -> Option<&MarkerTree> {
match self {
Self::Pep508(requirement) => requirement.marker.as_ref(),
Self::Unnamed(requirement) => requirement.marker.as_ref(),
}
}

/// Return the version specifier or URL for the requirement.
pub fn version_or_url(&self) -> Option<VersionOrUrlRef> {
match self {
Self::Pep508(requirement) => match requirement.version_or_url.as_ref() {
Some(VersionOrUrl::VersionSpecifier(specifiers)) => {
Some(VersionOrUrlRef::VersionSpecifier(specifiers))
}
Some(VersionOrUrl::Url(url)) => Some(VersionOrUrlRef::Url(url)),
None => None,
},
Self::Unnamed(requirement) => Some(VersionOrUrlRef::Url(&requirement.url)),
}
}
}

impl From<Requirement> for RequirementsTxtRequirement {
fn from(requirement: Requirement) -> Self {
Self::Pep508(requirement)
}
}

impl From<UnnamedRequirement> for RequirementsTxtRequirement {
fn from(requirement: UnnamedRequirement) -> Self {
Self::Unnamed(requirement)
}
}

impl FromStr for Requirement {
type Err = Pep508Error;

Expand Down Expand Up @@ -558,7 +621,7 @@ impl Extras {
}
}

/// The actual version specifier or url to install
/// The actual version specifier or URL to install.
#[derive(Debug, Clone, Eq, Hash, PartialEq)]
pub enum VersionOrUrl {
/// A PEP 440 version specifier set
Expand All @@ -567,6 +630,15 @@ pub enum VersionOrUrl {
Url(VerbatimUrl),
}

/// Unowned version specifier or URL to install.
#[derive(Debug, Clone, Eq, Hash, PartialEq)]
pub enum VersionOrUrlRef<'a> {
/// A PEP 440 version specifier set
VersionSpecifier(&'a VersionSpecifiers),
/// A installable URL
Url(&'a VerbatimUrl),
}

/// A [`Cursor`] over a string.
#[derive(Debug, Clone)]
pub struct Cursor<'a> {
Expand Down
77 changes: 55 additions & 22 deletions crates/uv-installer/src/site_packages.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,7 @@ use url::Url;

use distribution_types::{InstalledDist, InstalledMetadata, InstalledVersion, Name};
use pep440_rs::{Version, VersionSpecifiers};
use pep508_rs::{Requirement, VerbatimUrl};
use pep508_rs::{Requirement, RequirementsTxtRequirement, VerbatimUrl};
use requirements_txt::EditableRequirement;
use uv_cache::{ArchiveTarget, ArchiveTimestamp};
use uv_interpreter::PythonEnvironment;
Expand Down Expand Up @@ -80,8 +80,11 @@ impl<'a> SitePackages<'a> {
.push(idx);

// Index the distribution by URL.
if let Some(url) = dist_info.as_editable() {
by_url.entry(url.clone()).or_insert_with(Vec::new).push(idx);
if let InstalledDist::Url(dist) = &dist_info {
by_url
.entry(dist.url.clone())
.or_insert_with(Vec::new)
.push(idx);
}

// Add the distribution to the database.
Expand Down Expand Up @@ -144,6 +147,17 @@ impl<'a> SitePackages<'a> {
.collect()
}

/// Returns the distributions installed from the given URL, if any.
pub fn get_urls(&self, url: &Url) -> Vec<&InstalledDist> {
let Some(indexes) = self.by_url.get(url) else {
return Vec::new();
};
indexes
.iter()
.flat_map(|&index| &self.distributions[index])
.collect()
}

/// Returns the editable distribution installed from the given URL, if any.
pub fn get_editables(&self, url: &Url) -> Vec<&InstalledDist> {
let Some(indexes) = self.by_url.get(url) else {
Expand All @@ -152,6 +166,7 @@ impl<'a> SitePackages<'a> {
indexes
.iter()
.flat_map(|&index| &self.distributions[index])
.filter(|dist| dist.is_editable())
.collect()
}

Expand All @@ -162,7 +177,14 @@ impl<'a> SitePackages<'a> {
};
indexes
.iter()
.filter_map(|index| std::mem::take(&mut self.distributions[*index]))
.filter_map(|index| {
let dist = &mut self.distributions[*index];
if dist.as_ref().is_some_and(InstalledDist::is_editable) {
std::mem::take(dist)
} else {
None
}
})
.collect()
}

Expand Down Expand Up @@ -268,20 +290,20 @@ impl<'a> SitePackages<'a> {
/// Returns `true` if the installed packages satisfy the given requirements.
pub fn satisfies(
&self,
requirements: &[Requirement],
requirements: &[RequirementsTxtRequirement],
editables: &[EditableRequirement],
constraints: &[Requirement],
) -> Result<bool> {
let mut stack = Vec::<Requirement>::with_capacity(requirements.len());
let mut stack = Vec::<RequirementsTxtRequirement>::with_capacity(requirements.len());
let mut seen =
FxHashSet::with_capacity_and_hasher(requirements.len(), BuildHasherDefault::default());

// Add the direct requirements to the queue.
for dependency in requirements {
if dependency.evaluate_markers(self.venv.interpreter().markers(), &[])
&& seen.insert(dependency.clone())
{
stack.push(dependency.clone());
if dependency.evaluate_markers(self.venv.interpreter().markers(), &[]) {
if seen.insert(dependency.clone()) {
stack.push(dependency.clone());
}
}
}

Expand Down Expand Up @@ -317,9 +339,11 @@ impl<'a> SitePackages<'a> {
if dependency.evaluate_markers(
self.venv.interpreter().markers(),
&requirement.extras,
) && seen.insert(dependency.clone())
{
stack.push(dependency);
) {
let dependency = RequirementsTxtRequirement::from(dependency);
if seen.insert(dependency.clone()) {
stack.push(dependency);
}
}
}
}
Expand All @@ -332,20 +356,27 @@ impl<'a> SitePackages<'a> {

// Verify that all non-editable requirements are met.
while let Some(requirement) = stack.pop() {
let installed = self.get_packages(&requirement.name);
let installed = match &requirement {
RequirementsTxtRequirement::Pep508(requirement) => {
self.get_packages(&requirement.name)
}
RequirementsTxtRequirement::Unnamed(requirement) => {
self.get_urls(requirement.url.raw())
}
};
match installed.as_slice() {
[] => {
// The package isn't installed.
return Ok(false);
}
[distribution] => {
// Validate that the installed version matches the requirement.
match &requirement.version_or_url {
match requirement.version_or_url() {
// Accept any installed version.
None => {}

// If the requirement comes from a URL, verify by URL.
Some(pep508_rs::VersionOrUrl::Url(url)) => {
Some(pep508_rs::VersionOrUrlRef::Url(url)) => {
let InstalledDist::Url(installed) = &distribution else {
return Ok(false);
};
Expand All @@ -365,7 +396,7 @@ impl<'a> SitePackages<'a> {
}
}

Some(pep508_rs::VersionOrUrl::VersionSpecifier(version_specifier)) => {
Some(pep508_rs::VersionOrUrlRef::VersionSpecifier(version_specifier)) => {
// The installed version doesn't satisfy the requirement.
if !version_specifier.contains(distribution.version()) {
return Ok(false);
Expand All @@ -375,7 +406,7 @@ impl<'a> SitePackages<'a> {

// Validate that the installed version satisfies the constraints.
for constraint in constraints {
if constraint.name != requirement.name {
if constraint.name != *distribution.name() {
continue;
}

Expand Down Expand Up @@ -426,10 +457,12 @@ impl<'a> SitePackages<'a> {
for dependency in metadata.requires_dist {
if dependency.evaluate_markers(
self.venv.interpreter().markers(),
&requirement.extras,
) && seen.insert(dependency.clone())
{
stack.push(dependency);
requirement.extras(),
) {
let dependency = RequirementsTxtRequirement::from(dependency);
if seen.insert(dependency.clone()) {
stack.push(dependency);
}
}
}
}
Expand Down
35 changes: 18 additions & 17 deletions crates/uv/src/commands/pip_install.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -81,17 +81,8 @@ pub(crate) async fn pip_install(
let start = Instant::now();

// Read all requirements from the provided sources.
let NamedRequirements {
project,
requirements,
constraints,
overrides,
editables,
index_url,
extra_index_urls,
no_index,
find_links,
} = read_requirements(requirements, constraints, overrides, extras, connectivity).await?;
let spec =
read_requirements(requirements, constraints, overrides, extras, connectivity).await?;

// Detect the current Python interpreter.
let venv = if let Some(python) = python.as_ref() {
Expand Down Expand Up @@ -137,9 +128,9 @@ pub(crate) async fn pip_install(
// magnitude faster to validate the environment than to resolve the requirements.
if reinstall.is_none()
&& upgrade.is_none()
&& site_packages.satisfies(&requirements, &editables, &constraints)?
&& site_packages.satisfies(&spec.requirements, &spec.editables, &spec.constraints)?
{
let num_requirements = requirements.len() + editables.len();
let num_requirements = spec.requirements.len() + spec.editables.len();
let s = if num_requirements == 1 { "" } else { "s" };
writeln!(
printer.stderr(),
Expand All @@ -157,6 +148,19 @@ pub(crate) async fn pip_install(
return Ok(ExitStatus::Success);
}

// Convert from unnamed to named requirements.
let NamedRequirements {
project,
requirements,
constraints,
overrides,
editables,
index_url,
extra_index_urls,
no_index,
find_links,
} = NamedRequirements::from_spec(spec)?;

// Determine the tags, markers, and interpreter to use for resolution.
let interpreter = venv.interpreter().clone();
let tags = venv.interpreter().tags()?;
Expand Down Expand Up @@ -338,7 +342,7 @@ async fn read_requirements(
overrides: &[RequirementsSource],
extras: &ExtrasSpecification<'_>,
connectivity: Connectivity,
) -> Result<NamedRequirements, Error> {
) -> Result<RequirementsSpecification, Error> {
// If the user requests `extras` but does not provide a pyproject toml source
if !matches!(extras, ExtrasSpecification::None)
&& !requirements
Expand Down Expand Up @@ -376,9 +380,6 @@ async fn read_requirements(
}
}

// Convert from unnamed to named requirements.
let spec = NamedRequirements::from_spec(spec)?;

Ok(spec)
}

Expand Down
3 changes: 2 additions & 1 deletion crates/uv/src/commands/pip_uninstall.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -81,7 +81,8 @@ pub(crate) async fn pip_uninstall(
// Index the current `site-packages` directory.
let site_packages = uv_installer::SitePackages::from_executable(&venv)?;

// Sort and deduplicate the packages, which are keyed by name.
// Sort and deduplicate the packages, which are keyed by name. Like `pip`, we ignore the
// dependency specifier (even if it's a URL).
let packages = {
let mut packages = requirements
.into_iter()
Expand Down

0 comments on commit 34bef37

Please sign in to comment.