Use RUSTC_WRAPPER if no other wrapper is provided

[LeonMatthesKDAB]

Previously, the rustc_wrapper_fallback was only called if the tool
was not a full path.
With this patch, the RUSTC_WRAPPER is always used, even if the tool provided is an exact path.
Providing a wrapper manually is still possible and overrides the RUSTC_WRAPPER.

If the path to the tool includes spaces it is otherwise impossible to provide a compiler cache like sccache through the CXX or CC environment variables, as the arguments will be split up incorrectly.

See also: corrosion-rs/corrosion#474

[NobodyXu]

I'd recommend you to add #[allow(dead_code)] for line 2385

[LeonMatthesKDAB]

I'd recommend you to add #[allow(dead_code)] for line 2385

Hi, thank you. That has fixed the CI issues.
Unsure where that issue was coming from in the first place, as I haven't touched that code at all...

Probably just an update in nightly.

[NobodyXu]

If the path to the tool includes spaces it is otherwise impossible to provide a compiler cache like sccache through the CXX or CC environment variables, as the arguments will be split up incorrectly.

P.S. Looking at the source code, seems like cc actually supports splitting env CC='sccache cc'.

This is just a rant from me, doesn't affect merging of this PR.

[NobodyXu]

It would be great if you can setup a new CI that tests using RUSTC_WRAPPER for tools with relative and absolute pathas you describe, honestly a lot of stuff in cc are untested, so I'd prefer to use this opportunity to add some test for this.

[LeonMatthesKDAB]

If the path to the tool includes spaces it is otherwise impossible to provide a compiler cache like sccache through the CXX or CC environment variables, as the arguments will be split up incorrectly.

P.S. Looking at the source code, seems like cc actually supports splitting env CC='sccache cc'.

This is just a rant from me, doesn't affect merging of this PR.

Yes, it does, but only if the path to cc doesn't include spaces.

On windows, our CI finds the path to cc is something like:
C:\Program Files\...\cc
If this is encoded in CC, what cc-rs ends up generating is:
sccache "C:\Program" "Files\...\cc".
Which then means sccache thinks C:\Program is the compiler, which of course doesn't make sense.

For this to work correctly, we'd need #847 .
But the sentiment seems to be that it's not an ideal solution.
The patch here can work with the current behavior, as spaces are supported if CC is a direct file path.
Just the RUSTC_WRAPPER wasn't supported in that case, which would still be useful to support.
It can speed up our CI builds by more than half!

You can follow the corrosion in the corrision issue I mentioned, which hits the problem.

[NobodyXu]

Thanks I see, this is something I didn't realize could happen in production where path includes spaces.

[NobodyXu]

It would be great if you could verify it works, then I would have the confidence of approving and merging this PR.

Edit:

Sorry if I sound rude, I just want some confidence it would work as intended (cc has a history of merging PRs later found out to not work as intended and yanked the release, which is why v0.1.84 is yanked).

It would be great if you could try this in your project and veirfy that it work as intended.

[LeonMatthesKDAB]

Hi, of course.
We have tried this here: KDAB/cxx-qt#820 (with a backported version of this patch in cc-rs 1.0.79)
And it seems to work.

For some reason our Windows CI doesn't gain much speed when running under ctest, which we haven't fully investigated.
But e.g. our macOS CI has speed up from ~12 min. to ~6 min 🥳

[Be-ing]

It would be great if you could verify it works, then I would have the confidence of approving and merging this PR.

This behavior should also be documented, though considering that none of the environment variable usage is documented, I don't think that should hold up this PR. I opened a new issue for that: #920.

[NobodyXu]

Thank you!