Const eval all of version.rs#7923
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (5)
✅ Files skipped from review due to trivial changes (1)
🚧 Files skipped from review as they are similar to previous changes (3)
📝 WalkthroughWalkthroughRefactors version and Git metadata handling to build-time: build.rs now emits new environment variables and formatted timestamps; version.rs exposes compile-time constants derived from env!; sys and settings are updated to read the new constants instead of runtime helper functions. ChangesBuild-time version metadata refactoring
Sequence Diagram(s)sequenceDiagram
participant Build as build.rs (build script)
participant Env as compile env (RUSTPYTHON_*)
participant Version as crates::vm::version (compile-time)
participant Runtime as sys/settings (runtime)
Build->>Env: emit RUSTPYTHON_GIT_IDENTIFIER, RUSTPYTHON_BUILD_INFO, RUSTPYTHON_VERSION_LEFT/RIGHT, RUSTPYTHON_RELEASE_LEVEL_N
Env-->>Version: env! reads (MAJOR/MINOR/..., GIT_IDENTIFIER, RUSTPYTHON_BUILD_INFO, VERSION_LEFT/RIGHT)
Version->>Runtime: expose constants (RUSTPYTHON_VERSION, GIT_IDENTIFIER, WINVER)
Runtime->>Version: read constants for sys.version, sys._git, settings::version()
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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. Comment |
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
crates/vm/build.rs (1)
73-81:⚠️ Potential issue | 🟠 Major | ⚡ Quick winUse
SOURCE_DATE_EPOCHand UTC timezone for reproducible build metadata.The
git_timestamp()function usesSystemTime::now()as a fallback, andget_git_timestamp_datetime()converts that Unix epoch toDateTime<Local>(line 129), then formats it with patterns like"%b %e %Y"(line 136) and"%H:%M:%S"(line 141). This embeds local timezone information and current system time intoRUSTPYTHON_BUILD_INFO, producing different formatted timestamps across equivalent builds in different timezones or at different times. The code already references reproducible-builds.org on line 98 but does not honor the spec. The reproducible-builds standard requires checkingSOURCE_DATE_EPOCHand using stable (UTC) timezone rendering to ensure identical sources produce identical binaries.Suggested fix
-use chrono::{Local, prelude::DateTime}; +use chrono::{Utc, prelude::DateTime}; use core::time::Duration; @@ fn git_timestamp() -> String { - git(&["log", "-1", "--format=%ct"]).unwrap_or_else(|_| { - SystemTime::now() - .duration_since(UNIX_EPOCH) - .unwrap_or_default() - .as_secs() - .to_string() - }) + env::var("SOURCE_DATE_EPOCH") + .ok() + .or_else(|| git(&["log", "-1", "--format=%ct"]).ok()) + .unwrap_or_else(|| { + SystemTime::now() + .duration_since(UNIX_EPOCH) + .unwrap_or_default() + .as_secs() + .to_string() + }) } @@ -fn get_git_timestamp_datetime() -> DateTime<Local> { +fn get_git_timestamp_datetime() -> DateTime<Utc> { let timestamp = git_timestamp().parse::<u64>().unwrap_or_default(); let datetime = UNIX_EPOCH + Duration::from_secs(timestamp); - datetime.into() + DateTime::<Utc>::from(datetime) }🤖 Prompt for 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. In `@crates/vm/build.rs` around lines 73 - 81, The git_timestamp()/get_git_timestamp_datetime() logic must honor SOURCE_DATE_EPOCH and always render timestamps in UTC for reproducible builds: change git_timestamp() to first look for the SOURCE_DATE_EPOCH env var and use that value (as a Unix epoch string) if present; only fall back to the git log output or SystemTime::now() when SOURCE_DATE_EPOCH is absent; update get_git_timestamp_datetime() to parse the epoch into a UTC DateTime (use chrono::Utc or equivalent) instead of DateTime<Local> and format with UTC-based patterns so RUSTPYTHON_BUILD_INFO is produced deterministically across timezones and builds.
🤖 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/vm/build.rs`:
- Around line 207-214: The match arm in function release_to_n currently uses
unreachable!() for unknown release strings, causing an opaque panic; update
release_to_n to explicitly panic with a clear, actionable message (including the
received release value and that it came from RUSTPYTHON_RELEASE_LEVEL) so
typos/invalid values produce a descriptive error instead of an opaque
build-script panic.
---
Outside diff comments:
In `@crates/vm/build.rs`:
- Around line 73-81: The git_timestamp()/get_git_timestamp_datetime() logic must
honor SOURCE_DATE_EPOCH and always render timestamps in UTC for reproducible
builds: change git_timestamp() to first look for the SOURCE_DATE_EPOCH env var
and use that value (as a Unix epoch string) if present; only fall back to the
git log output or SystemTime::now() when SOURCE_DATE_EPOCH is absent; update
get_git_timestamp_datetime() to parse the epoch into a UTC DateTime (use
chrono::Utc or equivalent) instead of DateTime<Local> and format with UTC-based
patterns so RUSTPYTHON_BUILD_INFO is produced deterministically across timezones
and builds.
🪄 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
Run ID: 049c90f6-085c-49b0-a376-3faee648595a
📒 Files selected for processing (5)
crates/vm/Cargo.tomlcrates/vm/build.rscrates/vm/src/stdlib/sys.rscrates/vm/src/version.rssrc/settings.rs
| fn release_to_n(release: &str) -> usize { | ||
| match release { | ||
| "alpha" => 0xA, | ||
| "beta" => 0xB, | ||
| "candidate" => 0xC, | ||
| "final" => 0xD, | ||
| _ => unreachable!(), | ||
| } |
There was a problem hiding this comment.
Make invalid release levels fail with an actionable message.
Line 213 turns a typo in RUSTPYTHON_RELEASE_LEVEL into an opaque build-script panic. An explicit panic message here is much easier to diagnose.
Suggested fix
fn release_to_n(release: &str) -> usize {
match release {
"alpha" => 0xA,
"beta" => 0xB,
"candidate" => 0xC,
"final" => 0xD,
- _ => unreachable!(),
+ _ => panic!("unsupported RUSTPYTHON_RELEASE_LEVEL: {release}"),
}
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| fn release_to_n(release: &str) -> usize { | |
| match release { | |
| "alpha" => 0xA, | |
| "beta" => 0xB, | |
| "candidate" => 0xC, | |
| "final" => 0xD, | |
| _ => unreachable!(), | |
| } | |
| fn release_to_n(release: &str) -> usize { | |
| match release { | |
| "alpha" => 0xA, | |
| "beta" => 0xB, | |
| "candidate" => 0xC, | |
| "final" => 0xD, | |
| _ => panic!("unsupported RUSTPYTHON_RELEASE_LEVEL: {release}"), | |
| } | |
| } |
🤖 Prompt for 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.
In `@crates/vm/build.rs` around lines 207 - 214, The match arm in function
release_to_n currently uses unreachable!() for unknown release strings, causing
an opaque panic; update release_to_n to explicitly panic with a clear,
actionable message (including the received release value and that it came from
RUSTPYTHON_RELEASE_LEVEL) so typos/invalid values produce a descriptive error
instead of an opaque build-script panic.
`version.rs` essentially consists of constants that can be baked in at compile time. I moved most of `version.rs` to `build.rs`. The constants are passed via rustc's environment then stored in the binary.
a6f57bb to
9f877e1
Compare
version.rsessentially consists of constants that can be baked in at compile time. I moved most ofversion.rstobuild.rs. The constants are passed via rustc's environment then stored in the binary.Summary by CodeRabbit
Refactor
Chores