Skip to content

[NFC] Adding a message to indicate LLVM_CCACHE_BUILD's future deprecation #90624

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

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

qiongsiwu
Copy link
Contributor

@qiongsiwu qiongsiwu commented Apr 30, 2024

This PR adds a message to indicate that LLVM_CCACHE_BUILD will be deprecated and suggests using CMAKE_C_COMPILER_LAUNCHER and CMAKE_CXX_COMPILER_LAUNCHER.

@qiongsiwu qiongsiwu requested review from daltenty and tru April 30, 2024 15:58
@qiongsiwu qiongsiwu self-assigned this Apr 30, 2024
@qiongsiwu qiongsiwu requested a review from daltenty April 30, 2024 21:37
@qiongsiwu qiongsiwu changed the title [AIX] Using ccache-swig as the ccache Binary Using ccache-swig if ccache cannot be found Apr 30, 2024
@qiongsiwu qiongsiwu changed the title Using ccache-swig if ccache cannot be found Use ccache-swig if ccache cannot be found Apr 30, 2024
Copy link
Member

@daltenty daltenty left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

(one thing I note with this ccache support appears directly in llvm/CMakeList.txt, that makes me think this doesn't work for runtimes. but I think that's out of scope for this PR)

@tru
Copy link
Collaborator

tru commented May 1, 2024

I don't this we should add more stuff to LLVM_ENABLE_CCACHE. I think people should use CMAKE_C/CXX_COMPILER_LAUNCHER instead.

@daltenty
Copy link
Member

daltenty commented May 1, 2024

I don't this we should add more stuff to LLVM_ENABLE_CCACHE. I think people should use CMAKE_C/CXX_COMPILER_LAUNCHER instead.

Thanks for the tip. Reading the CMake docs that seems a reasonable solution.

Should we make mention of this in https://github.com/llvm/llvm-project/blob/main/llvm/docs/CMake.rst and/or issue a message about this option being deprecated? Those docs are what originally led us down this path.

@tru
Copy link
Collaborator

tru commented May 1, 2024

Yes - I would welcome some documentation updates that points out the upstream solution and that the LLVM_ option is mainly kept around as a legacy thing. I tried to remove it a while ago but encountered push back, so I would avoid saying it's totally deprecated.

cc @Endilll

@Endilll
Copy link
Contributor

Endilll commented May 1, 2024

I'd like to second Tobias' feedback. CMAKE_<LANG>_COMPILER_LAUNCHER has been around since CMake 3.4, while our minimum requirement in 3.20, so there are no compatibility reasons to do this.

On top of that, I think compiler launcher should be treated as a part of toolchain. That is, if present, it should be either set by user on the command line, or in a toolchain file. Notably, we shouldn't try to be smart about it after project() is called, like unofficial (yet Kitware-hosted) documentation has been saying for at least 6 years about CMAKE_<LANG>_COMPILER: https://gitlab.kitware.com/cmake/community/-/wikis/FAQ#method-3-avoid-use-set

@qiongsiwu qiongsiwu changed the title Use ccache-swig if ccache cannot be found [NFC] Adding a message to indicate LLVM_CCACHE_BUILD's future deprecation May 21, 2024
@qiongsiwu
Copy link
Contributor Author

Thanks for your comments everyone! The PR is updated to only add a CMake message, and the documentation is updated to reflect our suggestion to use CMAKE_C_COMPILER_LAUNCHER and CMAKE_CXX_COMPILER_LAUNCHER.

@qiongsiwu
Copy link
Contributor Author

Ping for more comments. If there are none, I will land this PR soon. Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants