[CMake] Fix using precompiled headers with ccache #131397
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Using precompiled headers with ccache requires special accommodations. Add the required CCACHE option and clang compiler flag to CMake. Signed-off-by: Kajetan Puchalski <[email protected]>
|
@tblah @DavidTruby @mgorny @Meinersbur for review. I'm not sure whether this needs special handling for Windows w.r.t. that I also wasn't sure whether to add |
|
In my experience |
tblah
requested review from
petrhosek,
mgorny,
DavidTruby,
Meinersbur and
tblah
|
@mrkajetanp Did you check whether with these changes, ccache does cache those compilations? I am using ccache under Windows and works fine. I could not reproduce the error in https://lab.llvm.org/staging/#/builders/206/builds/174. Support for msvc has been added to ccache relatively recently. |
A fresh ccache log is showing cache hits for the precompiled headers with this patch, yes. |
Meinersbur
reviewed
Mar 17, 2025
|
Just to save people from clicking through the related PRs, here's the ccache documentation I based this off of: |
Meinersbur
reviewed
Mar 17, 2025
Since ccache 4.8, it is possible to pass configuration options directly to ccache on the command line as opposed to through environment variables. This is the intended way for CMake to configure ccache, as described on the GitHub wiki for the project: https://github.com/ccache/ccache/wiki/CMake Rework the way ccache is configured by LLVM accordingly. Signed-off-by: Kajetan Puchalski <[email protected]>
Reference ccache documentation: https://ccache.dev/manual/latest.html#_precompiled_headers For clang, only pass -Xclang -fno-pch-timestamp if precompiled headers are enabled in CMake. For gcc, pass -fpch-preprocess in the same scenario. Signed-off-by: Kajetan Puchalski <[email protected]>
With ccache only supporting passing arguments on the command line since version 4.8, a workaround is needed for systems with older versions. Whenever possible, we want to use the new way of configuring ccache because it will ensure uniform behaviour across platforms in a way that just setting environment variables might not. Signed-off-by: Kajetan Puchalski <[email protected]>
DavidTruby
approved these changes
Mar 20, 2025
There was a problem hiding this comment.
I've done a little investigation and the reason this flag isn't default anyway when using .pch files is essentially to work around/protect against dodgy build systems that won't rebuild the pch properly in all cases when the .h has changed. So clang will error if the .pch hasn't been updated properly.
We can rely on CMake handling this correctly; the target has a dependency on the .pch and the .pch has a dependency on the .h so there will be no issue here. So I think it's fine to just add this flag in every case even when ccache/other caching software isn't being used.
LGTM on this basis but please wait for the other reviewers to comment to check that they agree. Thanks for the fix!
|
@Meinersbur Do you think this is okay to merge now? |
petrhosek
reviewed
Mar 31, 2025
petrhosek
reviewed
Mar 31, 2025
flang - Explicitly match gcc to add gcc-specific flag. Signed-off-by: Kajetan Puchalski <[email protected]>
mgorny
approved these changes
Apr 6, 2025
|
@petrhosek are you happy for this to be merged? |
thesamesam
approved these changes
Apr 8, 2025
|
There are bug reports like the one in #134726 waiting for this to be merged, so given there are 4 approvals I'll just go ahead and merge this. If something needs to be changed or if there are other objections, we can fix or revert later. |
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/117/builds/8469 Here is the relevant piece of the build log for the reference |
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/88/builds/10144 Here is the relevant piece of the build log for the reference |
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/54/builds/8057 Here is the relevant piece of the build log for the reference |
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/155/builds/8202 Here is the relevant piece of the build log for the reference |
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/7/builds/13115 Here is the relevant piece of the build log for the reference |
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/160/builds/15952 Here is the relevant piece of the build log for the reference |
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/180/builds/15941 Here is the relevant piece of the build log for the reference |
mrkajetanp
added a commit
that referenced
this pull request
Apr 8, 2025
This reverts commit e8dc8ad.
mrkajetanp
added a commit
that referenced
this pull request
Apr 8, 2025
Reverts #131397 Reverting for now on account of build bot failures on certain platforms.
|
Reverted for now on account of the Windows issues. I think there's a difference with how CMake expands |
|
I find this change hard to get an overview of; you're both passing new options to ccache, and also changing how options are passed to ccache (previously only env variables, now both env vars and as command arguments, if I understand correctly?). Does this change need the change in how ccache options are passed? And can that change be split out from the functional change in passing more options wrt PCH, so that we first have a NFC change that only changes how ccache options are passed, followed by a potentially functional change in what options are passed? |
Certainly. Originally this was just a change to add the required flags for PCH, but in a review comment I was asked to also refactor the way we configure ccache while at it because it is currently using some antipatterns. It does make more sense to do it separately and the other way around, I'll do that. |
llvm-sync bot
pushed a commit
to arm/arm-toolchain
that referenced
this pull request
Apr 8, 2025
… (#134848) Reverts llvm/llvm-project#131397 Reverting for now on account of build bot failures on certain platforms.
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/136/builds/3477 Here is the relevant piece of the build log for the reference |
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/33/builds/14498 Here is the relevant piece of the build log for the reference |
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/187/builds/5289 Here is the relevant piece of the build log for the reference |
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/42/builds/4053 Here is the relevant piece of the build log for the reference |
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/38/builds/2868 Here is the relevant piece of the build log for the reference |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Using precompiled headers with ccache requires special accommodations.
Add the required CCACHE option and clang compiler flag to CMake.