Skip to content

Commit

Permalink
Apply --no-install options when constructing resolution (#7277)
Browse files Browse the repository at this point in the history
## Summary

We need to apply the `--no-install` filters earlier, such that we don't
error if we only have a source distribution for a given package when
`--no-build` is provided but that package is _omitted_.

Closes #7247.
  • Loading branch information
charliermarsh authored Sep 11, 2024
1 parent 7021b15 commit 15792a3
Show file tree
Hide file tree
Showing 7 changed files with 114 additions and 82 deletions.
1 change: 0 additions & 1 deletion Cargo.lock

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

1 change: 0 additions & 1 deletion crates/uv-configuration/Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,6 @@ workspace = true

[dependencies]
cache-key = { workspace = true }
distribution-types = { workspace = true }
pep508_rs = { workspace = true, features = ["schemars"] }
platform-tags = { workspace = true }
pypi-types = { workspace = true }
Expand Down
96 changes: 32 additions & 64 deletions crates/uv-configuration/src/install_options.rs
Original file line number Diff line number Diff line change
@@ -1,15 +1,16 @@
use std::collections::BTreeSet;

use rustc_hash::FxHashSet;
use tracing::debug;

use distribution_types::{Name, Resolution};
use pep508_rs::PackageName;

#[derive(Debug, Clone, Default)]
pub struct InstallOptions {
/// Omit the project itself from the resolution.
pub no_install_project: bool,
/// Omit all workspace members (including the project itself) from the resolution.
pub no_install_workspace: bool,
/// Omit the specified packages from the resolution.
pub no_install_package: Vec<PackageName>,
}

Expand All @@ -26,81 +27,48 @@ impl InstallOptions {
}
}

pub fn filter_resolution(
&self,
resolution: Resolution,
project_name: Option<&PackageName>,
members: &BTreeSet<PackageName>,
) -> Resolution {
// If `--no-install-project` is set, remove the project itself.
let resolution = self.apply_no_install_project(resolution, project_name);

// If `--no-install-workspace` is set, remove the project and any workspace members.
let resolution = self.apply_no_install_workspace(resolution, members);

// If `--no-install-package` is provided, remove the requested packages.
self.apply_no_install_package(resolution)
}

fn apply_no_install_project(
&self,
resolution: Resolution,
project_name: Option<&PackageName>,
) -> Resolution {
if !self.no_install_project {
return resolution;
}

let Some(project_name) = project_name else {
debug!("Ignoring `--no-install-project` for virtual workspace");
return resolution;
};

resolution.filter(|dist| dist.name() != project_name)
}

fn apply_no_install_workspace(
&self,
resolution: Resolution,
members: &BTreeSet<PackageName>,
) -> Resolution {
if !self.no_install_workspace {
return resolution;
}

resolution.filter(|dist| !members.contains(dist.name()))
}

fn apply_no_install_package(&self, resolution: Resolution) -> Resolution {
if self.no_install_package.is_empty() {
return resolution;
}

let no_install_packages = self.no_install_package.iter().collect::<FxHashSet<_>>();

resolution.filter(|dist| !no_install_packages.contains(dist.name()))
}

