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

Add support for CMAKE_C/CXX_COMPILER_LAUNCHER #474

Closed
wants to merge 2 commits into from
Closed

Add support for CMAKE_C/CXX_COMPILER_LAUNCHER #474

wants to merge 2 commits into from

Conversation

LeonMatthesKDAB
Copy link
Contributor

Also fix _CORROSION_RUST_CARGO_TARGET_UNDERSCORE, which didn't actually use underscores

We found that setting the CMAKE_C_COMPILER_LAUNCHER to sccache or ccache managed to speed up builds of CXX-Qt by ~50% from 116s to 48s.
However, Corrosion didn't pass this along to Cargo, so add support for this.

@LeonMatthesKDAB
Copy link
Contributor Author

I just noticed this is causing issues on our Windows CI, due to issues with spaces...
See: https://github.com/KDAB/cxx-qt/actions/runs/7423274184/job/20200354471?pr=791#step:15:147

I'll try to fix it.

@LeonMatthesKDAB LeonMatthesKDAB force-pushed the allow-cmake-compiler-launcher branch 3 times, most recently from 9e2053d to 76f414a Compare January 5, 2024 15:26
@jschwe
Copy link
Collaborator

jschwe commented Jan 5, 2024

Also fix _CORROSION_RUST_CARGO_TARGET_UNDERSCORE, which didn't actually use underscores

Ah indeed - that is unfortunate. Thanks for catching it!

I just noticed this is causing issues on our Windows CI, due to issues with spaces...

I suspect the most robust way would be to add upstream support in cc-rs via a new variable like CC_ENCODED in the style of CARGO_ENCODED_RUSTFLAGS. cc-rs claims the spaces are not supported in CFLAGS and i suspect this extends to the CC family of variables too.

FYI, if you would just like to enable this in your CI (ingnoring windows for now) without updating corrosion, I would suggest to just force the _CORROSION_RUST_CARGO_TARGET_UNDERSCORE cache variable to the correct value, and then set CC_${Rust_CARGO_TARGET} (the non-underscore variant) via corrosion_set_env_vars to your desired value.

@LeonMatthesKDAB
Fix _CORROSION_RUST_CARGO_TARGET_UNDERSCORE
e02f684 
This variable didn't actually store an underscore value but used
kebab-style, which couldn't be overwritten.
@LeonMatthesKDAB
Add support for CMAKE_C/CXX_COMPILER_LAUNCHER
880b36c
@LeonMatthesKDAB LeonMatthesKDAB force-pushed the allow-cmake-compiler-launcher branch from 76f414a to 880b36c Compare January 10, 2024 10:50
@LeonMatthesKDAB LeonMatthesKDAB mentioned this pull request Jan 10, 2024
Merged
@LeonMatthesKDAB
Copy link
Contributor Author

LeonMatthesKDAB commented Jan 10, 2024 •
edited
Loading

Hi @jschwe,
Thank you for the suggestion, I wasn't aware of the CARGO_ENCODED_RUSTFLAGS. CC_ENCODED indeed sounds like something that could solve multiple white-space issues for cc-rs at once. I'll try to find some time to upstream that to cc-rs and adjust this PR accordingly.

For now I've split up the fix for the underscore issue, so that it can be merged already in #475

@Be-ing
Copy link

Be-ing commented Jan 16, 2024

You can also put

[build]
rustc-wrapper = "sccache"

in ~/.cargo/config.toml to tell Cargo to always use sccache. Though yes it would be nice to have Corrosion pass this along from CMake.

@jschwe
Copy link
Collaborator

jschwe commented Jan 16, 2024

To clarify: The original issue (as I understand it) is about compiling C/C++ code with (s)ccache. Does setting this flag in the config file really cause C++ code compiled via cc-rs to be wrapped by sccache?

Personally I think it is easy enough for the user to set the required rust flag for compiling Rust code with sccache, but pull requests to improve the user experience are always welcome as long as they don't risk regressions in other places.

@Be-ing
Copy link

Be-ing commented Jan 16, 2024 •
edited
Loading

Ah, sorry I got confused. CXX-Qt's CI sets the RUSTC_WRAPPER environment variable so rustc's compilations get cached by sccache. cc-rs's compilations getting cached is another matter. It looks like cc-rs already has code to handle this case and even some magic to use sccache for C/C++ when RUSTC_WRAPPER is set and CC is unset (I don't think the latter has an effect when using Corrosion because Corrosion always sets the CC environment variable?). So I don't think there are changes needed upstream in cc-rs; it should already handle CC_{target} being set with a space as this PR does.

Looking at the CI failure, the error there is coming from the link-cplusplus crate, not cc-rs.

@LeonMatthesKDAB
Copy link
Contributor Author

LeonMatthesKDAB commented Jan 17, 2024 •
edited
Loading

Looking at the CI failure, the error there is coming from the link-cplusplus crate, not cc-rs.

Hi @Be-ing, nice to hear from you :)

I may not be reading the log correctly, but I think the error is in the build script of link-cplusplus, which in turn does use the cc crate.
So I think in the end we do need to fix something in cc::Build.
The problem there just seems to be that cc::Build can handle that sccache is prefixed to CC, but it can't handle the case that the argument to sccache has spaces inside.
e.g. it passes this:
CC="sccache /my space directory/gcc" as separate arguments to sccache: sccache "/my" "space" "directory/gcc".

So sccache thinks it should invoke the compiler /my with the arguments space and directory/gcc. Which of course doesn't make sense :/

@LeonMatthesKDAB LeonMatthesKDAB mentioned this pull request Jan 22, 2024
Merged
LeonMatthesKDAB added a commit to LeonMatthesKDAB/cc-rs that referenced this pull request Jan 24, 2024
@LeonMatthesKDAB
Use RUSTC_WRAPPER if no other wrapper is provided
a756a78 
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
@LeonMatthesKDAB LeonMatthesKDAB mentioned this pull request Jan 24, 2024
Merged
@LeonMatthesKDAB
Copy link
Contributor Author

I've found that RUSTC_WRAPPER is used by cc-rs as a fallback to detect if sccache should be used.
This however isn't respected if the CC or CXX environment variable are set to an absolute path.
I'm trying to upstream a change to cc-rs here that changes that: rust-lang/cc-rs#918

If this PR is merged, this one would no longer be necessary, at least for our use-case, as we can just set RUSTC_WRAPPER in our CI.

@Be-ing
Copy link

Be-ing commented Jan 26, 2024

I like that solution better. If RUSTC_WRAPPER tells Cargo to use sccache, I'd like cc-rs to use sccache for C & C++ compilation, regardless of Corrosion, CMake, or any other tooling around Cargo. That solution will also help speed up building projects where Cargo does the whole build.

@LeonMatthesKDAB
Copy link
Contributor Author

@Be-ing I completely agree.
If cc-rs merges the PR, we can close this one I think.
I'll just leave it open until that happens, in case it's not accepted by cc-rs for whatever reason.

NobodyXu pushed a commit to rust-lang/cc-rs that referenced this pull request Jan 26, 2024
@LeonMatthesKDAB
Use RUSTC_WRAPPER if no other wrapper is provided (#918)
4d36cfa 
* Use RUSTC_WRAPPER if no other wrapper is provided

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

* Add #[allow(dead_code)] to ArchSpec::Catalyst

This should suppress new warnings generated by the nightly toolchain.
@LeonMatthesKDAB
Copy link
Contributor Author

rust-lang/cc-rs#918 is merged now 🥳

That can be used without change to corrosion 👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@LeonMatthesKDAB @jschwe @Be-ing