cmake: Rework WITH_CCACHE
#215
Conversation
cmake/ccache.cmake
Outdated
| else() | ||
| set(ccache_hints) | ||
| if(EXISTS "$ENV{ChocolateyInstall}") | ||
| # Bypass a shim executable provided by Chocolatey. |
There was a problem hiding this comment.
Is this not something that users of this package manger should be fixing on their own machine? In any case, the comment doesn't explain why it's needed (same goes for all other comments here).
There was a problem hiding this comment.
Is this not something that users of this package manger should be fixing on their own machine?
That's indeed unfortunate that Chocalately's ccache.exe shim executable can't be moved or copied without being broken. But it is our intention to copy it in order to integrate with MSVC. Therefore, I believe that this is our responsibility to provide a workaround for such a case. Besides, it is only a single path hint.
Of course, this workaround can be removed in the future if it leads to extra maintenance burden.
In any case, the comment doesn't explain why it's needed (same goes for all other comments here).
I’ve reworked the comments in the hope of addressing your comment.
There was a problem hiding this comment.
But it is our intention to copy it in order to integrate with MSVC.
I still don't understand why we even need to do this though, to make ccache "work".
|
Rebased. |
hebasto
force-pushed
the
240527-cmake-FT
branch
3 times, most recently
from
a9b113e
to
cc9fdc9
Compare
|
I've removed stuff, which is specific to the Chocolatey package manager. As native Windows jobs in our CI use |
cmake/ccache.cmake
Outdated
| # file COPYING or https://opensource.org/license/mit/. | ||
|
|
||
| if(MSVC AND WITH_CCACHE) | ||
| # For integration of ccache and MSVC, please refer to |
There was a problem hiding this comment.
I thought we were going to remove all of this for now? If not, it's still not clear to me why forcing ccache usage into MSVC / Visual Studio is the right thing to be doing, or what the trade-offs are (i.e what is the effect of using the "old" debug information format, I assume this degrades / breaks other things, and a quick google suggests this used(?) to brake incremental linking, and would currently be incompatible with hot reloading etc). Nothing is mentioned here, or in the PR description, and I'd be suprised if this answer is "no side-effects".
cmake/ccache.cmake
Outdated
| find_program(CCACHE_EXECUTABLE ccache) | ||
| if(CCACHE_EXECUTABLE) | ||
| file(COPY_FILE ${CCACHE_EXECUTABLE} ${CMAKE_BINARY_DIR}/cl.exe ONLY_IF_DIFFERENT) | ||
| list(APPEND CMAKE_VS_GLOBALS |
There was a problem hiding this comment.
I didn't previously that this is configuring settings Visual Studio. Our build system really shouldn't need to pass settings to peoples code editors. If we are just copy-pasting what is in the cmake wiki, why don't we include "TrackFileAccess=false" & "UseMultiToolTask=true" here? According to https://cmake.org/cmake/help/latest/variable/CMAKE_VS_GLOBALS.html, the setting being used here, is also meant to be set by a toolchain file?
cmake/ccache.cmake
Outdated
| "CLToolPath=${CMAKE_BINARY_DIR}" | ||
| "DebugInformationFormat=OldStyle" | ||
| ) | ||
| set(CMAKE_VS_NO_COMPILE_BATCHING ON) |
There was a problem hiding this comment.
So this turns off parallelism in builds entirely, to be able to use ccache?
There was a problem hiding this comment.
Parallelism is provided by means of "UseMultiToolTask=true".
Sure. MSVC support has been removed. |
`WITH_CCACHE` is an opportunistic option now and its default is `ON`.
|
ACK 4cd15f1 Tested on Debian and saw ccache running and second builds were much faster. I also uninstalled ccache and configure correctly configured it to be off. I then tried to force ccache on and configure would not allow it to be on without ccache installed. |
…_required()` af5e6b4 [FIXUP] cmake: Enable CMP0141 policy after `cmake_minimum_required()` (Hennadii Stepanov) Pull request description: Otherwise, the policy setting won't get any effect. See https://cmake.org/cmake/help/latest/policy/CMP0141.html --- After #215, the `CMAKE_MSVC_DEBUG_INFORMATION_FORMAT` CMake's abstraction is not used in our code directly, but it is still convenient for the user to be able to use it when configuring the build system from the command line or an IDE. --- This change can be easily verified on any system (not Windows only) by injecting the following commands: ```cmake cmake_policy(GET CMP0141 result) message("${result}") ``` ACKs for top commit: m3dwards: ACK af5e6b4 Tree-SHA512: 98dccc7f97b3b496a121a6e0961b60c344a2bc7f2e19193ea1936a9776c5ebf7eec6c47e6159332997b48ea2ce3220a801054c3c9d1d8c7be927add4458bfb92
The `CMAKE_MSVC_DEBUG_INFORMATION_FORMAT` variable has not been used since the merge of #215 in the CMake staging branch.
This PR makes
WITH_CCACHEan opportunistic boolean option with the default valueON.Also the build system learned to tell ccache in the masquerade mode, which addresses this #93 (comment).
The
TristateOptionmodule has been deleted as planned.