Skip to content

rustc: Fail immediately if linking returns status code != 0 #18239

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

Merged
merged 1 commit into from
Oct 28, 2014

Conversation

msiemens
Copy link
Contributor

If rustc fails to link a library (as described in #17951), it still tries to rename the non-existent file on Windows. I'm not sure how to test this as we'd have to reproduce a linking failure...

Closes #17951

@rust-highfive
Copy link
Contributor

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @nikomatsakis (or someone else) soon.

@alexcrichton
Copy link
Member

I think this may actually be somewhat of a red herring, the fs::rename(..).unwrap() should always succeed if the linker succeeded, but in this case the linker didn't succeed.

Could you instead modify line 717 (above this change) to actually look at the status returned by .status()? The return code is currently ignored as Ok(_), but we should test it for status.success() and fail immediately if it didn't have a 0 exit status.

@msiemens
Copy link
Contributor Author

Sure, will do.

@msiemens msiemens changed the title rustc: Check that renaming the link object succeeds on Windows rustc: Fail immediately if linking returns status code != 0 Oct 22, 2014
@msiemens
Copy link
Contributor Author

@alexcrichton I've added the changes you proposed. For that I reverted my original commit. Would it be better if I remove it completely?

@alexcrichton
Copy link
Member

Thanks @msiemens! Could you squash the commits together (removing the revert), and also it may be helpful to print the command line that failed (just use the Show impl for cmd)

@msiemens msiemens force-pushed the fix-ice-rename-failed branch from 53cd39e to b6e71da Compare October 23, 2014 17:29
@msiemens msiemens force-pushed the fix-ice-rename-failed branch from b6e71da to 53ac852 Compare October 23, 2014 17:32
@msiemens
Copy link
Contributor Author

Done. Is the new error message okay?

@nikomatsakis
Copy link
Contributor

(Sorry, to be clear, that was just a "for future reference" sort of comment, not a criticism or reason not to land the patch.)

alexcrichton added a commit to alexcrichton/rust that referenced this pull request Oct 27, 2014
@bors bors merged commit 53ac852 into rust-lang:master Oct 28, 2014
@msiemens msiemens deleted the fix-ice-rename-failed branch October 28, 2014 07:40
lnicola pushed a commit to lnicola/rust that referenced this pull request Oct 17, 2024
…puqv, r=davidbarsky

chore: rename `salsa` to `ra_salsa`

Laying some groundwork to start before I import the new Salsa crate. Here's why:
1. As part of the migration, `@darichey,` `@Wilfred,` and I will create new Salsa equivalents of the existing databases/query groups. We'll get them to compile crate-by-crate.
2. Once we wrote all equivalents of all queries, we'd start to refactor usage sites of the vendored Salsa to use the new Salsa databases.
3. Starting porting usage sites of old Salsa to the new Salsa.
4. Remove the vendored `ra_salsa`; declare victory.
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.

ICE: Result::unwrap() on an Err value: couldn't rename path
5 participants