-
Notifications
You must be signed in to change notification settings - Fork 13.5k
rustc: Fix an ICE when -o bare-path #23210
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
Conversation
r? @eddyb (rust_highfive has picked a reviewer for you, use r? to override) |
@richo you can put tests that invoke |
Thanks @pnkfelix I added the test |
@richo our |
(p.s. even though |
Ooof, my bad! Thanks for the ping |
r? @alexcrichton (i think you were the one who was recently involved in discussions about how |
@@ -954,8 +954,11 @@ pub fn build_output_filenames(input: &Input, | |||
if *odir != None { | |||
sess.warn("ignoring --out-dir flag due to -o flag."); | |||
} | |||
|
|||
let cur_dir = Path::new("."); |
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.
This default is actually more appropriately the empty string due to the way that normalization works currently.
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.
Gah, it won't let me comment above the diff. Should the PathBuf
on line 930 also be the empty 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.
Yes that's fine.
@pnkfelix yes thanks for the r?, this is a known issue with the path module and I discussed it at great length with @aturon. This is considered a bug of the module and @aturon is working on a fix for it soon (which will cause this to "just work"). I ran into this ICE a few days ago and decided to just let it be while the path module is fixed, but I'm also OK landing this in the interim. |
I'd rather land this (or something like it), I spent a fair while poking around in my code on the basis that "there's no way I'll fix that path though, it was fairly unclear if the empty string would do what I wanted. |
rustc will ICE if you specify an outfile path that is bare without a directory. As a workaround, before this -o ./foo will work It wasn't clear to me where I could put a test that actually invokes rustc from a shell, although I think I can add doctests to that machinery in librustc_driver that will arrange for this to be called with arguments that would trigger the ICE
rustc will ICE if you specify an outfile path that is bare without a directory. As a workaround, before this -o ./foo will work
I rebased this to fix the test, but it looks like it'll go out in a rollup anyway. |
(This merged, bfcf53f. The test was fixed in a separate commit.) |
Sure. My reasoning was basically to optimise for not wasting 2 hours of builder time if the rollup failed. Thanks! |
In general if you have to fix something when it's already been included in a rolluo, write it as a separate commit and comment on the rollup so that I can cherry pick. Thanks! |
rustc will ICE if you specify an outfile path that is bare without a
directory. As a workaround, before this -o ./foo will work
It wasn't clear to me where I could put a test that actually invokes rustc from a shell, although I think I can add doctests to that machinery in librustc_driver that will arrange for this to be called with arguments that would trigger the ICE