This is an archive of the discontinued LLVM Phabricator instance.

[CMake] Remove custom ccache CMake logic
AcceptedPublic

Authored by thieta on Feb 6 2023, 11:38 PM.

Details

Summary

CMake supports CMAKE_CXX_COMPILER_LAUNCHER since CMake 3.4
so this custom CMake logic we had in LLVM can now be removed.

The only downside with this is that we can't set ccache
options from LLVM CMake, but it's arguable that this doesn't
belong in LLVM but should be done in a script calling the
build.

This was discussed in the forums here:

https://discourse.llvm.org/t/tips-for-incremental-building/67289/4?u=tobiashieta

Diff Detail

Event Timeline

thieta created this revision.Feb 6 2023, 11:38 PM
thieta requested review of this revision.Feb 6 2023, 11:38 PM
Herald added projects: Restricted Project, Restricted Project. · View Herald TranscriptFeb 6 2023, 11:38 PM

We could add deprecation warning for this something like:

if(LLVM_CCACHE_BUILD)
  message(FATAL "Use CMAKE_CXX_COMPILER_LAUNCHER=ccache instead")
endi()

But it seems like we haven't really done this before for CMake options, so I skipped it in this case.

barannikov88 added a subscriber: barannikov88.EditedFeb 7 2023, 1:57 AM

I'm afraid this change will go unnoticed by build bots. They must be updated first, and a deprecation warning could help (before removing the support).

thieta added a comment.Feb 7 2023, 2:09 AM

Happy to do a deprecation warning or error.

I am thinking we should just error if you pass the option instead of having it around doing something still. Does that sound too annoying for the bot owners or fine?

thieta updated this revision to Diff 495621.Feb 7 2023, 12:55 PM

Add a warning when you try to pass LLVM_CCACHE_BUILD=ON to CMake

Looks good to me including the cmake logic. I've been building with the launcher environment variables since I posted the Discourse post. I can't really speak to the build bot side, which doesn't look to have been updated based on the most recently build of this diff.

phosek accepted this revision.Feb 12 2023, 3:46 PM

LGTM

This revision is now accepted and ready to land.Feb 12 2023, 3:46 PM
This revision was landed with ongoing or failed builds.Feb 12 2023, 11:42 PM
This revision was automatically updated to reflect the committed changes.

Basically every bot broke with this change, so I reverted it for now and posted to discourse: https://discourse.llvm.org/t/llvm-ccache-build-is-deprecated/68431

asb reopened this revision.Feb 13 2023, 3:09 AM
asb added a subscriber: asb.

I think it would make sense to document CMAKE_C_COMPILER_LAUNCHER and CMAKE_CXX_COMPILER_LAUNCHER under "Frequently-used CMake variables" in CMake.rst. (Also explicitly marking this review as reopened given the commit was reverted).

This revision is now accepted and ready to land.Feb 13 2023, 3:09 AM
jrtc27 added a subscriber: jrtc27.Feb 13 2023, 9:30 AM
jrtc27 added inline comments.
llvm/CMakeLists.txt
237

FATAL_ERROR and deprecated seem at odds with each other? If it's an error then it's not just deprecated, it's straight up removed.

I found this option quite convenient, the motivation to remove this isn’t clear to me from the description: what’s the trade off with keeping it?

My motivation was that people asked to add support for new cache programs (sscache and more). And now that this is supported in upstream CMake in a nice way I think our build system is way too big and complicated to duplicate functionality that's already in upstream CMake. It's more switches to keep testing and update.

Is it a big thing? No. But in a complicated build system every little matters. IMHO.

Plus the upstream option is not that much more complicated to use.

I think getting rid of this option is good for us and our users in the long term.

Looking past the short-term adoption shift, having LLVM's build flows be using features of CMake that work on any CMake project lowers the LLVM-specific learning curve of new contributors. Having our current users migrate also means that current users will socialize the CMake approach rather than socializing the legacy-LLVM approach. Deleting the code from the build system means less code to support.

All of these seem worth it for some short-term migration pain.

ccotter added a comment.EditedFeb 13 2023, 11:10 AM

Another benefit is (and I might have been using the options incorrectly, which furthers the pointd made earlier I think) is that the recursive cmake invocations did not end up forwarding the LLVM_CCACHE_BUILD value, so only the top level build would be using ccache. The *COMPILER_LAUNCHER variable seems to get passed down correctly, although I'm sure if that's explicit or a happy coincidence.

Ok - looking at the logic, I think there is some explicit forwarding of the launcher variables, but not the LLVM_BUILD_CCACHE variable. In either case, the launcher approach is implemented better for the recursive builds.

@jrtc27 raises a good point - we have discussed deprecating the option but made it a hard error. Should we instead make it a soft error for now and a hard error later on?

Honestly, I was not expecting this option to be used as much as it seems like it is. So maybe it makes sense to make it a soft error to start.

Any thoughts on this so I can make the appropriate changes?

@jrtc27 raises a good point - we have discussed deprecating the option but made it a hard error. Should we instead make it a soft error for now and a hard error later on?

Honestly, I was not expecting this option to be used as much as it seems like it is. So maybe it makes sense to make it a soft error to start.

Any thoughts on this so I can make the appropriate changes?

I'd be fine starting with a warning and make it an error after the LLVM 17 branch point.

I think getting rid of this option is good for us and our users in the long term.

Looking past the short-term adoption shift, having LLVM's build flows be using features of CMake that work on any CMake project lowers the LLVM-specific learning curve of new contributors.

I don't see how it is applicable here: there is no "LLVM-specific learning" to be applied here, the generic CMake flow will "just work". However it just makes everyone's configuration more cumbersome unnecessarily. I type -DLLVM_CCACHE_BUILD=ON directly without thinking about it, similarly for -DLLVM_ENABLE_LLD=ON, I wouldn't get the alternatives right.

My motivation was that people asked to add support for new cache programs (sscache and more)

Seems like this could be supported trivially by having -DLLVM_CCACHE_BUILD=sscache for example?\

I think our build system is way too big and complicated to duplicate functionality that's already in upstream CMake.

I don't see this as a "duplication" right now, but as a convenient wrapper on top of the functionality provided by CMake. The code you're deleting illustrates well how non-intrusive the wrapper is actually.