-
Notifications
You must be signed in to change notification settings - Fork 24
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
Use ccache in windows CI #1736
Use ccache in windows CI #1736
Conversation
Signed-off-by: Sylvain Leclerc <[email protected]>
Signed-off-by: Sylvain Leclerc <[email protected]>
Signed-off-by: Sylvain Leclerc <[email protected]>
Signed-off-by: Sylvain Leclerc <[email protected]>
Signed-off-by: Sylvain Leclerc <[email protected]>
Signed-off-by: Sylvain Leclerc <[email protected]>
Signed-off-by: Sylvain Leclerc <[email protected]>
This reverts commit 933aa97.
This reverts commit e1fbcdd.
This reverts commit 53b24b9.
This reverts commit 4f38e7d.
Signed-off-by: Sylvain Leclerc <[email protected]>
This reverts commit 89a579a.
Signed-off-by: Sylvain Leclerc <[email protected]>
Signed-off-by: Sylvain Leclerc <[email protected]>
Signed-off-by: Sylvain Leclerc <[email protected]>
Kudos, SonarCloud Quality Gate passed! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Whouldn't ccache use and particularities on Windows be useful to figure in a ADR or at least some comments somewhere?
# Downloads ccache, and copies it to "cl.exe" in order to trick cmake into using it, | ||
# see ccache wiki for background on using it with MSVC: | ||
# https://github.com/ccache/ccache/wiki/MS-Visual-Studio | ||
- name: Install ccache |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can't we use vcpkg to install ccache?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't find it in vcpkg repo.
A possibility was to install it with chocolatey, like we do for wget/zip above, but it just makes it more difficult to implement the "masquerading" technique because choco will install it in it own nested folders, and we must take care of not using the shim executable that it creates.
Installs from choco also seem slower compared to the plain download from github relase.
@@ -10,7 +10,7 @@ if (NOT WIN32) | |||
set(COMMON_GCC_FLAGS "${COMMON_GCC_FLAGS} -pipe -msse -msse2 -Wunused-but-set-variable -Wunused-but-set-parameter") | |||
set(COMMON_GCC_FLAGS "${COMMON_GCC_FLAGS} -Werror=return-type") | |||
endif() | |||
set(COMMON_MSVC_FLAGS "/W3 /MP4 /Zi ") | |||
set(COMMON_MSVC_FLAGS "/W3 /MP4") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What's /zi option? Why is it removed?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
See explanation in the PR description:
Had to remove hard-coded MSVC compiler option /Zi which asked for the generation of debug information, which is anyway not useful in CI.
In local environment, this will instead be added as necessary by cmake, depending on the type of build requested.
So, actually this hard coded flag did not have anything to do here (actually the other ones would deserve to be questioned too, but it's not the scope of the PR).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(ccache does not support this flag, probably because it has the effect of producing additional output files, not only compiled code)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Had to remove hard-coded MSVC compiler option /Zi which asked for the generation of debug information, which is anyway not useful in CI.
Please keep in mind that common-settings.cmake is also used for local builds, for which debug info might be useful.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Indeed, CMake knows how to handle this alone, when building configuration Debug
or RelWithDebInfo
.
Setting compiler-specific flags in cmake files should be avoided as much as possible.
The developer is responsible to use the configuration he needs, we should not add debug information for Release
config.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My concern is that by manually setting COMMON_MSVC_FLAGS
, we might override what was generated by CMake from CMAKE_BUILD_TYPE
.
A general discussion is outside of the scope of this PR.
But with this PR specifically, we don't want to lose debug info when CMAKE_BUILD_TYPE=Release/RelWithDebugInfo
Good question: I guess it does if we want to use it in local developer environment, but I am not sure it is really useful. |
build time = 4min15s instead of 15min+ that's awesome! |
To be honest I don't know how it works exactly ... clearly it's still the already present MSVC which is called in the end by ccache, so in some way the "fake" cl.exe knows where to find the "true" one, but I don't know how. |
I saw that using such variables (like the |
- name: ccache | ||
uses: hendrikmuhs/[email protected] | ||
with: | ||
key: windows |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
key: windows | |
key: windows-ccache |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The action already appends that to cache names (see in ubuntu workflow, we also use the OS name as a key)
Goal
The goal is to implement compiler output caching for the windows CI, using ccache, to speed up the build.
See issue #1718.
Implementation
Followed the guidelines from https://github.com/ccache/ccache/wiki/MS-Visual-Studio :
Had to remove hard-coded MSVC compiler option
/Zi
which asked for the generation of debug information, which is anyway not useful in CI.In local environment, this will instead be added as necessary by cmake, depending on the type of build requested.
Result
Seems to work quite well, see job here:
https://github.com/AntaresSimulatorTeam/Antares_Simulator/actions/runs/6731632879/job/18299509143