Skip to content

time: use C altzone when available#8357

Open
lms0806 wants to merge 2 commits into
RustPython:mainfrom
lms0806:time-use-c-altzone
Open

time: use C altzone when available#8357
lms0806 wants to merge 2 commits into
RustPython:mainfrom
lms0806:time-use-c-altzone

Conversation

@lms0806

@lms0806 lms0806 commented Jul 23, 2026

Copy link
Copy Markdown

Summary

Fixed an issue where the existing altzone was simply calculated as timezone - 3600 without checking for the presence of altzone in libc.

During the build process, the code now checks for the existence of altzone and implements logic consistent with CPython.

Summary by CodeRabbit

  • Bug Fixes
    • Improved time.altzone accuracy by using the host-provided native daylight-saving offset when available.
    • Added a CPython-compatible fallback for platforms where the native offset cannot be detected.
    • Enhanced correctness across supported environments, including cases where capability detection is unavailable during builds.

@coderabbitai

coderabbitai Bot commented Jul 23, 2026

Copy link
Copy Markdown
Contributor

Review Change Stack

📝 Walkthrough

Walkthrough

The host environment now detects CPython-compatible altzone support at build time, exposes native or fallback timezone behavior through tz::altzone(), and uses that value for Python’s time.altzone.

Changes

Host altzone support

Layer / File(s) Summary
Build-time altzone capability probe
crates/host_env/Cargo.toml, crates/host_env/build.rs
Adds the cc build dependency and probes CPython’s altzone support with platform and cross-compilation checks.
Runtime altzone exposure
crates/host_env/src/time.rs, crates/vm/src/stdlib/time.rs
Adds conditional native binding and fallback logic, then uses it for the Python time.altzone attribute.

Estimated code review effort: 3 (Moderate) | ~20 minutes

Sequence Diagram(s)

sequenceDiagram
  participant Cargo
  participant BuildScript as host_env build.rs
  participant CCompiler as C compiler
  participant HostTime as host_env time::tz
  participant PythonTime as Python time module

  Cargo->>BuildScript: Run build script
  BuildScript->>CCompiler: Compile altzone probe
  CCompiler-->>BuildScript: Return compilation status
  BuildScript-->>Cargo: Emit has_altzone cfg when supported
  PythonTime->>HostTime: Request altzone()
  alt has_altzone
    HostTime-->>PythonTime: Return native altzone
  else fallback
    HostTime-->>PythonTime: Return timezone() - 3600
  end
Loading

Suggested reviewers: shaharnaveh

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Docstring Coverage ✅ Passed Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title clearly matches the main change: using the C library's altzone when available, with a fallback otherwise.
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Actionable comments posted: 1

🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@crates/host_env/build.rs`:
- Around line 21-25: Update the cross-compilation guard in build.rs to detect
target-specific compiler variables supported by cc::Build—CC_<target>,
CC_<target_with_underscores>, and TARGET_CC—before returning. Only skip the
altzone probe when host and target differ and none of these compiler settings
are present; preserve the existing cc::Build compiler resolution afterward.
🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

  • Push a commit to this branch (recommended)
  • Create a new PR with the fixes

ℹ️ Review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yml

Review profile: CHILL

Plan: Pro Plus

Run ID: 8279beb4-35d7-49b8-bba8-4514bbf2f0a4

📥 Commits

Reviewing files that changed from the base of the PR and between 28454cc and b14b6a4.

⛔ Files ignored due to path filters (1)
  • Cargo.lock is excluded by !**/*.lock
📒 Files selected for processing (4)
  • crates/host_env/Cargo.toml
  • crates/host_env/build.rs
  • crates/host_env/src/time.rs
  • crates/vm/src/stdlib/time.rs

Comment thread crates/host_env/build.rs
@lms0806
lms0806 marked this pull request as draft July 23, 2026 23:43
@lms0806
lms0806 marked this pull request as ready for review July 23, 2026 23:46

@ShaharNaveh ShaharNaveh left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

lgtm overall!

tysm:)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants