Skip to content
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.

Sign up for GitHub

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

Jump to bottom

fix: Fix unusual errors with RUSTC_WRAPPER #5983

Merged
merged 1 commit into from
Sep 6, 2018
Merged

fix: Fix unusual errors with RUSTC_WRAPPER #5983

merged 1 commit into from
Sep 6, 2018

Conversation

alexcrichton
Copy link
Member

This commit fixes the interaction of cargo fix and RUSTC_WRAPPER, ensuring
that Cargo at least doesn't die internally. For now RUSTC_WRAPPER is
overridden for normal execution but we can eventually one day probably support
RUSTC_WRAPPER!

Closes #5981

@alexcrichton
fix: Fix unusual errors with RUSTC_WRAPPER
51be742 
This commit fixes the interaction of `cargo fix` and `RUSTC_WRAPPER`, ensuring
that Cargo at least doesn't die internally. For now `RUSTC_WRAPPER` is
overridden for normal execution but we can eventually one day probably support
`RUSTC_WRAPPER`!

Closes rust-lang#5981
@rust-highfive
Copy link

r? @matklad

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

@alexcrichton
Copy link
Member Author

r? @dwijnand

@bors: delegate=dwijnand

@bors
Copy link
Contributor

bors commented Sep 5, 2018

✌️ @dwijnand can now approve this pull request

dwijnand
dwijnand approved these changes Sep 5, 2018
View reviewed changes
Copy link
Member

@dwijnand dwijnand left a comment

Choose a reason for hiding this comment

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

Just sharing my thoughts on 2 parts.

But this is good to land as is, for me.

src/cargo/core/compiler/compilation.rs
// (which is typically set to `sccache` for now). Eventually we'll
// probably want to implement `RUSTC_WRAPPER` for `cargo fix`, but we'll
// leave that open as a bug for now.
let mut rustc = if bcx.build_config.cargo_as_rustc_wrapper {
Copy link
Member

@dwijnand dwijnand Sep 5, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

This option sounds more generic than it actually is. Before looking it up I would've assumed it was also used for at least cargo rustc, maybe cargo rustdoc too (given the chat in #5958).

But, no, it's just set in ops::fix.

tests/testsuite/fix.rs

#[test]
fn does_not_crash_with_rustc_wrapper() {
// We don't have /usr/bin/env on Windows.
Copy link
Member

Choose a reason for hiding this comment

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

So? It's expressly ignored, so whatever you set RUSTC_WRAPPER doesn't even need to exist on non-windows, no?

@dwijnand
Copy link
Member

dwijnand commented Sep 5, 2018

@bors: r+

@bors
Copy link
Contributor

bors commented Sep 5, 2018

📌 Commit 51be742 has been approved by dwijnand

@bors
Copy link
Contributor

bors commented Sep 5, 2018

⌛ Testing commit 51be742 with merge 65a6fa6933f779d0661203994b89cf5960374122...

@dwijnand
Copy link
Member

dwijnand commented Sep 5, 2018 •
edited
Loading

I also manually tested this fixes my case in #5981:

$ dcargo new --lib foo && command cd foo && dcargo fix --lib
     Created library `foo` project
    Checking foo v0.1.0 (file:///Users/dnw/Desktop/foo)
    Finished dev [unoptimized + debuginfo] target(s) in 0.55s

🎉

dwijnand
dwijnand reviewed Sep 5, 2018
View reviewed changes
tests/testsuite/fix.rs
r#"
[package]
name = "foo"
version = "0.1.0"
Copy link
Member

Choose a reason for hiding this comment

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

Old habits die hard 😉

@dwijnand dwijnand mentioned this pull request Sep 5, 2018
Merged
@bors
Copy link
Contributor

bors commented Sep 6, 2018

💔 Test failed - status-appveyor

@dwijnand
Copy link
Member

dwijnand commented Sep 6, 2018 

thread 'fix::fix_path_deps' panicked at '
Expected: execs
    but: differences:
  1 - |[FIXING] bar/src/lib.rs (1 fix)|
    + |    Checking foo v0.1.0 (CWD)|
  2 - |[CHECKING] foo v0.1.0 ([..])|
    + |      Fixing bar\src\lib.rs (1 fix)|

@bors: retry

@bors
Copy link
Contributor

bors commented Sep 6, 2018

⌛ Testing commit 51be742 with merge 3f51fc606cf615bbaa9ee4f2ae7cae1eb3166bb9...

@bors
Copy link
Contributor

bors commented Sep 6, 2018

💔 Test failed - status-appveyor

@dwijnand
Copy link
Member

dwijnand commented Sep 6, 2018 

---- build_script::switch_features_rerun stdout ----
running `C:\projects\cargo\target\debug\cargo.exe run -v --features=foo`
running `C:\projects\cargo\target\debug\cargo.exe run -v`
running `C:\projects\cargo\target\debug\cargo.exe run -v --features=foo`
thread 'build_script::switch_features_rerun' panicked at '
Expected: execs
    but: exited with exit code: 101
--- stdout
--- stderr
       Fresh foo v0.0.1 (file:///C:/projects/cargo/target/cit/t353/foo)
error: failed to link or copy `C:\projects\cargo\target\cit\t353\foo\target\debug\deps\foo-5dfa908d8c0c3515.exe` to `C:\projects\cargo\target\cit\t353\foo\target\debug\foo.exe`
Caused by:
  Access is denied. (os error 5)
', tests\testsuite\support\mod.rs:731:13

@bors: retry

@bors
Copy link
Contributor

bors commented Sep 6, 2018

⌛ Testing commit 51be742 with merge b15ee25...

bors added a commit that referenced this pull request Sep 6, 2018
@bors
Auto merge of #5983 - alexcrichton:fix-rustc-wrapper, r=dwijnand
b15ee25 
fix: Fix unusual errors with `RUSTC_WRAPPER`

This commit fixes the interaction of `cargo fix` and `RUSTC_WRAPPER`, ensuring
that Cargo at least doesn't die internally. For now `RUSTC_WRAPPER` is
overridden for normal execution but we can eventually one day probably support
`RUSTC_WRAPPER`!

Closes #5981
@bors
Copy link
Contributor

bors commented Sep 6, 2018

☀️ Test successful - status-appveyor, status-travis
Approved by: dwijnand
Pushing b15ee25 to master...

@bors bors merged commit 51be742 into rust-lang:master Sep 6, 2018
@ehuss ehuss mentioned this pull request Sep 7, 2018
Closed
@alexcrichton alexcrichton deleted the fix-rustc-wrapper branch October 12, 2018 17:23
@ehuss ehuss added this to the 1.30.0 milestone Feb 6, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@dwijnand dwijnand dwijnand approved these changes

Assignees

@matklad matklad

Labels
None yet
Projects
None yet
Milestone
1.30.0
Development

Successfully merging this pull request may close these issues.

cargo fix doesn't work with RUSTC_WRAPPER=sccache
6 participants
@alexcrichton @rust-highfive @bors @dwijnand @ehuss @matklad