-
Notifications
You must be signed in to change notification settings - Fork 1.3k
use cargo rustc for macro expansion so -Zbuild-std can be supported
#5214
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
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||
|---|---|---|---|---|---|---|
|
|
@@ -13,6 +13,7 @@ use syn::visit::Visit; | |||||
| use thiserror::Error; | ||||||
|
|
||||||
| use crate::ffi_items::FfiItems; | ||||||
| use crate::macro_expansion::expand_with_args; | ||||||
| use crate::template::{ | ||||||
| CTestTemplate, | ||||||
| RustTestTemplate, | ||||||
|
|
@@ -32,7 +33,6 @@ use crate::{ | |||||
| Type, | ||||||
| Union, | ||||||
| VolatileItemKind, | ||||||
| expand, | ||||||
| get_build_target, | ||||||
| }; | ||||||
|
|
||||||
|
|
@@ -78,6 +78,10 @@ pub struct TestGenerator { | |||||
| cfg: Vec<(String, Option<String>)>, | ||||||
| /// A list of functions that remaps names used in the tests. | ||||||
| mapped_names: Vec<MappedName>, | ||||||
| /// Extra command line args to pass to cargo when generating macro expansions. | ||||||
| macro_expansion_cargo_args: Vec<String>, | ||||||
| /// Crate name to use when performing macro expansion. | ||||||
| crate_name: Option<String>, | ||||||
| /// The programming language to generate tests in. | ||||||
| pub(crate) language: Language, | ||||||
| /// A list of functions that determine what items to skip all tests for. | ||||||
|
|
@@ -723,6 +727,22 @@ impl TestGenerator { | |||||
| self | ||||||
| } | ||||||
|
|
||||||
| /// Configures extra arguments to be passed to cargo during macro expansion. | ||||||
| /// This can be used, for example, to pass `-Zbuild-std`` if required. | ||||||
| pub fn macro_expansion_cargo_args_mut(&mut self) -> &mut Vec<String> { | ||||||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
Simpler name, there's only one expansion |
||||||
| &mut self.macro_expansion_cargo_args | ||||||
| } | ||||||
|
|
||||||
| /// Configures the crate name which should be used during macro expansion. | ||||||
| /// | ||||||
| /// If the tested crate uses `#![crate_name = "..."]`, this must be called with the | ||||||
| /// same name. Otherwise, there will be an error about `--crate-name` not | ||||||
| /// matching. | ||||||
|
tgross35 marked this conversation as resolved.
|
||||||
| pub fn crate_name(&mut self, name: String) -> &mut Self { | ||||||
| self.crate_name = Some(name); | ||||||
| self | ||||||
| } | ||||||
|
skrap marked this conversation as resolved.
|
||||||
|
|
||||||
| /// Configures whether tests for the type of a field is skipped or not. | ||||||
| /// | ||||||
| /// The closure is given a Rust struct as well as a field within that | ||||||
|
|
@@ -1087,7 +1107,14 @@ impl TestGenerator { | |||||
| crate_path: impl AsRef<Path>, | ||||||
| output_file_path: impl AsRef<Path>, | ||||||
| ) -> Result<PathBuf, GenerationError> { | ||||||
| let expanded = expand(&crate_path, &self.cfg, get_build_target(self)?).map_err(|e| { | ||||||
| let expanded = expand_with_args( | ||||||
| &crate_path, | ||||||
| &self.cfg, | ||||||
| get_build_target(self)?, | ||||||
| self.crate_name.as_deref(), | ||||||
| &self.macro_expansion_cargo_args, | ||||||
| ) | ||||||
| .map_err(|e| { | ||||||
| GenerationError::MacroExpansion(crate_path.as_ref().to_path_buf(), e.to_string()) | ||||||
| })?; | ||||||
| let ast = syn::parse_file(&expanded) | ||||||
|
|
||||||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,4 +1,3 @@ | ||
| use std::env; | ||
| use std::fs::canonicalize; | ||
| use std::path::Path; | ||
| use std::process::Command; | ||
|
|
@@ -14,19 +13,63 @@ pub fn expand<P: AsRef<Path>>( | |
| cfg: &[(String, Option<String>)], | ||
| target: String, | ||
| ) -> Result<String> { | ||
| let rustc = env::var("RUSTC").unwrap_or_else(|_| String::from("rustc")); | ||
|
skrap marked this conversation as resolved.
|
||
| expand_with_args(crate_path, cfg, target, None, &[]) | ||
| } | ||
|
|
||
| /// Extension of `expand` which allows a crate_name override for | ||
| /// libc and other clients which require crate_name to match the | ||
| /// original crate's name. | ||
| pub(crate) fn expand_with_args<P: AsRef<Path>>( | ||
| crate_path: P, | ||
| cfg: &[(String, Option<String>)], | ||
| target: String, | ||
| crate_name: Option<&str>, | ||
| extra_cargo_args: &[String], | ||
| ) -> Result<String> { | ||
| let dir = tempfile::tempdir()?; | ||
|
|
||
| // make a Cargo.toml pointing to the crate path | ||
| let cargo_toml_path = dir.path().join("Cargo.toml"); | ||
| let crate_name = crate_name.unwrap_or("ctest-expansion-tmp"); | ||
| // FIXME(#5238): allow building using an existing manifest | ||
| let cargo_toml = format!( | ||
| r#" | ||
| [package] | ||
| name = '''{crate_name}''' # Needs to match #![crate_name = "..."] if it exists. | ||
| edition = '''{EDITION}''' | ||
| [lib] | ||
| path = '''{}''' | ||
| [lints.rust] | ||
| unexpected_cfgs = "allow" | ||
|
tgross35 marked this conversation as resolved.
|
||
| "#, | ||
|
skrap marked this conversation as resolved.
|
||
| canonicalize(crate_path)?.display() | ||
| ); | ||
| std::fs::write(cargo_toml_path, cargo_toml)?; | ||
|
|
||
| let mut cmd = Command::new(rustc); | ||
| let mut cmd = Command::new(std::env::var("CARGO").unwrap_or("cargo".into())); | ||
| cmd.env("RUSTC_BOOTSTRAP", "1") | ||
| .arg("-Zunpretty=expanded") | ||
| .arg("--edition") | ||
| .arg(EDITION) // By default, -Zunpretty=expanded uses 2015 edition. | ||
| .arg(canonicalize(crate_path)?); | ||
| .current_dir(dir.path()) | ||
| .arg("rustc") | ||
| .arg("--lib") | ||
| .arg("--profile") | ||
| .arg("check") | ||
| .args(extra_cargo_args); | ||
|
|
||
| // In order to avoid warnings causing failures here when the | ||
| // environment's RUSTFLAGS contains -Dwarnings (or similar), | ||
| // we will clear RUSTFLAGS. | ||
| cmd.env_remove("RUSTFLAGS"); | ||
|
Comment on lines
+58
to
+61
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Change to
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I am guessing this will fail because it'll do away with things like
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Fair point, feel free to leave it for now if you haven't already tested and I'll poke around some more afterwards |
||
|
|
||
| // set an independent target dir so we don't deadlock on the cargo build lock. | ||
| cmd.arg("--target-dir").arg(dir.path().join("target")); | ||
|
|
||
| if !target.is_empty() { | ||
| cmd.arg("--target").arg(target); | ||
| } | ||
|
|
||
| cmd.arg("--"); | ||
| cmd.arg("-Zunpretty=expanded"); | ||
|
|
||
| // `libc` uses non standard cfg flags as well, which have to be manually expanded. | ||
| for (k, v) in cfg { | ||
| match v { | ||
|
|
@@ -39,7 +82,11 @@ pub fn expand<P: AsRef<Path>>( | |
|
|
||
| if !output.status.success() { | ||
| let stderr = std::str::from_utf8(&output.stderr)?; | ||
| return Err(format!("macro expansion failed with {}: {}", output.status, stderr).into()); | ||
| return Err(format!( | ||
| "macro expansion failed with {}: {}, {:?}", | ||
| output.status, stderr, cmd | ||
| ) | ||
| .into()); | ||
| } | ||
|
|
||
| let expanded = std::str::from_utf8(&output.stdout)?.to_string(); | ||
|
|
||
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.
Mismatched backtick