-
Notifications
You must be signed in to change notification settings - Fork 710
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
Remove the buildpath from debug info in files compiled by gcc and clang. #802
base: main
Are you sure you want to change the base?
Conversation
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.
Thanks for doing this.
I'm a little unsure what negative side effect this might have on debugging ring but it seems reasonable.
build.rs
Outdated
@@ -524,6 +524,16 @@ fn obj_path(out_dir: &Path, src: &Path, obj_ext: &str) -> PathBuf { | |||
out_path | |||
} | |||
|
|||
#[allow(box_pointers)] |
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.
Let's remove the use of box pointers. We shouldn't need it for this.
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.
Please add a comment here explaining that this strips the build path for the purpose of reproducible builds.
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 removed the box pointers and I added a comment but I'm not super sure about the wording.
build.rs
Outdated
@@ -570,6 +580,7 @@ fn cc( | |||
|
|||
if target.env() != "msvc" { | |||
let _ = c.define("_XOPEN_SOURCE", Some("700")); | |||
let _ = c.flag(remap_debug_prefix()); |
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.
Rather than call remap_debug_prefix()
for each compilation, let's call it once before any compilation, cache it, and pass it as an argument to cc()
. In particular, from my experience writing makefiles it's best to avoid evaluating env::current_dir()
more than once.
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'm calling it once in compile()
. But I just realized that's still called once per file. Sorry about that. I'll do another iteration on this PR.
1b974bd
to
910515b
Compare
@pietro Sorry, I didn't notice that you updated this PR. I'll be changing the build system soon and I'll look into this when I do so. |
I changed the base branch to "main" from "master" so I can remove "master" today. |
fn cc( | ||
file: &Path, | ||
ext: &str, | ||
target: &Target, | ||
warnings_are_errors: bool, | ||
extra_flags: Vec<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.
I understand you did this at my request so that we wouldn't duplicate the work in remap_debug_prefix()
per source file. I realize that this complication is really caused by the structure of the build.rs code. I have refactored it in PR #1697 so that changes like the one in this PR don't have to try to solve the issues themselves.
use std::env; | ||
let cwd_path = env::current_dir().expect("Couldn't get current directory"); | ||
let mut flag = String::from("-fdebug-prefix-map="); | ||
flag.push_str(cwd_path.to_str().expect("Invalid path")); |
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.
Fixes #715.