Skip to content

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

Closed
wants to merge 3 commits into from
Closed

Conversation

richo
Copy link
Contributor

@richo richo commented Mar 9, 2015

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

@rust-highfive
Copy link
Contributor

r? @eddyb

(rust_highfive has picked a reviewer for you, use r? to override)

@pnkfelix
Copy link
Member

pnkfelix commented Mar 9, 2015

@richo you can put tests that invoke rustc from a shell into src/tests/run-make; those tests are all makefile driven.

@richo
Copy link
Contributor Author

richo commented Mar 9, 2015

Thanks @pnkfelix I added the test

@pnkfelix
Copy link
Member

pnkfelix commented Mar 9, 2015

@richo our make tidy script is pretty unforgiving; you need to add a license to src/run-make/bare-outfile/foo.rs

@pnkfelix
Copy link
Member

pnkfelix commented Mar 9, 2015

(p.s. even though make check can take a long time, make tidy is fast enough that I recommend running it locally before you push, if you remember.)

@richo
Copy link
Contributor Author

richo commented Mar 9, 2015

Ooof, my bad! Thanks for the ping

@pnkfelix
Copy link
Member

pnkfelix commented Mar 9, 2015

r? @alexcrichton (i think you were the one who was recently involved in discussions about how -o should be handled ... its too late at night for me to look it up)

@rust-highfive rust-highfive assigned alexcrichton and unassigned eddyb Mar 9, 2015
@@ -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(".");
Copy link
Member

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.

Copy link
Contributor Author

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?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes that's fine.

@alexcrichton
Copy link
Member

@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.

@richo
Copy link
Contributor Author

richo commented Mar 9, 2015

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 -o could be broken".

I'll fix that path though, it was fairly unclear if the empty string would do what I wanted.

@richo
Copy link
Contributor Author

richo commented Mar 9, 2015

r? @alexcrichton

@alexcrichton
Copy link
Member

@bors: r+ 1036361

Manishearth added a commit to Manishearth/rust that referenced this pull request Mar 9, 2015
 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
richo added 3 commits March 9, 2015 09:05
rustc will ICE if you specify an outfile path that is bare without a
directory. As a workaround, before this -o ./foo will work
@richo
Copy link
Contributor Author

richo commented Mar 9, 2015

I rebased this to fix the test, but it looks like it'll go out in a rollup anyway.

@alexcrichton
Copy link
Member

@bors: r+ a837be1

In general though I would avoid rebasing unless necessary, especially if it's part of a rollup already.

@Manishearth
Copy link
Member

(This merged, bfcf53f. The test was fixed in a separate commit.)

@richo
Copy link
Contributor Author

richo commented Mar 9, 2015

Sure. My reasoning was basically to optimise for not wasting 2 hours of builder time if the rollup failed. Thanks!

@richo richo closed this Mar 9, 2015
@Manishearth
Copy link
Member

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!

@frewsxcv frewsxcv mentioned this pull request Mar 10, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants