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

Build source distributions at top-level of cache #8905

Merged
merged 1 commit into from
Nov 8, 2024
Merged

Build source distributions at top-level of cache #8905

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
15 changes: 3 additions & 12 deletions crates/uv-build-frontend/src/lib.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,7 @@ use std::rc::Rc;
use std::str::FromStr;
use std::sync::LazyLock;
use std::{env, iter};
use tempfile::{tempdir_in, TempDir};
use tempfile::TempDir;
use tokio::io::AsyncBufReadExt;
use tokio::process::Command;
use tokio::sync::{Mutex, Semaphore};
Expand All @@ -30,7 +30,7 @@ use tracing::{debug, info_span, instrument, Instrument};
use uv_configuration::{BuildKind, BuildOutput, ConfigSettings, LowerBound, SourceStrategy};
use uv_distribution::RequiresDist;
use uv_distribution_types::{IndexLocations, Resolution};
use uv_fs::{rename_with_retry, PythonExt, Simplified};
use uv_fs::{PythonExt, Simplified};
use uv_pep440::Version;
use uv_pep508::PackageName;
use uv_pypi_types::{Requirement, VerbatimParsedUrl};
Expand Down Expand Up @@ -656,16 +656,7 @@ impl SourceBuild {
pub async fn build(&self, wheel_dir: &Path) -> Result<String, Error> {
// The build scripts run with the extracted root as cwd, so they need the absolute path.
let wheel_dir = std::path::absolute(wheel_dir)?;

// Prevent clashes from two uv processes building distributions in parallel.
let tmp_dir = tempdir_in(&wheel_dir)?;
let filename = self
.pep517_build(tmp_dir.path(), &self.pep517_backend)
.await?;

let from = tmp_dir.path().join(&filename);
let to = wheel_dir.join(&filename);
rename_with_retry(from, to).await?;
let filename = self.pep517_build(&wheel_dir, &self.pep517_backend).await?;
Ok(filename)
}

Expand Down
17 changes: 16 additions & 1 deletion crates/uv-distribution/src/source/mod.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1751,6 +1751,13 @@ impl<'a, T: BuildContext> SourceDistributionBuilder<'a, T> {
}
}

// Build into a temporary directory, to prevent partial builds.
let build = self
.build_context
.cache()
.environment()
Copy link
Member

Choose a reason for hiding this comment

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

Sort of unclear that this returns a temporary directory and surprising that it's appropriate for building wheels in.

Copy link
Member

Choose a reason for hiding this comment

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

@charliermarsh just pinging since you merged as I commented and it might get lost.

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 think it's the intent of the bucket? I can look at making it clearer though.

Copy link
Member

Choose a reason for hiding this comment

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

First it was unclear if the goal of the removed code was being upheld

    // Prevent clashes from two uv processes building distributions in parallel.
   let tmp_dir = tempdir_in(&wheel_dir)?;

Then I was confused by the documentation for the method

/// Create an ephemeral Python environment in the cache.

I'm not super familiar with the cache methods though. Not a big deal either way.

.map_err(Error::CacheWrite)?;

// Build the wheel.
fs::create_dir_all(&cache_shard)
.await
Expand All @@ -1773,10 +1780,18 @@ impl<'a, T: BuildContext> SourceDistributionBuilder<'a, T> {
)
.await
.map_err(Error::Build)?
.wheel(cache_shard)
.wheel(build.path())
.await
.map_err(Error::Build)?;

// Move the wheel to the cache.
rename_with_retry(
build.path().join(&disk_filename),
cache_shard.join(&disk_filename),
)
.await
.map_err(Error::CacheWrite)?;

// Read the metadata from the wheel.
let filename = WheelFilename::from_str(&disk_filename)?;
let metadata = read_wheel_metadata(&filename, &cache_shard.join(&disk_filename))?;
Expand Down
Loading