/// Returns `true` if a package passes the install filters.
pub fn include_package(
&self,
package: &PackageName,
project_name: &PackageName,
project_name: Option<&PackageName>,
members: &BTreeSet<PackageName>,
) -> bool {
// If `--no-install-project` is set, remove the project itself. The project is always
// part of the workspace.
if (self.no_install_project || self.no_install_workspace) && package == project_name {
return false;
// If `--no-install-project` is set, remove the project itself.
if self.no_install_project {
if let Some(project_name) = project_name {
if package == project_name {
debug!("Omitting `{package}` from resolution due to `--no-install-project`");
return false;
}
}
}

// If `--no-install-workspace` is set, remove the project and any workspace members.
if self.no_install_workspace && members.contains(package) {
return false;
if self.no_install_workspace {
// In some cases, the project root might be omitted from the list of workspace members
// encoded in the lockfile. (But we already checked this above if `--no-install-project`
// is set.)
if !self.no_install_project {
if let Some(project_name) = project_name {
if package == project_name {
debug!(
"Omitting `{package}` from resolution due to `--no-install-workspace`"
);
return false;
}
}
}

if members.contains(package) {
debug!("Omitting `{package}` from resolution due to `--no-install-workspace`");
return false;
}
}

// If `--no-install-package` is provided, remove the requested packages.
if self.no_install_package.contains(package) {
debug!("Omitting `{package}` from resolution due to `--no-install-package`");
return false;
}

Expand Down
27 changes: 17 additions & 10 deletions crates/uv-resolver/src/lock/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -30,7 +30,7 @@ use pypi_types::{
redact_git_credentials, HashDigest, ParsedArchiveUrl, ParsedGitUrl, Requirement,
RequirementSource, ResolverMarkerEnvironment,
};
use uv_configuration::{BuildOptions, ExtrasSpecification};
use uv_configuration::{BuildOptions, ExtrasSpecification, InstallOptions};
use uv_distribution::DistributionDatabase;
use uv_fs::{relative_to, PortablePath, PortablePathBuf};
use uv_git::{GitReference, GitSha, RepositoryReference, ResolvedRepositoryReference};
Expand Down Expand Up @@ -547,6 +547,7 @@ impl Lock {
extras: &ExtrasSpecification,
dev: &[GroupName],
build_options: &BuildOptions,
install_options: &InstallOptions,
) -> Result<Resolution, LockError> {
let mut queue: VecDeque<(&Package, Option<&ExtraName>)> = VecDeque::new();
let mut seen = FxHashSet::default();
Expand Down Expand Up @@ -633,15 +634,21 @@ impl Lock {
}
}
}
map.insert(
dist.id.name.clone(),
ResolvedDist::Installable(dist.to_dist(
project.workspace().install_path(),
tags,
build_options,
)?),
);
hashes.insert(dist.id.name.clone(), dist.hashes());
if install_options.include_package(
&dist.id.name,
project.project_name(),
&self.manifest.members,
) {
map.insert(
dist.id.name.clone(),
ResolvedDist::Installable(dist.to_dist(
project.workspace().install_path(),
tags,
build_options,
)?),
);
hashes.insert(dist.id.name.clone(), dist.hashes());
}
}
let diagnostics = vec![];
Ok(Resolution::new(map, hashes, diagnostics))
Expand Down
2 changes: 1 addition & 1 deletion crates/uv-resolver/src/lock/requirements_txt.rs
Original file line number Diff line number Diff line change
Expand Up @@ -134,7 +134,7 @@ impl<'lock> RequirementsTxtExport<'lock> {
let mut nodes: Vec<Node> = petgraph
.node_references()
.filter(|(_index, package)| {
install_options.include_package(&package.id.name, root_name, lock.members())
install_options.include_package(&package.id.name, Some(root_name), lock.members())
})
.map(|(index, package)| Node {
package,
Expand Down
14 changes: 9 additions & 5 deletions crates/uv/src/commands/project/sync.rs
Original file line number Diff line number Diff line change
Expand Up @@ -219,15 +219,19 @@ pub(super) async fn do_sync(
let tags = venv.interpreter().tags()?;

// Read the lockfile.
let resolution = lock.to_resolution(target, &markers, tags, extras, &dev, build_options)?;
let resolution = lock.to_resolution(
target,
&markers,
tags,
extras,
&dev,
build_options,
&install_options,
)?;

// Always skip virtual projects, which shouldn't be built or installed.
let resolution = apply_no_virtual_project(resolution);

// Filter resolution based on install-specific options.
let resolution =
install_options.filter_resolution(resolution, target.project_name(), lock.members());

// Add all authenticated sources to the cache.
for url in index_locations.urls() {
store_credentials_from_url(url);
Expand Down
55 changes: 55 additions & 0 deletions crates/uv/tests/sync.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1222,6 +1222,61 @@ fn no_install_package() -> Result<()> {
Ok(())
}

/// Ensure that `--no-build` isn't enforced for projects that aren't installed in the first place.
#[test]
fn no_install_project_no_build() -> 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 = ["anyio==3.7.0"]
[build-system]
requires = ["setuptools>=42"]
build-backend = "setuptools.build_meta"
"#,
)?;

// Generate a lockfile.
context.lock().assert().success();

// `--no-build` should raise an error, since we try to install the project.
uv_snapshot!(context.filters(), context.sync().arg("--no-build"), @r###"
success: false
exit_code: 2
----- stdout -----
----- stderr -----
warning: Failed to validate existing lockfile: distribution project==0.1.0 @ editable+. can't be installed because it is marked as `--no-build` but has no binary distribution
Resolved 4 packages in [TIME]
error: distribution project==0.1.0 @ editable+. can't be installed because it is marked as `--no-build` but has no binary distribution
"###);

// But it's fine to combine `--no-install-project` with `--no-build`. We shouldn't error, since
// we aren't building the project.
uv_snapshot!(context.filters(), context.sync().arg("--no-install-project").arg("--no-build"), @r###"
success: true
exit_code: 0
----- stdout -----
----- stderr -----
warning: Failed to validate existing lockfile: distribution project==0.1.0 @ editable+. can't be installed because it is marked as `--no-build` but has no binary distribution
Resolved 4 packages in [TIME]
Prepared 3 packages in [TIME]
Installed 3 packages in [TIME]
+ anyio==3.7.0
+ idna==3.6
+ sniffio==1.3.1
"###);

Ok(())
}

/// Convert from a package to a virtual project.
#[test]
fn convert_to_virtual() -> Result<()> {
Expand Down

0 comments on commit 15792a3

Please sign in to comment.