-
Notifications
You must be signed in to change notification settings - Fork 13.6k
Allow running tests with GCC #144687
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: master
Are you sure you want to change the base?
Allow running tests with GCC #144687
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 |
---|---|---|
|
@@ -1769,7 +1769,10 @@ NOTE: if you're sure you want to do this, please open an issue as to why. In the | |
cmd.arg("--host").arg(&*compiler.host.triple); | ||
cmd.arg("--llvm-filecheck").arg(builder.llvm_filecheck(builder.config.host_target)); | ||
|
||
if let Some(codegen_backend) = builder.config.default_codegen_backend(compiler.host) { | ||
if let Some(codegen_backend) = builder.config.codegen_backends(compiler.host).first() { | ||
cmd.arg("--codegen-backend").arg(codegen_backend); | ||
} else if let Some(codegen_backend) = builder.config.default_codegen_backend(compiler.host) | ||
{ | ||
cmd.arg("--codegen-backend").arg(&codegen_backend); | ||
} | ||
|
||
|
@@ -1917,6 +1920,11 @@ NOTE: if you're sure you want to do this, please open an issue as to why. In the | |
cmd.arg(exclude); | ||
} | ||
|
||
if builder.config.codegen_backends(target).first().map(|b| b.as_str()) == Some("gcc") { | ||
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. Same here. |
||
cmd.arg("--extra-library-path"); | ||
cmd.arg(builder.config.libgccjit_root()); | ||
} | ||
|
||
// Get paths from cmd args | ||
let paths = match &builder.config.cmd { | ||
Subcommand::Test { .. } => &builder.config.paths[..], | ||
|
Original file line number | Diff line number | Diff line change | ||
---|---|---|---|---|
|
@@ -563,6 +563,12 @@ impl Builder<'_> { | |||
println!("using sysroot {sysroot_str}"); | ||||
} | ||||
|
||||
if self.config.codegen_backends(target).first().map(|b| b.as_str()) == Some("gcc") { | ||||
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. Same here. |
||||
let gcc_sysroot = self.config.libgccjit_root(); | ||||
let gcc_sysroot = gcc_sysroot.as_os_str().to_str().expect("sysroot should be UTF-8"); | ||||
cargo.env("LD_LIBRARY_PATH", gcc_sysroot); | ||||
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. How is it done for cg_llvm? 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. Why isn't libgccjit copied into the sysroot like for libLLVM? 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. Excellent question, absolutely no clue. 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. That works for build, but not for dist, we do not want to dist libgccjit.so by default with rustup (at least not yet). If build did this automatically, we would have to explicitly skip it when creating the dist archives (normally we mostly copy all .so files to them, I think). 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. Dist has an explicit list of files to include in each component, right? It has to for each .so files to not end up in every component. So the cg_gcc component can include just cg_gcc.so and not libgccjit.so. 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 don't think it's very explicit in this case:
To clarify, the |
||||
} | ||||
|
||||
let mut rustflags = Rustflags::new(target); | ||||
if build_compiler_stage != 0 { | ||||
if let Ok(s) = env::var("CARGOFLAGS_NOT_BOOTSTRAP") { | ||||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -1226,6 +1226,13 @@ impl Config { | |
self.out.join(self.host_target).join("ci-rustc") | ||
} | ||
|
||
pub(crate) fn libgccjit_root(&self) -> PathBuf { | ||
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. Considering we don't store GCC in the same ___location depending whether we build it or we download it, I found it better to handle it in one place instead, hence the creation of this method. |
||
match self.gcc_ci_mode { | ||
GccCiMode::BuildLocally => self.out.join(self.host_target).join("gcc/install/lib"), | ||
GccCiMode::DownloadFromCi => self.out.join(self.host_target).join("ci-gcc/lib"), | ||
} | ||
} | ||
|
||
/// Determine whether llvm should be linked dynamically. | ||
/// | ||
/// If `false`, llvm should be linked statically. | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -1556,6 +1556,11 @@ impl<'test> TestCx<'test> { | |
// In stage 0, make sure we use `stage0-sysroot` instead of the bootstrap sysroot. | ||
rustc.arg("--sysroot").arg(&self.config.sysroot_base); | ||
} | ||
// If the provided codegen backend is not LLVM, we need to pass it. | ||
if !matches!(self.config.codegen_backend, crate::CodegenBackend::Llvm) { | ||
let lib_path = self.config.sysroot_base.join(self.config.codegen_backend.as_str()); | ||
rustc.arg(format!("-Zcodegen-backend={}", lib_path)); | ||
} | ||
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. This always matches the default codegen backend, right? In that case no explicit |
||
|
||
// Optionally prevent default --target if specified in test compile-flags. | ||
let custom_target = self.props.compile_flags.iter().any(|x| x.starts_with("--target")); | ||
|
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.
Since we don't seem to run commands for each registered backend, I simply look at the first for now. Should I add a FIXME comment on it?
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.
I don't understand this change, why didn't you just use
default_codegen_backend
? It does the same thing (looks at the first codegen backend).Uh oh!
There was an error while loading. Please reload this page.
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.
default_codegen_backend()
is literally implemented ascodegen_backends().first()
.Edit: for some reason github didn't show me Kobzol's comment before.
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.
I just submitted it now 😆 It was a pending comment, but GH uses the original time when it was written, not when it was submitted, which is a bit confusing.