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.

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

Add support for wrapping cargo's rustc invocations by setting RUSTC_WRAPPER #3887

Merged
merged 1 commit into from
Apr 19, 2017

Conversation

luser
Copy link
Contributor

@luser luser commented Mar 31, 2017

To use sccache for cargo builds we need a simple way to get sccache into the rustc commandline when cargo invokes rustc. Currently this is only possible by hard-linking or copying the sccache binary to be named rustc and then either setting RUSTC to its path or putting it first in $PATH, both of which are sort of clunky and require manual steps even if installing sccache via cargo install.

This patch adds support for a RUSTC_WRAPPER environment variable which, if set, will simply be inserted as the actual binary for all rustc process execution, with rustc and all other rustc arguments following.

I didn't add any tests for this, I couldn't figure out the right place to put them, and presumably we'd need to build a helper binary of some sort to use as the wrapper. If you've got suggestions for how to do that properly I'd be happy to write tests.

This works well in my local testing:

luser@eye7:/build/read-process-memory$ /build/cargo/target/release/cargo clean; time RUSTC_WRAPPER=/build/sccache2/target/release/sccache RUSTC=/home/luser/.rustup/toolchains/beta-x86_64-unknown-linux-gnu/bin/rustc /build/cargo/target/release/cargo build
   Compiling getopts v0.2.14
   Compiling log v0.3.6
   Compiling libc v0.2.16
   Compiling rand v0.3.14
   Compiling pulldown-cmark v0.0.3
   Compiling tempdir v0.3.5
   Compiling skeptic v0.5.0
   Compiling read-process-memory v0.1.2-pre (file:///build/read-process-memory)
    Finished dev [unoptimized + debuginfo] target(s) in 7.31 secs

real	0m7.733s
user	0m0.060s
sys	0m0.036s
luser@eye7:/build/read-process-memory$ /build/cargo/target/release/cargo clean; time RUSTC_WRAPPER=/build/sccache2/target/release/sccache RUSTC=/home/luser/.rustup/toolchains/beta-x86_64-unknown-linux-gnu/bin/rustc /build/cargo/target/release/cargo build
   Compiling getopts v0.2.14
   Compiling libc v0.2.16
   Compiling log v0.3.6
   Compiling pulldown-cmark v0.0.3
   Compiling rand v0.3.14
   Compiling tempdir v0.3.5
   Compiling skeptic v0.5.0
   Compiling read-process-memory v0.1.2-pre (file:///build/read-process-memory)
    Finished dev [unoptimized + debuginfo] target(s) in 0.97 secs

real	0m1.049s
user	0m0.060s
sys	0m0.036s

The use of beta rustc is just to pick up the fix for making --emit=dep-info faster (which should ship in 1.17). If this patch ships in cargo then in the future developers should simply be able to cargo install sccache; export RUSTC_WRAPPER=sccache and cargo build as normal, but benefit from local sccache caching.

@rust-highfive
Copy link

r? @brson

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

@luser luser mentioned this pull request Mar 31, 2017
@luser
Copy link
Contributor Author

luser commented Mar 31, 2017

A Travis build failed because I have a line that's >100 characters. I fixed it locally, I'll push it after I get review since there will assuredly be other changes to make.

@alexcrichton
Copy link
Member

Looks great to me, thanks @luser!

@brson, @matklad, thoughts about the naming/API addition here?

@matklad
Copy link
Member

matklad commented Apr 1, 2017

Seems related to #3815. Why doesn't existing RUSTC work for this use case? That is, why "hard-linking or copying the sccache binary to be named rustc" is needed? Could export RUSTC=sccache be made to work?

@alexcrichton
Copy link
Member

@matklad oh the use case here I think is a little different than #3815 where most cargo developers won't use #3815 day-to-day, but many may use sccache day-to-day (once it's working well w/ rust code).

Unfortunately RUSTC=sccache won't work b/c then sccache won't know what mode it's being invoked in. Currently it uses argv[1] to learn about what program is being emulated.

I do believe, though, that RUSTC=path/to/sccache/hard/linked/as/rustc works, however.

@matklad
Copy link
Member

matklad commented Apr 1, 2017

I think this solution may be specific to sscache, because it fixes particular command line format (i.e. rustc would be argv[1], and not, for example --bin rustc -- other args). This is totally ok, of course!

Some alternatives (I don't think any of them is better than this PR)

  • Perhaps there is some other way for sscache to figure out the mode, besides looking at argv[0]/argv[1]? For example, we can pass yet one more env variable to the child process :)

  • We can extend RUSTC to allow to tweak the format of the command line, so that you can do RUSTC=sscacnhe $rustc $@.

  • We can name environmental variable CCACHE to be more specific about the use case.

I didn't add any tests for this, I couldn't figure out the right place to put them, and presumably we'd need to build a helper binary of some sort to use as the wrapper.

I think you can specify /usr/bin/env as a wrapper. This won't work on windows, but it should be OK to just skip the test then.

@luser
Copy link
Contributor Author

luser commented Apr 3, 2017

Perhaps there is some other way for sscache to figure out the mode, besides looking at argv[0]/argv[1]? For example, we can pass yet one more env variable to the child process :)

The problem here is that you then have no way to also tell cargo to use a specific rustc except for munging $PATH. We will definitely hit this in practice in Firefox CI where we point RUSTC at a specific toolchain that we've unpacked somewhere.

As it stands this is definitely a sccache-specific option right now. However, I've seen lots of tools over the years that wrap C++ compiler invocations in a similar manner (ccache and distcc are the first two that come to mind), so I wouldn't be surprised to see someone find another interesting use case here.

We can name environmental variable CCACHE to be more specific about the use case.

I would disagree with this, as this would not work if you set CCACHE=ccache, nor will setting RUSTC_WRAPPER=sccache cause crates that do C compilation to use sccache as ccache by default.

@matklad
Copy link
Member

matklad commented Apr 3, 2017

The problem here is that you then have no way to also tell cargo to use a specific rustc except for munging $PATH

I didn't think of that, thanks! Looks great to me both implementation- and API-wise!

@luser
Copy link
Contributor Author

luser commented Apr 5, 2017

I fixed the overly-long line and added a test that uses /usr/bin/env, thanks for the suggestion!

@bors
Copy link
Contributor

bors commented Apr 6, 2017

☔ The latest upstream changes (presumably #3898) made this pull request unmergeable. Please resolve the merge conflicts.

@luser
Copy link
Contributor Author

luser commented Apr 17, 2017

Can this be merged?

@bors
Copy link
Contributor

bors commented Apr 18, 2017

☔ The latest upstream changes (presumably #3930) made this pull request unmergeable. Please resolve the merge conflicts.

@alexcrichton alexcrichton added the relnotes Release-note worthy label Apr 19, 2017
@alexcrichton
Copy link
Member

@luser oh dear sorry about that! I think this fell off my radar by accident...

@bors: r+

@bors
Copy link
Contributor

bors commented Apr 19, 2017

📌 Commit 20f23d9 has been approved by alexcrichton

@bors
Copy link
Contributor

bors commented Apr 19, 2017

⌛ Testing commit 20f23d9 with merge 8326a36...

bors added a commit that referenced this pull request Apr 19, 2017
Add support for wrapping cargo's rustc invocations by setting RUSTC_WRAPPER

To use sccache for cargo builds we need a simple way to get sccache into the rustc commandline when cargo invokes rustc. Currently this is only possible by hard-linking or copying the `sccache` binary to be named `rustc` and then either setting `RUSTC` to its path or putting it first in `$PATH`, both of which are sort of clunky and require manual steps even if installing sccache via `cargo install`.

This patch adds support for a `RUSTC_WRAPPER` environment variable which, if set, will simply be inserted as the actual binary for all rustc process execution, with rustc and all other rustc arguments following.

I didn't add any tests for this, I couldn't figure out the right place to put them, and presumably we'd need to build a helper binary of some sort to use as the wrapper. If you've got suggestions for how to do that properly I'd be happy to write tests.

This works well in my local testing:
```
luser@eye7:/build/read-process-memory$ /build/cargo/target/release/cargo clean; time RUSTC_WRAPPER=/build/sccache2/target/release/sccache RUSTC=/home/luser/.rustup/toolchains/beta-x86_64-unknown-linux-gnu/bin/rustc /build/cargo/target/release/cargo build
   Compiling getopts v0.2.14
   Compiling log v0.3.6
   Compiling libc v0.2.16
   Compiling rand v0.3.14
   Compiling pulldown-cmark v0.0.3
   Compiling tempdir v0.3.5
   Compiling skeptic v0.5.0
   Compiling read-process-memory v0.1.2-pre (file:///build/read-process-memory)
    Finished dev [unoptimized + debuginfo] target(s) in 7.31 secs

real	0m7.733s
user	0m0.060s
sys	0m0.036s
luser@eye7:/build/read-process-memory$ /build/cargo/target/release/cargo clean; time RUSTC_WRAPPER=/build/sccache2/target/release/sccache RUSTC=/home/luser/.rustup/toolchains/beta-x86_64-unknown-linux-gnu/bin/rustc /build/cargo/target/release/cargo build
   Compiling getopts v0.2.14
   Compiling libc v0.2.16
   Compiling log v0.3.6
   Compiling pulldown-cmark v0.0.3
   Compiling rand v0.3.14
   Compiling tempdir v0.3.5
   Compiling skeptic v0.5.0
   Compiling read-process-memory v0.1.2-pre (file:///build/read-process-memory)
    Finished dev [unoptimized + debuginfo] target(s) in 0.97 secs

real	0m1.049s
user	0m0.060s
sys	0m0.036s
```

The use of beta rustc is just to pick up the fix for making `--emit=dep-info` faster (which should ship in 1.17). If this patch ships in cargo then in the future developers should simply be able to `cargo install sccache; export RUSTC_WRAPPER=sccache` and `cargo build` as normal, but benefit from local sccache caching.
@bors
Copy link
Contributor

bors commented Apr 19, 2017

☀️ Test successful - status-appveyor, status-travis
Approved by: alexcrichton
Pushing 8326a36 to master...

@mmstick
Copy link

mmstick commented Aug 3, 2017

Why not just have Cargo check if sccache exists in PATH, and use it by default, without a need to set an environment variable?

@luser
Copy link
Contributor Author

luser commented Aug 3, 2017

Why not just have Cargo check if sccache exists in PATH, and use it by default, without a need to set an environment variable?

We were not sure how well it would actually work in practice! Also enabling caching might make some people uneasy.

@ehuss ehuss added this to the 1.18.0 milestone Feb 6, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
relnotes Release-note worthy
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants