-
Notifications
You must be signed in to change notification settings - Fork 111
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.
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
Prepare for stabilization of sanitizers #376
Merged
Merged
Changes from 7 commits
Commits
Show all changes
11 commits
Select commit
Hold shift + click to select a range
988fdd5
Build out rustc version detection infrastructure
Shnatsel 5f24537
cargo fmt
Shnatsel 7c5ee32
Use anyhow for error handling in rustc version detection
Shnatsel a93a462
Actually wire up all the sanitizer flag handling machinery
Shnatsel 5556207
minor stuff
Shnatsel 3eecd3d
Fix -Zsanitizer-memory-track-origins flag which was not changed
Shnatsel 92ebfee
Inline sanitizer flag selection now that it's infallible
Shnatsel 50f66f7
Better encapsulate compiler version discovery
Shnatsel 230c3cc
Set placeholder version values to u32::MAX to make the PR mergeable i…
Shnatsel e57a70b
Fix typo in comment
Shnatsel a52c0e5
Less verbose comment
Shnatsel File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,129 @@ | ||
//! Rust compiler version detection | ||
|
||
use std::{cmp::Ordering, process::Command, str::FromStr}; | ||
|
||
use anyhow::Context; | ||
|
||
/// Checks if the compiler currently in use is nightly, or `RUSTC_BOOTSTRAP` is set to get nightly features on stable | ||
pub fn is_nightly(version_string: &str) -> bool { | ||
version_string.contains("-nightly ") || std::env::var_os("RUSTC_BOOTSTRAP").is_some() | ||
} | ||
|
||
/// Returns the output of `rustc --version` | ||
pub fn rust_version_string() -> anyhow::Result<String> { | ||
// The path to rustc can be specified via an environment variable: | ||
// https://doc.rust-lang.org/cargo/reference/environment-variables.html#environment-variables-cargo-reads | ||
let rustc_path = std::env::var_os("RUSTC").unwrap_or("rustc".into()); | ||
let raw_output = Command::new(rustc_path) | ||
.arg("--version") | ||
.output() | ||
.context("Failed to invoke rustc! Is it in your $PATH?")? | ||
.stdout; | ||
String::from_utf8(raw_output).context("`rustc --version` returned non-text output somehow") | ||
} | ||
|
||
#[derive(Debug, Copy, Clone, Eq, PartialEq, PartialOrd)] | ||
pub struct RustVersion { | ||
pub major: u32, | ||
pub minor: u32, | ||
// we don't care about the patch version and it's a bit of a pain to parse | ||
} | ||
|
||
impl Ord for RustVersion { | ||
fn cmp(&self, other: &Self) -> Ordering { | ||
match self.major.cmp(&other.major) { | ||
Ordering::Equal => self.minor.cmp(&other.minor), | ||
other => other, | ||
} | ||
} | ||
} | ||
|
||
impl FromStr for RustVersion { | ||
type Err = &'static str; | ||
|
||
fn from_str(s: &str) -> Result<Self, Self::Err> { | ||
let s = s | ||
.strip_prefix("rustc ") | ||
.ok_or("Rust version string does not start with 'rustc'!")?; | ||
let mut iter = s.split('.'); | ||
let major: u32 = iter | ||
.next() | ||
.ok_or("No major version found in `rustc --version` output!")? | ||
.parse() | ||
.map_err(|_| { | ||
"Failed to parse major version in `rustc --version` output as a number!" | ||
})?; | ||
let minor: u32 = iter | ||
.next() | ||
.ok_or("No minor version found in `rustc --version` output!")? | ||
.parse() | ||
.map_err(|_| { | ||
"Failed to parse minor version in `rustc --version` output as a number!" | ||
})?; | ||
Ok(RustVersion { major, minor }) | ||
} | ||
} | ||
|
||
/// Checks whether the compiler supports sanitizers on stable channel. | ||
/// Such compilers (even nightly) do not support `-Zsanitizer` flag, | ||
/// and require a different combination of flags even on nightly. | ||
/// | ||
/// Stabilization PR with more info: <https://github.com/rust-lang/rust/pull/123617> | ||
impl RustVersion { | ||
pub fn has_sanitizers_on_stable(&self) -> bool { | ||
// TODO: the release that stabilizes sanitizers is not currently known. | ||
// This value is a PLACEHOLDER. | ||
let release_that_stabilized_sanitizers = RustVersion { | ||
major: 1, | ||
minor: 85, | ||
}; | ||
Shnatsel marked this conversation as resolved.
Show resolved
Hide resolved
|
||
self >= &release_that_stabilized_sanitizers | ||
} | ||
} | ||
|
||
#[cfg(test)] | ||
mod tests { | ||
use super::*; | ||
|
||
#[test] | ||
fn test_parsing_stable() { | ||
let version_string = "rustc 1.78.0 (9b00956e5 2024-04-29)"; | ||
let result = RustVersion::from_str(version_string).unwrap(); | ||
assert_eq!( | ||
result, | ||
RustVersion { | ||
major: 1, | ||
minor: 78 | ||
} | ||
); | ||
assert!(!is_nightly(version_string)) | ||
} | ||
|
||
#[test] | ||
fn test_parsing_nightly() { | ||
let version_string = "rustc 1.81.0-nightly (d7f6ebace 2024-06-16)"; | ||
let result = RustVersion::from_str(version_string).unwrap(); | ||
assert_eq!( | ||
result, | ||
RustVersion { | ||
major: 1, | ||
minor: 81 | ||
} | ||
); | ||
assert!(is_nightly(version_string)) | ||
} | ||
|
||
#[test] | ||
fn test_parsing_future_stable() { | ||
let version_string = "rustc 2.356.1 (deadfaced 2029-04-01)"; | ||
let result = RustVersion::from_str(version_string).unwrap(); | ||
assert_eq!( | ||
result, | ||
RustVersion { | ||
major: 2, | ||
minor: 356 | ||
} | ||
); | ||
assert!(!is_nightly(version_string)) | ||
} | ||
} |
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nitpick: seems like these two steps could be collapsed into a single, fallible
RustVersion
constructor:No need to make API-consumers do two things when they could do one; just an opportunity for them to mess things up in between or not connect the two interfaces properly.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The reason these are two separate steps is that we need the version string too, to detect whether the compiler is nightly or not.
I considered adding the version string to the
RustVersion
struct, but that would make it no longerCopy
, and neither API looked obviously better than the other.If you want me to encapsulate everything, I guess I could turn
RustVersion
into this:and with the nightly flag being resolved in
RustVersion
we don't need to expose the string outside the module at all.