-
Notifications
You must be signed in to change notification settings - Fork 6.3k
Speed up CI builds with ccache (seeded from develop) #16385
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
base: develop
Are you sure you want to change the base?
Conversation
|
Thank you for your contribution to the Solidity compiler! A team member will follow up shortly. If you haven't read our contributing guidelines and our review checklist before, please do it now, this makes the reviewing process and accepting your contribution smoother. If you have any questions or need our help, feel free to post them in the PR or talk to us directly on the #solidity-dev channel on Matrix. |
5897d83 to
1d6e22d
Compare
scripts/ci/build_win.sh
Outdated
| export CCACHE_NOHASHDIR=1 | ||
| export CCACHE_COMPILERTYPE=msvc | ||
| # Hard-coded MSVC cl.exe path | ||
| CCACHE_COMPILER="C:/Program Files (x86)/Microsoft Visual Studio/2019/BuildTools/VC/Tools/MSVC/14.29.30133/bin/Hostx64/x64/cl.exe" |
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.
won't that very easily break?
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.
You mean the hard-coded path?
I cannot find a reliable way to find that path.
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.
vswhere.exe might be able to do that for you (or vcvarsall or something)
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.
clonker
left a comment
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.
some preliminary comments. very nice work altogether! i was thinking whether we should perhaps have another environment variable that globally enables or disables ccache entirely. that way, if there are problems, we can quickly switch it off.
…tag` command - Remove CCACHE_ENABLED and use CCACHE_DISABLE instead
| export CCACHE_DIR="$HOME/.ccache" | ||
| export CCACHE_BASEDIR="$ROOTDIR" | ||
| export CCACHE_NOHASHDIR=1 | ||
| CMAKE_OPTIONS="${CMAKE_OPTIONS:-} -DCMAKE_C_COMPILER_LAUNCHER=ccache -DCMAKE_CXX_COMPILER_LAUNCHER=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.
We already have ccache detection in place, I’m wondering whether it would be better to let CMake handle this automatically and remove -DCMAKE_C_COMPILER_LAUNCHER=ccache and -DCMAKE_CXX_COMPILER_LAUNCHER=ccache from here. See: https://github.com/argotorg/solidity/blob/develop/cmake/EthCcache.cmake).
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.
We would probably need to add set(CMAKE_C_COMPILER_LAUNCHER ${CCACHE}) on the cmake ccache config as well.
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 kinda like that it's explicit. Never understood why we need this EthCcache.cmake in the first place.
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.
Haha sure. We can keep it as it is then, and don't change the cake cmake 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.
Never change the cake config
| - ~/.ccache | ||
| run_with_ccache_unless_tag: |
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 | |
| run_with_ccache_unless_tag: | |
| - ~/.ccache | |
| run_with_ccache_unless_tag: |
| export CCACHE_DIR="$HOME/.ccache" | ||
| export CCACHE_BASEDIR="$ROOTDIR" | ||
| export CCACHE_NOHASHDIR=1 | ||
| CMAKE_OPTIONS="${CMAKE_OPTIONS:-} -DCMAKE_C_COMPILER_LAUNCHER=ccache -DCMAKE_CXX_COMPILER_LAUNCHER=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 think that for build.sh and build_ossfuzz.sh we can remove this and let cmake set it. For Windows, it seems that we may need to explicitly set using CMAKE_VS_GLOBALS
| CMAKE_VS_GLOBALS="CLToolPath=${WIN_PWD}/deps/ccache;UseMultiToolTask=true" | ||
| CMAKE_VS_GLOBALS_ARG=(-DCMAKE_VS_GLOBALS="$CMAKE_VS_GLOBALS") |
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 this be done by cmake? See: https://github.com/ccache/ccache/wiki/MS-Visual-Studio#usage-with-cmake
Maybe we can add that you our cmake ccache 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.
Since #16385 (comment) is @clonker’s preference, and I don’t have a strong opinion either way, I’m okay with keeping some configurations out of CMake and explicitly defining them in the build scripts.
Depends on #16384
For the purpose of testing instead of creating the cache from
develop, the cache is created by this branchci-ccache. We can later create another PR starting from this branch to see if the builds are indeed using the cache generated byci-ccache, before merging this PR let's remember to fix the occurrences ofci-ccachein the code.Results
In this PR, I edited a single file and the CI is successfully using ccache for all build job. These are the results of the speedup:
In total we save ~23 minutes (or around 2$) each run.