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.

Sign up for GitHub

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

Jump to bottom

Allow specific --only-binary and --no-binary packages to override :all: #4067

Merged
merged 1 commit into from
Jun 12, 2024
Merged

Allow specific --only-binary and --no-binary packages to override :all: #4067

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
13 changes: 12 additions & 1 deletion crates/uv-dispatch/src/lib.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -218,6 +218,7 @@ impl<'a> BuildContext for BuildDispatch<'a> {
site_packages,
&Reinstall::default(),
&NoBinary::default(),
&NoBuild::default(),
&HashStrategy::default(),
self.index_locations,
self.cache(),
Expand Down Expand Up @@ -316,7 +317,17 @@ impl<'a> BuildContext for BuildDispatch<'a> {
) -> Result<SourceBuild> {
match self.no_build {
NoBuild::All => debug_assert!(
matches!(build_kind, BuildKind::Editable),
match self.no_binary {
// Allow `all` to be overridden by specific binary exclusions
NoBinary::Packages(packages) => {
if let Some(dist) = dist {
packages.contains(dist.name())
} else {
false
}
}
_ => false,
} || matches!(build_kind, BuildKind::Editable),
zanieb marked this conversation as resolved.
Show resolved Hide resolved
"Only editable builds are exempt from 'no build' checks"
),
NoBuild::None => {}
Expand Down
14 changes: 12 additions & 2 deletions crates/uv-distribution/src/distribution_database.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -147,7 +147,11 @@ impl<'a, Context: BuildContext> DistributionDatabase<'a, Context> {
) -> Result<LocalWheel, Error> {
let no_binary = match self.build_context.no_binary() {
NoBinary::None => false,
NoBinary::All => true,
NoBinary::All => match self.build_context.no_build() {
// Allow `all` to be overridden by specific build exclusions
NoBuild::Packages(packages) => !packages.contains(dist.name()),
_ => true,
},
NoBinary::Packages(packages) => packages.contains(dist.name()),
};

Expand Down Expand Up @@ -415,7 +419,13 @@ impl<'a, Context: BuildContext> DistributionDatabase<'a, Context> {
hashes: HashPolicy<'_>,
) -> Result<ArchiveMetadata, Error> {
let no_build = match self.build_context.no_build() {
NoBuild::All => true,
NoBuild::All => match self.build_context.no_binary() {
// Allow `all` to be overridden by specific binary exclusions
NoBinary::Packages(packages) => {
!source.name().is_some_and(|name| packages.contains(name))
}
_ => true,
},
NoBuild::None => false,
NoBuild::Packages(packages) => {
source.name().is_some_and(|name| packages.contains(name))
Expand Down
10 changes: 8 additions & 2 deletions crates/uv-distribution/src/source/mod.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,7 +28,7 @@ use uv_cache::{
use uv_client::{
CacheControl, CachedClientError, Connectivity, DataWithCachePolicy, RegistryClient,
};
use uv_configuration::{BuildKind, NoBuild, PreviewMode};
use uv_configuration::{BuildKind, NoBinary, NoBuild, PreviewMode};
use uv_extract::hash::Hasher;
use uv_fs::{write_atomic, LockedFile};
use uv_types::{BuildContext, SourceBuildTrait};
Expand Down Expand Up @@ -1381,7 +1381,13 @@ impl<'a, T: BuildContext> SourceDistributionBuilder<'a, T> {

// Guard against build of source distributions when disabled.
let no_build = match self.build_context.no_build() {
NoBuild::All => true,
NoBuild::All => match self.build_context.no_binary() {
// Allow `all` to be overridden by specific binary exclusions
NoBinary::Packages(packages) => {
!source.name().is_some_and(|name| packages.contains(name))
}
_ => true,
},
NoBuild::None => false,
NoBuild::Packages(packages) => {
source.name().is_some_and(|name| packages.contains(name))
Expand Down
9 changes: 7 additions & 2 deletions crates/uv-installer/src/plan.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,7 @@ use distribution_types::{
use platform_tags::Tags;
use pypi_types::{Requirement, RequirementSource};
use uv_cache::{ArchiveTimestamp, Cache, CacheBucket, WheelCache};
use uv_configuration::{NoBinary, Reinstall};
use uv_configuration::{NoBinary, NoBuild, Reinstall};
use uv_distribution::{
BuiltWheelIndex, HttpArchivePointer, LocalArchivePointer, RegistryWheelIndex,
};
Expand Down Expand Up @@ -57,6 +57,7 @@ impl<'a> Planner<'a> {
mut site_packages: SitePackages,
reinstall: &Reinstall,
no_binary: &NoBinary,
no_build: &NoBuild,
hasher: &HashStrategy,
index_locations: &IndexLocations,
cache: &Cache,
Expand Down Expand Up @@ -109,7 +110,11 @@ impl<'a> Planner<'a> {
// Check if installation of a binary version of the package should be allowed.
let no_binary = match no_binary {
NoBinary::None => false,
NoBinary::All => true,
NoBinary::All => match no_build {
// Allow `all` to be overridden by specific build exclusions
NoBuild::Packages(packages) => !packages.contains(&requirement.name),
_ => true,
},
NoBinary::Packages(packages) => packages.contains(&requirement.name),
};

Expand Down
33 changes: 27 additions & 6 deletions crates/uv-resolver/src/flat_index.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -78,8 +78,14 @@ impl FlatIndex {
DistFilename::WheelFilename(filename) => {
let version = filename.version.clone();

let compatibility =
Self::wheel_compatibility(&filename, &file.hashes, tags, hasher, no_binary);
let compatibility = Self::wheel_compatibility(
&filename,
&file.hashes,
tags,
hasher,
no_binary,
no_build,
);
let dist = RegistryBuiltWheel {
filename,
file: Box::new(file),
Expand All @@ -95,8 +101,13 @@ impl FlatIndex {
}
}
DistFilename::SourceDistFilename(filename) => {
let compatibility =
Self::source_dist_compatibility(&filename, &file.hashes, hasher, no_build);
let compatibility = Self::source_dist_compatibility(
&filename,
&file.hashes,
hasher,
no_build,
no_binary,
);
let dist = RegistrySourceDist {
name: filename.name.clone(),
version: filename.version.clone(),
Expand All @@ -121,11 +132,16 @@ impl FlatIndex {
hashes: &[HashDigest],
hasher: &HashStrategy,
no_build: &NoBuild,
no_binary: &NoBinary,
) -> SourceDistCompatibility {
// Check if source distributions are allowed for this package.
let no_build = match no_build {
NoBuild::None => false,
NoBuild::All => true,
NoBuild::All => match no_binary {
// Allow `all` to be overridden by specific binary exclusions
NoBinary::Packages(packages) => !packages.contains(&filename.name),
_ => true,
},
NoBuild::Packages(packages) => packages.contains(&filename.name),
};

Expand Down Expand Up @@ -155,11 +171,16 @@ impl FlatIndex {
tags: Option<&Tags>,
hasher: &HashStrategy,
no_binary: &NoBinary,
no_build: &NoBuild,
) -> WheelCompatibility {
// Check if binaries are allowed for this package.
let no_binary = match no_binary {
NoBinary::None => false,
NoBinary::All => true,
NoBinary::All => match no_build {
// Allow `all` to be overridden by specific build exclusions
NoBuild::Packages(packages) => !packages.contains(&filename.name),
_ => true,
},
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could we instead create a struct that couples NoBinary and NoBuild, and lets us query for the no_binary or no_build status, abstracting away this repeated logic? It seems really easy to miss a site as-is.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah we should totally do that as noted at #4067 (comment) but I'd rather do it in a follow-up.

I didn't want to invest in a refactor until we were on the same page about the behavior. I could do it here too if you think this makes sense.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the behavior seems ok. Do you want to do it in this PR or na?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't really mind doing it here or at least writing the follow-up before merge.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

See #4284 — opted for a separate pull request for review purposes.

NoBinary::Packages(packages) => packages.contains(&filename.name),
};

Expand Down
34 changes: 22 additions & 12 deletions crates/uv-resolver/src/version_map.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -96,18 +96,28 @@ impl VersionMap {
},
}
}
// Check if binaries are allowed for this package.
let no_binary = match no_binary {
NoBinary::None => false,
NoBinary::All => true,
NoBinary::Packages(packages) => packages.contains(package_name),
};
// Check if source distributions are allowed for this package.
let no_build = match no_build {
NoBuild::None => false,
NoBuild::All => true,
NoBuild::Packages(packages) => packages.contains(package_name),
};
let (no_binary, no_build) = (
// Check if binaries are allowed for this package.
match no_binary {
NoBinary::None => false,
NoBinary::All => match no_build {
// Allow `all` to be overridden by specific build exclusions
NoBuild::Packages(packages) => !packages.contains(package_name),
_ => true,
},
NoBinary::Packages(packages) => packages.contains(package_name),
},
// Check if source distributions are allowed for this package.
match no_build {
NoBuild::None => false,
NoBuild::All => match no_binary {
// Allow `all` to be overridden by specific binary exclusions
NoBinary::Packages(packages) => !packages.contains(package_name),
_ => true,
},
NoBuild::Packages(packages) => packages.contains(package_name),
},
);
let allowed_yanks = allowed_yanks
.allowed_versions(package_name)
.cloned()
Expand Down
1 change: 1 addition & 0 deletions crates/uv/src/commands/pip/install.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -391,6 +391,7 @@ pub(crate) async fn pip_install(
Modifications::Sufficient,
&reinstall,
&no_binary,
&no_build,
link_mode,
compile,
&index_locations,
Expand Down
6 changes: 4 additions & 2 deletions crates/uv/src/commands/pip/operations.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,8 +21,8 @@ use pypi_types::Requirement;
use uv_cache::Cache;
use uv_client::{BaseClientBuilder, RegistryClient};
use uv_configuration::{
Concurrency, Constraints, ExtrasSpecification, NoBinary, Overrides, PreviewMode, Reinstall,
Upgrade,
Concurrency, Constraints, ExtrasSpecification, NoBinary, NoBuild, Overrides, PreviewMode,
Reinstall, Upgrade,
};
use uv_dispatch::BuildDispatch;
use uv_distribution::DistributionDatabase;
Expand Down Expand Up @@ -290,6 +290,7 @@ pub(crate) async fn install(
modifications: Modifications,
reinstall: &Reinstall,
no_binary: &NoBinary,
no_build: &NoBuild,
link_mode: LinkMode,
compile: bool,
index_urls: &IndexLocations,
Expand Down Expand Up @@ -317,6 +318,7 @@ pub(crate) async fn install(
site_packages,
reinstall,
no_binary,
no_build,
hasher,
index_urls,
cache,
Expand Down
1 change: 1 addition & 0 deletions crates/uv/src/commands/pip/sync.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -343,6 +343,7 @@ pub(crate) async fn pip_sync(
Modifications::Exact,
reinstall,
&no_binary,
&no_build,
link_mode,
compile,
&index_locations,
Expand Down
1 change: 1 addition & 0 deletions crates/uv/src/commands/project/mod.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -350,6 +350,7 @@ pub(crate) async fn update_environment(
pip::operations::Modifications::Sufficient,
&reinstall,
&no_binary,
&no_build,
link_mode,
compile,
index_locations,
Expand Down
1 change: 1 addition & 0 deletions crates/uv/src/commands/project/sync.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -174,6 +174,7 @@ pub(super) async fn do_sync(
Modifications::Sufficient,
&reinstall,
&no_binary,
&no_build,
link_mode,
compile,
index_locations,
Expand Down
Loading
Loading