Skip to content

Calculate file options for WheelWriter once and cache the result #2865

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

Merged
messense merged 1 commit into from
Nov 21, 2025
+56 −46
Merged

Calculate file options for WheelWriter once and cache the result #2865

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
46 changes: 41 additions & 5 deletions src/build_context.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -33,6 +33,7 @@ use std::io;
use std::path::{Path, PathBuf};
use std::str::FromStr;
use tracing::instrument;
use zip::DateTime;

/// Insert wasm launcher scripts as entrypoints and the wasmtime dependency
fn bin_wasi_helper(
Expand Down Expand Up @@ -752,14 +753,18 @@ impl BuildContext {
let platform = self.get_platform_tag(platform_tags)?;
let tag = format!("cp{major}{min_minor}-abi3-{platform}");

let file_options = self
.compression
.get_file_options()
.last_modified_time(zip_mtime());
let mut writer = WheelWriter::new(
&tag,
&self.out,
&self.project_layout.project_root,
&self.metadata24,
std::slice::from_ref(&tag),
self.excludes(Format::Wheel)?,
self.compression,
file_options,
)?;
self.add_external_libs(&mut writer, &[&artifact], &[ext_libs])?;

Expand Down Expand Up @@ -831,14 +836,18 @@ impl BuildContext {
) -> Result<BuiltWheelMetadata> {
let tag = python_interpreter.get_tag(self, platform_tags)?;

let file_options = self
.compression
.get_file_options()
.last_modified_time(zip_mtime());
let mut writer = WheelWriter::new(
&tag,
&self.out,
&self.project_layout.project_root,
&self.metadata24,
std::slice::from_ref(&tag),
self.excludes(Format::Wheel)?,
self.compression,
file_options,
)?;
self.add_external_libs(&mut writer, &[&artifact], &[ext_libs])?;

Expand Down Expand Up @@ -955,14 +964,18 @@ impl BuildContext {
) -> Result<BuiltWheelMetadata> {
let (tag, tags) = self.get_universal_tags(platform_tags)?;

let file_options = self
.compression
.get_file_options()
.last_modified_time(zip_mtime());
let mut writer = WheelWriter::new(
&tag,
&self.out,
&self.project_layout.project_root,
&self.metadata24,
&tags,
self.excludes(Format::Wheel)?,
self.compression,
file_options,
)?;
self.add_external_libs(&mut writer, &[&artifact], &[ext_libs])?;

Expand Down Expand Up @@ -1028,14 +1041,18 @@ impl BuildContext {
) -> Result<BuiltWheelMetadata> {
let (tag, tags) = self.get_universal_tags(platform_tags)?;

let file_options = self
.compression
.get_file_options()
.last_modified_time(zip_mtime());
let mut writer = WheelWriter::new(
&tag,
&self.out,
&self.project_layout.project_root,
&self.metadata24,
&tags,
self.excludes(Format::Wheel)?,
self.compression,
file_options,
)?;
self.add_external_libs(&mut writer, &[&artifact], &[ext_libs])?;

Expand Down Expand Up @@ -1128,14 +1145,18 @@ impl BuildContext {
self.metadata24.clone()
};

let file_options = self
.compression
.get_file_options()
.last_modified_time(zip_mtime());
let mut writer = WheelWriter::new(
&tag,
&self.out,
&self.project_layout.project_root,
&metadata24,
&tags,
self.excludes(Format::Wheel)?,
self.compression,
file_options,
)?;

if self.project_layout.python_module.is_some() && self.target.is_wasi() {
Expand Down Expand Up @@ -1396,6 +1417,21 @@ fn emcc_version() -> Result<String> {
Ok(trimmed.into())
}

/// Returns a DateTime representing the value SOURCE_DATE_EPOCH environment variable
/// Note that the earliest timestamp a zip file can represent is 1980-01-01
fn zip_mtime() -> DateTime {
let res = env::var("SOURCE_DATE_EPOCH")
.context("") // Only using context() to unify the error types
.and_then(|epoch| {
let epoch: i64 = epoch.parse()?;
let dt = time::OffsetDateTime::from_unix_timestamp(epoch)?;
let dt = DateTime::try_from(dt)?;
Ok(dt)
});

res.unwrap_or_default()
Comment on lines +1423 to +1432
Copy link

Copilot AI Nov 21, 2025

Choose a reason for hiding this comment

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

[nitpick] The empty context string context("") is unusual and appears to be used solely to unify error types. Consider using a more descriptive error message or using .ok() followed by .and_then() to avoid needing a context at all.

For example:

fn zip_mtime() -> DateTime {
    env::var("SOURCE_DATE_EPOCH")
        .ok()
        .and_then(|epoch| {
            let epoch: i64 = epoch.parse().ok()?;
            let dt = time::OffsetDateTime::from_unix_timestamp(epoch).ok()?;
            DateTime::try_from(dt).ok()
        })
        .unwrap_or_default()
}
Suggested change
let res = env::var("SOURCE_DATE_EPOCH")
.context("") // Only using context() to unify the error types
.and_then(|epoch| {
let epoch: i64 = epoch.parse()?;
let dt = time::OffsetDateTime::from_unix_timestamp(epoch)?;
let dt = DateTime::try_from(dt)?;
Ok(dt)
});
res.unwrap_or_default()
env::var("SOURCE_DATE_EPOCH")
.ok()
.and_then(|epoch| {
let epoch: i64 = epoch.parse().ok()?;
let dt = time::OffsetDateTime::from_unix_timestamp(epoch).ok()?;
DateTime::try_from(dt).ok()
})
.unwrap_or_default()

Copilot uses AI. Check for mistakes.
Copy link
Copy Markdown
Contributor Author

@e-nomem e-nomem Nov 21, 2025

Choose a reason for hiding this comment

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

I'm not convinced that writing .ok()? everywhere is better than just ? but the code does the same thing either way so 🤷

}
Comment on lines +1422 to +1433
Copy link

Copilot AI Nov 21, 2025

Choose a reason for hiding this comment

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

The old mtime() method in wheel_writer.rs enforced a minimum date of 1980-01-01 (the earliest timestamp a zip file can represent), but this logic is missing in the new zip_mtime() function. This could cause issues if SOURCE_DATE_EPOCH is set to a date before 1980-01-01.

The old implementation included:

let min_dt = time::Date::from_calendar_date(1980, time::Month::January, 1)
    .unwrap()
    .midnight()
    .assume_offset(time::UtcOffset::UTC);
let dt = dt.max(min_dt);

Consider adding this validation to zip_mtime():

fn zip_mtime() -> DateTime {
    let res = env::var("SOURCE_DATE_EPOCH")
        .context("")
        .and_then(|epoch| {
            let epoch: i64 = epoch.parse()?;
            let dt = time::OffsetDateTime::from_unix_timestamp(epoch)?;
            // Ensure the date is at least 1980-01-01 (earliest zip timestamp)
            let min_dt = time::Date::from_calendar_date(1980, time::Month::January, 1)
                .unwrap()
                .midnight()
                .assume_offset(time::UtcOffset::UTC);
            let dt = dt.max(min_dt);
            let dt = DateTime::try_from(dt)?;
            Ok(dt)
        });
    
    res.unwrap_or_default()
}

Copilot uses AI. Check for mistakes.
Copy link
Copy Markdown
Contributor Author

@e-nomem e-nomem Nov 21, 2025

Choose a reason for hiding this comment

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

zip::DateTime::try_from() will return an error if the timestamp is before Jan 1 1980. Also zip::DateTime::default() returns a datetime of Jan 1 1980 so, together with the unwrap_or_default(), this code can never return a timestamp earlier than Jan 1 1980.


#[cfg(test)]
mod tests {
use super::{iphoneos_deployment_target, macosx_deployment_target};
Expand Down
2 changes: 1 addition & 1 deletion src/compression.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -86,7 +86,7 @@ impl CompressionOptions {
}
}

pub(crate) fn get_file_options(&self) -> zip::write::FileOptions<'_, ()> {
pub(crate) fn get_file_options(&self) -> zip::write::FileOptions<'static, ()> {
let method = if cfg!(feature = "faster-tests") {
// Unlike users which can use the develop subcommand, the tests have to go through
// packing a zip which pip than has to unpack. This makes this 2-3 times faster
Expand Down
54 changes: 14 additions & 40 deletions src/module_writer/wheel_writer.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,3 @@
use std::env;
use std::io;
use std::io::Read;
use std::io::Write as _;
Expand All @@ -7,15 +6,13 @@ use std::path::PathBuf;

use anyhow::Context as _;
use anyhow::Result;
use anyhow::anyhow;
use fs_err::File;
use ignore::overrides::Override;
use normpath::PathExt as _;
use tracing::debug;
use zip::DateTime;
use zip::ZipWriter;
use zip::write::SimpleFileOptions;

use crate::CompressionOptions;
use crate::Metadata24;
use crate::project_layout::ProjectLayout;

Expand All @@ -33,7 +30,7 @@ pub struct WheelWriter {
wheel_path: PathBuf,
file_tracker: FileTracker,
excludes: Override,
compression: CompressionOptions,
file_options: SimpleFileOptions,
}

impl ModuleWriter for WheelWriter {
Expand All @@ -57,15 +54,9 @@ impl ModuleWriter for WheelWriter {
// The zip standard mandates using unix style paths
let target = target.to_str().unwrap().replace('\\', "/");

let mut options = self
.compression
.get_file_options()
let options = self
.file_options
.unix_permissions(default_permission(executable));

if let Ok(mtime) = self.mtime() {
options = options.last_modified_time(mtime);
}

self.zip.start_file(target.clone(), options)?;
let mut writer = StreamSha256::new(&mut self.zip);

Expand All @@ -90,7 +81,7 @@ impl WheelWriter {
metadata24: &Metadata24,
tags: &[String],
excludes: Override,
compression: CompressionOptions,
file_options: SimpleFileOptions,
) -> Result<WheelWriter> {
let wheel_path = wheel_dir.join(format!(
"{}-{}-{}.whl",
Expand All @@ -108,7 +99,7 @@ impl WheelWriter {
wheel_path,
file_tracker: FileTracker::default(),
excludes,
compression,
file_options,
};

write_dist_info(&mut builder, pyproject_dir, metadata24, tags)?;
Expand Down Expand Up @@ -152,29 +143,11 @@ impl WheelWriter {
self.excludes.matched(path.as_ref(), false).is_whitelist()
}

/// Returns a DateTime representing the value SOURCE_DATE_EPOCH environment variable
/// Note that the earliest timestamp a zip file can represent is 1980-01-01
fn mtime(&self) -> Result<DateTime> {
let epoch: i64 = env::var("SOURCE_DATE_EPOCH")?.parse()?;
let dt = time::OffsetDateTime::from_unix_timestamp(epoch)?;
let min_dt = time::Date::from_calendar_date(1980, time::Month::January, 1)
.unwrap()
.midnight()
.assume_offset(time::UtcOffset::UTC);
let dt = dt.max(min_dt);

let dt = DateTime::try_from(dt).map_err(|_| anyhow!("Failed to build zip DateTime"))?;
Ok(dt)
}

/// Creates the record file and finishes the zip
pub fn finish(mut self) -> Result<PathBuf, io::Error> {
let mut options = self.compression.get_file_options();
let mtime = self.mtime().ok();
if let Some(mtime) = mtime {
options = options.last_modified_time(mtime);
}

let options = self
.file_options
.unix_permissions(default_permission(false));
let record_filename = self.record_file.to_str().unwrap().replace('\\', "/");
debug!("Adding {}", record_filename);
self.zip.start_file(&record_filename, options)?;
Expand Down Expand Up @@ -212,6 +185,10 @@ mod tests {
fn wheel_writer_no_compression() -> Result<(), Box<dyn std::error::Error>> {
let metadata = Metadata24::new("dummy".to_string(), Version::new([1, 0]));
let tmp_dir = TempDir::new()?;
let compression_options = CompressionOptions {
compression_method: CompressionMethod::Stored,
..Default::default()
};

let writer = WheelWriter::new(
"no compression",
Expand All @@ -220,10 +197,7 @@ mod tests {
&metadata,
&[],
Override::empty(),
CompressionOptions {
compression_method: CompressionMethod::Stored,
..Default::default()
},
compression_options.get_file_options(),
)?;

writer.finish()?;
Expand Down
Loading