Fix sys.implementation#7793
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 (3)
🚧 Files skipped from review as they are similar to previous changes (3)
📝 WalkthroughWalkthroughAdds compile-time git and implementation-version constants, exposes an IMPLEMENTATION version in sys, extends ChangesImplementation Version Differentiation & Build Metadata
Sequence Diagram(s)sequenceDiagram
participant Build as Build Script (build.rs)
participant Cargo as Cargo/rustc
participant Version as version.rs
participant Stdlib as stdlib/sys.rs
participant Runtime as Runtime Init
Build->>Cargo: emit env vars (GIT_*, RELEASE_LEVEL, SERIAL, VERSION_IMPL_*)
Cargo->>Version: embed env consts as compile-time constants
Version->>Stdlib: expose impl constants and GIT_REVISION
Stdlib->>Runtime: initialize `sys._git` (3-tuple) and `sys.implementation` from IMPLEMENTATION data
Estimated Code Review Effort🎯 4 (Complex) | ⏱️ ~45 minutes 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 |
b48fe63 to
27c3960
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (5)
crates/vm/src/stdlib/sys.rs (1)
668-681: ⚡ Quick win
cache_tagis ambiguous when minor version reaches double digits.
format!("{NAME}-{}{}", MAJOR_IMPL, MINOR_IMPL)concatenates without a separator. For RustPython0.5.xthis yieldsrustpython-05, which works today. Once minor crosses 10 (or major does), e.g.1.10.xand11.0.xboth producerustpython-110, masking incompatible bytecode caches under the same tag.CPython gets away with
cpython-314because Python's minor versions stay single-digit in practice, but RustPython's own versioning is independent and likely to roll over. Consider using a separator so the tag remains unambiguous.♻️ Proposed change
- let cache_tag = format!("{NAME}-{}{}", version::MAJOR_IMPL, version::MINOR_IMPL); + let cache_tag = format!("{NAME}-{}_{}", version::MAJOR_IMPL, version::MINOR_IMPL);🤖 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/src/stdlib/sys.rs` around lines 668 - 681, The cache_tag construction in function implementation() currently builds cache_tag with format!("{NAME}-{}{}", version::MAJOR_IMPL, version::MINOR_IMPL) which concatenates major and minor without a separator and can produce ambiguous tags (e.g., 1.10 vs 11.0). Change the formatting of cache_tag in implementation() to include a clear separator between MAJOR_IMPL and MINOR_IMPL (for example a dot or hyphen: "{NAME}-{}.{}" or "{NAME}-{}-{}"), using the existing version::MAJOR_IMPL and version::MINOR_IMPL symbols so the produced cache_tag is unambiguous across version boundaries.crates/vm/build.rs (1)
52-52: 💤 Low value
"000000000"(9 chars) doesn't matchgit rev-parse --shortlength.
--shortdefaults to 7 hex chars (longer only if needed for uniqueness). A 9-char placeholder is harmless but inconsistent with what real builds produce.- git(&["rev-parse", "--short", "HEAD"]).unwrap_or_else(|_| "000000000".into()) + git(&["rev-parse", "--short", "HEAD"]).unwrap_or_else(|_| "0000000".into())🤖 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` at line 52, The placeholder commit hash used in build.rs for git(&["rev-parse", "--short", "HEAD"]).unwrap_or_else(|_| "000000000".into()) is 9 chars but --short normally yields 7 chars; change the fallback string to a 7-character placeholder (e.g. "0000000"). Update the unwrap_or_else call so it returns "0000000". This keeps the placeholder length consistent with git rev-parse --short output while leaving the rest of the expression unchanged.crates/vm/src/version.rs (3)
105-112: 💤 Low valueEmpty/invalid timestamp silently falls back to Unix epoch.
When
RUSTPYTHON_GIT_TIMESTAMPis missing or non-numeric (which can easily happen given thecommand()issue flagged inbuild.rs), bothunwrap_or_default()calls collapse to0, andget_build_info()ends up emitting"Jan 1 1970, 00:00:00". That's misleading "build date" output. At minimum, it'd be worth either (a) emitting an explicit "unknown" date/time when the timestamp is absent, or (b) ensuring the build-time fallback inbuild.rssubstitutesnow()so the build date reflects compilation time.🤖 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/src/version.rs` around lines 105 - 112, The function get_git_timestamp_datetime currently defaults missing or non-numeric RUSTPYTHON_GIT_TIMESTAMP to 0 (Unix epoch); change it to detect absence/parse failure and avoid treating 0 as a valid timestamp: parse option_env!("RUSTPYTHON_GIT_TIMESTAMP") with explicit error handling (e.g., return Option<DateTime<Local>> or Result) in get_git_timestamp_datetime, and update callers (e.g., get_build_info) to emit an explicit "unknown" or similar placeholder when the timestamp is missing/invalid instead of formatting Jan 1 1970; alternatively, ensure build.rs supplies a real timestamp by falling back to now() so the env var is always numeric.
77-94: 💤 Low valueSeparator logic gates on
GIT_REVISIONbut the format substitutesgit_identifier.
separatoris":"only whenGIT_REVISIONis non-empty; otherwise it's"". The format produces"{id}{sep}{revision}". IfGIT_REVISIONis empty butgit_identifieris"default"(i.e., both tag and branch were empty), the output becomes"default, <date>, <time>"— fine. But ifGIT_REVISIONis non-empty andgit_identifieris empty (so the format substitutes"default"), you still get"default:<hash>, ...", which reads oddly becausedefaultis a synthetic placeholder rather than a real ref. Consider gating the separator on thegit_identifierinstead, or omitdefaultwhen a real revision exists:- let separator = if GIT_REVISION.is_empty() { "" } else { ":" }; let git_identifier = get_git_identifier(); + let id = if git_identifier.is_empty() { "default" } else { git_identifier }; + let separator = if GIT_REVISION.is_empty() || id == "default" { "" } else { ":" }; format!( "{id}{sep}{revision}, {date:.20}, {time:.9}", - id = if git_identifier.is_empty() { - "default" - } else { - git_identifier - }, + id = id, sep = separator, revision = GIT_REVISION, date = get_git_date(), time = get_git_time(), )🤖 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/src/version.rs` around lines 77 - 94, get_build_info currently computes separator based on GIT_REVISION which can produce "default:<hash>" when get_git_identifier() returns empty; update get_build_info so the separator and the displayed identifier are decided from get_git_identifier() instead: call get_git_identifier(), then set a display identifier (empty string if git_identifier.is_empty()) and set separator to ":" only when both display identifier and GIT_REVISION are non-empty; change the format operands (id/sep/revision) accordingly so you never print the synthetic "default" next to a real revision (references: function get_build_info, variable GIT_REVISION, function get_git_identifier, local git_identifier and separator).
22-35: 💤 Low value
VERSION_HEX_IMPLsilently truncates components above 255.
(MAJOR_IMPL << 24) | (MINOR_IMPL << 16) | (MICRO_IMPL << 8) | ...packs each component into one byte. CPython's hexversion uses 8-bit fields, so this is the right encoding, but unlike CPython there's no static assertion that the components fit. Cargo allows e.g.version = "0.300.0", which would silently overflow into the minor slot and corruptsys.implementation.hexversion. Worth aconstassertion since this is computed at compile time and free at runtime:pub const VERSION_HEX_IMPL: usize = (MAJOR_IMPL << 24) | (MINOR_IMPL << 16) | (MICRO_IMPL << 8) | (RELEASELEVEL_N << 4) | SERIAL; +const _: () = { + assert!(MAJOR_IMPL <= 0xFF, "MAJOR_IMPL must fit in 8 bits"); + assert!(MINOR_IMPL <= 0xFF, "MINOR_IMPL must fit in 8 bits"); + assert!(MICRO_IMPL <= 0xFF, "MICRO_IMPL must fit in 8 bits"); +};🤖 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/src/version.rs` around lines 22 - 35, Add a compile-time assertion that MAJOR_IMPL, MINOR_IMPL, and MICRO_IMPL all fit in 8 bits (<= 0xFF) before computing VERSION_HEX_IMPL so versions like 0.300.0 cannot silently overflow; locate the constants MAJOR_IMPL, MINOR_IMPL, MICRO_IMPL and add const checks (for example via a const assert macro or a const-evaluation check that emits compile_error! when a value > 0xFF) that fail the build if any component is out of range, then leave the packing expression for VERSION_HEX_IMPL ((MAJOR_IMPL << 24) | (MINOR_IMPL << 16) | (MICRO_IMPL << 8) | (RELEASELEVEL_N << 4) | SERIAL) unchanged.
🤖 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 77-82: The command() helper currently ignores process exit status
and returns stdout even when the child failed; update the function (command) to
check output.status.success() after Command::output(), and if false convert that
into an io::Error (e.g., using io::Error::new with io::ErrorKind::Other and
include stderr/stdout text) so callers will receive Err and the existing
unwrap_or_else fallbacks for
RUSTPYTHON_GIT_HASH/RUSTPYTHON_GIT_TAG/RUSTPYTHON_GIT_BRANCH will run; ensure
you still decode stdout on success via String::from_utf8 and map UTF-8 errors
into io::Error as before.
- Around line 51-66: The git helper outputs (used by git_hash, git_timestamp,
git_tag, git_branch) include trailing newlines because command() doesn't trim
stdout; update command() to trim_end() (or equivalent) on the captured stdout
before returning so all callers (and thus the values set in main() and the
RUSTPYTHON_GIT_TIMESTAMP env var parsed in version.rs) receive newline-free
strings and avoid parsing/malformed build-info issues.
In `@crates/vm/src/stdlib/sys.rs`:
- Around line 1761-1768: The IMPLEMENTATION constant currently reuses the Python
target's version constants (version::RELEASELEVEL and version::SERIAL),
conflating target Python version with RustPython's own interpreter version;
update the IMPLEMENTATION construction in sys.rs so that it uses
RustPython-specific release level and serial values instead of
version::RELEASELEVEL/version::SERIAL (e.g., set releaselevel to a
RustPython-specific literal like "alpha" when MAJOR_IMPL < 1 and serial to 0),
while keeping major/minor/micro from version::MAJOR_IMPL/MINOR_IMPL/MICRO_IMPL
so sys.implementation.version reflects the interpreter's own release status and
not the target Python.
---
Nitpick comments:
In `@crates/vm/build.rs`:
- Line 52: The placeholder commit hash used in build.rs for git(&["rev-parse",
"--short", "HEAD"]).unwrap_or_else(|_| "000000000".into()) is 9 chars but
--short normally yields 7 chars; change the fallback string to a 7-character
placeholder (e.g. "0000000"). Update the unwrap_or_else call so it returns
"0000000". This keeps the placeholder length consistent with git rev-parse
--short output while leaving the rest of the expression unchanged.
In `@crates/vm/src/stdlib/sys.rs`:
- Around line 668-681: The cache_tag construction in function implementation()
currently builds cache_tag with format!("{NAME}-{}{}", version::MAJOR_IMPL,
version::MINOR_IMPL) which concatenates major and minor without a separator and
can produce ambiguous tags (e.g., 1.10 vs 11.0). Change the formatting of
cache_tag in implementation() to include a clear separator between MAJOR_IMPL
and MINOR_IMPL (for example a dot or hyphen: "{NAME}-{}.{}" or "{NAME}-{}-{}"),
using the existing version::MAJOR_IMPL and version::MINOR_IMPL symbols so the
produced cache_tag is unambiguous across version boundaries.
In `@crates/vm/src/version.rs`:
- Around line 105-112: The function get_git_timestamp_datetime currently
defaults missing or non-numeric RUSTPYTHON_GIT_TIMESTAMP to 0 (Unix epoch);
change it to detect absence/parse failure and avoid treating 0 as a valid
timestamp: parse option_env!("RUSTPYTHON_GIT_TIMESTAMP") with explicit error
handling (e.g., return Option<DateTime<Local>> or Result) in
get_git_timestamp_datetime, and update callers (e.g., get_build_info) to emit an
explicit "unknown" or similar placeholder when the timestamp is missing/invalid
instead of formatting Jan 1 1970; alternatively, ensure build.rs supplies a real
timestamp by falling back to now() so the env var is always numeric.
- Around line 77-94: get_build_info currently computes separator based on
GIT_REVISION which can produce "default:<hash>" when get_git_identifier()
returns empty; update get_build_info so the separator and the displayed
identifier are decided from get_git_identifier() instead: call
get_git_identifier(), then set a display identifier (empty string if
git_identifier.is_empty()) and set separator to ":" only when both display
identifier and GIT_REVISION are non-empty; change the format operands
(id/sep/revision) accordingly so you never print the synthetic "default" next to
a real revision (references: function get_build_info, variable GIT_REVISION,
function get_git_identifier, local git_identifier and separator).
- Around line 22-35: Add a compile-time assertion that MAJOR_IMPL, MINOR_IMPL,
and MICRO_IMPL all fit in 8 bits (<= 0xFF) before computing VERSION_HEX_IMPL so
versions like 0.300.0 cannot silently overflow; locate the constants MAJOR_IMPL,
MINOR_IMPL, MICRO_IMPL and add const checks (for example via a const assert
macro or a const-evaluation check that emits compile_error! when a value > 0xFF)
that fail the build if any component is out of range, then leave the packing
expression for VERSION_HEX_IMPL ((MAJOR_IMPL << 24) | (MINOR_IMPL << 16) |
(MICRO_IMPL << 8) | (RELEASELEVEL_N << 4) | SERIAL) unchanged.
🪄 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: 377eee15-6b2c-4ef2-8b30-90828b573daf
📒 Files selected for processing (3)
crates/vm/build.rscrates/vm/src/stdlib/sys.rscrates/vm/src/version.rs
| pub const MAJOR_IMPL: usize = match usize::from_str_radix(env!("CARGO_PKG_VERSION_MAJOR"), 10) { | ||
| Ok(v) => v, | ||
| Err(_) => panic!("Compile with Cargo to get 'CARGO_PKG_VERSION_MAJOR'"), | ||
| }; |
There was a problem hiding this comment.
Does this work?
| pub const MAJOR_IMPL: usize = match usize::from_str_radix(env!("CARGO_PKG_VERSION_MAJOR"), 10) { | |
| Ok(v) => v, | |
| Err(_) => panic!("Compile with Cargo to get 'CARGO_PKG_VERSION_MAJOR'"), | |
| }; | |
| pub const MAJOR_IMPL: usize = usize::from_str_radix(env!("CARGO_PKG_VERSION_MAJOR"), 10) | |
| .expect("Compile with Cargo to get 'CARGO_PKG_VERSION_MAJOR'"); |
If it does, I suggest doing likewise for MINOR_IMPL and MICRO_IMPL.
There was a problem hiding this comment.
Actually, this might not work because .expect is not const.
There was a problem hiding this comment.
Unfortunately, Result::unwrap, and its variations like expect(), as well as Result::ok are all const unstable. The code is a little ugly but calculating these values once at compile time is worth it, I think!
There was a problem hiding this comment.
The expect, unwrap, expect_err, and unwrap_err methods on Result<T, E> are not even const at all because the internal method unwrap_failed itself is not const.
27c3960 to
f5719e2
Compare
|
Force pushed to address the bunny's lints. 🐰 While addressing its lints, I added two env vars to |
`sys.version_info` and `sys.implementation` are two different versioning schemes. `version_info` refers to the target Python version while `implementation` refers to the implementation version. For RustPython, `version_info` should be 3.14 because it currently targets Python 3.14. This was correct and remains unchanged. `implementation`, however, should be set to RustPython's version. I updated the code to correctly get RustPython's version from the environment. I also ensured values are hard coded at build time which reduces the overall code that needs to be run at runtime as well as removes some unneeded allocations. Closes: RustPython#7770
f5719e2 to
8501a01
Compare
sys.version_infoandsys.implementationare two different versioning schemes.version_inforefers to the target Python version whileimplementationrefers to the implementation version.For RustPython,
version_infoshould be 3.14 because it currently targets Python 3.14. This was correct and remains unchanged.implementation, however, should be set to RustPython's version. I updated the code to correctly get RustPython's version from the environment. I also ensured values are hard coded at build time which reduces the overall code that needs to be run at runtime as well as removes some unneeded allocations.Closes: #7770
Summary by CodeRabbit